Enable the :convert-timezone driver feature (#255)

* Enable `:convert-timezone` driver feature * Remove unnecessary report-tz arg from parseDateTimeBestEffort * Add `:convert-timezone` to CHANGELOG
ClickHouse · Jun 27, 2024 · c4b4b00 · c4b4b00
1 parent 9cc0399
commit c4b4b00
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 29 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,10 +3,11 @@
 ### New features
 
 * Enabled `:set-timezone` ([docs](https://www.metabase.com/docs/latest/configuring-metabase/localization#report-timezone)) Metabase feature in the driver. ([#200](https://github.com/ClickHouse/metabase-clickhouse-driver/issues/200))
+* Enabled `:convert-timezone` ([docs](https://www.metabase.com/docs/latest/questions/query-builder/expressions/converttimezone)) Metabase feature in the driver. ([#254](https://github.com/ClickHouse/metabase-clickhouse-driver/issues/254))
 
 ### Other
 
-* The driver now uses [`session_timezone` ClickHouse setting](https://clickhouse.com/docs/en/operations/settings/settings#session_timezone). This is necessary to support the `:set-timezone` feature; however, this setting [was introduced in 23.6](https://clickhouse.com/docs/en/whats-new/changelog/2023#236), which makes it the minimal required ClickHouse version to work with the driver.
+* The driver now uses [`session_timezone` ClickHouse setting](https://clickhouse.com/docs/en/operations/settings/settings#session_timezone). This is necessary to support the `:set-timezone` and `:convert-timezone` features; however, this setting [was introduced in 23.6](https://clickhouse.com/docs/en/whats-new/changelog/2023#236), which makes it the minimal required ClickHouse version to work with the driver.
 
 # 1.50.0
 

diff --git a/src/metabase/driver/clickhouse.clj b/src/metabase/driver/clickhouse.clj
@@ -39,7 +39,7 @@
                               :foreign-keys                    (not config/is-test?)
                               :now                             true
                               :set-timezone                    true
-                              :convert-timezone                false ;; https://github.com/ClickHouse/metabase-clickhouse-driver/issues/254
+                              :convert-timezone                true
                               :test/jvm-timezone-setting       false
                               :schemas                         true
                               :datetime-diff                   true
@@ -50,12 +50,6 @@
 (def ^:private default-connection-details
   {:user "default" :password "" :dbname "default" :host "localhost" :port "8123"})
 
-;; (defn- get-report-timezone-id-safely
-;;   []
-;;   (try
-;;     (qp.timezone/report-timezone-id-if-supported)
-;;     (catch Throwable _e nil)))
-
 (defn- connection-details->spec* [details]
   (let [;; ensure defaults merge on top of nils
         details (reduce-kv (fn [m k v] (assoc m k (or v (k default-connection-details))))

diff --git a/src/metabase/driver/clickhouse_qp.clj b/src/metabase/driver/clickhouse_qp.clj
@@ -8,10 +8,12 @@
             [metabase.driver.clickhouse-version :as clickhouse-version]
             [metabase.driver.sql-jdbc.execute :as sql-jdbc.execute]
             [metabase.driver.sql.query-processor :as sql.qp :refer [add-interval-honeysql-form]]
+            [metabase.driver.sql.util :as sql.u]
             [metabase.driver.sql.util.unprepare :as unprepare]
             [metabase.legacy-mbql.util :as mbql.u]
             [metabase.query-processor.timezone :as qp.timezone]
             [metabase.util :as u]
+            [metabase.util.date-2 :as u.date]
             [metabase.util.honey-sql-2 :as h2x]
             [metabase.util.log :as log])
   (:import [com.clickhouse.data.value ClickHouseArrayValue]
@@ -195,23 +197,19 @@
 ;;; HoneySQL forms
 ;;; ------------------------------------------------------------------------------------
 
-;; Commented out until we enable :convert-timezone feature - this implementation is still not correct
-;; There are several failing assertions in metabase.query-processor-test.date-time-zone-functions-test
-;; See also: https://github.com/ClickHouse/metabase-clickhouse-driver/issues/254
-#_(defmethod sql.qp/->honeysql [:clickhouse :convert-timezone]
-    [driver [_ arg target-timezone source-timezone]]
-    (let [expr          (sql.qp/->honeysql driver (cond-> arg (string? arg) u.date/parse))
-          with-tz-info? (h2x/is-of-type? expr #"(?:nullable\(|lowcardinality\()?(datetime64\(\d, {0,1}'.*|datetime\(.*)")
-          _             (sql.u/validate-convert-timezone-args with-tz-info? target-timezone source-timezone)
-          inner         (if (not with-tz-info?)
-                          [:'plus
-                           expr
-                           [:'toIntervalSecond
-                            [:'minus
-                             [:'timeZoneOffset [:'now target-timezone]]
-                             [:'timeZoneOffset [:'now source-timezone]]]]]
-                          [:'toTimeZone expr target-timezone])]
-      inner))
+(defmethod sql.qp/->honeysql [:clickhouse :convert-timezone]
+  [driver [_ arg target-timezone source-timezone]]
+  (let [expr          (sql.qp/->honeysql driver (cond-> arg (string? arg) u.date/parse))
+        with-tz-info? (h2x/is-of-type? expr #"(?:nullable\(|lowcardinality\()?(datetime64\(\d, {0,1}'.*|datetime\(.*)")
+        _             (sql.u/validate-convert-timezone-args with-tz-info? target-timezone source-timezone)]
+    (if (not with-tz-info?)
+      [:'plus
+       expr
+       [:'toIntervalSecond
+        [:'minus
+         [:'timeZoneOffset [:'toTimeZone expr target-timezone]]
+         [:'timeZoneOffset [:'toTimeZone expr source-timezone]]]]]
+      [:'toTimeZone expr target-timezone])))
 
 (defmethod sql.qp/current-datetime-honeysql-form :clickhouse
   [_]
@@ -227,11 +225,10 @@
 
 (defmethod sql.qp/->honeysql [:clickhouse LocalDateTime]
   [_ ^java.time.LocalDateTime t]
-  (let [formatted       (t/format "yyyy-MM-dd HH:mm:ss.SSS" t)
-        report-timezone (h2x/literal (or (get-report-timezone-id-safely) "UTC"))]
+  (let [formatted (t/format "yyyy-MM-dd HH:mm:ss.SSS" t)]
     (if (zero? (.getNano t))
-      [:'parseDateTimeBestEffort   formatted   report-timezone]
-      [:'parseDateTime64BestEffort formatted 3 report-timezone])))
+      [:'parseDateTimeBestEffort   formatted]
+      [:'parseDateTime64BestEffort formatted 3])))
 
 (defmethod sql.qp/->honeysql [:clickhouse ZonedDateTime]
   [_ ^java.time.ZonedDateTime t]