From d6bc5e93876c6421e97300937fcaf27735d4f80a Mon Sep 17 00:00:00 2001
From: metamben <103100869+metamben@users.noreply.github.com>
Date: Thu, 4 Aug 2022 21:39:47 +0300
Subject: [PATCH] Recursively nest expressions (#24404)
---
.../23862-cc-group-by-nested.cy.spec.js | 2 +-
.../query_processor/util/nest_query.clj | 21 ++--
.../driver/sql/query_processor_test.clj | 51 ++++++----
.../query_processor/util/nest_query_test.clj | 97 ++++++++++++-------
.../nested_queries_test.clj | 14 +++
5 files changed, 120 insertions(+), 65 deletions(-)
diff --git a/frontend/test/metabase/scenarios/custom-column/reproductions/23862-cc-group-by-nested.cy.spec.js b/frontend/test/metabase/scenarios/custom-column/reproductions/23862-cc-group-by-nested.cy.spec.js
index a1d4ba5a04a..35d59a628af 100644
--- a/frontend/test/metabase/scenarios/custom-column/reproductions/23862-cc-group-by-nested.cy.spec.js
+++ b/frontend/test/metabase/scenarios/custom-column/reproductions/23862-cc-group-by-nested.cy.spec.js
@@ -22,7 +22,7 @@ const questionDetails = {
},
};
-describe.skip("issue 23862", () => {
+describe("issue 23862", () => {
beforeEach(() => {
restore();
cy.signInAsAdmin();
diff --git a/src/metabase/query_processor/util/nest_query.clj b/src/metabase/query_processor/util/nest_query.clj
index 36dfe1fd8da..2f91124162c 100644
--- a/src/metabase/query_processor/util/nest_query.clj
+++ b/src/metabase/query_processor/util/nest_query.clj
@@ -100,13 +100,14 @@
"Pushes the `:source-table`/`:source-query`, `:expressions`, and `:joins` in the top-level of the query into a
`:source-query` and updates `:expression` references and `:field` clauses with `:join-alias`es accordingly. See
tests for examples. This is used by the SQL QP to make sure expressions happen in a subselect."
- [{:keys [expressions], :as query}]
- (if (empty? expressions)
- query
- (let [{:keys [source-query], :as query} (nest-source query)
- query (rewrite-fields-and-expressions query)
- source-query (assoc source-query :expressions expressions)]
- (-> query
- (dissoc :source-query :expressions)
- (assoc :source-query source-query)
- add/add-alias-info))))
+ [query]
+ (let [{:keys [expressions], :as query} (m/update-existing query :source-query nest-expressions)]
+ (if (empty? expressions)
+ query
+ (let [{:keys [source-query], :as query} (nest-source query)
+ query (rewrite-fields-and-expressions query)
+ source-query (assoc source-query :expressions expressions)]
+ (-> query
+ (dissoc :source-query :expressions)
+ (assoc :source-query source-query)
+ add/add-alias-info)))))
diff --git a/test/metabase/driver/sql/query_processor_test.clj b/test/metabase/driver/sql/query_processor_test.clj
index 17cf559a156..a4bc882044b 100644
--- a/test/metabase/driver/sql/query_processor_test.clj
+++ b/test/metabase/driver/sql/query_processor_test.clj
@@ -347,14 +347,22 @@
source.LONGITUDE AS LONGITUDE
source.PRICE AS PRICE
source.double_id AS double_id]
- :from [{:select [VENUES.ID AS ID
- VENUES.NAME AS NAME
- VENUES.CATEGORY_ID AS CATEGORY_ID
- VENUES.LATITUDE AS LATITUDE
- VENUES.LONGITUDE AS LONGITUDE
- VENUES.PRICE AS PRICE
- (VENUES.ID * 2) AS double_id]
- :from [VENUES]}
+ :from [{:select [source.ID AS ID
+ source.NAME AS NAME
+ source.CATEGORY_ID AS CATEGORY_ID
+ source.LATITUDE AS LATITUDE
+ source.LONGITUDE AS LONGITUDE
+ source.PRICE AS PRICE
+ source.double_id AS double_id]
+ :from [{:select [VENUES.ID AS ID
+ VENUES.NAME AS NAME
+ VENUES.CATEGORY_ID AS CATEGORY_ID
+ VENUES.LATITUDE AS LATITUDE
+ VENUES.LONGITUDE AS LONGITUDE
+ VENUES.PRICE AS PRICE
+ (VENUES.ID * 2) AS double_id]
+ :from [VENUES]}
+ source]}
source]
:limit [1]}
(-> (mt/mbql-query venues
@@ -843,19 +851,24 @@
Q1.CC AS Q1__CC]
:from [{:select [source.CATEGORY AS CATEGORY
source.count AS count
- (1 + 1) AS CC]
- :from [{:select [PRODUCTS.CATEGORY AS CATEGORY
- count (*) AS count]
- :from [PRODUCTS]
- :group-by [PRODUCTS.CATEGORY]
- :order-by [PRODUCTS.CATEGORY ASC]}
- source]} source]
+ source.CC AS CC]
+ :from [{:select [source.CATEGORY AS CATEGORY
+ source.count AS count
+ (1 + 1) AS CC]
+ :from [{:select [PRODUCTS.CATEGORY AS CATEGORY
+ count (*) AS count]
+ :from [PRODUCTS]
+ :group-by [PRODUCTS.CATEGORY]
+ :order-by [PRODUCTS.CATEGORY ASC]}
+ source]}
+ source]}
+ source]
:left-join [{:select [source.CATEGORY AS CATEGORY
- source.count AS count
- source.CC AS CC]
+ source.count AS count
+ source.CC AS CC]
:from [{:select [source.CATEGORY AS CATEGORY
- source.count AS count
- (1 + 1) AS CC]
+ source.count AS count
+ (1 + 1) AS CC]
:from [{:select [PRODUCTS.CATEGORY AS CATEGORY
count (*) AS count]
:from [PRODUCTS]
diff --git a/test/metabase/query_processor/util/nest_query_test.clj b/test/metabase/query_processor/util/nest_query_test.clj
index c4e411f84eb..abbc5010b18 100644
--- a/test/metabase/query_processor/util/nest_query_test.clj
+++ b/test/metabase/query_processor/util/nest_query_test.clj
@@ -214,41 +214,68 @@
:limit 1})]
(mt/with-native-query-testing-context query
(is (partial= (mt/$ids venues
- {:fields [[:field %id {::add/source-table ::add/source
- ::add/source-alias "ID"
- ::add/desired-alias "ID"
- ::add/position 0}]
- [:field "x_2" {:base-type :type/Float
- ::add/source-table ::add/source
- ::add/source-alias "x_2"
- ::add/desired-alias "x_2"
- ::add/position 1}]]
- :source-query {:expressions {"x" [:*
- [:field %price {::add/source-table ::add/source
- ::add/source-alias "PRICE"}]
- 4]}
- :fields [[:field %id {::add/source-table ::add/source
- ::add/source-alias "ID"
- ::add/desired-alias "ID"
- ::add/position 0}]
- [:field "x" {:base-type :type/Float
- ::add/source-table ::add/source
- ::add/source-alias "x"
- ::add/desired-alias "x"
- ::add/position 1}]
- [:expression "x" {::add/desired-alias "x_2"
- ::add/position 2}]]
- :source-query {:source-table $$venues
- :expressions {"x" [:*
- [:field %price {::add/source-table $$venues
- ::add/source-alias "PRICE"}]
- 2]}
- :fields [[:field %id {::add/source-table $$venues
- ::add/source-alias "ID"
- ::add/desired-alias "ID"
- ::add/position 0}]
- [:expression "x" {::add/desired-alias "x"
- ::add/position 1}]]}}
+ {:fields
+ [[:field %id #::add{:source-table ::add/source
+ :source-alias "ID"
+ :desired-alias "ID"
+ :position 0}]
+ [:field "x_2" {:base-type :type/Float
+ ::add/source-table ::add/source
+ ::add/source-alias "x_2"
+ ::add/desired-alias "x_2"
+ ::add/position 1}]]
+ :source-query
+ {:expressions
+ {"x" [:*
+ [:field %price #::add{:source-table ::add/source
+ :source-alias "PRICE"}]
+ 4]}
+ :fields
+ [[:field %id #::add{:source-table ::add/source
+ :source-alias "ID"
+ :desired-alias "ID"}]
+ [:field "x" {:base-type :type/Float
+ ::add/source-table ::add/source
+ ::add/source-alias "x"
+ ::add/desired-alias "x"}]
+ [:expression "x" #::add{:desired-alias "x_2"}]]
+ :source-query
+ {:fields
+ [[:field %id #::add{:source-table ::add/source
+ :source-alias "ID"
+ :desired-alias "ID"}]
+ [:field "x" {:base-type :type/Float
+ ::add/source-table ::add/source
+ ::add/source-alias "x"
+ ::add/desired-alias "x"}]]
+ :source-query
+ {:source-table $$venues
+ :expressions
+ {"x" [:*
+ [:field %price #::add{:source-table $$venues
+ :source-alias "PRICE"
+ :desired-alias "PRICE"}]
+ 2]}
+ :fields
+ [[:field %id #::add{:source-table $$venues
+ :source-alias "ID"
+ :desired-alias "ID"}]
+ [:field %name #::add{:source-table $$venues
+ :source-alias "NAME"
+ :desired-alias "NAME"}]
+ [:field %category_id #::add{:source-table $$venues
+ :source-alias "CATEGORY_ID"
+ :desired-alias "CATEGORY_ID"}]
+ [:field %latitude #::add{:source-table $$venues
+ :source-alias "LATITUDE"
+ :desired-alias "LATITUDE"}]
+ [:field %longitude #::add{:source-table $$venues
+ :source-alias "LONGITUDE"
+ :desired-alias "LONGITUDE"}]
+ [:field %price #::add{:source-table $$venues
+ :source-alias "PRICE"
+ :desired-alias "PRICE"}]
+ [:expression "x" #::add{:desired-alias "x"}]]}}}
:limit 1})
(nest-expressions query))))))
(testing "Ignores source-query from joins (#20809)"
diff --git a/test/metabase/query_processor_test/nested_queries_test.clj b/test/metabase/query_processor_test/nested_queries_test.clj
index 73d8731f271..b195b7427ed 100644
--- a/test/metabase/query_processor_test/nested_queries_test.clj
+++ b/test/metabase/query_processor_test/nested_queries_test.clj
@@ -267,6 +267,20 @@
{:aggregation [:count]
:breakout [$price]}))))))))))
+(deftest grouped-expression-in-card-test
+ (testing "Nested grouped expressions work (#23862)."
+ (mt/with-temp Card [card {:dataset_query
+ (mt/mbql-query venues
+ {:aggregation [[:count]]
+ :breakout [[:expression "Price level"]]
+ :expressions {"Price level" [:case [[[:> $price 2] "expensive"]] {:default "budget"}]}
+ :limit 2})}]
+ (is (= [["budget" 81]
+ ["expensive" 19]]
+ (mt/rows
+ (qp/process-query
+ (query-with-source-card card))))))))
+
(deftest card-id-native-source-queries-test
(let [run-native-query
(fn [sql]
--
GitLab