From c4b4b008ca7daaaf21f1f3f45bd301a8d899967d Mon Sep 17 00:00:00 2001 From: Serge Klochkov <3175289+slvrtrn@users.noreply.github.com> Date: Thu, 27 Jun 2024 15:52:06 +0200 Subject: [PATCH] Enable the `:convert-timezone` driver feature (#255) * Enable `:convert-timezone` driver feature * Remove unnecessary report-tz arg from parseDateTimeBestEffort * Add `:convert-timezone` to CHANGELOG --- CHANGELOG.md | 3 ++- src/metabase/driver/clickhouse.clj | 8 +----- src/metabase/driver/clickhouse_qp.clj | 39 +++++++++++++-------------- 3 files changed, 21 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b12efd..629a6c0 100644 --- 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 index 20be397..22d03ed 100644 --- 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 index f9cd1a7..7875cdb 100644 --- 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]