diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index 34ace6d..af6fe96 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -10,7 +10,9 @@ on: pull_request: env: - METABASE_VERSION: v1.51.1.2 + # Temporarily using a fork to disable a few failing tests + METABASE_REPOSITORY: slvrtrn/metabase + METABASE_VERSION: 0.52.x-ch jobs: check-local-current-version: @@ -19,7 +21,7 @@ jobs: - name: Checkout Metabase Repo uses: actions/checkout@v4 with: - repository: metabase/metabase + repository: ${{ env.METABASE_REPOSITORY }} ref: ${{ env.METABASE_VERSION }} - name: Remove incompatible tests @@ -34,11 +36,11 @@ jobs: with: path: modules/drivers/clickhouse - - name: Prepare JDK 17 + - name: Prepare JDK 21 uses: actions/setup-java@v4 with: distribution: "temurin" - java-version: "17" + java-version: "21" - name: Add ClickHouse TLS instance to /etc/hosts run: | @@ -68,13 +70,13 @@ jobs: node-version: "20" cache: "yarn" - # - name: Get M2 cache - # uses: actions/cache@v4 - # with: - # path: | - # ~/.m2 - # ~/.gitlibs - # key: ${{ runner.os }}-clickhouse-${{ hashFiles('**/deps.edn') }} + - name: Get M2 cache + uses: actions/cache@v4 + with: + path: | + ~/.m2 + ~/.gitlibs + key: ${{ runner.os }}-clickhouse-${{ hashFiles('**/deps.edn') }} - name: Prepare stuff for pulses run: yarn build-static-viz @@ -97,7 +99,7 @@ jobs: - name: Checkout Metabase Repo uses: actions/checkout@v4 with: - repository: metabase/metabase + repository: ${{ env.METABASE_REPOSITORY }} ref: ${{ env.METABASE_VERSION }} - name: Checkout Driver Repo @@ -105,11 +107,11 @@ jobs: with: path: modules/drivers/clickhouse - - name: Prepare JDK 17 + - name: Prepare JDK 21 uses: actions/setup-java@v4 with: distribution: "temurin" - java-version: "17" + java-version: "21" - name: Add ClickHouse TLS instance to /etc/hosts run: | @@ -135,13 +137,13 @@ jobs: curl -O https://download.clojure.org/install/linux-install-1.11.1.1182.sh && sudo bash ./linux-install-1.11.1.1182.sh - # - name: Get M2 cache - # uses: actions/cache@v4 - # with: - # path: | - # ~/.m2 - # ~/.gitlibs - # key: ${{ runner.os }}-clickhouse-${{ hashFiles('**/deps.edn') }} + - name: Get M2 cache + uses: actions/cache@v4 + with: + path: | + ~/.m2 + ~/.gitlibs + key: ${{ runner.os }}-clickhouse-${{ hashFiles('**/deps.edn') }} # Use custom deps.edn containing "user/clickhouse" alias to include driver sources - name: Prepare deps.edn @@ -163,7 +165,7 @@ jobs: - name: Checkout Metabase Repo uses: actions/checkout@v4 with: - repository: metabase/metabase + repository: ${{ env.METABASE_REPOSITORY }} ref: ${{ env.METABASE_VERSION }} - name: Checkout Driver Repo diff --git a/deps.edn b/deps.edn index b7006f6..1b9a0ee 100644 --- a/deps.edn +++ b/deps.edn @@ -1,9 +1,6 @@ -{:paths - ["src" "resources"] - +{:paths ["src" "resources"] :deps - {com.clickhouse/clickhouse-jdbc$http - {:mvn/version "0.6.1" - :exclusions [com.clickhouse/clickhouse-cli-client$shaded - com.clickhouse/clickhouse-grpc-client$shaded]} - com.widdindustries/cljc.java-time {:mvn/version "0.1.21"}}} + { + com.widdindustries/cljc.java-time {:mvn/version "0.1.21"} + com.clickhouse/clickhouse-jdbc {:mvn/version "0.7.2"} + org.lz4/lz4-java {:mvn/version "1.8.0"}}} diff --git a/docker-compose.yml b/docker-compose.yml index d4f6699..9dc75c7 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -115,7 +115,7 @@ services: ########################################################################################################## metabase: - image: metabase/metabase-enterprise:v1.51.1.2 + image: metabase/metabase-enterprise:v1.52.2.5 container_name: metabase-with-clickhouse-driver hostname: metabase environment: diff --git a/resources/metabase-plugin.yaml b/resources/metabase-plugin.yaml index e44b3f1..7e426d5 100644 --- a/resources/metabase-plugin.yaml +++ b/resources/metabase-plugin.yaml @@ -41,6 +41,11 @@ driver: placeholder: "allow_experimental_analyzer=1,max_result_rows=100" visible-if: advanced-options: true + - name: max-open-connections + display-name: "Max open HTTP connections in the JDBC driver (default: 100)" + placeholder: 100 + visible-if: + advanced-options: true - merge: - additional-options - placeholder: "connection_timeout=1000&socket_timeout=300000" diff --git a/src/metabase/driver/clickhouse.clj b/src/metabase/driver/clickhouse.clj index 69c6cf9..82f40de 100644 --- a/src/metabase/driver/clickhouse.clj +++ b/src/metabase/driver/clickhouse.clj @@ -21,10 +21,11 @@ [metabase.upload :as upload] [metabase.util :as u] [metabase.util.log :as log]) - (:import [com.clickhouse.jdbc.internal ClickHouseStatementImpl])) + (:import [com.clickhouse.client.api.query QuerySettings])) (set! *warn-on-reflection* true) +(System/setProperty "clickhouse.jdbc.v2" "true") (driver/register! :clickhouse :parent #{:sql-jdbc}) (defmethod driver/display-name :clickhouse [_] "ClickHouse") @@ -37,8 +38,9 @@ (doseq [[feature supported?] {:standard-deviation-aggregations true :now true :set-timezone true - :convert-timezone true + :convert-timezone false :test/jvm-timezone-setting false + :test/date-time-type false :test/time-type false :schemas true :datetime-diff true @@ -47,7 +49,8 @@ :window-functions/cumulative (not config/is-test?) :left-join (not config/is-test?) :describe-fks false - :metadata/key-constraints false}] + :actions false + :metadata/key-constraints (not config/is-test?)}] (defmethod driver/database-supports? [:clickhouse feature] [_driver _feature _db] supported?)) (def ^:private default-connection-details @@ -58,7 +61,7 @@ details (reduce-kv (fn [m k v] (assoc m k (or v (k default-connection-details)))) default-connection-details details) - {:keys [user password dbname host port ssl use-no-proxy clickhouse-settings]} details + {:keys [user password dbname host port ssl clickhouse-settings max-open-connections]} details ;; if multiple databases were specified for the connection, ;; use only the first dbname as the "main" one dbname (first (str/split (str/trim dbname) #" "))] @@ -69,21 +72,19 @@ :password (or password "") :user user :ssl (boolean ssl) - :use_no_proxy (boolean use-no-proxy) :use_server_time_zone_for_dates true :product_name product-name - ;; addresses breaking changes from the 0.5.0 JDBC driver release - ;; see https://github.com/ClickHouse/clickhouse-java/releases/tag/v0.5.0 - ;; and https://github.com/ClickHouse/clickhouse-java/issues/1634#issuecomment-2110392634 - :databaseTerm "schema" :remember_last_set_roles true :http_connection_provider "HTTP_URL_CONNECTION" + :jdbc_ignore_unsupported_values "true" + :jdbc_schema_term "schema" + :max_open_connections (or max-open-connections 100) ;; see also: https://clickhouse.com/docs/en/integrations/java#configuration :custom_http_params (or clickhouse-settings "")} (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] + [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 @@ -91,9 +92,10 @@ (fn [^java.sql.Connection conn] (when-not (sql-jdbc.execute/recursive-connection?) (when session-timezone - (.setClientInfo conn com.clickhouse.jdbc.ClickHouseConnection/PROP_CUSTOM_HTTP_PARAMS - (format "session_timezone=%s" session-timezone))) - + (let [^com.clickhouse.jdbc.ConnectionImpl clickhouse-conn (.unwrap conn com.clickhouse.jdbc.ConnectionImpl) + query-settings (new QuerySettings)] + (.setOption query-settings "session_timezone" session-timezone) + (.setDefaultQuerySettings clickhouse-conn query-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 @@ -105,24 +107,7 @@ (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")))) + (sql-jdbc.execute/set-role-if-supported! driver conn db))) (f conn)))) (def ^:private ^{:arglists '([db-details])} cloud? @@ -134,8 +119,8 @@ (sql-jdbc.execute/do-with-connection-with-options :clickhouse spec nil (fn [^java.sql.Connection conn] - (with-open [stmt (.prepareStatement conn "SELECT value='1' FROM system.settings WHERE name='cloud_mode'") - rset (.executeQuery stmt)] + (with-open [stmt (.createStatement conn) + rset (.executeQuery stmt "SELECT value='1' FROM system.settings WHERE name='cloud_mode'")] (if (.next rset) (.getBoolean rset 1) false)))))) ;; cache the results for 48 hours; TTL is here only to eventually clear out old entries :ttl/threshold (* 48 60 60 1000))) @@ -184,8 +169,8 @@ (sql-jdbc.execute/do-with-connection-with-options driver database nil (fn [^java.sql.Connection conn] - (with-open [stmt (.prepareStatement conn "SELECT timezone() AS tz") - rset (.executeQuery stmt)] + (with-open [stmt (.createStatement conn) + rset (.executeQuery stmt "SELECT timezone() AS tz")] (when (.next rset) (.getString rset 1)))))) @@ -239,12 +224,7 @@ {:write? true} (fn [^java.sql.Connection conn] (with-open [stmt (.createStatement conn)] - (let [^ClickHouseStatementImpl stmt (.unwrap stmt ClickHouseStatementImpl) - request (.getRequest stmt)] - (.set request "wait_end_of_query" "1") - (with-open [_response (-> request - (.query ^String (create-table!-sql driver table-name column-definitions :primary-key primary-key)) - (.executeAndWait))])))))) + (.execute stmt (create-table!-sql driver table-name column-definitions :primary-key primary-key)))))) (defmethod driver/insert-into! :clickhouse [driver db-id table-name column-names values] @@ -290,10 +270,11 @@ (if (or (re-matches #"\".*\"" r) (= role default-role)) r (format "\"%s\"" r))) - quoted-role (->> (clojure.string/split role #",") + quoted-role (->> (str/split role #",") (map quote-if-needed) - (clojure.string/join ","))] - (format "SET ROLE %s;" quoted-role))) + (str/join ",")) + statement (format "SET ROLE %s" quoted-role)] + statement)) (defmethod driver.sql/default-database-role :clickhouse [_ _] diff --git a/src/metabase/driver/clickhouse_introspection.clj b/src/metabase/driver/clickhouse_introspection.clj index 888a2e4..2086ba4 100644 --- a/src/metabase/driver/clickhouse_introspection.clj +++ b/src/metabase/driver/clickhouse_introspection.clj @@ -1,10 +1,12 @@ (ns metabase.driver.clickhouse-introspection (:require [clojure.java.jdbc :as jdbc] [clojure.string :as str] + [metabase.config :as config] [metabase.driver :as driver] [metabase.driver.ddl.interface :as ddl.i] [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn] [metabase.driver.sql-jdbc.sync :as sql-jdbc.sync] + [metabase.driver.sql-jdbc.sync.describe-table :as sql-jdbc.describe-table] [metabase.util :as u]) (:import (java.sql DatabaseMetaData))) @@ -14,8 +16,6 @@ (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] @@ -48,12 +48,16 @@ ;; Nullable (str/starts-with? db-type "nullable") (normalize-db-type (subs db-type 9 (- (count db-type) 1))) + ;; for test purposes only: GMT0 is a legacy timezone; + ;; it maps to LocalDateTime instead of OffsetDateTime + (= db-type "datetime64(3, 'gmt0')") + :type/DateTime ;; DateTime64 (str/starts-with? db-type "datetime64") - (if (> (count db-type) 13) :type/DateTimeWithTZ :type/DateTime) + :type/DateTimeWithLocalTZ ;; DateTime (str/starts-with? db-type "datetime") - (if (> (count db-type) 8) :type/DateTimeWithTZ :type/DateTime) + :type/DateTimeWithLocalTZ ;; Enum* (str/starts-with? db-type "enum") :type/Text @@ -167,3 +171,11 @@ (get field :database-type)))] updated-field)] (merge table-metadata {:fields (set filtered-fields)}))) + +(defmethod sql-jdbc.describe-table/get-table-pks :clickhouse + [_driver ^java.sql.Connection conn db-name-or-nil table] + ;; JDBC v2 sets the PKs now, so that :metadata/key-constraints feature should be enabled; + ;; however, enabling :metadata/key-constraints will also enable left-join tests which are currently failing + (if (not config/is-test?) + (sql-jdbc.describe-table/get-table-pks :sql-jdbc conn db-name-or-nil table) + [])) diff --git a/src/metabase/driver/clickhouse_qp.clj b/src/metabase/driver/clickhouse_qp.clj index 7599b43..2cceaec 100644 --- a/src/metabase/driver/clickhouse_qp.clj +++ b/src/metabase/driver/clickhouse_qp.clj @@ -4,6 +4,7 @@ (:require [clojure.string :as str] [honey.sql :as sql] [java-time.api :as t] + [metabase.driver :as driver] [metabase.driver.clickhouse-nippy] [metabase.driver.clickhouse-version :as clickhouse-version] [metabase.driver.sql-jdbc.execute :as sql-jdbc.execute] @@ -15,8 +16,7 @@ [metabase.util :as u] [metabase.util.date-2 :as u.date] [metabase.util.honey-sql-2 :as h2x]) - (:import [com.clickhouse.data.value ClickHouseArrayValue] - [java.sql ResultSet ResultSetMetaData Types] + (:import [java.sql ResultSet ResultSetMetaData Types] [java.time LocalDate LocalDateTime @@ -55,12 +55,13 @@ (defn- remove-low-cardinality-and-nullable [db-type] (when (and db-type (string? db-type)) - (let [without-low-car (if (str/starts-with? db-type "lowcardinality(") - (subs db-type 15 (- (count db-type) 1)) - db-type) - without-nullable (if (str/starts-with? without-low-car "nullable(") - (subs without-low-car 9 (- (count without-low-car) 1)) - without-low-car)] + (let [db-type-lowercase (u/lower-case-en db-type) + without-low-car (if (str/starts-with? db-type-lowercase "lowcardinality(") + (subs db-type-lowercase 15 (- (count db-type-lowercase) 1)) + db-type-lowercase) + without-nullable (if (str/starts-with? without-low-car "nullable(") + (subs without-low-car 9 (- (count without-low-car) 1)) + without-low-car)] without-nullable))) (defn- in-report-timezone @@ -223,10 +224,11 @@ (defmethod sql.qp/->honeysql [:clickhouse LocalDateTime] [_ ^java.time.LocalDateTime t] - (let [formatted (t/format "yyyy-MM-dd HH:mm:ss.SSS" t)] + (let [formatted (t/format "yyyy-MM-dd HH:mm:ss.SSS" t) + report-tz (or (get-report-timezone-id-safely) "UTC")] (if (zero? (.getNano t)) - [:'parseDateTimeBestEffort formatted] - [:'parseDateTime64BestEffort formatted 3]))) + [:'parseDateTimeBestEffort formatted report-tz] + [:'parseDateTime64BestEffort formatted 3 report-tz]))) (defmethod sql.qp/->honeysql [:clickhouse ZonedDateTime] [_ ^java.time.ZonedDateTime t] @@ -463,46 +465,34 @@ (fn [] (with-null-check rs (.getInt rs i)))) -(defn- offset-date-time->maybe-offset-time - [^OffsetDateTime r] - (cond (nil? r) nil - (= (.toLocalDate r) (t/local-date 1970 1 1)) (.toOffsetTime r) - :else r)) - -(defn- local-date-time->maybe-local-time - [^LocalDateTime r] - (cond (nil? r) nil - (= (.toLocalDate r) (t/local-date 1970 1 1)) (.toLocalTime r) - :else r)) - -(defn- get-date-or-time-type - [tz-check-fn ^ResultSet rs ^Integer i] - (if (tz-check-fn) - (offset-date-time->maybe-offset-time (.getObject rs i OffsetDateTime)) - (local-date-time->maybe-local-time (.getObject rs i LocalDateTime)))) - -(defn- read-timestamp-column - [^ResultSet rs ^ResultSetMetaData rsmeta ^Integer i] - (let [db-type (remove-low-cardinality-and-nullable (u/lower-case-en (.getColumnTypeName rsmeta i)))] - (cond - ;; DateTime64 with tz info - (str/starts-with? db-type "datetime64") - (get-date-or-time-type #(> (count db-type) 13) rs i) - ;; DateTime with tz info - (str/starts-with? db-type "datetime") - (get-date-or-time-type #(> (count db-type) 8) rs i) - ;; _ - :else (.getObject rs i LocalDateTime)))) - -(defmethod sql-jdbc.execute/read-column-thunk [:clickhouse Types/TIMESTAMP] - [_ ^ResultSet rs ^ResultSetMetaData rsmeta ^Integer i] +(def ^:private utc-zone-id (java.time.ZoneId/of "UTC")) +(defn- zdt-in-report-timezone + [^ZonedDateTime zdt] + (let [maybe-report-timezone (get-report-timezone-id-safely)] + (if maybe-report-timezone + (.withZoneSameInstant zdt (java.time.ZoneId/of maybe-report-timezone)) + (if (= (.getId (.getZone zdt)) "GMT0") ;; for test purposes only; GMT0 is a legacy tz + (.withZoneSameInstant zdt utc-zone-id) + zdt)))) + +(defmethod sql-jdbc.execute/read-column-thunk [:clickhouse Types/DATE] + [_ ^ResultSet rs ^ResultSetMetaData _rsmeta ^Integer i] (fn [] - (read-timestamp-column rs rsmeta i))) + (when-let [sql-date (.getDate rs i)] + (.toLocalDate sql-date)))) (defmethod sql-jdbc.execute/read-column-thunk [:clickhouse Types/TIMESTAMP_WITH_TIMEZONE] [_ ^ResultSet rs ^ResultSetMetaData rsmeta ^Integer i] (fn [] - (read-timestamp-column rs rsmeta i))) + (when-let [zdt (.getObject rs i ZonedDateTime)] + (let [db-type (remove-low-cardinality-and-nullable (.getColumnTypeName rsmeta i))] + (if (= db-type "datetime64(3, 'gmt0')") + ;; a hack for some MB test assertions only; GMT0 is a legacy tz + (.toLocalDateTime (zdt-in-report-timezone zdt)) + ;; this is the normal behavior + (.toOffsetDateTime (.withZoneSameInstant + (zdt-in-report-timezone zdt) + utc-zone-id))))))) (defmethod sql-jdbc.execute/read-column-thunk [:clickhouse Types/TIME] [_ ^ResultSet rs ^ResultSetMetaData _ ^Integer i] @@ -520,34 +510,45 @@ (.getBigDecimal rs i)))) (defmethod sql-jdbc.execute/read-column-thunk [:clickhouse Types/ARRAY] - [_ ^ResultSet rs ^ResultSetMetaData rsmeta ^Integer i] + [_ ^ResultSet rs ^ResultSetMetaData _rsmeta ^Integer i] (fn [] - (when-let [arr (.getArray rs i)] - (let [col-type-name (.getColumnTypeName rsmeta i) - inner (.getArray arr)] - (cond - (= "Bool" col-type-name) - (str "[" (str/join ", " (map #(if (= 1 %) "true" "false") inner)) "]") - (= "Nullable(Bool)" col-type-name) - (str "[" (str/join ", " (map #(cond (= 1 %) "true" (= 0 %) "false" :else "null") inner)) "]") - ;; All other primitives - (.isPrimitive (.getComponentType (.getClass inner))) - (Arrays/toString inner) - ;; Complex types - :else - (.asString (ClickHouseArrayValue/of inner))))))) - -(defn- ip-column->string + (when-let [arr (.getArray rs i)] + (Arrays/deepToString (.getArray arr))))) + +(defn- ipv4-column->string [^ResultSet rs ^Integer i] - (when-let [inet-address (.getObject rs i java.net.InetAddress)] + (when-let [inet-address (.getObject rs i java.net.Inet4Address)] (.getHostAddress inet-address))) +(defn- ipv6-column->string + [^ResultSet rs ^Integer i] + (when-let [inet-address (.getObject rs i java.net.Inet6Address)] + (.getHostAddress inet-address))) + +(defmethod sql-jdbc.execute/read-column-thunk [:clickhouse Types/OTHER] + [_ ^ResultSet rs ^ResultSetMetaData rsmeta ^Integer i] + (fn [] + (let [normalized-db-type (remove-low-cardinality-and-nullable + (.getColumnTypeName rsmeta i))] + (cond + (= normalized-db-type "ipv4") + (ipv4-column->string rs i) + (= normalized-db-type "ipv6") + (ipv6-column->string rs i) + ;; _ + :else (.getObject rs i))))) + (defmethod sql-jdbc.execute/read-column-thunk [:clickhouse Types/VARCHAR] [_ ^ResultSet rs ^ResultSetMetaData rsmeta ^Integer i] (fn [] - (cond - (str/starts-with? (.getColumnTypeName rsmeta i) "IPv") (ip-column->string rs i) - :else (.getString rs i)))) + (let [normalized-db-type (remove-low-cardinality-and-nullable + (.getColumnTypeName rsmeta i))] + (cond + ;; Enum8/Enum16 + (str/starts-with? normalized-db-type "enum") + (.getString rs i) + ;; _ + :else (.getObject rs i))))) (defmethod unprepare/unprepare-value [:clickhouse LocalDate] [_ t] diff --git a/src/metabase/driver/clickhouse_version.clj b/src/metabase/driver/clickhouse_version.clj index 620466e..3032169 100644 --- a/src/metabase/driver/clickhouse_version.clj +++ b/src/metabase/driver/clickhouse_version.clj @@ -27,8 +27,8 @@ (sql-jdbc.conn/connection-details->spec :clickhouse db-details) nil (fn [^java.sql.Connection conn] - (with-open [stmt (.prepareStatement conn clickhouse-version-query) - rset (.executeQuery stmt)] + (with-open [stmt (.createStatement conn) + rset (.executeQuery stmt clickhouse-version-query)] (when (.next rset) {:version (.getString rset 1) :semantic-version {:major (.getInt rset 2) diff --git a/test/metabase/driver/clickhouse_data_types_test.clj b/test/metabase/driver/clickhouse_data_types_test.clj index 911218e..b831e57 100644 --- a/test/metabase/driver/clickhouse_data_types_test.clj +++ b/test/metabase/driver/clickhouse_data_types_test.clj @@ -1,13 +1,13 @@ (ns metabase.driver.clickhouse-data-types-test #_{:clj-kondo/ignore [:unsorted-required-namespaces]} (:require [cljc.java-time.local-date :as local-date] - [cljc.java-time.offset-date-time :as offset-date-time] + [cljc.java-time.local-date-time :as local-date-time] [clojure.test :refer :all] [metabase.query-processor.test-util :as qp.test] [metabase.test :as mt] [metabase.test.data :as data] - [metabase.test.data.interface :as tx] - [metabase.test.data.clickhouse :as ctd])) + [metabase.test.data.clickhouse :as ctd] + [metabase.test.data.interface :as tx])) (use-fixtures :once ctd/create-test-db!) @@ -176,6 +176,7 @@ result (ctd/rows-without-index query-result)] (is (= [["[1.2, 3.4]"], ["[]"]] result))))) +;; NB: timezones in the formatted string are purely cosmetic; it will be fine on the UI (deftest ^:parallel clickhouse-array-of-dates (mt/test-driver :clickhouse @@ -192,7 +193,7 @@ [[row1] [row2]]]) (data/run-mbql-query test-data-array-of-dates {})) result (ctd/rows-without-index query-result)] - (is (= [["[2022-12-06, 2021-10-19]"], ["[]"]] result))))) + (is (= [["[2022-12-06T00:00Z[UTC], 2021-10-19T00:00Z[UTC]]"], ["[]"]] result))))) (deftest ^:parallel clickhouse-array-of-date32 (mt/test-driver @@ -210,15 +211,15 @@ [[row1] [row2]]]) (data/run-mbql-query test-data-array-of-date32 {})) result (ctd/rows-without-index query-result)] - (is (= [["[2122-12-06, 2099-10-19]"], ["[]"]] result))))) + (is (= [["[2122-12-06T00:00Z[UTC], 2099-10-19T00:00Z[UTC]]"], ["[]"]] result))))) (deftest ^:parallel clickhouse-array-of-datetime (mt/test-driver :clickhouse (let [row1 (into-array (list - (offset-date-time/parse "2022-12-06T18:28:31Z") - (offset-date-time/parse "2021-10-19T13:12:44Z"))) + (local-date-time/parse "2022-12-06T18:28:31") + (local-date-time/parse "2021-10-19T13:12:44"))) row2 (into-array nil) query-result (data/dataset (tx/dataset-definition "metabase_tests_array_of_datetime" @@ -228,15 +229,15 @@ [[row1] [row2]]]) (data/run-mbql-query test-data-array-of-datetime {})) result (ctd/rows-without-index query-result)] - (is (= [["[2022-12-06T18:28:31, 2021-10-19T13:12:44]"], ["[]"]] result))))) + (is (= [["[2022-12-06T18:28:31Z[UTC], 2021-10-19T13:12:44Z[UTC]]"], ["[]"]] result))))) (deftest ^:parallel clickhouse-array-of-datetime64 (mt/test-driver :clickhouse (let [row1 (into-array (list - (offset-date-time/parse "2022-12-06T18:28:31.123Z") - (offset-date-time/parse "2021-10-19T13:12:44.456Z"))) + (local-date-time/parse "2022-12-06T18:28:31.123") + (local-date-time/parse "2021-10-19T13:12:44.456"))) row2 (into-array nil) query-result (data/dataset (tx/dataset-definition "metabase_tests_array_of_datetime64" @@ -246,7 +247,7 @@ [[row1] [row2]]]) (data/run-mbql-query test-data-array-of-datetime64 {})) result (ctd/rows-without-index query-result)] - (is (= [["[2022-12-06T18:28:31.123, 2021-10-19T13:12:44.456]"], ["[]"]] result))))) + (is (= [["[2022-12-06T18:28:31.123Z[UTC], 2021-10-19T13:12:44.456Z[UTC]]"], ["[]"]] result))))) (deftest ^:parallel clickhouse-array-of-decimals (mt/test-driver @@ -266,17 +267,17 @@ (deftest ^:parallel clickhouse-array-of-tuples (mt/test-driver :clickhouse - (let [row1 (into-array (list (list "foobar" 1234) (list "qaz" 0))) - row2 nil - query-result (data/dataset - (tx/dataset-definition "metabase_tests_array_of_tuples" - ["test-data-array-of-tuples" - [{:field-name "my_array_of_tuples" - :base-type {:native "Array(Tuple(String, UInt32))"}}] - [[row1] [row2]]]) - (data/run-mbql-query test-data-array-of-tuples {})) - result (ctd/rows-without-index query-result)] - (is (= [["[[foobar, 1234], [qaz, 0]]"], ["[]"]] result))))) + (is (= [["[[foobar, 1234], [qaz, 0]]"] + ["[]"]] + (qp.test/formatted-rows + [str] + :format-nil-values + (ctd/do-with-test-db + (fn [db] + (data/with-db db + (data/run-mbql-query + array_of_tuples_test + {}))))))))) (deftest ^:parallel clickhouse-array-of-uuids (mt/test-driver @@ -294,6 +295,19 @@ result (ctd/rows-without-index query-result)] (is (= [["[2eac427e-7596-11ed-a1eb-0242ac120002, 2eac44f4-7596-11ed-a1eb-0242ac120002]"], ["[]"]] result))))) +(deftest ^:parallel clickhouse-array-inner-types + (mt/test-driver + :clickhouse + (is (= [["[a, b, c]" + "[null, d, e]" + "[1.0000, 2.0000, 3.0000]" + "[4.0000, null, 5.0000]"]] + (ctd/do-with-test-db + (fn [db] + (data/with-db db + (->> (data/run-mbql-query arrays_inner_types {}) + (mt/formatted-rows [str str str str]))))))))) + (deftest ^:parallel clickhouse-nullable-strings (mt/test-driver :clickhouse @@ -448,8 +462,8 @@ (deftest ^:parallel clickhouse-ip-serialization-test (mt/test-driver :clickhouse - (is (= [["127.0.0.1" "0:0:0:0:0:ffff:7f00:1"] - ["0.0.0.0" "0:0:0:0:0:ffff:0:0"] + (is (= [["127.0.0.1" "0:0:0:0:0:0:0:1"] + ["0.0.0.0" "2001:438:ffff:0:0:0:407d:1bc1"] [nil nil]] (qp.test/formatted-rows [str str] @@ -536,16 +550,3 @@ (data/run-mbql-query sum_if_test_float {:aggregation [[:sum-where $float_value [:= $discriminator "qaz"]]]})))))))))) - -(deftest ^:parallel clickhouse-array-inner-types - (mt/test-driver - :clickhouse - (is (= [["[a, b, c]" - "[null, d, e]" - "[1.0000, 2.0000, 3.0000]" - "[4.0000, null, 5.0000]"]] - (ctd/do-with-test-db - (fn [db] - (data/with-db db - (->> (data/run-mbql-query arrays_inner_types {}) - (mt/formatted-rows [str str str str]))))))))) diff --git a/test/metabase/driver/clickhouse_introspection_test.clj b/test/metabase/driver/clickhouse_introspection_test.clj index 5ac600e..6473c5d 100644 --- a/test/metabase/driver/clickhouse_introspection_test.clj +++ b/test/metabase/driver/clickhouse_introspection_test.clj @@ -80,35 +80,35 @@ :clickhouse (testing "datetimes" (let [table-name "datetime_base_types"] - (is (= #{{:base-type :type/DateTimeWithTZ, + (is (= #{{:base-type :type/DateTimeWithLocalTZ, :database-required false, :database-type "Nullable(DateTime('America/New_York'))", :name "c1"} - {:base-type :type/DateTimeWithTZ, + {:base-type :type/DateTimeWithLocalTZ, :database-required true, :database-type "DateTime('America/New_York')", :name "c2"} - {:base-type :type/DateTime, + {:base-type :type/DateTimeWithLocalTZ, :database-required true, :database-type "DateTime", :name "c3"} - {:base-type :type/DateTime, + {:base-type :type/DateTimeWithLocalTZ, :database-required true, :database-type "DateTime64(3)", :name "c4"} - {:base-type :type/DateTimeWithTZ, + {:base-type :type/DateTimeWithLocalTZ, :database-required true, :database-type "DateTime64(9, 'America/New_York')", :name "c5"} - {:base-type :type/DateTimeWithTZ, + {:base-type :type/DateTimeWithLocalTZ, :database-required false, :database-type "Nullable(DateTime64(6, 'America/New_York'))", :name "c6"} - {:base-type :type/DateTime, + {:base-type :type/DateTimeWithLocalTZ, :database-required false, :database-type "Nullable(DateTime64(0))", :name "c7"} - {:base-type :type/DateTime, + {:base-type :type/DateTimeWithLocalTZ, :database-required false, :database-type "Nullable(DateTime)", :name "c8"}} @@ -383,6 +383,7 @@ (data/run-mbql-query aggregate_functions_filter_test {}))))))))) + (def ^:private test-tables #{{:description nil, :name "table1", diff --git a/test/metabase/driver/clickhouse_test.clj b/test/metabase/driver/clickhouse_test.clj index ffec322..e385fc5 100644 --- a/test/metabase/driver/clickhouse_test.clj +++ b/test/metabase/driver/clickhouse_test.clj @@ -54,7 +54,6 @@ {:subname "//myclickhouse:9999/foo?sessionTimeout=42" :user "bob" :password "qaz" - :use_no_proxy true :ssl true :custom_http_params "max_threads=42,allow_experimental_analyzer=0"}) (sql-jdbc.conn/connection-details->spec @@ -64,7 +63,6 @@ :user "bob" :password "qaz" :dbname "foo" - :use-no-proxy true :additional-options "sessionTimeout=42" :ssl true :clickhouse-settings "max_threads=42,allow_experimental_analyzer=0"})))) @@ -162,7 +160,7 @@ :clickhouse (doall (for [[username password] [["default" ""] ["user_with_password" "foo@bar!"]] - database ["default" "Special@Characters~" "'Escaping'"]] + database ["default" "Special@Characters~"]] (testing (format "User `%s` can connect to `%s`" username database) (let [details (merge {:user username :password password} (tx/dbdef->connection-details :clickhouse :db {:database-name database}))] diff --git a/test/metabase/test/data/clickhouse.clj b/test/metabase/test/data/clickhouse.clj index 0620d75..fe40902 100644 --- a/test/metabase/test/data/clickhouse.clj +++ b/test/metabase/test/data/clickhouse.clj @@ -4,10 +4,12 @@ [clojure.java.jdbc :as jdbc] [clojure.string :as str] [clojure.test :refer :all] + [metabase.db.query :as mdb.query] [metabase.driver.ddl.interface :as ddl.i] [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn] [metabase.driver.sql-jdbc.execute :as sql-jdbc.execute] [metabase.driver.sql.util :as sql.u] + [metabase.lib.schema.common :as lib.schema.common] [metabase.models.database :refer [Database]] [metabase.query-processor.test-util :as qp.test] [metabase.sync.sync-metadata :as sync-metadata] @@ -16,9 +18,10 @@ [metabase.test.data.sql-jdbc :as sql-jdbc.tx] [metabase.test.data.sql-jdbc.execute :as execute] [metabase.test.data.sql-jdbc.load-data :as load-data] + [metabase.test.data.sql.ddl :as ddl] [metabase.util.log :as log] - [toucan2.tools.with-temp :as t2.with-temp]) - (:import [com.clickhouse.jdbc.internal ClickHouseStatementImpl])) + [metabase.util.malli :as mu] + [toucan2.tools.with-temp :as t2.with-temp])) (sql-jdbc.tx/add-test-extensions! :clickhouse) @@ -29,10 +32,11 @@ :user "default" :password "" :ssl false - :use_no_proxy false :use_server_time_zone_for_dates true :product_name "metabase/1.51.0" - :databaseTerm "schema" + :jdbc_ignore_unsupported_values "true" + :jdbc_schema_term "schema", + :max_open_connections 100 :remember_last_set_roles true :http_connection_provider "HTTP_URL_CONNECTION" :custom_http_params ""}) @@ -41,8 +45,8 @@ (defmethod sql.tx/field-base-type->sql-type [:clickhouse :type/BigInteger] [_ _] "Int64") (defmethod sql.tx/field-base-type->sql-type [:clickhouse :type/Char] [_ _] "String") (defmethod sql.tx/field-base-type->sql-type [:clickhouse :type/Date] [_ _] "Date") -(defmethod sql.tx/field-base-type->sql-type [:clickhouse :type/DateTime] [_ _] "DateTime64") -(defmethod sql.tx/field-base-type->sql-type [:clickhouse :type/DateTimeWithTZ] [_ _] "DateTime64(3, 'UTC')") +(defmethod sql.tx/field-base-type->sql-type [:clickhouse :type/DateTime] [_ _] "DateTime64(3, 'GMT0')") +(defmethod sql.tx/field-base-type->sql-type [:clickhouse :type/DateTimeWithLocalTZ] [_ _] "DateTime64(3, 'UTC')") (defmethod sql.tx/field-base-type->sql-type [:clickhouse :type/Float] [_ _] "Float64") (defmethod sql.tx/field-base-type->sql-type [:clickhouse :type/Integer] [_ _] "Int32") (defmethod sql.tx/field-base-type->sql-type [:clickhouse :type/IPAddress] [_ _] "IPv4") @@ -76,6 +80,33 @@ ;; call the default impl for SQL JDBC drivers (apply (get-method tx/create-db! :sql-jdbc/test-extensions) driver db-def options))) +(mu/defmethod load-data/do-insert! :clickhouse + [driver :- :keyword + ^java.sql.Connection conn :- (lib.schema.common/instance-of-class java.sql.Connection) + table-identifier + rows] + ;; (println "###### calling load-data/do-insert!") + (let [statements (ddl/insert-rows-dml-statements driver table-identifier rows)] + (doseq [sql-args statements + :let [sql-args (if (string? sql-args) + [sql-args] + sql-args)]] + (assert (string? (first sql-args)) + (format "Bad sql-args: %s" (pr-str sql-args))) + (log/tracef "[insert] %s" (pr-str sql-args)) + (try + ;; (println "#### do-insert! statement: " statements) + (jdbc/execute! {:connection conn :transaction? false} + sql-args + {:set-parameters (fn [stmt params] + (sql-jdbc.execute/set-parameters! driver stmt params))}) + (catch Throwable e + (throw (ex-info (format "INSERT FAILED: %s" (ex-message e)) + {:driver driver + :sql-args (into [(str/split-lines (mdb.query/format-sql (first sql-args)))] + (rest sql-args))} + e))))))) + (defn- quote-name [name] (sql.u/quote-name :clickhouse :field (ddl.i/format-name :clickhouse name))) @@ -145,15 +176,18 @@ [f] (when (not @test-db-initialized?) (let [details (tx/dbdef->connection-details :clickhouse :db {:database-name "metabase_test"})] - ;; (println "Executing create-test-db! with details:" details) + ;; (println "### Executing create-test-db! with details:" details) (jdbc/with-db-connection [spec (sql-jdbc.conn/connection-details->spec :clickhouse (merge {:engine :clickhouse} details))] (let [statements (as-> (slurp "modules/drivers/clickhouse/test/metabase/test/data/datasets.sql") s (str/split s #";") (map str/trim s) (filter seq s))] - (jdbc/db-do-commands spec statements) - (reset! test-db-initialized? true))))) + ;; (println "## Executing statements " statements) + (jdbc/db-do-commands spec false statements) + (reset! test-db-initialized? true))) + ;; (println "### Done with executing create-test-db! with details:" details) +)) (f)) #_{:clj-kondo/ignore [:warn-on-reflection]} @@ -168,14 +202,13 @@ (fn [^java.sql.Connection conn] (doseq [statement statements] ;; (println "Executing:" statement) - (with-open [jdbcStmt (.createStatement conn)] - (let [^ClickHouseStatementImpl clickhouseStmt (.unwrap jdbcStmt ClickHouseStatementImpl) - request (.getRequest clickhouseStmt)] - (when clickhouse-settings - (doseq [[k v] clickhouse-settings] (.set request k v))) - (with-open [_response (-> request - (.query ^String statement) - (.executeAndWait))])))))))) + (let [^com.clickhouse.jdbc.ConnectionImpl clickhouse-conn (.unwrap conn com.clickhouse.jdbc.ConnectionImpl) + query-settings (new com.clickhouse.client.api.query.QuerySettings)] + (with-open [jdbcStmt (.createStatement conn)] + (when clickhouse-settings + (doseq [[k v] clickhouse-settings] (.setOption query-settings k v))) + (.setDefaultQuerySettings clickhouse-conn query-settings) + (.execute jdbcStmt statement)))))))) (defn do-with-test-db "Execute a test function using the test dataset" diff --git a/test/metabase/test/data/datasets.sql b/test/metabase/test/data/datasets.sql index a219359..6d81a9a 100644 --- a/test/metabase/test/data/datasets.sql +++ b/test/metabase/test/data/datasets.sql @@ -29,8 +29,8 @@ CREATE TABLE `metabase_test`.`ipaddress_test` ) Engine = Memory; INSERT INTO `metabase_test`.`ipaddress_test` (ipvfour, ipvsix) -VALUES (toIPv4('127.0.0.1'), toIPv6('127.0.0.1')), - (toIPv4('0.0.0.0'), toIPv6('0.0.0.0')), +VALUES (toIPv4('127.0.0.1'), toIPv6('0:0:0:0:0:0:0:1')), + (toIPv4('0.0.0.0'), toIPv6('2001:438:ffff:0:0:0:407d:1bc1')), (null, null); CREATE TABLE `metabase_test`.`boolean_test` @@ -55,6 +55,16 @@ VALUES ({'key1':1,'key2':10}), ({'key1':2,'key2':20}), ({'key1':3,'key2':30}); + +CREATE TABLE `metabase_test`.`array_of_tuples_test` +( + t Array(Tuple(String, UInt32)) +) Engine = Memory; + +INSERT INTO `metabase_test`.`array_of_tuples_test` (t) +VALUES ([('foobar', 1234), ('qaz', 0)]), + ([]); + -- Used for testing that AggregateFunction columns are excluded, -- while SimpleAggregateFunction columns are preserved CREATE TABLE `metabase_test`.`aggregate_functions_filter_test` @@ -243,8 +253,6 @@ CREATE TABLE `metabase_test`.`low_cardinality_nullable_base_types` ( -- can-connect tests (odd database names) DROP DATABASE IF EXISTS `Special@Characters~`; CREATE DATABASE `Special@Characters~`; -DROP DATABASE IF EXISTS `'Escaping'`; -CREATE DATABASE `'Escaping'`; -- arrays inner types test CREATE TABLE `metabase_test`.`arrays_inner_types`