From dcc0b77998585c108672880434e6b93258e0f008 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Fri, 6 May 2022 09:43:21 -0700
Subject: [PATCH] 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 :cry:
* Fix MongoDB for Tuesday
---
.../metabase/driver/mongo/query_processor.clj | 12 ++++++---
src/metabase/driver.clj | 3 ++-
src/metabase/driver/common.clj | 27 ++++++++++++++-----
src/metabase/driver/postgres.clj | 10 +++++--
src/metabase/driver/sql/query_processor.clj | 24 ++++++++++++-----
.../date_bucketing_test.clj | 18 +++++++++++++
6 files changed, 75 insertions(+), 19 deletions(-)
diff --git a/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj b/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj
index 5278e18eb97..5aa4b8f21ac 100644
--- a/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj
+++ b/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}}))
diff --git a/src/metabase/driver.clj b/src/metabase/driver.clj
index c6d34f3927b..ba7ae9d66bf 100644
--- a/src/metabase/driver.clj
+++ b/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)
diff --git a/src/metabase/driver/common.clj b/src/metabase/driver/common.clj
index 793fffd7e77..75e19acc79f 100644
--- a/src/metabase/driver/common.clj
+++ b/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)))
diff --git a/src/metabase/driver/postgres.clj b/src/metabase/driver/postgres.clj
index d69cdb45485..6b2396db90c 100644
--- a/src/metabase/driver/postgres.clj
+++ b/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]
diff --git a/src/metabase/driver/sql/query_processor.clj b/src/metabase/driver/sql/query_processor.clj
index a6621bf20cc..10ce0ef31d2 100644
--- a/src/metabase/driver/sql/query_processor.clj
+++ b/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
diff --git a/test/metabase/query_processor_test/date_bucketing_test.clj b/test/metabase/query_processor_test/date_bucketing_test.clj
index 9c0cbf5283e..41d40987ecf 100644
--- a/test/metabase/query_processor_test/date_bucketing_test.clj
+++ b/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)
--
GitLab