From 919d02ae027de3b8e466f455a795d3a4e65fcc82 Mon Sep 17 00:00:00 2001
From: Case Nelson <case@metabase.com>
Date: Wed, 8 Feb 2023 19:29:47 -0700
Subject: [PATCH] Adding support for timezone awareness to mongo (#27937)
* Adding support for timezone awareness to mongo
* Remove unused ddl multimethod
* Remove mongo from the sql driver type test
* Fix tests and linters
* Add test for filtering datetimes by date in timezone
* Use test-data-with-timezones dataset as it should load in more dbs
* Exclude broken drivers, vertica especially returns no rows
* Address pr review, add timezone to isodate, include a full year of dates in the general timezone test
* Assert against each row
* Presto-jdbc year was not applying timezone
---
.../mongo/src/metabase/driver/mongo.clj | 1 +
.../metabase/driver/mongo/query_processor.clj | 35 ++---
.../src/metabase/driver/presto_jdbc.clj | 4 +
src/metabase/driver/ddl/interface.clj | 7 -
src/metabase/driver/postgres.clj | 18 ---
.../query_processor_test/timezones_test.clj | 123 +++++++++++++++---
6 files changed, 127 insertions(+), 61 deletions(-)
diff --git a/modules/drivers/mongo/src/metabase/driver/mongo.clj b/modules/drivers/mongo/src/metabase/driver/mongo.clj
index 408e5039f34..f7c8c139f3b 100644
--- a/modules/drivers/mongo/src/metabase/driver/mongo.clj
+++ b/modules/drivers/mongo/src/metabase/driver/mongo.clj
@@ -233,6 +233,7 @@
:nested-fields
:nested-queries
:native-parameters
+ :set-timezone
:standard-deviation-aggregations]]
(defmethod driver/supports? [:mongo feature] [_driver _feature] true))
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 2ff36336128..e2ca69bc12f 100644
--- a/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj
+++ b/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj
@@ -210,7 +210,7 @@
(defn- day-of-week
[column]
- (mongo-let [day_of_week (add-start-of-week-offset {$dayOfWeek column}
+ (mongo-let [day_of_week (add-start-of-week-offset {$dayOfWeek {:date column :timezone (qp.timezone/results-timezone-id)}}
(driver.common/start-of-week-offset :mongo))]
{$cond {:if {$eq [day_of_week 0]}
:then 7
@@ -250,6 +250,10 @@
(days-till-start-of-first-full-week column))]
{:$toInt {:$add [1 {:$ceil {:$divide [{:$subtract [doy dtsofw]} 7]}}]}}))
+(defn- extract
+ [op column]
+ {op {:date column :timezone (qp.timezone/results-timezone-id)}})
+
(defn- with-rvalue-temporal-bucketing
[field unit]
(if (= unit :default)
@@ -267,15 +271,15 @@
(truncate-to-resolution column unit)))]
(case unit
:default column
- :second-of-minute {$second column}
+ :second-of-minute (extract $second column)
:minute (truncate :minute)
- :minute-of-hour {$minute column}
+ :minute-of-hour (extract $minute column)
:hour (truncate :hour)
- :hour-of-day {$hour column}
+ :hour-of-day (extract $hour column)
:day (truncate :day)
:day-of-week (day-of-week column)
- :day-of-month {$dayOfMonth column}
- :day-of-year {$dayOfYear column}
+ :day-of-month (extract $dayOfMonth column)
+ :day-of-year (extract $dayOfYear column)
:week (if supports-dateTrunc?
(truncate :week)
(truncate-to-resolution (week column) :day))
@@ -284,31 +288,32 @@
(week column))]
{:$ceil {$divide [{$dayOfYear week-start}
7.0]}})
- :week-of-year-iso {:$isoWeek column}
+ :week-of-year-iso (extract :$isoWeek column)
:week-of-year-us (week-of-year column :us)
:week-of-year-instance (week-of-year column :instance)
:month (truncate :month)
- :month-of-year {$month column}
+ :month-of-year (extract $month column)
;; For quarter we'll just subtract enough days from the current date to put it in the correct month and
;; stringify it as yyyy-MM Subtracting (($dayOfYear(column) % 91) - 3) days will put you in correct month.
;; Trust me.
:quarter
(if supports-dateTrunc?
(truncate :quarter)
- (mongo-let [#_{:clj-kondo/ignore [:unused-binding]} parts {:$dateToParts {:date column}}]
- {:$dateFromParts {:year :$$parts.year
- :month {$subtract [:$$parts.month
- {$mod [{$add [:$$parts.month 2]}
- 3]}]}}}))
+ (mongo-let [#_{:clj-kondo/ignore [:unused-binding]} parts {:$dateToParts {:date column :timezone (qp.timezone/results-timezone-id)}}]
+ {:$dateFromParts {:year :$$parts.year
+ :month {$subtract [:$$parts.month
+ {$mod [{$add [:$$parts.month 2]}
+ 3]}]}
+ :timezone (qp.timezone/results-timezone-id)}}))
:quarter-of-year
- {:$ceil {$divide [{$month column} 3.0]}}
+ {:$toInt {:$ceil {$divide [(extract $month column) 3.0]}}}
:year
(truncate :year)
:year-of-era
- {$year column})))))
+ (extract $year column))))))
(defmethod ->rvalue :field
[[_ id-or-name {:keys [temporal-unit] ::add/keys [source-alias]}]]
diff --git a/modules/drivers/presto-jdbc/src/metabase/driver/presto_jdbc.clj b/modules/drivers/presto-jdbc/src/metabase/driver/presto_jdbc.clj
index cd96878423a..c859b6e7d87 100644
--- a/modules/drivers/presto-jdbc/src/metabase/driver/presto_jdbc.clj
+++ b/modules/drivers/presto-jdbc/src/metabase/driver/presto_jdbc.clj
@@ -151,6 +151,10 @@
[_ _ expr]
(hx/call :date_trunc (hx/literal :year) (in-report-zone expr)))
+(defmethod sql.qp/date [:presto-jdbc :year-of-era]
+ [_ _ expr]
+ (hx/call :year (in-report-zone expr)))
+
(defmethod sql.qp/unix-timestamp->honeysql [:presto-jdbc :seconds]
[_ _ expr]
(let [report-zone (qp.timezone/report-timezone-id-if-supported :presto-jdbc)]
diff --git a/src/metabase/driver/ddl/interface.clj b/src/metabase/driver/ddl/interface.clj
index a419d1da3c2..c8b57994cb9 100644
--- a/src/metabase/driver/ddl/interface.clj
+++ b/src/metabase/driver/ddl/interface.clj
@@ -27,13 +27,6 @@
(defmethod format-name :default [_ table-or-field-name] table-or-field-name)
-(defmulti field-base-type->sql-type
- "A suitable db type for a base-type per database."
- {:arglists '([driver base-type])}
- (fn [driver base-type]
- [(driver/dispatch-on-initialized-driver driver) (keyword base-type)])
- :hierarchy #'driver/hierarchy)
-
(defmulti check-can-persist
"Verify that the source database is acceptable to persist. Returns a tuple of a boolean and `:persist.check/valid` in
the event it was successful or a keyword indicating the reason for failure.
diff --git a/src/metabase/driver/postgres.clj b/src/metabase/driver/postgres.clj
index a8333e240a7..a2271c629c2 100644
--- a/src/metabase/driver/postgres.clj
+++ b/src/metabase/driver/postgres.clj
@@ -11,7 +11,6 @@
[metabase.db.spec :as mdb.spec]
[metabase.driver :as driver]
[metabase.driver.common :as driver.common]
- [metabase.driver.ddl.interface :as ddl.i]
[metabase.driver.postgres.actions :as postgres.actions]
[metabase.driver.postgres.ddl :as postgres.ddl]
[metabase.driver.sql-jdbc.common :as sql-jdbc.common]
@@ -550,23 +549,6 @@
(keyword "timestamp with time zone") :type/DateTimeWithTZ
(keyword "timestamp without time zone") :type/DateTime})
-(doseq [[base-type db-type] {:type/BigInteger "BIGINT"
- :type/Boolean "BOOL"
- :type/Date "DATE"
- :type/DateTime "TIMESTAMP"
- :type/DateTimeWithTZ "TIMESTAMP WITH TIME ZONE"
- :type/DateTimeWithLocalTZ "TIMESTAMP WITH TIME ZONE"
- :type/Decimal "DECIMAL"
- :type/Float "FLOAT"
- :type/Integer "INTEGER"
- :type/IPAddress "INET"
- :type/Text "TEXT"
- :type/Time "TIME"
- :type/TimeWithTZ "TIME WITH TIME ZONE"
- :type/UUID "UUID"}]
- ;; todo: we get DB types in the metadata, let's persist these in model metadata
- (defmethod ddl.i/field-base-type->sql-type [:postgres base-type] [_ _] db-type))
-
(defmethod sql-jdbc.sync/database-type->base-type :postgres
[_driver column]
(if (contains? *enum-types* column)
diff --git a/test/metabase/query_processor_test/timezones_test.clj b/test/metabase/query_processor_test/timezones_test.clj
index 9a0a734b03f..f635aceabe9 100644
--- a/test/metabase/query_processor_test/timezones_test.clj
+++ b/test/metabase/query_processor_test/timezones_test.clj
@@ -1,6 +1,7 @@
(ns metabase.query-processor-test.timezones-test
(:require
[clojure.set :as set]
+ [clojure.string :as str]
[clojure.test :refer :all]
[java-time :as t]
[metabase.driver :as driver]
@@ -9,6 +10,7 @@
[metabase.query-processor :as qp]
[metabase.test :as mt]
[metabase.test.data.sql :as sql.tx]
+ [metabase.util.date-2 :as u.date]
[metabase.util.honeysql-extensions :as hx]
[toucan.db :as db]))
@@ -38,7 +40,7 @@
(defn timezone-aware-column-drivers
"Drivers that support the equivalent of `TIMESTAMP WITH TIME ZONE` columns."
[]
- (conj (set-timezone-drivers) :h2 :bigquery-cloud-sdk :sqlserver :mongo))
+ (conj (set-timezone-drivers) :h2 :bigquery-cloud-sdk :sqlserver))
;; TODO - we should also do similar tests for timezone-unaware columns
(deftest result-rows-test
@@ -164,26 +166,26 @@
:target ["dimension" ["template-tag" "just_a_date"]]
:value "2014-08-02"}]}})
-(deftest native-params-filter-test
+(deftest native-sql-params-filter-test
;; parameters always get `date` bucketing so doing something the between stuff we do below is basically just going
;; to match anything with a `2014-08-02` date
- (mt/test-drivers (set-timezone-drivers)
- (when (driver/supports? driver/*driver* :native-parameters)
- (mt/dataset test-data-with-timezones
- (mt/with-temporary-setting-values [report-timezone "America/Los_Angeles"]
- (testing "Native dates should be parsed with the report timezone"
- (doseq [[params-description query] (native-params-queries)]
- (testing (format "Query with %s" params-description)
- (is (= [[6 "Shad Ferdynand" "2014-08-02T05:30:00-07:00"]
- [7 "Conchúr Tihomir" "2014-08-02T02:30:00-07:00"]]
- (mt/formatted-rows [int identity identity]
- (qp/process-query
- (merge
- {:database (mt/id)
- :type :native}
- query)))))))))))))
-
-
+ (mt/test-drivers (filter
+ #(isa? driver/hierarchy % :sql)
+ (set/intersection (set-timezone-drivers)
+ (mt/normal-drivers-with-feature :native-parameters)))
+ (mt/dataset test-data-with-timezones
+ (mt/with-temporary-setting-values [report-timezone "America/Los_Angeles"]
+ (testing "Native dates should be parsed with the report timezone"
+ (doseq [[params-description query] (native-params-queries)]
+ (testing (format "Query with %s" params-description)
+ (is (= [[6 "Shad Ferdynand" "2014-08-02T05:30:00-07:00"]
+ [7 "Conchúr Tihomir" "2014-08-02T02:30:00-07:00"]]
+ (mt/formatted-rows [int identity identity]
+ (qp/process-query
+ (merge
+ {:database (mt/id)
+ :type :native}
+ query))))))))))))
;; Make sure TIME values are handled consistently (#10366)
(defn- attempts []
@@ -223,13 +225,92 @@
(when (supports-datetime-with-zone-id?)
{:datetime_tz_id (t/zoned-date-time "2019-11-01T00:23:18.331-07:00[America/Los_Angeles]")})))
-(deftest time-timezone-handling-test
+(deftest sql-time-timezone-handling-test
;; Actual value : "2019-11-01T00:23:18.331-07:00[America/Los_Angeles]"
;; Oracle doesn't have a time type
- (mt/test-drivers (set-timezone-drivers)
+ (mt/test-drivers (filter #(isa? driver/hierarchy % :sql) (set-timezone-drivers))
(mt/dataset attempted-murders
(doseq [timezone [nil "US/Pacific" "US/Eastern" "Asia/Hong_Kong"]]
(mt/with-temporary-setting-values [report-timezone timezone]
(let [expected (expected-attempts)
actual (select-keys (attempts) (keys expected))]
(is (= expected actual))))))))
+
+(mt/defdataset all-dates-leap-year
+ (let [start-date #t "2012-01-01T01:30:54Z"]
+ [["alldates" [{:field-name "dt"
+ :base-type :type/DateTimeWithTZ}]
+ (for [i (range 366)]
+ [(u.date/add start-date :day i)])]]))
+
+(deftest general-timezone-support-test
+ (mt/dataset all-dates-leap-year
+ (mt/test-drivers (set-timezone-drivers)
+ (let [extract-units (disj u.date/extract-units :day-of-year)
+ ;; :week-of-year-instance is the behavior of u.date/extract (based on public-settings start-of-week)
+ extract-translate {:year :year-of-era :week-of-year :week-of-year-us}
+ trunc-units (disj u.date/truncate-units :millisecond :second)]
+ (doseq [timezone ["Pacific/Honolulu" "America/Los_Angeles" "UTC" "Pacific/Auckland"]
+ :let [expected-rows (for [i (range 366)
+ :let [expected-datetime (u.date/add #t "2012-01-01T01:30:54Z" :day i)
+ in-tz (u.date/with-time-zone-same-instant expected-datetime timezone)]]
+ (concat
+ (for [extract-unit extract-units]
+ [extract-unit (u.date/extract in-tz extract-unit)])
+ (for [trunc-unit trunc-units]
+ [trunc-unit
+ (-> in-tz
+ (u.date/truncate trunc-unit)
+ u.date/format-sql
+ (str/replace #" " "T"))])
+ [[:dt_tz
+ (-> in-tz
+ u.date/format-sql
+ (str/replace #" " "T"))]]))]]
+ (mt/with-temporary-setting-values [report-timezone timezone]
+ (let [rows (->> (mt/run-mbql-query alldates
+ {:expressions (->> extract-units
+ (map
+ (fn [extract-unit]
+ [extract-unit [:temporal-extract
+ [:field (mt/id :alldates :dt) nil]
+ (get extract-translate extract-unit extract-unit)]]))
+ (into {}))
+ :fields (concat
+ (for [extract-unit extract-units]
+ [:expression extract-unit])
+ (for [trunc-unit trunc-units]
+ [:field (mt/id :alldates :dt)
+ {:temporal-unit trunc-unit}])
+ [[:field (mt/id :alldates :dt)]])
+ :order-by [[:asc (mt/id :alldates :id)]]})
+ (mt/rows)
+ (map (fn [row]
+ (map vector
+ (concat
+ (for [extract-unit extract-units]
+ extract-unit)
+ (for [trunc-unit trunc-units]
+ trunc-unit)
+ [:dt_tz])
+ row))))]
+ (doseq [[expected-row row] (map vector expected-rows rows)]
+ (is (= expected-row row))))))))))
+
+(deftest filter-datetime-by-date-in-timezone-test
+ (mt/test-drivers (set-timezone-drivers)
+ (mt/dataset test-data-with-timezones
+ (doseq [[timezone date-filter] [["US/Pacific" "2014-07-02"]
+ ["US/Eastern" "2014-07-02"]
+ ["UTC" "2014-07-03"]
+ ["Asia/Hong_Kong" "2014-07-03"]]
+ :let [expected (-> (u.date/with-time-zone-same-instant #t "2014-07-03T01:30:00Z" timezone)
+ (u.date/format-sql)
+ (str/replace #" " "T"))]]
+ (mt/with-temporary-setting-values [report-timezone timezone]
+ (is (= [expected]
+ (mt/first-row
+ (mt/run-mbql-query users
+ {:fields [$last_login]
+ :filter [:and [:= $id 12]
+ [:= $last_login date-filter]]})))))))))
--
GitLab