Skip to content
Snippets Groups Projects
Unverified Commit b67ca77b authored by Cam Saul's avatar Cam Saul Committed by GitHub
Browse files

Only nest expressions referenced in breakouts or aggregations (#42302) 

* Only nest expressions referenced in breakouts or aggregations

* Test fixes :wrench:

* Oracle test update

* Improved Oracle test
parent 7249b7df
Branches
Tags
No related merge requests found
Expand all Hide whitespace changes
Inline Side-by-side
Showing
with 225 additions and 297 deletions
.clj-kondo/hooks/clojure/test.clj
+ 0
− 2
View file @ b67ca77b
......@@ -31,7 +31,6 @@
metabase.test.util.log/with-log-messages-for-level
metabase.test.util.misc/with-single-admin-user
metabase.test.util/with-all-users-permission
metabase.test.util/with-column-remappings
metabase.test.util/with-discarded-collections-perms-changes
metabase.test.util/with-env-keys-renamed-by
metabase.test.util/with-locale
......@@ -49,7 +48,6 @@
metabase.test/with-actions-test-data-and-actions-enabled
metabase.test/with-actions-test-data-tables
metabase.test/with-all-users-permission
metabase.test/with-column-remappings
metabase.test/with-discarded-collections-perms-changes
metabase.test/with-env-keys-renamed-by
metabase.test/with-expected-messages
......
modules/drivers/oracle/test/metabase/driver/oracle_test.clj
+ 45
− 39
View file @ b67ca77b
......@@ -12,6 +12,7 @@
[metabase.driver.sql-jdbc.sync :as sql-jdbc.sync]
[metabase.driver.sql.query-processor :as sql.qp]
[metabase.driver.util :as driver.u]
[metabase.lib.test-util :as lib.tu]
[metabase.models.database :refer [Database]]
[metabase.models.field :refer [Field]]
[metabase.models.table :refer [Table]]
......@@ -225,48 +226,53 @@
[[username-binding & [username]] & body]
`(do-with-temp-user ~username (fn [~username-binding] ~@body)))
(deftest subselect-test
(deftest ^:parallel subselect-test
(testing "Don't try to generate queries with SELECT (...) AS source, Oracle hates `AS`"
;; TODO -- seems WACK that we actually have to create objects for this to work and can't just stick them in the QP
;; store.
(t2.with-temp/with-temp [Database db {:name "db"
:engine :oracle}
Table table {:db_id (:id db)
:schema "public"
:name "table"}
Field field {:table_id (:id table)
:name "field"
:display_name "Field"
:database_type "char"
:base_type :type/Text}]
(qp.store/with-metadata-provider (u/the-id db)
(let [hsql (sql.qp/mbql->honeysql :oracle
{:query {:source-table (:id table)
:expressions {"s" [:substring [:field (:id field) nil] 2]}
:fields [[:expression "s"]]
:limit 3}})]
(testing (format "Honey SQL =\n%s" (u/pprint-to-str hsql))
(is (= [["SELECT"
" *"
"FROM"
" ("
" SELECT"
" \"source\".\"s\" \"s\""
" FROM"
" ("
" SELECT"
" \"public\".\"table\".\"field\" \"field\","
" SUBSTR(\"public\".\"table\".\"field\", 2) \"s\""
" FROM"
" \"public\".\"table\""
" ) \"source\""
" )"
"WHERE"
" rownum <= 3"]]
(-> (sql.qp/format-honeysql :oracle hsql)
vec
(update 0 (partial driver/prettify-native-form :oracle))
(update 0 str/split-lines))))))))))
(qp.store/with-metadata-provider (lib.tu/mock-metadata-provider
{:database {:id 1
:name "db"
:engine :oracle}
:tables [{:id 1
:db-id 1
:schema "public"
:name "table"}]
:fields [{:id 1
:table-id 1
:name "field"
:display-name "Field"
:database-type "char"
:base-type :type/Text}]})
(let [hsql (sql.qp/mbql->honeysql :oracle
{:query {:source-query {:source-table 1
:expressions {"s" [:substring [:field 1 nil] 2]}
:fields [[:field 1 nil]
[:expression "s"]]}
:fields [[:field "s" {:base-type :type/Text}]]
:limit 3}})]
(testing (format "Honey SQL =\n%s" (u/pprint-to-str hsql))
(is (= [["SELECT"
" *"
"FROM"
" ("
" SELECT"
" \"source\".\"s\" \"s\""
" FROM"
" ("
" SELECT"
" \"public\".\"table\".\"field\" \"field\","
" SUBSTR(\"public\".\"table\".\"field\", 2) \"s\""
" FROM"
" \"public\".\"table\""
" ) \"source\""
" )"
"WHERE"
" rownum <= 3"]]
(-> (sql.qp/format-honeysql :oracle hsql)
vec
(update 0 (partial driver/prettify-native-form :oracle))
(update 0 str/split-lines)))))))))
(deftest return-clobs-as-text-test
(mt/test-driver :oracle
......
src/metabase/models/params/custom_values.clj
+ 23
− 21
View file @ b67ca77b
......@@ -7,6 +7,7 @@
"
(:require
[clojure.string :as str]
[metabase.lib.util.match :as lib.util.match]
[metabase.models.card :refer [Card]]
[metabase.models.interface :as mi]
[metabase.query-processor :as qp]
......@@ -49,28 +50,29 @@
Maybe we should lower it for the sake of displaying a parameter dropdown."
1000)
(defn- values-from-card-query
[card value-field query]
(let [value-base-type (:base_type (qp.util/field->field-info value-field (:result_metadata card)))
expressions (get-in card [:dataset_query :query :expressions])]
[card value-field-ref query]
(let [value-base-type (:base_type (qp.util/field->field-info value-field-ref (:result_metadata card)))
value-field-ref (lib.util.match/replace value-field-ref
[:expression expr-name opts]
[:field expr-name (merge {:base-type value-base-type} opts)]
[:expression expr-name]
[:field expr-name {:base-type value-base-type}])]
{:database (:database_id card)
:type :query
:query (merge
(cond-> {:source-table (format "card__%d" (:id card))
:breakout [value-field]
:limit *max-rows*}
expressions
(assoc :expressions expressions))
{:filter [:and
[(if (isa? value-base-type :type/Text)
:not-empty
:not-null)
value-field]
(when query
(if-not (isa? value-base-type :type/Text)
[:= value-field query]
[:contains [:lower value-field] (u/lower-case-en query)]))]})
:query {:source-table (format "card__%d" (:id card))
:breakout [value-field-ref]
:limit *max-rows*
:filter [:and
[(if (isa? value-base-type :type/Text)
:not-empty
:not-null)
value-field-ref]
(when query
(if-not (isa? value-base-type :type/Text)
[:= value-field-ref query]
[:contains [:lower value-field-ref] (u/lower-case-en query)]))]}
:middleware {:disable-remaps? true}}))
(mu/defn values-from-card
......@@ -91,9 +93,9 @@
(values-from-card card value-field nil))
([card :- (ms/InstanceOf Card)
value-field :- ms/Field
value-field-ref :- ms/LegacyFieldOrExpressionReference
query :- [:any]]
(let [mbql-query (values-from-card-query card value-field query)
(let [mbql-query (values-from-card-query card value-field-ref query)
result (qp/process-query mbql-query)
values (get-in result [:data :rows])]
{:values values
......
src/metabase/query_processor/util/nest_query.clj
+ 27
− 8
View file @ b67ca77b
......@@ -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)))))
src/metabase/util/malli/schema.clj
+ 5
− 4
View file @ b67ca77b
......@@ -161,8 +161,9 @@
(isa? k :Relation/*))))]
(deferred-tru "value must be a valid field semantic or relation type (keyword or string).")))
(def Field
"Schema for a valid Field for API usage."
(def LegacyFieldOrExpressionReference
"Schema for a valid legacy `:field` or `:expression` reference for API usage. TODO -- why are these passed into the
REST API at all? MBQL clauses are not things we should ask for as API parameters."
(mu/with-api-error-message
[:fn (fn [k]
((comp (mc/validator mbql.s/Field)
......@@ -271,8 +272,8 @@
[:map
[:values {:optional true} [:* :any]]
[:card_id {:optional true} PositiveInt]
[:value_field {:optional true} Field]
[:label_field {:optional true} Field]]))
[:value_field {:optional true} LegacyFieldOrExpressionReference]
[:label_field {:optional true} LegacyFieldOrExpressionReference]]))
(def RemappedFieldValue
"Has two components:
......
test/metabase/driver/sql/query_processor_test.clj
+ 57
− 95
View file @ b67ca77b
......@@ -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]}
......
test/metabase/models/params/custom_values_test.clj
+ 4
− 4
View file @ b67ca77b
......@@ -9,7 +9,7 @@
;;; --------------------------------------------- source=card ----------------------------------------------
(deftest with-mbql-card-test
(deftest ^:parallel with-mbql-card-test
(doseq [model? [true false]]
(testing (format "source card is a %s" (if model? "model" "question"))
(binding [custom-values/*max-rows* 3]
......@@ -33,7 +33,7 @@
(mt/$ids $venues.name)
"bakery"))))))))))
(deftest with-mbql-card-test-2
(deftest ^:parallel with-mbql-card-test-2
(doseq [model? [true false]]
(testing (format "source card is a %s" (if model? "model" "question"))
(binding [custom-values/*max-rows* 3]
......@@ -73,7 +73,7 @@
[:field (mt/id :categories :name) {:source-field (mt/id :venues :category_id)}]
"bakery"))))))))))
(deftest with-mbql-card-test-3
(deftest ^:parallel with-mbql-card-test-3
(doseq [model? [true false]]
(testing (format "source card is a %s" (if model? "model" "question"))
(binding [custom-values/*max-rows* 3]
......@@ -100,7 +100,7 @@
(mt/$ids $venues.category_id)
2)))))))))))
(deftest with-native-card-test
(deftest ^:parallel with-native-card-test
(doseq [model? [true false]]
(testing (format "source card is a %s with native question" (if model? "model" "question"))
(binding [custom-values/*max-rows* 3]
......
test/metabase/query_processor/util/nest_query_test.clj
+ 63
− 123
View file @ b67ca77b
This diff is collapsed.
test/metabase/util/malli/schema_test.clj
+ 1
− 1
View file @ b67ca77b
......@@ -42,7 +42,7 @@
{:schema ms/FieldSemanticTypeKeywordOrString
:failed-cases [1 :type/FK]
:success-cases [:type/Category "type/Category"]}
{:schema ms/Field
{:schema ms/LegacyFieldOrExpressionReference
:failed-cases [[:aggregation 0] [:field "name" {}]]
:success-cases [[:field 3 nil] ["field" "name" {:base-type :type/Float}]]}
{:schema ms/Map
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment