From dad00648a79f529a252c221ecc5d4e6fb1d1b41f Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Fri, 11 Feb 2022 15:15:50 -0800
Subject: [PATCH] BigQuery should qualify columns from source query with
`source` in the `SELECT` clause (#20434)
* Fix BigQuery not qualifying source columns
* Don't fix on 42 for the deprecated BigQuery driver
* You know what I'll fix it after all. Why not.
* Oops I updated the wrong part of the test
---
.../bigquery_cloud_sdk/query_processor.clj | 8 +-
.../query_processor_test.clj | 31 ++++----
.../explicit_joins_test.clj | 75 ++++++++++---------
3 files changed, 58 insertions(+), 56 deletions(-)
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 61ca55437dd..5b4d2f0086e 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
@@ -511,12 +511,8 @@
;; if the Field is from a join or source table, record this fact so that we know never to qualify it with the
;; project ID no matter what
(binding [*field-is-from-join-or-source-query?* (not (integer? source-table))]
- ;; if this Field is from a source table DO NOT qualify it at all.
- (let [field-clause (cond-> field-clause
- (= source-table ::add/source)
- (mbql.u/update-field-options assoc ::add/source-table ::add/none))]
- (-> (parent-method driver field-clause)
- (with-temporal-type (temporal-type field-clause)))))))
+ (-> (parent-method driver field-clause)
+ (with-temporal-type (temporal-type field-clause))))))
(defmethod sql.qp/->honeysql [:bigquery-cloud-sdk :relative-datetime]
[driver clause]
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 0fcc601694a..0ed1631cf95 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
@@ -787,8 +787,8 @@
:breakout [!month.date]}
:limit 2})]
(mt/with-native-query-testing-context query
- (is (sql= {:select '[count AS count
- count (*) AS count_2]
+ (is (sql= {:select '[source.count AS count
+ count (*) AS count_2]
:from [(let [prefix (project-id-prefix-if-set)]
{:select ['date_trunc (list (symbol (str prefix 'v3_test_data.checkins.date)) 'month) 'AS 'date
'count '(*) 'AS 'count]
@@ -819,17 +819,20 @@
{:name "CE", :display-name "CE"}]]
:limit 10}))))))))
-
(deftest no-qualify-breakout-field-name-with-subquery-test
(mt/test-driver :bigquery-cloud-sdk
- (testing "Breakout field name is not qualified if it is from source query (#18742)"
- (is (sql= '{:select [source AS source
- count (*) AS count]
- :from [(select 1 as val "2" as source)
- source]
- :group-by [source]
- :order-by [source ASC]}
- (mt/mbql-query checkins
- {:aggregation [[:count]]
- :breakout [[:field "source" {:base-type :type/Text}]],
- :source-query {:native "select 1 as `val`, '2' as `source`"}}))))))
+ (testing "Make sure columns name `source` in source query work correctly (#18742)"
+ (let [query (mt/mbql-query checkins
+ {:aggregation [[:count]]
+ :breakout [[:field "source" {:base-type :type/Text}]],
+ :source-query {:native "select 1 as `val`, '2' as `source`"}})]
+ (is (sql= '{:select [source.source AS source
+ count (*) AS count]
+ :from [(select 1 as val "2" as source)
+ source]
+ :group-by [source]
+ :order-by [source ASC]}
+ query))
+ (mt/with-native-query-testing-context query
+ (is (= [["2" 1]]
+ (mt/rows (qp/process-query query)))))))))
diff --git a/test/metabase/query_processor_test/explicit_joins_test.clj b/test/metabase/query_processor_test/explicit_joins_test.clj
index ed4ab22bb2a..e1c7221b6fb 100644
--- a/test/metabase/query_processor_test/explicit_joins_test.clj
+++ b/test/metabase/query_processor_test/explicit_joins_test.clj
@@ -690,39 +690,42 @@
(qp/process-query query))))))))))
(deftest join-against-multiple-saved-questions-with-same-column-test
- (testing "Should be able to join multiple against saved questions on the same column (#15863)"
- (mt/dataset sample-dataset
- (let [q1 (mt/mbql-query products {:breakout [$category], :aggregation [[:count]]})
- q2 (mt/mbql-query products {:breakout [$category], :aggregation [[:sum $price]]})
- q3 (mt/mbql-query products {:breakout [$category], :aggregation [[:avg $rating]]})
- metadata (fn [query]
- {:post [(some? %)]}
- (-> query qp/process-query :data :results_metadata :columns))
- query-card (fn [query]
- {:dataset_query query, :result_metadata (metadata query)})]
- (mt/with-temp* [Card [{card-1-id :id} (query-card q1)]
- Card [{card-2-id :id} (query-card q2)]
- Card [{card-3-id :id} (query-card q3)]]
- (let [query (mt/mbql-query products
- {:source-table (format "card__%d" card-1-id)
- :joins [{:fields :all
- :source-table (format "card__%d" card-2-id)
- :condition [:=
- $category
- &Q2.category]
- :alias "Q2"}
- {:fields :all
- :source-table (format "card__%d" card-3-id)
- :condition [:=
- $category
- &Q3.category]
- :alias "Q3"}]})]
- (mt/with-native-query-testing-context query
- (let [results (qp/process-query query)]
- (is (= ["Category" "Count" "Q2 → Category" "Q2 → Sum" "Q3 → Category" "Q3 → Avg"]
- (map :display_name (get-in results [:data :results_metadata :columns]))))
- (is (= [["Doohickey" 42 "Doohickey" 2185.89 "Doohickey" 3.73]
- ["Gadget" 53 "Gadget" 3019.2 "Gadget" 3.43]
- ["Gizmo" 51 "Gizmo" 2834.88 "Gizmo" 3.64]
- ["Widget" 54 "Widget" 3109.31 "Widget" 3.15]]
- (mt/formatted-rows [str int str 2.0 str 2.0] results)))))))))))
+ (testing "Should be able to join multiple against saved questions on the same column (#15863, #20362, #20413)"
+ (mt/test-drivers (mt/normal-drivers-with-feature :nested-queries :left-join)
+ (mt/dataset sample-dataset
+ (let [q1 (mt/mbql-query products {:breakout [$category], :aggregation [[:count]]})
+ q2 (mt/mbql-query products {:breakout [$category], :aggregation [[:sum $price]]})
+ q3 (mt/mbql-query products {:breakout [$category], :aggregation [[:avg $rating]]})
+ metadata (fn [query]
+ {:post [(some? %)]}
+ (-> query qp/process-query :data :results_metadata :columns))
+ query-card (fn [query]
+ {:dataset_query query, :result_metadata (metadata query)})]
+ (mt/with-temp* [Card [{card-1-id :id} (query-card q1)]
+ Card [{card-2-id :id} (query-card q2)]
+ Card [{card-3-id :id} (query-card q3)]]
+ (let [query (mt/mbql-query products
+ {:source-table (format "card__%d" card-1-id)
+ :joins [{:fields :all
+ :source-table (format "card__%d" card-2-id)
+ :condition [:=
+ $category
+ &Q2.category]
+ :alias "Q2"}
+ {:fields :all
+ :source-table (format "card__%d" card-3-id)
+ :condition [:=
+ $category
+ &Q3.category]
+ :alias "Q3"}]
+ :order-by [[:asc $category]]})]
+ (mt/with-native-query-testing-context query
+ (let [results (qp/process-query query)]
+ (when (#{:postgres :h2} driver/*driver*)
+ (is (= ["Category" "Count" "Q2 → Category" "Q2 → Sum" "Q3 → Category" "Q3 → Avg"]
+ (map :display_name (get-in results [:data :results_metadata :columns])))))
+ (is (= [["Doohickey" 42 "Doohickey" 2185.89 "Doohickey" 3.73]
+ ["Gadget" 53 "Gadget" 3019.2 "Gadget" 3.43]
+ ["Gizmo" 51 "Gizmo" 2834.88 "Gizmo" 3.64]
+ ["Widget" 54 "Widget" 3109.31 "Widget" 3.15]]
+ (mt/formatted-rows [str int str 2.0 str 2.0] results))))))))))))
--
GitLab