Merge branch 'only-nest-expressions-in-breakouts-or-aggregations' int…

…o offset-part-2
metabase · May 6, 2024 · b99b38b · b99b38b
2 parents 0e032bc + c98cb0c
commit b99b38b
Show file tree

Hide file tree

Showing 3 changed files with 147 additions and 226 deletions.
diff --git a/src/metabase/query_processor/util/nest_query.clj b/src/metabase/query_processor/util/nest_query.clj
@@ -144,18 +144,37 @@
  (assoc join :qp/refs (:qp/refs query)))
  joins))))))
 
+(defn- should-nest-expressions?
+ "Whether we should nest the expressions in a inner query; true if
+
+ 1. there are some expression definitions in the inner query, AND
+
+ 2. there are some breakouts OR some aggregations in the inner query
+
+ 3. AND the breakouts/aggregations contain at least `:expression` reference."
+ [{:keys [expressions], breakouts :breakout, aggregations :aggregation, :as _inner-query}]
+ (and
+ ;; 1. has some expression definitions
+ (seq expressions)
+ ;; 2. has some breakouts or aggregations
+ (or (seq breakouts)
+ (seq aggregations))
+ ;; 3. contains an `:expression` ref
+ (lib.util.match/match-one (concat breakouts aggregations)
+ :expression)))
+
 (defn nest-expressions
  "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."
- [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
+ [inner-query]
+ (let [{:keys [expressions], :as inner-query} (m/update-existing inner-query :source-query nest-expressions)]
+ (if-not (should-nest-expressions? inner-query)
+ inner-query
+ (let [{:keys [source-query], :as inner-query} (nest-source inner-query)
+ inner-query  (rewrite-fields-and-expressions inner-query)
+ source-query  (assoc source-query :expressions expressions)]
+ (-> inner-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
@@ -373,22 +373,14 @@
  source.LONGITUDE AS LONGITUDE
  source.PRICE AS PRICE
  source.double_id AS double_id]
- :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]}
- AS source]}
+ :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]}
  AS source]
  :limit [1]}
  (-> (lib.tu.macros/mbql-query venues
@@ -479,42 +471,30 @@
  sql.qp-test-util/sql->sql-map)))))
 
 (def ^:private reference-aggregation-expressions-in-joins-test-expected-sql
- '{: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.RelativePrice AS RelativePrice
- source.CategoriesStats__CATEGORY_ID AS CategoriesStats__CATEGORY_ID
- source.CategoriesStats__MaxPrice AS CategoriesStats__MaxPrice
- source.CategoriesStats__AvgPrice AS CategoriesStats__AvgPrice
- source.CategoriesStats__MinPrice AS CategoriesStats__MinPrice]
- :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
- CAST (VENUES.PRICE AS float)
- /
- CASE WHEN CategoriesStats.AvgPrice = 0 THEN NULL
- ELSE CategoriesStats.AvgPrice END AS RelativePrice
- CategoriesStats.CATEGORY_ID AS CategoriesStats__CATEGORY_ID
- CategoriesStats.MaxPrice AS CategoriesStats__MaxPrice
- CategoriesStats.AvgPrice AS CategoriesStats__AvgPrice
- CategoriesStats.MinPrice AS CategoriesStats__MinPrice]
- :from [VENUES]
- :left-join [{:select [VENUES.CATEGORY_ID AS CATEGORY_ID
- MAX (VENUES.PRICE) AS MaxPrice
- AVG (VENUES.PRICE) AS AvgPrice
- MIN (VENUES.PRICE) AS MinPrice]
- :from [VENUES]
- :group-by [VENUES.CATEGORY_ID]
- :order-by [VENUES.CATEGORY_ID ASC]} AS CategoriesStats
- ON VENUES.CATEGORY_ID = CategoriesStats.CATEGORY_ID]}
- AS source]
- :limit [3]})
+ '{: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
+ CAST (VENUES.PRICE AS float)
+ /
+ CASE WHEN CategoriesStats.AvgPrice = 0 THEN NULL
+ ELSE CategoriesStats.AvgPrice END AS RelativePrice
+ CategoriesStats.CATEGORY_ID AS CategoriesStats__CATEGORY_ID
+ CategoriesStats.MaxPrice AS CategoriesStats__MaxPrice
+ CategoriesStats.AvgPrice AS CategoriesStats__AvgPrice
+ CategoriesStats.MinPrice AS CategoriesStats__MinPrice]
+ :from [VENUES]
+ :left-join [{:select [VENUES.CATEGORY_ID AS CATEGORY_ID
+ MAX (VENUES.PRICE) AS MaxPrice
+ AVG (VENUES.PRICE) AS AvgPrice
+ MIN (VENUES.PRICE) AS MinPrice]
+ :from [VENUES]
+ :group-by [VENUES.CATEGORY_ID]
+ :order-by [VENUES.CATEGORY_ID ASC]} AS CategoriesStats
+ ON VENUES.CATEGORY_ID = CategoriesStats.CATEGORY_ID]
+ :limit [3]})
 
 (deftest ^:parallel reference-aggregation-expressions-in-joins-test
  (testing "See if we can correctly compile a query that references expressions that come from a join"
@@ -588,12 +568,9 @@
  [:expression "test"]]
  :limit 1})]
  (testing "Generated SQL"
- (is (= '{:select [source.PRICE AS PRICE
- source.test AS test]
- :from [{:select [TIMESTAMPADD ("second" VENUES.PRICE timestamp "1970-01-01T00:00:00Z") AS PRICE
- 1 * 1 AS test]
- :from [VENUES]}
- AS source]
+ (is (= '{:select [TIMESTAMPADD ("second" VENUES.PRICE timestamp "1970-01-01T00:00:00Z") AS PRICE
+ 1 * 1 AS test]
+ :from [VENUES]
  :limit [1]}
  (-> query mbql->native sql.qp-test-util/sql->sql-map)))
  (testing "Results"
@@ -804,19 +781,14 @@
 
 (deftest ^:parallel floating-point-division-test
  (testing "Make sure FLOATING POINT division is done when dividing by expressions/fields"
- (is (= '{:select [source.my_cool_new_field AS my_cool_new_field]
- :from [{:select [VENUES.ID AS ID
- VENUES.PRICE AS PRICE
- VENUES.PRICE + 2 AS big_price
- CAST
- (VENUES.PRICE AS float)
- /
- CASE WHEN (VENUES.PRICE + 2) = 0 THEN NULL
- ELSE VENUES.PRICE + 2
- END AS my_cool_new_field]
- :from [VENUES]}
- AS source]
- :order-by [source.ID ASC]
+ (is (= '{:select [CAST
+ (VENUES.PRICE AS float)
+ /
+ CASE WHEN (VENUES.PRICE + 2) = 0 THEN NULL
+ ELSE VENUES.PRICE + 2
+ END AS my_cool_new_field]
+ :from [VENUES]
+ :order-by [VENUES.ID ASC]
  :limit [3]}
  (-> (lib.tu.macros/mbql-query venues
  {:expressions {:big_price [:+ $price 2]
@@ -829,10 +801,8 @@
 
 (deftest ^:parallel floating-point-division-test-2
  (testing "Don't generate unneeded casts to FLOAT for the numerator if it is a number literal"
- (is (= '{:select [source.my_cool_new_field AS my_cool_new_field]
- :from [{:select [2.0 / 4.0 AS my_cool_new_field]
- :from [VENUES]}
- AS source]
+ (is (= '{:select [2.0 / 4.0 AS my_cool_new_field]
+ :from [VENUES]
  :limit [1]}
  (-> (lib.tu.macros/mbql-query venues
  {:expressions {:my_cool_new_field [:/ 2 4]}
@@ -900,30 +870,22 @@
  Q1.CC AS Q1__CC]
  :from [{:select [source.CATEGORY AS CATEGORY
  source.count AS count
- 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]}
- AS source]}
+ 1 + 1 AS CC]
+ :from [{:select [PRODUCTS.CATEGORY AS CATEGORY
+ COUNT (*) AS count]
+ :from [PRODUCTS]
+ :group-by [PRODUCTS.CATEGORY]
+ :order-by [PRODUCTS.CATEGORY ASC]}
  AS source]}
  AS source]
  :left-join [{:select [source.CATEGORY AS CATEGORY
  source.count AS count
- 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]}
- AS source]}
+ 1 + 1 AS CC]
+ :from [{:select [PRODUCTS.CATEGORY AS CATEGORY
+ COUNT (*) AS count]
+ :from [PRODUCTS]
+ :group-by [PRODUCTS.CATEGORY]
+ :order-by [PRODUCTS.CATEGORY ASC]}
  AS source]}
  AS Q1 ON source.CC = Q1.CC]
  :limit [1]}