Fix :Postgres :day-of-week extracts coming back as 0..6 instead of 1..7 (#22167)

* Fix :Postgres :day-of-week extracts coming back as 0..6 instead of 1..7

* Test Tuesday-Saturday per @dpsutton suggestion and fix issues with those as well 

* Fix MongoDB for Tuesday

modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj

@@ -157,11 +157,17 @@
             (name id-or-name))
     temporal-unit (with-lvalue-temporal-bucketing temporal-unit)))
 
+(defn- add-start-of-week-offset [expr offset]
+  (cond
+    (zero? offset) expr
+    (neg? offset)  (recur expr (+ offset 7))
+    :else          {$mod [{$add [expr offset]}
+                          7]}))
+
 (defn- day-of-week
   [column]
-  (mongo-let [day_of_week {$mod [{$add [{$dayOfWeek column}
-                                        (driver.common/start-of-week-offset :mongo)]}
-                                 7]}]
+  (mongo-let [day_of_week (add-start-of-week-offset {$dayOfWeek column}
+                                                    (driver.common/start-of-week-offset :mongo))]
     {$cond {:if   {$eq [day_of_week 0]}
             :then 7
             :else day_of_week}}))

src/metabase/driver.clj

@@ -659,8 +659,9 @@
 
 (defmethod default-field-order ::driver [_] :database)
 
+;; TODO -- this can vary based on session variables or connection options
 (defmulti db-start-of-week
-  "Return start of week for given database"
+  "Return the day that is considered to be the start of week by `driver`. Should return a keyword such as `:sunday`."
   {:added "0.37.0" :arglists '([driver])}
   dispatch-on-initialized-driver
   :hierarchy #'hierarchy)

src/metabase/driver/common.clj

@@ -18,7 +18,8 @@
            org.joda.time.format.DateTimeFormatter))
 
 (def connection-error-messages
-  "Generic error messages that drivers should return in their implementation of `humanize-connection-error-message`."
+  "Generic error messages that drivers should return in their implementation
+  of [[metabase.driver/humanize-connection-error-message]]."
   {:cannot-connect-check-host-and-port
    (str (deferred-tru "Hmm, we couldn''t connect to the database.")
         " "
@@ -443,12 +444,26 @@
   []
   (.indexOf days-of-week (setting/get-value-of-type :keyword :start-of-week)))
 
-(s/defn start-of-week-offset :- s/Int
-  "Return the offset for start of week to have the week start on [[metabase.public-settings/start-of-week]] given
-  `driver`."
-  [driver]
-  (let [db-start-of-week     (.indexOf days-of-week (driver/db-start-of-week driver))
+(defn start-of-week-offset-for-day
+  "Like [[start-of-week-offset]] but takes a `start-of-week` keyword like `:sunday` rather than ` driver`. Returns the
+  offset (as a negative number) needed to adjust a day of week in the range 1..7 with `start-of-week` as one to a day
+  of week in the range 1..7 with [[metabase.public-settings/start-of-week]] as 1."
+  [start-of-week]
+  (let [db-start-of-week     (.indexOf days-of-week start-of-week)
         target-start-of-week (start-of-week->int)
         delta                (int (- target-start-of-week db-start-of-week))]
     (* (Integer/signum delta)
        (- 7 (Math/abs delta)))))
+
+(s/defn start-of-week-offset :- s/Int
+  "Return the offset needed to adjust a day of the week (in the range 1..7) returned by the `driver`, with `1`
+  corresponding to [[driver/db-start-of-week]], so that `1` corresponds to [[metabase.public-settings/start-of-week]] in
+  results.
+
+  e.g.
+
+  If `:my-driver` returns [[driver/db-start-of-week]] as `:sunday` (1 is Sunday, 2 is Monday, and so forth),
+  and [[metabase.public-settings/start-of-week]] is `:monday` (the results should have 1 as Monday, 2 as Tuesday... 7 is
+  Sunday), then the offset should be `-1`, because `:monday` returned by the driver (`2`) minus `1` = `1`."
+  [driver]
+  (start-of-week-offset-for-day (driver/db-start-of-week driver)))

src/metabase/driver/postgres.clj

@@ -247,8 +247,14 @@
 (defmethod sql.qp/date [:postgres :year]            [_ _ expr] (date-trunc :year expr))
 
 (defmethod sql.qp/date [:postgres :day-of-week]
-  [_ _ expr]
-  (sql.qp/adjust-day-of-week :postgres (extract-integer :dow expr)))
+  [_ driver expr]
+  ;; Postgres extract(dow ...) returns Sunday(0)...Saturday(6)
+  ;;
+  ;; Since that's different than what we normally consider the [[metabase.driver/db-start-of-week]] for Postgres
+  ;; (Monday) we need to pass in a custom offset here
+  (sql.qp/adjust-day-of-week driver
+                             (hx/+ (extract-integer :dow expr) 1)
+                             (driver.common/start-of-week-offset-for-day :sunday)))
 
 (defmethod sql.qp/date [:postgres :week]
   [_ _ expr]

src/metabase/driver/sql/query_processor.clj

@@ -138,22 +138,32 @@
       (truncate-fn expr))))
 
 (s/defn adjust-day-of-week
-  "Adjust day of week wrt start of week setting."
+  "Adjust day of week to respect the [[metabase.public-settings/start-of-week]] Setting.
+
+  The value a `:day-of-week` extract should return depends on the value of `start-of-week`, by default Sunday.
+
+  * `1` = first day of the week (e.g. Sunday)
+  * `7` = last day of the week (e.g. Saturday)
+
+  This assumes `day-of-week` as returned by the driver is already between `1` and `7` (adjust it if it's not). It
+  adjusts as needed to match `start-of-week` by the [[driver.common/start-of-week-offset]], which comes
+  from [[driver/db-start-of-week]]."
   ([driver day-of-week]
    (adjust-day-of-week driver day-of-week (driver.common/start-of-week-offset driver)))
 
   ([driver day-of-week offset]
    (adjust-day-of-week driver day-of-week offset hx/mod))
 
-  ([_driver
+  ([driver
     day-of-week
     offset :- s/Int
     mod-fn :- (s/pred fn?)]
-   (if (not= offset 0)
-     (hsql/call :case
-       (hsql/call := (mod-fn (hx/+ day-of-week offset) 7) 0) 7
-       :else                                                 (mod-fn (hx/+ day-of-week offset) 7))
-     day-of-week)))
+   (cond
+     (zero? offset) day-of-week
+     (neg? offset)  (recur driver day-of-week (+ offset 7) mod-fn)
+     :else          (hsql/call :case
+                      (hsql/call := (mod-fn (hx/+ day-of-week offset) 7) 0) 7
+                      :else                                                 (mod-fn (hx/+ day-of-week offset) 7)))))
 
 (defmulti quote-style
   "Return the quoting style that should be used by [HoneySQL](https://github.com/jkk/honeysql) when building a SQL

test/metabase/query_processor_test/date_bucketing_test.clj

@@ -1163,6 +1163,24 @@
                         :breakout    [!day-of-week.date]
                         :filter      [:between $date "2013-01-03" "2013-01-20"]}))))))))))
 
+(deftest first-day-of-week-for-day-of-week-bucketing-test
+  (testing "First day of week for `:day-of-week` bucketing should be the consistent (#17801)"
+    (mt/test-drivers (mt/normal-drivers-with-feature :basic-aggregations)
+      (let [query (mt/mbql-query checkins
+                    {:aggregation [[:count]]
+                     :breakout    [!day-of-week.date]})]
+        (doseq [[first-day-of-week expected-rows] {:sunday    [[1 135] [2 143] [3 153] [4 136] [5 139] [6 160] [7 134]]
+                                                   :monday    [[1 143] [2 153] [3 136] [4 139] [5 160] [6 134] [7 135]]
+                                                   :tuesday   [[1 153] [2 136] [3 139] [4 160] [5 134] [6 135] [7 143]]
+                                                   :wednesday [[1 136] [2 139] [3 160] [4 134] [5 135] [6 143] [7 153]]
+                                                   :thursday  [[1 139] [2 160] [3 134] [4 135] [5 143] [6 153] [7 136]]
+                                                   :friday    [[1 160] [2 134] [3 135] [4 143] [5 153] [6 136] [7 139]]
+                                                   :saturday  [[1 134] [2 135] [3 143] [4 153] [5 136] [6 139] [7 160]]}]
+          (mt/with-temporary-setting-values [start-of-week first-day-of-week]
+            (mt/with-native-query-testing-context query
+              (is (= expected-rows
+                     (mt/formatted-rows [int int] (qp/process-query query)))))))))))
+
 (deftest filter-by-current-quarter-test
   ;; Oracle doesn't work on March 31st because March 31st + 3 months = June 31st, which doesn't exist. See #10072
   (mt/test-drivers (disj (mt/normal-drivers) :oracle)