convert-timezone expression for sqlserver, mysql, mariadb, oracle, vertica (#25790)

* convert-timezone for mysql, mariadb, sqlserver, oracle

* sqlserver doesn't understand Asia/Ho_Chi_Minh

* vertica

* skip report-tz tests for sqlserver

* use the util fn to check args for convert-timezone

* fix for vertica not formatting in report-tz 

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

@@ -19,6 +19,7 @@
             [metabase.driver.sql.util :as sql.u]
             [metabase.driver.sql.util.unprepare :as unprepare]
             [metabase.models.secret :as secret]
+            [metabase.query-processor.timezone :as qp.timezone]
             [metabase.util :as u]
             [metabase.util.honeysql-extensions :as hx]
             [metabase.util.i18n :refer [trs]]
@@ -31,6 +32,10 @@
 
 (driver/register! :oracle, :parent #{:sql-jdbc ::sql.qp.empty-string-is-null/empty-string-is-null})
 
+(defmethod driver/database-supports? [:oracle :convert-timezone]
+  [_driver _feat _db]
+  true)
+
 (def ^:private database-type->base-type
   (sql-jdbc.sync/pattern-based-database-type->base-type
    [;; Any types -- see http://docs.oracle.com/cd/B28359_01/server.111/b28286/sql_elements001.htm#i107578
@@ -60,6 +65,7 @@
     ;; Spatial types -- see http://docs.oracle.com/cd/B28359_01/server.111/b28286/sql_elements001.htm#i107588
     [#"^SDO_"       :type/*]
     [#"STRUCT"      :type/*]
+    [#"TIMESTAMP(\(\d\))? WITH TIME ZONE" :type/DateTimeWithTZ]
     [#"TIMESTAMP"   :type/DateTime]
     [#"URI"         :type/Text]
     [#"XML"         :type/*]]))
@@ -205,6 +211,17 @@
    (driver.common/start-of-week-offset driver)
    (partial hsql/call (u/qualified-name ::mod))))
 
+(defmethod sql.qp/->honeysql [:oracle :convert-timezone]
+  [driver [_ arg target-timezone source-timezone]]
+  (let [expr          (sql.qp/->honeysql driver arg)
+        has-timezone? (hx/is-of-type? expr #"timestamp(\(\d\))? with time zone")]
+   (sql.u/validate-convert-timezone-args has-timezone? target-timezone source-timezone)
+   (-> (if has-timezone?
+         expr
+         (hsql/call :from_tz expr (or source-timezone (qp.timezone/results-timezone-id))))
+       (hx/->AtTimeZone target-timezone)
+       hx/->timestamp)))
+
 (def ^:private now (hsql/raw "SYSDATE"))
 
 (defmethod sql.qp/current-datetime-honeysql-form :oracle [_] now)

modules/drivers/presto-jdbc/src/metabase/driver/presto_jdbc.clj

@@ -69,15 +69,6 @@
   ;; connection zone
   (hx/cast timestamp-with-time-zone-db-type (u.date/format-sql t)))
 
-(defrecord AtTimeZone
-  ;; record type to support applying Presto's `AT TIME ZONE` operator to an expression
-  [expr zone]
-  hformat/ToSql
-  (to-sql [_]
-    (format "%s AT TIME ZONE %s"
-      (hformat/to-sql expr)
-      (hformat/to-sql (hx/literal zone)))))
-
 (defn- in-report-zone
   "Returns a HoneySQL form to interpret the `expr` (a temporal value) in the current report time zone, via Presto's
   `AT TIME ZONE` operator. See https://prestodb.io/docs/current/functions/datetime.html"
@@ -92,7 +83,7 @@
              ;; if one has already been set, don't do so again
              (not (::in-report-zone? (meta expr)))
              report-zone)
-      (-> (hx/with-database-type-info (->AtTimeZone expr report-zone) timestamp-with-time-zone-db-type)
+      (-> (hx/with-database-type-info (hx/->AtTimeZone expr report-zone) timestamp-with-time-zone-db-type)
         (vary-meta assoc ::in-report-zone? true))
       expr)))
 

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

 (ns metabase.driver.sqlserver
   "Driver for SQLServer databases. Uses the official Microsoft JDBC driver under the hood (pre-0.25.0, used jTDS)."
-  (:require [clojure.tools.logging :as log]
+  (:require [clojure.data.xml :as xml]
+            [clojure.java.io :as io]
+            [clojure.string :as str]
+            [clojure.tools.logging :as log]
             [honeysql.core :as hsql]
             [honeysql.helpers :as hh]
             [java-time :as t]
@@ -13,6 +16,7 @@
             [metabase.driver.sql-jdbc.execute :as sql-jdbc.execute]
             [metabase.driver.sql-jdbc.sync :as sql-jdbc.sync]
             [metabase.driver.sql.query-processor :as sql.qp]
+            [metabase.driver.sql.util :as sql.u]
             [metabase.driver.sql.util.unprepare :as unprepare]
             [metabase.mbql.util :as mbql.u]
             [metabase.query-processor.interface :as qp.i]
@@ -30,6 +34,10 @@
 ;; users in the UI
 (defmethod driver/supports? [:sqlserver :case-sensitivity-string-filter-options] [_ _] false)
 
+(defmethod driver/database-supports? [:sqlserver :convert-timezone]
+  [_driver _feature _database]
+  true)
+
 (defmethod driver/db-start-of-week :sqlserver
   [_]
   :sunday)
@@ -231,6 +239,37 @@
   ;; Work around this by converting the timestamps to minutes instead before calling DATEADD().
   (date-add :minute (hx// expr 60) (hx/literal "1970-01-01")))
 
+(defonce
+  ^{:private true
+    :doc     "A map of all zone-id to the corresponding window-zone.
+             I.e {\"Asia/Tokyo\" \"Tokyo Standard Time\"}"}
+  zone-id->windows-zone
+  (let [data (-> (io/resource "timezones/windowsZones.xml")
+                 io/reader
+                 xml/parse
+                 :content
+                 second
+                 :content
+                 first
+                 :content)]
+    (->> (for [mapZone data
+               :let [attrs       (:attrs mapZone)
+                     window-zone (:other attrs)
+                     zone-ids    (str/split (:type attrs) #" ")]]
+           (zipmap zone-ids (repeat window-zone)))
+         (apply merge {"UTC" "UTC"}))))
+
+(defmethod sql.qp/->honeysql [:sqlserver :convert-timezone]
+  [driver [_ arg target-timezone source-timezone]]
+  (let [expr            (sql.qp/->honeysql driver arg)
+        datetimeoffset? (hx/is-of-type? expr "datetimeoffset")]
+    (sql.u/validate-convert-timezone-args datetimeoffset? target-timezone source-timezone)
+    (-> (if datetimeoffset?
+          expr
+          (hx/->AtTimeZone expr (zone-id->windows-zone source-timezone)))
+        (hx/->AtTimeZone (zone-id->windows-zone target-timezone))
+        hx/->datetime)))
+
 (defmethod sql.qp/cast-temporal-string [:sqlserver :Coercion/ISO8601->DateTime]
   [_driver _semantic_type expr]
   (hx/->datetime expr))

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

 (ns metabase.driver.vertica
   (:require [clojure.java.jdbc :as jdbc]
             [clojure.set :as set]
+            [clojure.string :as str]
             [clojure.tools.logging :as log]
             [honeysql.core :as hsql]
             [honeysql.format :as hformat]
@@ -13,10 +14,12 @@
             [metabase.driver.sql-jdbc.sync :as sql-jdbc.sync]
             [metabase.driver.sql.query-processor :as sql.qp]
             [metabase.driver.sql.query-processor.empty-string-is-null :as sql.qp.empty-string-is-null]
+            [metabase.driver.sql.util :as sql.u]
+            [metabase.query-processor.timezone :as qp.timezone]
             [metabase.util.date-2 :as u.date]
             [metabase.util.honeysql-extensions :as hx]
             [metabase.util.i18n :refer [trs]])
-  (:import [java.sql ResultSet Types]))
+  (:import [java.sql ResultSet ResultSetMetaData Types]))
 
 (driver/register! :vertica, :parent #{:sql-jdbc
                                       ::sql-jdbc.legacy/use-legacy-classes-for-read-and-set
@@ -24,6 +27,10 @@
 
 (defmethod driver/supports? [:vertica :percentile-aggregations] [_ _] false)
 
+(defmethod driver/database-supports? [:vertica :convert-timezone]
+  [_driver _feature _database]
+  true)
+
 (defmethod driver/db-start-of-week :vertica
   [_]
   :monday)
@@ -92,6 +99,7 @@
 (defmethod sql.qp/date [:vertica :quarter]         [_ _ expr] (date-trunc :quarter expr))
 (defmethod sql.qp/date [:vertica :quarter-of-year] [_ _ expr] (extract-integer :quarter expr))
 (defmethod sql.qp/date [:vertica :year]            [_ _ expr] (date-trunc :year expr))
+(defmethod sql.qp/date [:vertica :year-of-era]     [_ _ expr] (extract-integer :year expr))
 
 (defmethod sql.qp/date [:vertica :week]
   [_ _ expr]
@@ -103,6 +111,17 @@
   [_ _ expr]
   (sql.qp/adjust-day-of-week :vertica (hsql/call :dayofweek_iso expr)))
 
+(defmethod sql.qp/->honeysql [:vertica :convert-timezone]
+  [driver [_ arg target-timezone source-timezone]]
+  (let [expr         (sql.qp/->honeysql driver arg)
+        timestamptz? (hx/is-of-type? expr "timestamptz")]
+    (sql.u/validate-convert-timezone-args timestamptz? target-timezone source-timezone)
+    (-> (if timestamptz?
+          expr
+          (hx/->AtTimeZone expr (or source-timezone (qp.timezone/results-timezone-id))))
+        (hx/->AtTimeZone target-timezone)
+        (hx/with-database-type-info "timestamp"))))
+
 (defmethod sql.qp/->honeysql [:vertica :concat]
   [driver [_ & args]]
   (->> args
@@ -178,3 +197,13 @@
     (let [t (u.date/parse s)]
       (log/tracef "(.getString rs %d) [TIME_WITH_TIMEZONE] -> %s -> %s" i s t)
       t)))
+
+;; for some reason vertica `TIMESTAMP WITH TIME ZONE` columns still come back as `Type/TIMESTAMP`, which seems like a
+;; bug with the JDBC driver?
+(defmethod sql-jdbc.execute/read-column [:vertica Types/TIMESTAMP]
+  [_ _ ^ResultSet rs ^ResultSetMetaData rsmeta ^Integer i]
+  (when-let [s (.getString rs i)]
+    (let [has-timezone? (= (str/lower-case (.getColumnTypeName rsmeta i)) "timestamptz")
+          t             (u.date/parse s (when has-timezone? "UTC"))]
+      (log/tracef "(.getString rs %d) [TIME_WITH_TIMEZONE] -> %s -> %s" i s t)
+      t)))

src/metabase/driver/mysql.clj

@@ -19,6 +19,7 @@
             [metabase.driver.sql-jdbc.sync :as sql-jdbc.sync]
             [metabase.driver.sql-jdbc.sync.describe-table :as sql-jdbc.describe-table]
             [metabase.driver.sql.query-processor :as sql.qp]
+            [metabase.driver.sql.util :as sql.u]
             [metabase.driver.sql.util.unprepare :as unprepare]
             [metabase.models.field :as field]
             [metabase.query-processor.error-type :as qp.error-type]
@@ -53,6 +54,10 @@
   [_driver _feat db]
   (-> db :options :persist-models-enabled))
 
+(defmethod driver/database-supports? [:mysql :convert-timezone]
+  [_driver _feature _db]
+  true)
+
 (defmethod driver/database-supports? [:mysql :datetime-diff]
   [_driver _feature _db]
   true)
@@ -353,6 +358,15 @@
                                 2)
                           (hx/literal "-01"))))
 
+(defmethod sql.qp/->honeysql [:mysql :convert-timezone]
+  [driver [_ arg target-timezone source-timezone]]
+  (let [expr       (sql.qp/->honeysql driver arg)
+        timestamp? (hx/is-of-type? expr "timestamp")]
+    (sql.u/validate-convert-timezone-args timestamp? target-timezone source-timezone)
+    (hx/with-database-type-info
+      (hsql/call :convert_tz expr (or source-timezone (qp.timezone/results-timezone-id)) target-timezone)
+      "datetime")))
+
 (defmethod sql.qp/->honeysql [:mysql :datetime-diff]
   [driver [_ x y unit]]
   (let [x (sql.qp/->honeysql driver x)

src/metabase/util/honeysql_extensions.clj

@@ -161,6 +161,14 @@
 (alter-meta! #'->TypedHoneySQLForm assoc :private true)
 (alter-meta! #'map->TypedHoneySQLForm assoc :private true)
 
+(p.types/defrecord+ AtTimeZone
+  [expr zone]
+  hformat/ToSql
+  (to-sql [_]
+    (clojure.core/format "(%s AT TIME ZONE %s)"
+            (hformat/to-sql expr)
+            (hformat/to-sql (literal zone)))))
+
 (def ^:private NormalizedTypeInfo
   {(s/optional-key ::database-type) (s/constrained
                                      su/NonBlankString
@@ -208,11 +216,17 @@
 
 (defn is-of-type?
   "Is `honeysql-form` a typed form with `database-type`?
+  Where `database-type` could be a string or a regex.
+
+    (is-of-type? expr \"datetime\") ; -> true
+    (is-of-type? expr #\"int*\") ; -> true"
 
-    (is-of-type? expr \"datetime\") ; -> true"
   [honeysql-form database-type]
-  (= (some-> honeysql-form type-info type-info->db-type str/lower-case)
-     (some-> database-type name str/lower-case)))
+  (let [form-type (some-> honeysql-form type-info type-info->db-type str/lower-case)]
+    (if (instance? java.util.regex.Pattern database-type)
+      (some? (re-find database-type form-type))
+      (= form-type
+         (some-> database-type name str/lower-case)))))
 
 (s/defn with-database-type-info
   "Convenience for adding only database type information to a `honeysql-form`. Wraps `honeysql-form` and returns a

test/metabase/query_processor_test/date_time_zone_functions_test.clj

@@ -358,19 +358,13 @@
                                $times.dt
                                [:convert-timezone [:field (mt/id :times :dt) nil] "Asia/Tokyo"]))))))
 
-          (mt/with-report-timezone-id "Europe/Rome"
-            (testing "convert from UTC to Asia/Tokyo(+09:00)"
-              (is (= ["2004-03-19T09:19:09+01:00" "2004-03-19T18:19:09+09:00"]
-                     (mt/$ids (test-convert-tz
-                                $times.dt
-                                [:convert-timezone [:field (mt/id :times :dt) nil] "Asia/Tokyo" "UTC"])))))
-            (testing "source-timezone is required"
-             (is (thrown-with-msg?
-                   clojure.lang.ExceptionInfo
-                   #"input column doesnt have a set timezone. Please set the source parameter in convertTimezone to convert it."
-                   (mt/$ids (test-convert-tz
-                               $times.dt
-                               [:convert-timezone [:field (mt/id :times :dt) nil] "Asia/Tokyo"])))))))
+          (when (driver/supports? driver/*driver* :set-timezone)
+            (mt/with-report-timezone-id "Europe/Rome"
+              (testing "results should be displayed in the converted timezone, not report-tz"
+                (is (= ["2004-03-19T09:19:09+01:00" "2004-03-19T17:19:09+09:00"]
+                       (mt/$ids (test-convert-tz
+                                  $times.dt
+                                  [:convert-timezone [:field (mt/id :times :dt) nil] "Asia/Tokyo" "Europe/Rome"]))))))))
 
         (testing "timestamp with time zone columns"
           (mt/with-report-timezone-id "UTC"
@@ -379,6 +373,7 @@
                      (mt/$ids (test-convert-tz
                                 $times.dt_tz
                                 [:convert-timezone [:field (mt/id :times :dt_tz) nil] "Asia/Tokyo"])))))
+
             (testing "timestamp with time zone columns shouldn't have `source-timezone`"
               (is (thrown-with-msg?
                     clojure.lang.ExceptionInfo
@@ -389,12 +384,13 @@
                                 "Asia/Tokyo"
                                 "UTC"]))))))
 
-          (mt/with-report-timezone-id "Europe/Rome"
-            (testing "the base timezone should be the timezone of column (Asia/Ho_Chi_Minh)"
-              (is (= ["2004-03-19T03:19:09+01:00" "2004-03-19T11:19:09+09:00"]
-                     (mt/$ids (test-convert-tz
-                                $times.dt_tz
-                                [:convert-timezone [:field (mt/id :times :dt_tz) nil] "Asia/Tokyo"])))))))))))
+          (when (driver/supports? driver/*driver* :set-timezone)
+            (mt/with-report-timezone-id "Europe/Rome"
+              (testing "the base timezone should be the timezone of column (Asia/Ho_Chi_Minh)"
+                (is (= ["2004-03-19T03:19:09+01:00" "2004-03-19T11:19:09+09:00"]
+                       (mt/$ids (test-convert-tz
+                                  $times.dt_tz
+                                  [:convert-timezone [:field (mt/id :times :dt_tz) nil] "Asia/Tokyo"]))))))))))))
 
 (deftest nested-convert-timezone-test
   (mt/test-drivers (mt/normal-drivers-with-feature :convert-timezone)
@@ -412,7 +408,7 @@
                          :fields      [$times.dt
                                        [:expression "converted"]
                                        [:expression "hour"]]})
-                      mt/rows
+                      (mt/formatted-rows [str str int])
                       first))))
 
         (testing "convert-timezone nested with date-math, date-extract"
@@ -430,7 +426,7 @@
                                        [:expression "converted"]
                                        [:expression "date-added"]
                                        [:expression "hour"]]})
-                      mt/rows
+                      (mt/formatted-rows [str str str int])
                       first))))
 
         (testing "extract hour should respect daylight savings times"
@@ -444,7 +440,7 @@
                          :fields      [$times.dt
                                        [:expression "converted"]
                                        [:expression "hour"]]})
-                      mt/rows))))
+                      (mt/formatted-rows [str str int])))))
 
         (testing "convert-timezone twice should works"
           (is (= ["2004-03-19T09:19:09Z"      ;; original column
@@ -452,9 +448,9 @@
                   "2004-03-19T18:19:09+09:00"];; at +09
                  (->> (mt/run-mbql-query
                         times
-                        {:expressions {"to-07"       [:convert-timezone $times.dt "Asia/Ho_Chi_Minh" "UTC"]
+                        {:expressions {"to-07"       [:convert-timezone $times.dt "Asia/Saigon" "UTC"]
                                        "to-07-to-09" [:convert-timezone [:expression "to-07"] "Asia/Tokyo"
-                                                      "Asia/Ho_Chi_Minh"]}
+                                                      "Asia/Saigon"]}
                          :filter      [:= $times.index 1]
                          :fields      [$times.dt
                                        [:expression "to-07"]
@@ -488,9 +484,9 @@
                               {:dataset_query
                                (mt/mbql-query
                                  times
-                                 {:expressions {"to-07"       [:convert-timezone $times.dt "Asia/Ho_Chi_Minh" "UTC"]
+                                 {:expressions {"to-07"       [:convert-timezone $times.dt "Asia/Saigon" "UTC"]
                                                 "to-07-to-09" [:convert-timezone [:expression "to-07"] "Asia/Tokyo"
-                                                               "Asia/Ho_Chi_Minh"]}
+                                                               "Asia/Saigon"]}
                                   :filter      [:= $times.index 1]
                                   :fields      [$times.dt
                                                 [:expression "to-07"]
@@ -511,7 +507,10 @@
                 (is (= [["2004-03-19T09:19:09Z"
                          "2004-03-19T16:19:09Z"
                          "2004-03-19T18:19:09Z"]]
-                       (->> (mt/native-query {:query         (format "select * from {{%s}} as source" card-tag)
+                       (->> (mt/native-query {:query         (format "select * from {{%s}} %s" card-tag
+                                                                    (case driver/*driver*
+                                                                      :oracle ""
+                                                                      "as source"))
                                               :template-tags {card-tag {:card-id      (:id card)
                                                                         :type         :card
                                                                         :display-name "CARD ID"

test/metabase/util/honeysql_extensions_test.clj

@@ -184,15 +184,17 @@
 
 (deftest ^:parallel is-of-type?-test
   (are [expr tyype expected] (= expected (hx/is-of-type? expr tyype))
-    typed-form     "text" true
-    typed-form     "TEXT" true
-    typed-form     :text  true
-    typed-form     :TEXT  true
-    typed-form     :te/xt false
-    typed-form     "date" false
-    typed-form     nil    false
-    nil            "date" false
-    :%current_date "date" false
+    typed-form     "text"   true
+    typed-form     "TEXT"   true
+    typed-form     :text    true
+    typed-form     :TEXT    true
+    typed-form     :te/xt   false
+    typed-form     "date"   false
+    typed-form     nil      false
+    typed-form     #"tex.*" true
+    typed-form     #"int*"  false
+    nil            "date"   false
+    :%current_date "date"   false
     ;; I guess this behavior makes sense? I guess untyped = "is of type nil"
     nil            nil    true
     :%current_date nil    true))