From 0f9505b4f961be6771811bfd883ddcd485e7c1af Mon Sep 17 00:00:00 2001 From: Serge Klochkov <3175289+slvrtrn@users.noreply.github.com> Date: Mon, 15 Jul 2024 17:46:38 +0200 Subject: [PATCH] Fix boolean array type introspection (#259) * Fix tests namespaces, prevent CH tests to run multiple times * Simplify the boolean array type introspection * Bump versions, update CHANGELOG --- CHANGELOG.md | 6 ++++ resources/metabase-plugin.yaml | 2 +- src/metabase/driver/clickhouse.clj | 2 +- src/metabase/driver/clickhouse_qp.clj | 18 +++-------- .../driver/clickhouse_data_types_test.clj | 32 ++++++++++++++++++- .../driver/clickhouse_impersonation_test.clj | 2 +- .../driver/clickhouse_introspection_test.clj | 4 ++- .../driver/clickhouse_substitution_test.clj | 2 +- .../clickhouse_temporal_bucketing_test.clj | 4 ++- test/metabase/driver/clickhouse_test.clj | 5 --- test/metabase/test/data/clickhouse.clj | 2 +- test/metabase/test/data/datasets.sql | 16 ++++++++++ 12 files changed, 69 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 629a6c0..3a65b7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +# 1.50.2 + +### Bug fixes + +* Fixed Array inner type introspection, which could cause reduced performance when querying tables containing arrays. ([#257](https://github.com/ClickHouse/metabase-clickhouse-driver/issues/257)) + # 1.50.1 ### New features diff --git a/resources/metabase-plugin.yaml b/resources/metabase-plugin.yaml index b41746c..4f55712 100644 --- a/resources/metabase-plugin.yaml +++ b/resources/metabase-plugin.yaml @@ -1,6 +1,6 @@ info: name: Metabase ClickHouse Driver - version: 1.50.1 + version: 1.50.2 description: Allows Metabase to connect to ClickHouse databases. contact-info: name: ClickHouse diff --git a/src/metabase/driver/clickhouse.clj b/src/metabase/driver/clickhouse.clj index 22d03ed..e2fba17 100644 --- a/src/metabase/driver/clickhouse.clj +++ b/src/metabase/driver/clickhouse.clj @@ -29,7 +29,7 @@ (driver/register! :clickhouse :parent #{:sql-jdbc}) (defmethod driver/display-name :clickhouse [_] "ClickHouse") -(def ^:private product-name "metabase/1.50.1") +(def ^:private product-name "metabase/1.50.2") (defmethod driver/prettify-native-form :clickhouse [_ native-form] diff --git a/src/metabase/driver/clickhouse_qp.clj b/src/metabase/driver/clickhouse_qp.clj index 7875cdb..5ab4b84 100644 --- a/src/metabase/driver/clickhouse_qp.clj +++ b/src/metabase/driver/clickhouse_qp.clj @@ -521,25 +521,17 @@ (.getLong rs i) (.getBigDecimal rs i)))) -(defn- get-array-base-type - [^ResultSetMetaData rsmeta ^Integer i] - (try - (-> (.columns rsmeta) - (.get i) - (.arrayBaseColumn) - (.originalTypeName)) - (catch Exception e - (log/error e "Failed to get the inner type of the array column")))) - (defmethod sql-jdbc.execute/read-column-thunk [:clickhouse Types/ARRAY] [_ ^ResultSet rs ^ResultSetMetaData rsmeta ^Integer i] (fn [] (when-let [arr (.getArray rs i)] - (let [array-base-type (get-array-base-type rsmeta i) - inner (.getArray arr)] + (let [col-type-name (.getColumnTypeName rsmeta i) + inner (.getArray arr)] (cond - (= array-base-type "Bool") + (= "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) diff --git a/test/metabase/driver/clickhouse_data_types_test.clj b/test/metabase/driver/clickhouse_data_types_test.clj index b414658..911218e 100644 --- a/test/metabase/driver/clickhouse_data_types_test.clj +++ b/test/metabase/driver/clickhouse_data_types_test.clj @@ -1,4 +1,4 @@ -(ns metabase.driver.clickhouse-test +(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] @@ -9,6 +9,8 @@ [metabase.test.data.interface :as tx] [metabase.test.data.clickhouse :as ctd])) +(use-fixtures :once ctd/create-test-db!) + (deftest ^:parallel clickhouse-decimals (mt/test-driver :clickhouse @@ -129,6 +131,21 @@ result (ctd/rows-without-index query-result)] (is (= [["[true, false, true]"], ["[]"]] result))))) +(deftest ^:parallel clickhouse-array-of-nullable-booleans + (mt/test-driver + :clickhouse + (let [row1 (into-array (list true false nil)) + row2 (into-array nil) + query-result (data/dataset + (tx/dataset-definition "metabase_tests_array_of_nullable_booleans" + ["test-data-array-of-booleans" + [{:field-name "my_array_of_nullable_booleans" + :base-type {:native "Array(Nullable(Boolean))"}}] + [[row1] [row2]]]) + (data/run-mbql-query test-data-array-of-booleans {})) + result (ctd/rows-without-index query-result)] + (is (= [["[true, false, null]"], ["[]"]] result))))) + (deftest ^:parallel clickhouse-array-of-uint8 (mt/test-driver :clickhouse @@ -519,3 +536,16 @@ (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_impersonation_test.clj b/test/metabase/driver/clickhouse_impersonation_test.clj index 74ad54f..239650c 100644 --- a/test/metabase/driver/clickhouse_impersonation_test.clj +++ b/test/metabase/driver/clickhouse_impersonation_test.clj @@ -1,4 +1,4 @@ -(ns metabase.driver.clickhouse-test +(ns metabase.driver.clickhouse-impersonation-test "SET ROLE (connection impersonation feature) tests on with single node or on-premise cluster setups." #_{:clj-kondo/ignore [:unsorted-required-namespaces]} (:require [clojure.test :refer :all] diff --git a/test/metabase/driver/clickhouse_introspection_test.clj b/test/metabase/driver/clickhouse_introspection_test.clj index 44b78f2..5ac600e 100644 --- a/test/metabase/driver/clickhouse_introspection_test.clj +++ b/test/metabase/driver/clickhouse_introspection_test.clj @@ -1,4 +1,4 @@ -(ns metabase.driver.clickhouse-test +(ns metabase.driver.clickhouse-introspection-test #_{:clj-kondo/ignore [:unsorted-required-namespaces]} (:require [clojure.test :refer :all] @@ -13,6 +13,8 @@ [metabase.test.data.interface :as tx] [toucan2.tools.with-temp :as t2.with-temp])) +(use-fixtures :once ctd/create-test-db!) + (defn- desc-table [table-name] (into #{} (map #(select-keys % [:name :database-type :base-type :database-required]) diff --git a/test/metabase/driver/clickhouse_substitution_test.clj b/test/metabase/driver/clickhouse_substitution_test.clj index 21a2086..d956a19 100644 --- a/test/metabase/driver/clickhouse_substitution_test.clj +++ b/test/metabase/driver/clickhouse_substitution_test.clj @@ -1,4 +1,4 @@ -(ns metabase.driver.clickhouse-test +(ns metabase.driver.clickhouse-substitution-test #_{:clj-kondo/ignore [:unsorted-required-namespaces]} (:require [clojure.test :refer :all] [java-time.api :as t] diff --git a/test/metabase/driver/clickhouse_temporal_bucketing_test.clj b/test/metabase/driver/clickhouse_temporal_bucketing_test.clj index 64dac84..2209af2 100644 --- a/test/metabase/driver/clickhouse_temporal_bucketing_test.clj +++ b/test/metabase/driver/clickhouse_temporal_bucketing_test.clj @@ -1,4 +1,4 @@ -(ns metabase.driver.clickhouse-test +(ns metabase.driver.clickhouse-temporal-bucketing-test #_{:clj-kondo/ignore [:unsorted-required-namespaces]} (:require [clojure.test :refer :all] @@ -7,6 +7,8 @@ [metabase.test.data :as data] [metabase.test.data.clickhouse :as ctd])) +(use-fixtures :once ctd/create-test-db!) + ;; See temporal_bucketing table definition ;; Fields values are (both in server and column timezones): ;; start_of_year == '2022-01-01 00:00:00' diff --git a/test/metabase/driver/clickhouse_test.clj b/test/metabase/driver/clickhouse_test.clj index af1fbde..c969572 100644 --- a/test/metabase/driver/clickhouse_test.clj +++ b/test/metabase/driver/clickhouse_test.clj @@ -5,11 +5,6 @@ [metabase.driver :as driver] [metabase.driver.clickhouse :as clickhouse] [metabase.driver.clickhouse-qp :as clickhouse-qp] - [metabase.driver.clickhouse-data-types-test] - [metabase.driver.clickhouse-impersonation-test] - [metabase.driver.clickhouse-introspection-test] - [metabase.driver.clickhouse-substitution-test] - [metabase.driver.clickhouse-temporal-bucketing-test] [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn] [metabase.models.database :refer [Database]] [metabase.query-processor.compile :as qp.compile] diff --git a/test/metabase/test/data/clickhouse.clj b/test/metabase/test/data/clickhouse.clj index 2a0f3a1..0fc0c08 100644 --- a/test/metabase/test/data/clickhouse.clj +++ b/test/metabase/test/data/clickhouse.clj @@ -31,7 +31,7 @@ :ssl false :use_no_proxy false :use_server_time_zone_for_dates true - :product_name "metabase/1.50.1" + :product_name "metabase/1.50.2" :databaseTerm "schema" :remember_last_set_roles true :http_connection_provider "HTTP_URL_CONNECTION" diff --git a/test/metabase/test/data/datasets.sql b/test/metabase/test/data/datasets.sql index 4f58ddf..a219359 100644 --- a/test/metabase/test/data/datasets.sql +++ b/test/metabase/test/data/datasets.sql @@ -245,3 +245,19 @@ 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` +( + `arr_str` Array(String), + `arr_nstr` Array(Nullable(String)), + `arr_dec` Array(Decimal(18, 4)), + `arr_ndec` Array(Nullable(Decimal(18, 4))) +) +ENGINE Memory; +INSERT INTO `metabase_test`.`arrays_inner_types` VALUES ( + ['a', 'b', 'c'], + [NULL, 'd', 'e'], + [1, 2, 3], + [4, NULL, 5] +);