From 017e25751388e2e1eae14e824e973a65149f580b Mon Sep 17 00:00:00 2001 From: Serge Klochkov <3175289+slvrtrn@users.noreply.github.com> Date: Mon, 13 Feb 2023 20:07:32 +0100 Subject: [PATCH] Filter (Simple)AggregateFunction columns (#140) * Filter (Simple)AggregateFunction columns * Update CHANGELOG and README --- CHANGELOG.md | 6 ++++ README.md | 4 +++ resources/metabase-plugin.yaml | 2 +- src/metabase/driver/clickhouse.clj | 29 ++++++++++++------- test/metabase/driver/clickhouse_test.clj | 27 +++++++++++++++++ .../metabase/driver/clickhouse_test_utils.clj | 6 +++- test/metabase/test/data/clickhouse.clj | 2 +- 7 files changed, 63 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 17b60e7..1882cf4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +# 1.0.2 + +### Bug fixes + +* As the underlying JDBC driver version does not support columns with `(Simple)AggregationFunction` type, these columns are now excluded from the table metadata and data browser result sets to prevent sync or data browsing errors. + # 1.0.1 ### Bug fixes diff --git a/README.md b/README.md index 88165da..4248aaf 100644 --- a/README.md +++ b/README.md @@ -128,6 +128,10 @@ The driver should work fine for many use cases. Please consider the following it * Metabase is a good tool for organizing questions, dashboards etc. and to give non-technical users a good way to explore the data and share their results. The driver cannot support all the cool special features of ClickHouse, e.g. array functions. You are free to use native queries, of course. +## Known limitations + +* As the underlying JDBC driver version does not support columns with `(Simple)AggregationFunction` type, these columns are excluded from the table metadata and data browser result sets to prevent sync or data browsing errors. + ## Contributing Check out our [contributing guide](./CONTRIBUTING.md). diff --git a/resources/metabase-plugin.yaml b/resources/metabase-plugin.yaml index e632b1b..9919084 100644 --- a/resources/metabase-plugin.yaml +++ b/resources/metabase-plugin.yaml @@ -1,6 +1,6 @@ info: name: Metabase ClickHouse Driver - version: 1.0.1 + version: 1.0.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 b6d0655..3cc9f15 100644 --- a/src/metabase/driver/clickhouse.clj +++ b/src/metabase/driver/clickhouse.clj @@ -63,9 +63,12 @@ (defmethod sql-jdbc.sync/database-type->base-type :clickhouse [_ database-type] - (database-type->base-type (str/replace (name database-type) - #"(?:Nullable|LowCardinality)\((\S+)\)" - "$1"))) + (let [base-type (database-type->base-type + (str/replace (name database-type) + #"(?:Nullable|LowCardinality)\((\S+)\)" + "$1"))] + base-type)) + (def ^:private excluded-schemas #{"system" "information_schema" "INFORMATION_SCHEMA"}) (defmethod sql-jdbc.sync/excluded-schemas :clickhouse [_] excluded-schemas) @@ -85,7 +88,7 @@ :use_no_proxy (boolean use-no-proxy) :use_server_time_zone_for_dates true ;; temporary hardcode until we get product_name setting with JDBC driver v0.4.0 - :client_name "metabase/1.0.1 clickhouse-jdbc/0.3.2-patch-11"} + :client_name "metabase/1.0.2 clickhouse-jdbc/0.3.2-patch-11"} (sql-jdbc.common/handle-additional-options details :separator-style :url))) (defn- to-relative-day-num @@ -518,12 +521,18 @@ (defmethod driver/describe-table :clickhouse [_ database table] - (let [t (sql-jdbc.sync/describe-table :clickhouse database table)] - (merge t - {:fields (set (for [f (:fields t)] - (update-in f [:database-type] - clojure.string/replace - #"^(Enum.+)\(.+\)" "$1")))}))) + (let [table-metadata (sql-jdbc.sync/describe-table :clickhouse database table) + filtered-fields (for [field (:fields table-metadata) + :let [updated-field + (update-in field [:database-type] + ;; Enum8(UInt8) -> Enum8 + clojure.string/replace #"^(Enum.+)\(.+\)" "$1")] + ;; Skip all (Simple)AggregateFunction columns + ;; JDBC does not support that and it crashes the data browser + :when (not (re-matches #"^.*AggregateFunction\(.+$" + (get field :database-type)))] + updated-field)] + (merge table-metadata {:fields (set filtered-fields)}))) (defmethod driver/display-name :clickhouse [_] "ClickHouse") diff --git a/test/metabase/driver/clickhouse_test.clj b/test/metabase/driver/clickhouse_test.clj index 068109a..c4b3265 100644 --- a/test/metabase/driver/clickhouse_test.clj +++ b/test/metabase/driver/clickhouse_test.clj @@ -499,3 +499,30 @@ :additional-options additional-options})] (metabase.driver/db-default-timezone :clickhouse spec)))))) +(deftest clickhouse-filtered-aggregate-functions-test + (mt/test-driver + :clickhouse + (testing "(Simple)AggregateFunction columns are filtered" + (testing "from the table metadata" + (is (= {:name "aggregate_functions_filter_test" + :fields #{{:name "i" + :database-type "UInt8" + :base-type :type/Integer + :database-position 0 + ; TODO: in Metabase 0.45.0-RC this returned true, + ; and now it is false, which is strange, cause it is not Nullable in the DDL + :database-required false}}} + (ctu/do-with-metabase-test-db + (fn [db] + (driver/describe-table :clickhouse db {:name "aggregate_functions_filter_test"})))))) + (testing "from the result set" + (is (= [[42]] + (qp.test/formatted-rows + [int] + :format-nil-values + (ctu/do-with-metabase-test-db + (fn [db] + (data/with-db db + (data/run-mbql-query + aggregate_functions_filter_test + {}))))))))))) diff --git a/test/metabase/driver/clickhouse_test_utils.clj b/test/metabase/driver/clickhouse_test_utils.clj index 3471139..06f1cc8 100644 --- a/test/metabase/driver/clickhouse_test_utils.clj +++ b/test/metabase/driver/clickhouse_test_utils.clj @@ -55,7 +55,11 @@ (str "INSERT INTO `metabase_test`.`boolean_test` (ID, b1, b2) VALUES" " (1, true, true)," " (2, false, true)," - " (3, true, false);")]] + " (3, true, false);") + (str "CREATE TABLE `metabase_test`.`aggregate_functions_filter_test` (" + " i UInt8, a AggregateFunction(uniq, String), b SimpleAggregateFunction(min, UInt8)" + ") ENGINE Memory;") + (str "INSERT INTO `metabase_test`.`aggregate_functions_filter_test` (i) VALUES (42);")]] (jdbc/execute! conn [sql])))) (defn do-with-metabase-test-db diff --git a/test/metabase/test/data/clickhouse.clj b/test/metabase/test/data/clickhouse.clj index a818f96..4d4f3f5 100644 --- a/test/metabase/test/data/clickhouse.clj +++ b/test/metabase/test/data/clickhouse.clj @@ -13,7 +13,7 @@ (sql-jdbc.tx/add-test-extensions! :clickhouse) -(def product-name "metabase/1.0.1 clickhouse-jdbc/0.3.2-patch-11") +(def product-name "metabase/1.0.2 clickhouse-jdbc/0.3.2-patch-11") (def default-connection-params {:classname "com.clickhouse.jdbc.ClickHouseDriver" :subprotocol "clickhouse" :subname "//localhost:8123/default"