Fix sqlserver does not respect `start-of-week` when bucket by `week` (#26474)

- Add tests to check when bucketing by week and week-of-year behave consistently, Closes #4910
- While doing the above, found that sqlserver does not respect `start-of-week` setting, so fix that. Fix #25356

modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj

@@ -168,15 +168,20 @@
   [_ _ expr]
   (date-part :dayofyear expr))
 
-;; Subtract the number of days needed to bring us to the first day of the week, then convert to date
-;; The equivalent SQL looks like:
-;;     CAST(DATEADD(day, 1 - DATEPART(weekday, %s), CAST(%s AS DATE)) AS DATETIME)
+;; Subtract the number of days needed to bring us to the first day of the week, then convert to back to orignal type
+(defn- trunc-week
+  [expr]
+  (let [original-type (if (= "datetimeoffset" (hx/type-info->db-type (hx/type-info expr)))
+                        "datetimeoffset"
+                        "datetime")]
+    (hx/cast original-type
+      (date-add :day
+                (hx/- 1 (date-part :weekday expr))
+                (hx/->date expr)))))
+
 (defmethod sql.qp/date [:sqlserver :week]
-  [_ _ expr]
-  (hx/->datetime
-   (date-add :day
-             (hx/- 1 (date-part :weekday expr) (driver.common/start-of-week-offset :sqlserver))
-             (hx/->date expr))))
+  [driver _ expr]
+  (sql.qp/adjust-start-of-week driver trunc-week expr))
 
 (defmethod sql.qp/date [:sqlserver :week-of-year-iso]
   [_ _ expr]

test/metabase/query_processor_test/date_bucketing_test.clj

@@ -724,6 +724,47 @@
     (is (= [[22 46] [23 47] [24 40] [25 60] [26 7]]
            (sad-toucan-incidents-with-bucketing :week-of-year :utc)))))
 
+
+(defn- fmt-str-or-int
+  [x]
+  (if (string? x)
+    (str x)
+    (int x)))
+
+(deftest week-of-year-and-week-count-should-be-consistent-test
+  (testing "consistent break out between weeks and week-of-year #4910"
+    (mt/test-drivers (mt/normal-drivers)
+      ;; 2019-01-01 is Tuesday, so set start-of-week to tuesday so
+      ;; breakout by week-of-year will have first row is the 1st week of year
+      (mt/with-temporary-setting-values [start-of-week :tuesday]
+        (mt/dataset sample-dataset
+          (letfn [(test-break-out [unit]
+                    (->> (mt/mbql-query orders
+                                        {:filter      [:between $created_at "2019-01-01" "2019-12-31"]
+                                         :breakout    [:field $created_at {:temporal-unit unit}]
+                                         :aggregation [[:count]]})
+                         mt/process-query
+                         (mt/formatted-rows [fmt-str-or-int int])))]
+            (testing "count result should be the same between week and week-of-year"
+              (is (= (map second (test-break-out :week))
+                     (map second (test-break-out :week-of-year))))
+              (is (= [127 124 136]
+                     (->> (test-break-out :week)
+                          (map second)
+                          (take 3)))))
+            (testing "make sure all drivers returns the same week column"
+              (is (= ["2019-01-01T00:00:00Z"
+                      "2019-01-08T00:00:00Z"
+                      "2019-01-15T00:00:00Z"]
+                     (->> (test-break-out :week)
+                          (map first)
+                          (take 3)))))
+            (testing "make sure all drivers returns the same week-of-year column"
+              (is (= [1 2 3]
+                     (->> (test-break-out :week-of-year)
+                          (map first)
+                          (take 3)))))))))))
+
 ;; All of the sad toucan events in the test data fit in June. The results are the same on all databases and the only
 ;; difference is how the beginning of hte month is represented, since we always return times with our dates
 (deftest group-by-month-test

test/metabase/query_processor_test/date_time_zone_functions_test.clj

@@ -163,12 +163,6 @@
         (testing title
           (is (= expected (test-temporal-extract query))))))))
 
-(defmacro with-start-of-week
-  "With start of week."
-  [start-of-week & body]
-  `(mt/with-temporary-setting-values [start-of-week ~start-of-week]
-     ~@body))
-
 (defn test-extract-week
   [field-id method]
   (->> (mt/mbql-query weeks {:expressions {"expr" [:get-week [:field field-id nil] method]}
@@ -188,7 +182,7 @@
           (is (= [52 52 1 1 1 1 1 1 1 53]
                  (test-extract-week (mt/id :weeks :d) :iso)))
           (testing "shouldn't change if start-of-week settings change"
-            (with-start-of-week :monday
+            (mt/with-temporary-setting-values [start-of-week :monday]
               (is (= [52 52 1 1 1 1 1 1 1 53]
                      (test-extract-week (mt/id :weeks :d) :iso)))))))
 
@@ -198,7 +192,7 @@
           (is (= [1 2 2 2 2 2 2 2 3 1]
                  (test-extract-week (mt/id :weeks :d) :us)))
           (testing "shouldn't change if start-of-week settings change"
-            (with-start-of-week :monday
+            (mt/with-temporary-setting-values [start-of-week :monday]
               (is (= [1 2 2 2 2 2 2 2 3 1]
                      (test-extract-week (mt/id :weeks :d) :us)))))))
 
@@ -206,7 +200,7 @@
         (is (= [1 2 2 2 2 2 2 2 3 1]
                (test-extract-week (mt/id :weeks :d) :instance)))
 
-        (with-start-of-week :monday
+        (mt/with-temporary-setting-values [start-of-week :monday]
           (is (= [1 1 2 2 2 2 2 2 2 1]
                  (test-extract-week (mt/id :weeks :d) :instance))))))))