From 75f2207de3ceb73fb3492244bf888681202e933f Mon Sep 17 00:00:00 2001
From: Jeff Evans <jeff303@users.noreply.github.com>
Date: Mon, 13 Sep 2021 10:02:34 -0500
Subject: [PATCH] Fix support for NUMERIC and BIGNUMERIC types (#17837)
Add parameter handling for BigDecimal that checks whether the value is in range for BIGNUMERIC or BIGDECIMAL and sets accordingly
Updating parameter test to hit both these cases
Modifying test code/macros to support a with-table semantic for testing the decimal types via a temp table
Writing new test to confirm that all numeric types are synced correctly and can be queried for
---
.../metabase/driver/bigquery_cloud_sdk.clj | 21 ++--
.../driver/bigquery_cloud_sdk/params.clj | 14 ++-
.../driver/bigquery_cloud_sdk/params_test.clj | 7 +-
.../driver/bigquery_cloud_sdk_test.clj | 95 +++++++++++++++----
4 files changed, 107 insertions(+), 30 deletions(-)
diff --git a/modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk.clj b/modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk.clj
index 6deec425eeb..b4713fde16a 100644
--- a/modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk.clj
+++ b/modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk.clj
@@ -92,16 +92,17 @@
(defn- bigquery-type->base-type [field-type]
(case field-type
- "BOOLEAN" :type/Boolean
- "FLOAT" :type/Float
- "INTEGER" :type/Integer
- "RECORD" :type/Dictionary ; RECORD -> field has a nested schema
- "STRING" :type/Text
- "DATE" :type/Date
- "DATETIME" :type/DateTime
- "TIMESTAMP" :type/DateTimeWithLocalTZ
- "TIME" :type/Time
- "NUMERIC" :type/Decimal
+ "BOOLEAN" :type/Boolean
+ "FLOAT" :type/Float
+ "INTEGER" :type/Integer
+ "RECORD" :type/Dictionary ; RECORD -> field has a nested schema
+ "STRING" :type/Text
+ "DATE" :type/Date
+ "DATETIME" :type/DateTime
+ "TIMESTAMP" :type/DateTimeWithLocalTZ
+ "TIME" :type/Time
+ "NUMERIC" :type/Decimal
+ "BIGNUMERIC" :type/Decimal
:type/*))
(s/defn ^:private table-schema->metabase-field-info
diff --git a/modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk/params.clj b/modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk/params.clj
index e109ce5dfa3..b544a1bc1bf 100644
--- a/modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk/params.clj
+++ b/modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk/params.clj
@@ -37,7 +37,19 @@
(defmethod ->QueryParameterValue clojure.lang.BigInt [v] (param "INT64" v))
(defmethod ->QueryParameterValue Float [v] (param "FLOAT64" v))
(defmethod ->QueryParameterValue Double [v] (param "FLOAT64" v))
-(defmethod ->QueryParameterValue java.math.BigDecimal [v] (param "FLOAT64" v))
+
+;; use the min and max values for the NUMERIC types to figure out if we need to set decimal params as NUMERIC
+;; or BIGNUMERIC
+(def ^:private ^:const ^BigDecimal max-bq-numeric-val (bigdec "9.9999999999999999999999999999999999999E+28"))
+(def ^:private ^:const ^BigDecimal min-bq-numeric-val (.negate max-bq-numeric-val))
+
+(defmethod ->QueryParameterValue java.math.BigDecimal [^BigDecimal v]
+ (if (or (and (< 0 (.signum v))
+ (< 0 (.compareTo v min-bq-numeric-val)))
+ (and (> 0 (.signum v))
+ (> 0 (.compareTo v max-bq-numeric-val))))
+ (param "BIGNUMERIC" v)
+ (param "NUMERIC" v)))
(defmethod ->QueryParameterValue java.time.LocalDate [t] (param "DATE" (u.date/format t)))
(defmethod ->QueryParameterValue java.time.LocalDateTime [t] (param "DATETIME" (u.date/format t)))
diff --git a/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk/params_test.clj b/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk/params_test.clj
index b0b2aeece21..f33a7d1ee79 100644
--- a/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk/params_test.clj
+++ b/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk/params_test.clj
@@ -17,7 +17,12 @@
[(bigint 100) {:base-type :type/Integer}]
[(float 100.0) {:base-type :type/Float}]
[(double 100.0) {:base-type :type/Float}]
- [(bigdec 100.0) {:base-type :type/Float, :v 100.0}]
+ ;; one case for NUMERIC/DECIMAL
+ [(bigdec "-2.3E+16") {:base-type :type/Decimal
+ :v "-23000000000000000"}]
+ ;; and one for BIGNUMERIC/BIGDECIMAL
+ [(bigdec "1.6E+31") {:base-type :type/Decimal
+ :v "16000000000000000000000000000000"}]
;; LocalDate
[#t "2020-05-26" {:base-type :type/Date
:v #t "2020-05-26T00:00Z[UTC]"}]
diff --git a/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk_test.clj b/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk_test.clj
index a7aa1bfb624..0a4a22904eb 100644
--- a/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk_test.clj
+++ b/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk_test.clj
@@ -144,29 +144,50 @@
(is (= "UTC"
(tu/db-timezone-id)))))
-(defn- do-with-view [f]
+(defn- do-with-temp-obj [name-fmt-str create-args-fn drop-args-fn f]
(driver/with-driver :bigquery-cloud-sdk
- (let [view-name (format "view_%s" (tu/random-name))]
+ (let [obj-name (format name-fmt-str (tu/random-name))]
(mt/with-temp-copy-of-db
(try
- (bigquery.tx/execute!
- (str "CREATE VIEW `v3_test_data.%s` "
- "AS "
- "SELECT v.id AS id, v.name AS venue_name, c.name AS category_name "
- "FROM `%s.v3_test_data.venues` v "
- "LEFT JOIN `%s.v3_test_data.categories` c "
- "ON v.category_id = c.id "
- "ORDER BY v.id ASC "
- "LIMIT 3")
- view-name
- (bigquery.tx/project-id)
- (bigquery.tx/project-id))
- (f view-name)
+ (apply bigquery.tx/execute! (create-args-fn obj-name))
+ (f obj-name)
(finally
- (bigquery.tx/execute! "DROP VIEW IF EXISTS `v3_test_data.%s`" view-name)))))))
+ (apply bigquery.tx/execute! (drop-args-fn obj-name))))))))
-(defmacro ^:private with-view [[view-name-binding] & body]
- `(do-with-view (fn [~(or view-name-binding '_)] ~@body)))
+(defmacro with-view [[view-name-binding] & body]
+ `(do-with-temp-obj "view_%s"
+ (fn [view-nm#] [(str "CREATE VIEW `v3_test_data.%s` AS "
+ "SELECT v.id AS id, v.name AS venue_name, c.name AS category_name "
+ "FROM `%s.v3_test_data.venues` v "
+ "LEFT JOIN `%s.v3_test_data.categories` c "
+ "ON v.category_id = c.id "
+ "ORDER BY v.id ASC "
+ "LIMIT 3")
+ view-nm#
+ (bigquery.tx/project-id)
+ (bigquery.tx/project-id)])
+ (fn [view-nm#] ["DROP VIEW IF EXISTS `v3_test_data.%s`" view-nm#])
+ (fn [~(or view-name-binding '_)] ~@body)))
+
+(def ^:private numeric-val "-1.2E20")
+(def ^:private decimal-val "2.3E16")
+(def ^:private bignumeric-val "-7.5E30")
+(def ^:private bigdecimal-val "5.2E35")
+
+(defmacro with-numeric-types-table [[table-name-binding] & body]
+ `(do-with-temp-obj "table_%s"
+ (fn [tbl-nm#] [(str "CREATE TABLE `v3_test_data.%s` AS SELECT "
+ "NUMERIC '%s' AS numeric_col, "
+ "DECIMAL '%s' AS decimal_col, "
+ "BIGNUMERIC '%s' AS bignumeric_col, "
+ "BIGDECIMAL '%s' AS bigdecimal_col")
+ tbl-nm#
+ ~numeric-val
+ ~decimal-val
+ ~bignumeric-val
+ ~bigdecimal-val])
+ (fn [tbl-nm#] ["DROP TABLE IF EXISTS `v3_test_data.%s`" tbl-nm#])
+ (fn [~(or table-name-binding '_)] ~@body)))
(deftest sync-views-test
(mt/test-driver :bigquery-cloud-sdk
@@ -256,3 +277,41 @@
(mt/run-mbql-query taxi_trips
{:filter [:= [:field (mt/id :taxi_trips :unique_key) nil]
"67794e631648a002f88d4b7f3ab0bcb6a9ed306a"]}))))))))))
+
+(deftest bigquery-specific-types-test
+ (testing "Table with decimal types"
+ (with-numeric-types-table [tbl-nm]
+ (is (contains? (:tables (driver/describe-database :bigquery-cloud-sdk (mt/db)))
+ {:schema nil, :name tbl-nm})
+ "`describe-database` should see the table")
+ (is (= {:schema nil
+ :name tbl-nm
+ :fields #{{:name "numeric_col", :database-type "NUMERIC", :base-type :type/Decimal, :database-position 0}
+ {:name "decimal_col", :database-type "NUMERIC", :base-type :type/Decimal, :database-position 1}
+ {:name "bignumeric_col"
+ :database-type "BIGNUMERIC"
+ :base-type :type/Decimal
+ :database-position 2}
+ {:name "bigdecimal_col"
+ :database-type "BIGNUMERIC"
+ :base-type :type/Decimal
+ :database-position 3}}}
+ (driver/describe-table :bigquery-cloud-sdk (mt/db) {:name tbl-nm}))
+ "`describe-table` should see the fields in the table")
+ (sync/sync-database! (mt/db))
+ (testing "We should be able to run queries against the table"
+ (doseq [[col-nm param-v] [[:numeric_col (bigdec numeric-val)]
+ [:decimal_col (bigdec decimal-val)]
+ [:bignumeric_col (bigdec bignumeric-val)]
+ [:bigdecimal_col (bigdec bigdecimal-val)]]]
+ (testing (format "filtering against %s" col-nm))
+ (is (= 1
+ (-> (mt/first-row
+ (mt/run-mbql-query nil
+ {:source-table (mt/id tbl-nm)
+ :aggregation [[:count]]
+ :parameters [{:name col-nm
+ :type :number/=
+ :target [:field (mt/id tbl-nm col-nm)]
+ :value [param-v]}]}))
+ first))))))))
--
GitLab