From bca22d9766fd665e46700b325db17717f8e21f28 Mon Sep 17 00:00:00 2001
From: Jeff Evans <jeff303@users.noreply.github.com>
Date: Tue, 28 Sep 2021 11:16:46 -0500
Subject: [PATCH] Fix BigQuery Percentile and Median expressions not using
correct backtick quoting (#17979)
Fix BigQuery Percentile and Median expressions not using correct backtick quoting
Ensure the identifier components are quoted by reifying `ToSql` for the `APPROX_QUANTILES` expression
Add test to ensure query is created properly
Mark `:percentile-aggregations` as being supported in the new driver
---
.../metabase/driver/bigquery_cloud_sdk.clj | 2 +-
.../bigquery_cloud_sdk/query_processor.clj | 22 +++++++++++++++----
.../query_processor_test.clj | 15 +++++++++++++
3 files changed, 34 insertions(+), 5 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 75330103412..60827be18eb 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
@@ -286,7 +286,7 @@
;;; | Other Driver Method Impls |
;;; +----------------------------------------------------------------------------------------------------------------+
-(defmethod driver/supports? [:bigquery-cloud-sdk :percentile-aggregations] [_ _] false)
+(defmethod driver/supports? [:bigquery-cloud-sdk :percentile-aggregations] [_ _] true)
(defmethod driver/supports? [:bigquery-cloud-sdk :expressions] [_ _] true)
diff --git a/modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk/query_processor.clj b/modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk/query_processor.clj
index 2ed3d53f37e..7ff0259293e 100644
--- a/modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk/query_processor.clj
+++ b/modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk/query_processor.clj
@@ -23,6 +23,7 @@
[metabase.util.date-2 :as u.date]
[metabase.util.honeysql-extensions :as hx]
[metabase.util.i18n :refer [tru]]
+ [pretty.core :refer [PrettyPrintable]]
[schema.core :as s]
[toucan.db :as db])
(:import [com.google.cloud.bigquery Field$Mode FieldValue]
@@ -415,13 +416,26 @@
[(Math/round x) (Math/round (Math/pow 10 power))]
(recur (* 10 x) (inc power)))))
+(defn- approx-quantiles
+ "HoneySQL form for the APPROX_QUANTILES invocation. The [OFFSET(...)] part after the function call is odd and
+ needs special treatment."
+ [driver expr offset quantiles]
+ (let [expr-hsql (sql.qp/->honeysql driver expr)]
+ (reify
+ hformat/ToSql
+ (to-sql [_]
+ (format "APPROX_QUANTILES(%s, %s)[OFFSET(%s)]"
+ (hformat/to-sql expr-hsql)
+ quantiles
+ offset))
+ PrettyPrintable
+ (pretty [_]
+ (format "APPROX_QUANTILES(%s, %s)[OFFSET(%s)]" (pr-str expr) (pr-str quantiles) (pr-str offset))))))
+
(defmethod sql.qp/->honeysql [:bigquery-cloud-sdk :percentile]
[driver [_ arg p]]
(let [[offset quantiles] (percentile->quantile p)]
- (hsql/raw (format "APPROX_QUANTILES(%s, %s)[OFFSET(%s)]"
- (hformat/to-sql (sql.qp/->honeysql driver arg))
- quantiles
- offset))))
+ (approx-quantiles driver arg offset quantiles)))
(defmethod sql.qp/->honeysql [:bigquery-cloud-sdk :median]
[driver [_ arg]]
diff --git a/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk/query_processor_test.clj b/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk/query_processor_test.clj
index d3405ab6bb7..870b80d5266 100644
--- a/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk/query_processor_test.clj
+++ b/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk/query_processor_test.clj
@@ -821,3 +821,18 @@
:source-query {:source-table (mt/id :checkins)
:aggregation [[:count]]
:breakout [!month.date]}})))))))
+
+(deftest custom-expression-args-quoted
+ (mt/test-driver :bigquery-cloud-sdk
+ (mt/dataset sample-dataset
+ (testing "Arguments to custom aggregation expression functions have backticks applied properly"
+ (is (= {:mbql? true
+ :params nil
+ :table-name "orders"
+ :query (str "SELECT APPROX_QUANTILES(`v3_sample_dataset.orders`.`quantity`, 10)[OFFSET(5)] AS `CE`"
+ " FROM `v3_sample_dataset.orders` LIMIT 10")}
+ (qp/query->native (mt/mbql-query orders
+ {:aggregation [[:aggregation-options
+ [:percentile $orders.quantity 0.5]
+ {:name "CE", :display-name "CE"}]]
+ :limit 10}))))))))
--
GitLab