Enable the :set-timezone driver feature; allow to change the report…

…ing timezone in the Localization settings (#253) * enable timezones in the driver * Rework :datetime-diff to support timezones * Add CHANGELOG, bump MB version in GHA * Cleanup, disable convert-timezone again, update CHANGELOG * Add a README entry about the supported ClickHouse versions (#251) * Update README * Add "now" override * Update the "now" HoneySQL, update README * Fix the introspection keyword/lowercase str issues * Do not call toTimeZone if it's equal the report tz already * toDate -> toStartOfDay * Fix an incorrect :day assertion
ClickHouse · Jun 27, 2024 · 9cc0399 · 9cc0399
1 parent 570f0cc
commit 9cc0399
Show file tree

Hide file tree

Showing 10 changed files with 360 additions and 157 deletions.
diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml
@@ -10,7 +10,7 @@ on:
   pull_request:
 
 env:
-  METABASE_VERSION: v0.50.3
+  METABASE_VERSION: v0.50.6
 
 jobs:
   check-local-current-version:

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,13 @@
+# 1.50.1
+
+### 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))
+
+### 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.
+
 # 1.50.0
 
 After Metabase 0.50.0, a new naming convention exists for the driver's releases. The new one is intended to reflect the Metabase version the driver is supposed to run on. For example, the driver version 1.50.0 means that it should be used with Metabase v0.50.x or Metabase EE 1.50.x _only_, and it is _not guaranteed_ that this particular version of the driver can work with the previous or the following versions of Metabase.

diff --git a/README.md b/README.md
@@ -24,10 +24,23 @@
 
 The driver aims to support the current stable and LTS releases (see [the related docs](https://clickhouse.com/docs/en/faq/operations/production#how-to-choose-between-clickhouse-releases)).
 
+After 1.50.1:
+
+| ClickHouse version      | Supported?  |
+|-------------------------|-------------|
+| 23.8+                   | ✔           |
+| 23.6 - 23.7             | Best effort |
+
+1.50.0 and earlier:
+
 | ClickHouse version      | Supported?  |
 |-------------------------|-------------|
 | 23.8+                   | ✔           |
-| 23.3-23.7               | Best effort |
+| 23.3 - 23.7             | Best effort |
+
+For [connection impersonation feature](https://www.metabase.com/learn/permissions/impersonation), the minimal required ClickHouse version is 24.4; otherwise, the feature is disabled by the driver.
+
+The [CSV Uploads feature](https://www.metabase.com/docs/latest/databases/uploads) currently works only with ClickHouse Cloud (see [this issue](https://github.com/ClickHouse/metabase-clickhouse-driver/issues/230) for more details).
 
 ## Installation
 

diff --git a/src/metabase/driver/clickhouse.clj b/src/metabase/driver/clickhouse.clj
@@ -17,9 +17,12 @@
             [metabase.driver.sql-jdbc.execute :as sql-jdbc.execute]
             [metabase.driver.sql.query-processor :as sql.qp]
             [metabase.driver.sql.util :as sql.u]
+            [metabase.lib.metadata :as lib.metadata]
+            [metabase.query-processor.store :as qp.store]
             [metabase.upload :as upload]
+            [metabase.util :as u]
             [metabase.util.log :as log])
-  (:import [com.clickhouse.jdbc.internal ClickHouseStatementImpl]))
+  (:import  [com.clickhouse.jdbc.internal ClickHouseStatementImpl]))
 
 (set! *warn-on-reflection* true)
 
@@ -34,19 +37,25 @@
 
 (doseq [[feature supported?] {:standard-deviation-aggregations true
                               :foreign-keys                    (not config/is-test?)
-                              :set-timezone                    false
-                              :convert-timezone                false
+                              :now                             true
+                              :set-timezone                    true
+                              :convert-timezone                false ;; https://github.com/ClickHouse/metabase-clickhouse-driver/issues/254
                               :test/jvm-timezone-setting       false
                               :schemas                         true
                               :datetime-diff                   true
                               :upload-with-auto-pk             false
                               :window-functions/offset         false}]
-
   (defmethod driver/database-supports? [:clickhouse feature] [_driver _feature _db] supported?))
 
 (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))))
@@ -76,6 +85,50 @@
       :custom_http_params "allow_experimental_analyzer=0"}
      (sql-jdbc.common/handle-additional-options details :separator-style :url))))
 
+(defmethod sql-jdbc.execute/do-with-connection-with-options :clickhouse
+  [driver db-or-id-or-spec {:keys [^String session-timezone write?] :as options} f]
+  (sql-jdbc.execute/do-with-resolved-connection
+   driver
+   db-or-id-or-spec
+   options
+   (fn [^java.sql.Connection conn]
+     (when-not (sql-jdbc.execute/recursive-connection?)
+       (let [settings (if session-timezone
+                        (format "allow_experimental_analyzer=0,session_timezone=%s" session-timezone)
+                        "allow_experimental_analyzer=0")]
+         (.setClientInfo conn com.clickhouse.jdbc.ClickHouseConnection/PROP_CUSTOM_HTTP_PARAMS settings))
+
+       (sql-jdbc.execute/set-best-transaction-level! driver conn)
+       (sql-jdbc.execute/set-time-zone-if-supported! driver conn session-timezone)
+       (when-let [db (cond
+                       ;; id?
+                       (integer? db-or-id-or-spec)
+                       (qp.store/with-metadata-provider db-or-id-or-spec
+                         (lib.metadata/database (qp.store/metadata-provider)))
+                       ;; db?
+                       (u/id db-or-id-or-spec)     db-or-id-or-spec
+                       ;; otherwise it's a spec and we can't get the db
+                       :else nil)]
+         (sql-jdbc.execute/set-role-if-supported! driver conn db))
+       (let [read-only? (not write?)]
+         (try
+           (log/trace (pr-str (list '.setReadOnly 'conn read-only?)))
+           (.setReadOnly conn read-only?)
+           (catch Throwable e
+             (log/debugf e "Error setting connection readOnly to %s" (pr-str read-only?)))))
+       (when-not write?
+         (try
+           (log/trace (pr-str '(.setAutoCommit conn true)))
+           (.setAutoCommit conn true)
+           (catch Throwable e
+             (log/debug e "Error enabling connection autoCommit"))))
+       (try
+         (log/trace (pr-str '(.setHoldability conn java.sql.ResultSet/CLOSE_CURSORS_AT_COMMIT)))
+         (.setHoldability conn java.sql.ResultSet/CLOSE_CURSORS_AT_COMMIT)
+         (catch Throwable e
+           (log/debug e "Error setting default holdability for connection"))))
+     (f conn))))
+
 (def ^:private ^{:arglists '([db-details])} cloud?
   "Returns true if the `db-details` are for a ClickHouse Cloud instance, and false otherwise. If it fails to connect
    to the database, it throws a java.sql.SQLException."
@@ -158,8 +211,7 @@
     ::upload/boolean                  "Nullable(Boolean)"
     ::upload/date                     "Nullable(Date32)"
     ::upload/datetime                 "Nullable(DateTime64(3))"
-    ;; FIXME: should be `Nullable(DateTime64(3))`
-    ::upload/offset-datetime          nil))
+    ::upload/offset-datetime          "Nullable(DateTime64(3))"))
 
 (defmethod driver/table-name-length-limit :clickhouse
   [_driver]

diff --git a/src/metabase/driver/clickhouse_introspection.clj b/src/metabase/driver/clickhouse_introspection.clj
@@ -12,59 +12,59 @@
 
 (def ^:private database-type->base-type
   (sql-jdbc.sync/pattern-based-database-type->base-type
-   [[#"Array"       :type/Array]
-    [#"Bool"        :type/Boolean]
-    [#"DateTime64"  :type/DateTime]
-    [#"DateTime"    :type/DateTime]
-    [#"Date"        :type/Date]
-    [#"Date32"      :type/Date]
-    [#"Decimal"     :type/Decimal]
-    [#"Enum8"       :type/Text]
-    [#"Enum16"      :type/Text]
-    [#"FixedString" :type/TextLike]
-    [#"Float32"     :type/Float]
-    [#"Float64"     :type/Float]
-    [#"Int8"        :type/Integer]
-    [#"Int16"       :type/Integer]
-    [#"Int32"       :type/Integer]
-    [#"Int64"       :type/BigInteger]
-    [#"IPv4"        :type/IPAddress]
-    [#"IPv6"        :type/IPAddress]
-    [#"Map"         :type/Dictionary]
-    [#"String"      :type/Text]
-    [#"Tuple"       :type/*]
-    [#"UInt8"       :type/Integer]
-    [#"UInt16"      :type/Integer]
-    [#"UInt32"      :type/Integer]
-    [#"UInt64"      :type/BigInteger]
-    [#"UUID"        :type/UUID]]))
+   [[#"array"       :type/Array]
+    [#"bool"        :type/Boolean]
+    [#"datetime64"  :type/DateTime]
+    [#"datetime"    :type/DateTime]
+    [#"date"        :type/Date]
+    [#"date32"      :type/Date]
+    [#"decimal"     :type/Decimal]
+    [#"enum8"       :type/Text]
+    [#"enum16"      :type/Text]
+    [#"fixedstring" :type/TextLike]
+    [#"float32"     :type/Float]
+    [#"float64"     :type/Float]
+    [#"int8"        :type/Integer]
+    [#"int16"       :type/Integer]
+    [#"int32"       :type/Integer]
+    [#"int64"       :type/BigInteger]
+    [#"ipv4"        :type/IPAddress]
+    [#"ipv6"        :type/IPAddress]
+    [#"map"         :type/Dictionary]
+    [#"string"      :type/Text]
+    [#"tuple"       :type/*]
+    [#"uint8"       :type/Integer]
+    [#"uint16"      :type/Integer]
+    [#"uint32"      :type/Integer]
+    [#"uint64"      :type/BigInteger]
+    [#"uuid"        :type/UUID]]))
 
 (defn- normalize-db-type
   [db-type]
   (cond
     ;; LowCardinality
-    (str/starts-with? db-type "LowCardinality")
+    (str/starts-with? db-type "lowcardinality")
     (normalize-db-type (subs db-type 15 (- (count db-type) 1)))
     ;; Nullable
-    (str/starts-with? db-type "Nullable")
+    (str/starts-with? db-type "nullable")
     (normalize-db-type (subs db-type 9 (- (count db-type) 1)))
     ;; DateTime64
-    (str/starts-with? db-type "DateTime64")
-    :type/DateTime ;; FIXME: should be type/DateTimeWithTZ (#200)
+    (str/starts-with? db-type "datetime64")
+    (if (> (count db-type) 13) :type/DateTimeWithTZ :type/DateTime)
     ;; DateTime
-    (str/starts-with? db-type "DateTime")
-    :type/DateTime ;; FIXME: should be type/DateTimeWithTZ (#200)
+    (str/starts-with? db-type "datetime")
+    (if (> (count db-type) 8) :type/DateTimeWithTZ :type/DateTime)
     ;; Enum*
-    (str/starts-with? db-type "Enum")
+    (str/starts-with? db-type "enum")
     :type/Text
     ;; Map
-    (str/starts-with? db-type "Map")
+    (str/starts-with? db-type "map")
     :type/Dictionary
     ;; Tuple
-    (str/starts-with? db-type "Tuple")
+    (str/starts-with? db-type "tuple")
     :type/*
     ;; SimpleAggregateFunction
-    (str/starts-with? db-type "SimpleAggregateFunction")
+    (str/starts-with? db-type "simpleaggregatefunction")
     (normalize-db-type (subs db-type (+ (str/index-of db-type ",") 2) (- (count db-type) 1)))
     ;; _
     :else (or (database-type->base-type (keyword db-type)) :type/*)))
@@ -73,7 +73,10 @@
 ;; Nullable(DateTime) -> :type/DateTime, SimpleAggregateFunction(sum, Int64) -> :type/BigInteger, etc
 (defmethod sql-jdbc.sync/database-type->base-type :clickhouse
   [_ database-type]
-  (normalize-db-type (subs (str database-type) 1)))
+  (let [db-type (if (keyword? database-type)
+                  (subs (str database-type) 1)
+                  database-type)]
+    (normalize-db-type (u/lower-case-en db-type))))
 
 (defmethod sql-jdbc.sync/excluded-schemas :clickhouse [_]
   #{"system" "information_schema" "INFORMATION_SCHEMA"})