Filter (Simple)AggregateFunction columns (#140)

* Filter (Simple)AggregateFunction columns * Update CHANGELOG and README
ClickHouse · Feb 13, 2023 · 017e257 · 017e257
1 parent 82374c2
commit 017e257
Show file tree

Hide file tree

Showing 7 changed files with 63 additions and 13 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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"