diff --git a/src/metabase/driver.clj b/src/metabase/driver.clj index 194574ee280793d158141927557c3d27bc514832..3dd3a943b0984ea99cb44543f1e5020855614f5e 100644 --- a/src/metabase/driver.clj +++ b/src/metabase/driver.clj @@ -491,13 +491,14 @@ (defmulti mbql->native "Transpile an MBQL query into the appropriate native query form. `query` will match the schema for an MBQL query in - `metabase.mbql.schema/Query`; this function should return a native query that conforms to that schema. + [[metabase.mbql.schema/Query]]; this function should return a native query that conforms to that schema. - If the underlying query language supports remarks or comments, the driver should use `query->remark` to generate an - appropriate message and include that in an appropriate place; alternatively a driver might directly include the - query's `:info` dictionary if the underlying language is JSON-based. + If the underlying query language supports remarks or comments, the driver should + use [[metabase.query-processor.util/query->remark]] to generate an appropriate message and include that in an + appropriate place; alternatively a driver might directly include the query's `:info` dictionary if the underlying + language is JSON-based. - The result of this function will be passed directly into calls to `execute-reducible-query`. + The result of this function will be passed directly into calls to [[execute-reducible-query]]. For example, a driver like Postgres would build a valid SQL expression and return a map such as: diff --git a/src/metabase/query_processor.clj b/src/metabase/query_processor.clj index 857ad7c185846d0ae83c27278d3407907c0993d3..85ef335c85bd94fd3c956afffb8304c28ca82fde 100644 --- a/src/metabase/query_processor.clj +++ b/src/metabase/query_processor.clj @@ -74,64 +74,124 @@ [column-level-perms-check :as ee.sandbox.columns] [row-level-restrictions :as ee.sandbox.rows]])) -;; ▼▼▼ POST-PROCESSING ▼▼▼ happens from TOP-TO-BOTTOM -(def default-middleware - "The default set of middleware applied to queries ran via `process-query`." - [#'mbql-to-native/mbql->native ; EXECUTE - #'check-features/check-features ; PRE - #'limit/limit-result-rows-middleware ; POST - #'limit/add-default-limit-middleware ; PRE - #'cache/maybe-return-cached-results ; EXECUTE - #'optimize-temporal-filters/optimize-temporal-filters ; PRE - #'validate-temporal-bucketing/validate-temporal-bucketing ; PRE - #'auto-parse-filter-values/auto-parse-filter-values ; PRE - #'wrap-value-literals/wrap-value-literals ; PRE - #'annotate/add-column-info ; POST - #'perms/check-query-permissions ; EXECUTE - #'pre-alias-ags/pre-alias-aggregations ; PRE - #'cumulative-ags/sum-cumulative-aggregation-columns-middleware ; POST - #'cumulative-ags/rewrite-cumulative-aggregations-middleware ; PRE +(def ^:private pre-processing-middleware + "Pre-processing middleware. Has the form + + (f query) -> query + + in 43+." + [#'check-features/check-features + #'limit/add-default-limit-middleware + #'optimize-temporal-filters/optimize-temporal-filters + #'validate-temporal-bucketing/validate-temporal-bucketing + #'auto-parse-filter-values/auto-parse-filter-values + #'wrap-value-literals/wrap-value-literals + #'pre-alias-ags/pre-alias-aggregations + #'cumulative-ags/rewrite-cumulative-aggregations-middleware ;; yes, this is called a second time, because we need to handle any joins that got added - (resolve 'ee.sandbox.rows/merge-sandboxing-metadata-middleware) ; POST - (resolve 'ee.sandbox.rows/apply-sandboxing-middleware) ; PRE - #'viz-settings/update-viz-settings ; POST - #'fix-bad-refs/fix-bad-references-middleware ; PRE - #'resolve-joined-fields/resolve-joined-fields ; PRE - #'resolve-joins/resolve-joins ; PRE - #'add-implicit-joins/add-implicit-joins ; PRE - #'large-int-id/convert-id-to-string ; POST - #'format-rows/format-rows ; POST - #'add-default-temporal-unit/add-default-temporal-unit ; PRE - #'desugar/desugar ; PRE - #'binning/update-binning-strategy ; PRE - #'resolve-fields/resolve-fields ; PRE - #'add-dim/remap-results-middleware ; POST - #'add-dim/add-remapped-columns-middleware ; PRE - #'implicit-clauses/add-implicit-clauses ; PRE - (resolve 'ee.sandbox.rows/merge-sandboxing-metadata-middleware) ; POST - (resolve 'ee.sandbox.rows/apply-sandboxing-middleware) ; PRE - #'upgrade-field-literals/upgrade-field-literals ; PRE - #'add-source-metadata/add-source-metadata-for-source-queries ; PRE - (resolve 'ee.sandbox.columns/maybe-apply-column-level-perms-check) ; EXECUTE - #'reconcile-bucketing/reconcile-breakout-and-order-by-bucketing ; PRE - #'bucket-datetime/auto-bucket-datetimes ; PRE - #'resolve-source-table/resolve-source-tables ; PRE - #'parameters/substitute-parameters ; PRE - #'resolve-referenced/resolve-referenced-card-resources ; PRE - #'expand-macros/expand-macros ; PRE - #'add-timezone-info/add-timezone-info ; POST - #'splice-params-in-response/splice-params-in-response ; POST - #'resolve-database-and-driver/resolve-database-and-driver ; AROUND - #'fetch-source-query/resolve-card-id-source-tables ; PRE - #'store/initialize-store ; AROUND - #'validate/validate-query ; PRE - #'perms/remove-permissions-key-middleware ; PRE - #'normalize/normalize ; PRE - #'add-rows-truncated/add-rows-truncated ; POST - (resolve 'ee.audit/handle-internal-queries) ; AROUND - #'results-metadata/record-and-return-metadata!]) ; EXECUTE + (resolve 'ee.sandbox.rows/apply-sandboxing-middleware) + #'fix-bad-refs/fix-bad-references-middleware + #'resolve-joined-fields/resolve-joined-fields + #'resolve-joins/resolve-joins + #'add-implicit-joins/add-implicit-joins + #'add-default-temporal-unit/add-default-temporal-unit + #'desugar/desugar + #'binning/update-binning-strategy + #'resolve-fields/resolve-fields + #'add-dim/add-remapped-columns-middleware + #'implicit-clauses/add-implicit-clauses + (resolve 'ee.sandbox.rows/apply-sandboxing-middleware) + #'upgrade-field-literals/upgrade-field-literals + #'add-source-metadata/add-source-metadata-for-source-queries + #'reconcile-bucketing/reconcile-breakout-and-order-by-bucketing + #'bucket-datetime/auto-bucket-datetimes + #'resolve-source-table/resolve-source-tables + #'parameters/substitute-parameters + #'resolve-referenced/resolve-referenced-card-resources + #'expand-macros/expand-macros + #'validate/validate-query + #'perms/remove-permissions-key-middleware]) ;; ▲▲▲ PRE-PROCESSING ▲▲▲ happens from BOTTOM-TO-TOP +(def ^:private compile-middleware + "Middleware for query compilation. Happens after pre-processing. Has the form + + (f query) -> query + + in 43+." + [#'mbql-to-native/mbql->native]) + +(def ^:private execution-middleware + "Middleware that happens after compilation, AROUND query execution itself. Has the form + + (f qp) -> qp + + Where `qp` has the form + + (f query rff context)" + ;; TODO -- limit SEEMS like it should be post-processing but it actually has to happen only if we don't return cached + ;; results. Otherwise things break. There's probably some way to fix this. e.g. maybe it doesn't do anything if the + ;; query has the `:cached?` key. + [#'limit/limit-result-rows-middleware + #'cache/maybe-return-cached-results + #'perms/check-query-permissions + (resolve 'ee.sandbox.columns/maybe-apply-column-level-perms-check)]) + +(def ^:private post-processing-middleware + "Post-processing middleware that transforms results. Has the form + + (f preprocessed-query rff) -> rff + + Where `rff` has the form + + (f metadata) -> rf" + ;; ▼▼▼ POST-PROCESSING ▼▼▼ happens from TOP-TO-BOTTOM + [#'annotate/add-column-info + #'cumulative-ags/sum-cumulative-aggregation-columns-middleware + #'viz-settings/update-viz-settings + #'large-int-id/convert-id-to-string + #'format-rows/format-rows + #'add-dim/remap-results-middleware + (resolve 'ee.sandbox.rows/merge-sandboxing-metadata-middleware) + #'add-timezone-info/add-timezone-info + #'splice-params-in-response/splice-params-in-response + #'add-rows-truncated/add-rows-truncated-middleware]) + +(def ^:private around-middleware + "Middleware that goes AROUND *all* the other middleware (even for pre-processing only or compilation only). Has the + form + + (f qp) -> qp + + Where `qp` has the form + + (f query rff context)" + [#'resolve-database-and-driver/resolve-database-and-driver + #'fetch-source-query/resolve-card-id-source-tables + #'store/initialize-store + ;; `normalize` has to be done at the very beginning or `resolve-card-id-source-tables` and the like might not work. + ;; It doesn't really need to be 'around' middleware tho. + #'normalize/normalize + (resolve 'ee.audit/handle-internal-queries) + ;; TODO -- I think this is actually supposed to be post-processing middleware? #idk¿? + #'results-metadata/record-and-return-metadata!]) + +;; query -> preprocessed = around + pre-process +;; query -> native = around + pre-process + compile +;; query -> results = around + pre-process + compile + execute + post-process = default-middleware + +(def default-middleware + "The default set of middleware applied to queries ran via [[process-query]]." + (into + [] + (comp cat (keep identity)) + [execution-middleware ; → → execute → → ↓ + compile-middleware ; ↑ compile ↓ + post-processing-middleware ; ↑ ↓ post-process + pre-processing-middleware ; ↑ pre-process ↓ + around-middleware])) ; ↑ query ↓ results + + ;; In REPL-based dev rebuild the QP every time it is called; this way we don't need to reload this namespace when ;; middleware is changed. Outside of dev only build the QP once for performance/locality (defn- base-qp [middleware] diff --git a/src/metabase/query_processor/middleware/add_rows_truncated.clj b/src/metabase/query_processor/middleware/add_rows_truncated.clj index dda6d2419a69290b919aba5a1b6dec2dba08e047..049970ec2b0da59e650ce1df039ac398384b4290 100644 --- a/src/metabase/query_processor/middleware/add_rows_truncated.clj +++ b/src/metabase/query_processor/middleware/add_rows_truncated.clj @@ -1,10 +1,17 @@ (ns metabase.query-processor.middleware.add-rows-truncated "Adds `:rows_truncated` to the query results if the results were truncated because of the query's constraints." (:require [metabase.query-processor.interface :as i] - [metabase.query-processor.util :as qputil])) + [metabase.query-processor.middleware.forty-three :as m.43] + [metabase.query-processor.middleware.limit :as limit])) -(defn- results-limit [{{:keys [max-results max-results-bare-rows]} :constraints, :as query}] - (or (when (qputil/query-without-aggregations-or-limits? query) +(defn- results-limit + [{{:keys [max-results max-results-bare-rows]} :constraints + {aggregations :aggregation, :keys [limit page], ::limit/keys [original-limit]} :query + :as query}] + (or (when (and (or (not limit) + (= original-limit nil)) + (not page) + (empty? aggregations)) max-results-bare-rows) max-results i/absolute-max-results)) @@ -26,13 +33,12 @@ (vswap! row-count inc) (rf result row))))) -(defn add-rows-truncated +(defn- add-rows-truncated [query rff] + (fn add-rows-truncated-rff* [metadata] + (add-rows-truncated-xform (results-limit query) (rff metadata)))) + +(def add-rows-truncated-middleware "Add `:rows_truncated` to the result if the results were truncated because of the query's constraints. Only affects QP results that are reduced to a map (e.g. the default reducing function; other reducing functions such as streaming to a CSV are unaffected.)" - [qp] - (fn [query rff context] - (qp query - (fn [metadata] - (add-rows-truncated-xform (results-limit query) (rff metadata))) - context))) + (m.43/wrap-43-post-processing-middleware add-rows-truncated)) diff --git a/src/metabase/query_processor/middleware/forty_three.clj b/src/metabase/query_processor/middleware/forty_three.clj index 95ec2fcf4234ba641e79e5eee5704e7690cc73d8..2e86530a72c68e15ed0ea790348e21c3f376bb8b 100644 --- a/src/metabase/query_processor/middleware/forty_three.clj +++ b/src/metabase/query_processor/middleware/forty_three.clj @@ -14,8 +14,8 @@ (f qp) -> (f query rff context)" [f] - (fn [qp] - (fn [query rff context] + (fn wrap-43-pre-processing-middleware-qp* [qp] + (fn wrap-43-pre-processing-middleware-fn* [query rff context] (qp (f query) rff context)))) (defn wrap-43-post-processing-middleware @@ -31,6 +31,6 @@ (f qp) -> (f query rff context)" [f] - (fn [qp] - (fn [query rff context] + (fn wrap-43-post-processing-middleware-qp* [qp] + (fn wrap-43-post-processing-middleware-fn* [query rff context] (qp query (f query rff) context)))) diff --git a/src/metabase/query_processor/middleware/limit.clj b/src/metabase/query_processor/middleware/limit.clj index 98f443015eb7b2c1a0397d7c67c0f00e04654d2e..1adacdcffff36ab507e0290f04397c0495f49f7c 100644 --- a/src/metabase/query_processor/middleware/limit.clj +++ b/src/metabase/query_processor/middleware/limit.clj @@ -8,11 +8,11 @@ ;;;; Pre-processing -(defn- add-limit [max-rows {query-type :type, :as query}] +(defn- add-limit [max-rows {query-type :type, {original-limit :limit}, :query, :as query}] (cond-> query (and (= query-type :query) (qputil/query-without-aggregations-or-limits? query)) - (assoc-in [:query :limit] max-rows))) + (update :query assoc :limit max-rows, ::original-limit original-limit))) (defn determine-query-max-rows "Given a `query`, return the max rows that should be returned. This is the first non-nil value from (in decreasing diff --git a/src/metabase/query_processor/middleware/results_metadata.clj b/src/metabase/query_processor/middleware/results_metadata.clj index 17d2f6f4ec170513f1d6a5abc6627460680c3498..bcdb87912c17849b44a4ca2f9c81342d1bf79824 100644 --- a/src/metabase/query_processor/middleware/results_metadata.clj +++ b/src/metabase/query_processor/middleware/results_metadata.clj @@ -4,12 +4,12 @@ as a checksum in the API response." (:require [clojure.tools.logging :as log] [metabase.driver :as driver] + [metabase.query-processor.middleware.forty-three :as m.43] [metabase.query-processor.reducible :as qp.reducible] [metabase.sync.analyze.query-results :as analyze.results] [metabase.util.i18n :refer [tru]] [toucan.db :as db])) - ;;; +----------------------------------------------------------------------------------------------------------------+ ;;; | Middleware | ;;; +----------------------------------------------------------------------------------------------------------------+ @@ -67,16 +67,15 @@ (rf (cond-> result (map? result) (update :data assoc - :results_metadata {:columns metadata} + :results_metadata {:columns metadata} :insights insights))))))) -(defn record-and-return-metadata! +(defn- record-and-return-metadata!* + [{{:keys [skip-results-metadata?]} :middleware, :as query} rff] + (let [record! (partial record-metadata! query)] + (fn record-and-return-metadata!-rff* [metadata] + (insights-xform metadata record! (rff metadata))))) + +(def record-and-return-metadata! "Middleware that records metadata about the columns returned when running the query." - [qp] - (fn [{{:keys [skip-results-metadata?]} :middleware, :as query} rff context] - (if skip-results-metadata? - (qp query rff context) - (let [record! (partial record-metadata! query) - rff' (fn [metadata] - (insights-xform metadata record! (rff metadata)))] - (qp query rff' context))))) + (m.43/wrap-43-post-processing-middleware record-and-return-metadata!*)) diff --git a/src/metabase/query_processor/reducible.clj b/src/metabase/query_processor/reducible.clj index 1c6a922dc3e3494a41c77a915ee92f30de57859f..f7563d6e1f0e62d71100c32d056f43473f953048 100644 --- a/src/metabase/query_processor/reducible.clj +++ b/src/metabase/query_processor/reducible.clj @@ -41,6 +41,8 @@ [middleware] (reduce (fn [qp middleware] + (when (var? middleware) + (assert (not (:private (meta middleware))) (format "%s is private" (pr-str middleware)))) (if (some? middleware) (middleware qp) qp)) @@ -194,7 +196,7 @@ {:pre [(fn? primary-rf) (sequential? additional-rfs) (every? fn? additional-rfs) (fn? combine)]} (let [additional-accs (volatile! (mapv (fn [rf] (rf)) additional-rfs))] - (fn + (fn combine-additional-reducing-fns-rf* ([] (primary-rf)) ([acc] diff --git a/src/metabase/query_processor/util.clj b/src/metabase/query_processor/util.clj index 1a2400570ef13c744cc89aa64d8373e64c86449f..5f7cb24a141e9f6345dd0cf1301455ee1c9adb32 100644 --- a/src/metabase/query_processor/util.clj +++ b/src/metabase/query_processor/util.clj @@ -9,7 +9,7 @@ [metabase.util.schema :as su] [schema.core :as s])) -;; TODO - I think most of the functions in this namespace that we don't remove could be moved to `metabase.mbql.util` +;; TODO - I think most of the functions in this namespace that we don't remove could be moved to [[metabase.mbql.util]] (defn ^:deprecated mbql-query? ;; not really needed anymore since we don't need to normalize tokens "Is the given query an MBQL query? @@ -22,7 +22,7 @@ [{{aggregations :aggregation, :keys [limit page]} :query}] (and (not limit) (not page) - (nil? aggregations))) + (empty? aggregations))) (defn default-query->remark "Generates the default query remark. Exists as a separate function so that overrides of the query->remark multimethod @@ -39,7 +39,7 @@ (defmulti query->remark "Generate an appropriate remark `^String` to be prepended to a query to give DBAs additional information about the query - being executed. See documentation for `mbql->native` and #2386. + being executed. See documentation for [[metabase.driver/mbql->native]] and #2386. for more information. So this turns your average 10, 20, 30 character query into a 110, 120, 130 etc character query. @@ -48,8 +48,7 @@ 'Hey, this is a 20 character query! What's it talking about, error at position 120?' This gets fixed, but in a spooky-action-at-a-distance way, in - `frontend/src/metabase/query_builder/components/VisualizationError.jsx` - " + `frontend/src/metabase/query_builder/components/VisualizationError.jsx`" {:arglists '(^String [driver query])} driver/dispatch-on-initialized-driver :hierarchy #'driver/hierarchy) diff --git a/test/metabase/query_processor/middleware/add_rows_truncated_test.clj b/test/metabase/query_processor/middleware/add_rows_truncated_test.clj index bf0c291b1a3c3e0c89363266228b47f606da5167..1cbe9c41e98014a4c8f05fc7a79e6ed22bdeab75 100644 --- a/test/metabase/query_processor/middleware/add_rows_truncated_test.clj +++ b/test/metabase/query_processor/middleware/add_rows_truncated_test.clj @@ -6,7 +6,7 @@ (defn- add-rows-truncated [query rows] (:result - (mt/test-qp-middleware add-rows-truncated/add-rows-truncated query rows))) + (mt/test-qp-middleware add-rows-truncated/add-rows-truncated-middleware query rows))) (deftest add-rows-truncated-test (testing "the default behavior is to treat the query as no aggregation and use :max-results-bare-rows" diff --git a/test/metabase/query_processor/middleware/limit_test.clj b/test/metabase/query_processor/middleware/limit_test.clj index bbd28cfc956b6d2620a21ddf5cc373339ffb9a42..fc6f2091917a2aa0aa59e2b4fdf3f7139cf7814a 100644 --- a/test/metabase/query_processor/middleware/limit_test.clj +++ b/test/metabase/query_processor/middleware/limit_test.clj @@ -38,6 +38,7 @@ ":row_count should match the limit added by middleware") (is (= {:constraints {:max-results 46} :type :query - :query {:limit 46}} + :query {:limit 46 + ::limit/original-limit nil}} (#'limit/add-default-limit query)) "Preprocessed query should have :limit added to it")))) diff --git a/test/metabase/query_processor/util/nest_query_test.clj b/test/metabase/query_processor/util/nest_query_test.clj index 05df20391a07280114b0d45758cb9b2bd0c88fd2..92abf605b1d46a8281d494053a7d4a563ae1ba60 100644 --- a/test/metabase/query_processor/util/nest_query_test.clj +++ b/test/metabase/query_processor/util/nest_query_test.clj @@ -27,180 +27,180 @@ remove-source-metadata)))) (deftest nest-expressions-test - (is (query= (mt/$ids venues - {:source-query {:source-table $$venues - :expressions {"double_price" [:* [:field %price {::add/source-table $$venues - ::add/source-alias "PRICE" - ::add/desired-alias "PRICE" - ::add/position 5}] - 2]} - :fields [[:field %id {::add/source-table $$venues - ::add/source-alias "ID" - ::add/desired-alias "ID" - ::add/position 0}] - [:field %name {::add/source-table $$venues - ::add/source-alias "NAME" - ::add/desired-alias "NAME" - ::add/position 1}] - [:field %category_id {::add/source-table $$venues - ::add/source-alias "CATEGORY_ID" - ::add/desired-alias "CATEGORY_ID" - ::add/position 2}] - [:field %latitude {::add/source-table $$venues - ::add/source-alias "LATITUDE" - ::add/desired-alias "LATITUDE" - ::add/position 3}] - [:field %longitude {::add/source-table $$venues - ::add/source-alias "LONGITUDE" - ::add/desired-alias "LONGITUDE" - ::add/position 4}] - [:field %price {::add/source-table $$venues - ::add/source-alias "PRICE" - ::add/desired-alias "PRICE" - ::add/position 5}] - [:expression "double_price" {::add/desired-alias "double_price" - ::add/position 6}]]} - :breakout [[:field %price {::add/source-table ::add/source - ::add/source-alias "PRICE" - ::add/desired-alias "PRICE" - ::add/position 0}]] - :aggregation [[:aggregation-options [:count] {:name "count" - ::add/desired-alias "count" - ::add/position 1}]] - :fields [[:field "double_price" {:base-type :type/Float - ::add/source-table ::add/source - ::add/source-alias "double_price" - ::add/desired-alias "double_price" - ::add/position 2}]] - :order-by [[:asc [:field %price {::add/source-table ::add/source - ::add/source-alias "PRICE" - ::add/desired-alias "PRICE" - ::add/position 0}]]]}) - (nest-expressions - (mt/mbql-query venues - {:expressions {"double_price" [:* $price 2]} - :breakout [$price] - :aggregation [[:count]] - :fields [[:expression "double_price"]]}))))) + (is (partial= (mt/$ids venues + {:source-query {:source-table $$venues + :expressions {"double_price" [:* [:field %price {::add/source-table $$venues + ::add/source-alias "PRICE" + ::add/desired-alias "PRICE" + ::add/position 5}] + 2]} + :fields [[:field %id {::add/source-table $$venues + ::add/source-alias "ID" + ::add/desired-alias "ID" + ::add/position 0}] + [:field %name {::add/source-table $$venues + ::add/source-alias "NAME" + ::add/desired-alias "NAME" + ::add/position 1}] + [:field %category_id {::add/source-table $$venues + ::add/source-alias "CATEGORY_ID" + ::add/desired-alias "CATEGORY_ID" + ::add/position 2}] + [:field %latitude {::add/source-table $$venues + ::add/source-alias "LATITUDE" + ::add/desired-alias "LATITUDE" + ::add/position 3}] + [:field %longitude {::add/source-table $$venues + ::add/source-alias "LONGITUDE" + ::add/desired-alias "LONGITUDE" + ::add/position 4}] + [:field %price {::add/source-table $$venues + ::add/source-alias "PRICE" + ::add/desired-alias "PRICE" + ::add/position 5}] + [:expression "double_price" {::add/desired-alias "double_price" + ::add/position 6}]]} + :breakout [[:field %price {::add/source-table ::add/source + ::add/source-alias "PRICE" + ::add/desired-alias "PRICE" + ::add/position 0}]] + :aggregation [[:aggregation-options [:count] {:name "count" + ::add/desired-alias "count" + ::add/position 1}]] + :fields [[:field "double_price" {:base-type :type/Float + ::add/source-table ::add/source + ::add/source-alias "double_price" + ::add/desired-alias "double_price" + ::add/position 2}]] + :order-by [[:asc [:field %price {::add/source-table ::add/source + ::add/source-alias "PRICE" + ::add/desired-alias "PRICE" + ::add/position 0}]]]}) + (nest-expressions + (mt/mbql-query venues + {:expressions {"double_price" [:* $price 2]} + :breakout [$price] + :aggregation [[:count]] + :fields [[:expression "double_price"]]}))))) (deftest nest-expressions-with-existing-non-expression-fields-test (testing "Other `:fields` besides the `:expressions` should be preserved in the top level" - (is (query= (mt/$ids checkins - {:source-query {:source-table $$checkins - :expressions {"double_id" [:* - [:field %checkins.id {::add/source-table $$checkins - ::add/source-alias "ID" - ::add/desired-alias "ID" - ::add/position 0}] - 2]} - :fields [[:field %id {::add/source-table $$checkins - ::add/source-alias "ID" - ::add/desired-alias "ID" - ::add/position 0}] - [:field %date {:temporal-unit :default - ::add/source-table $$checkins - ::add/source-alias "DATE" - ::add/desired-alias "DATE" - ::add/position 1}] - [:field %user_id {::add/source-table $$checkins - ::add/source-alias "USER_ID" - ::add/desired-alias "USER_ID" - ::add/position 2}] - [:field %venue_id {::add/source-table $$checkins - ::add/source-alias "VENUE_ID" - ::add/desired-alias "VENUE_ID" - ::add/position 3}] - [:expression "double_id" {::add/desired-alias "double_id" - ::add/position 4}]]} - :fields [[:field "double_id" {:base-type :type/Float - ::add/source-table ::add/source - ::add/source-alias "double_id" - ::add/desired-alias "double_id" - ::add/position 0}] - [:field %date {:temporal-unit :day - ::nest-query/outer-select true - ::add/source-table ::add/source - ::add/source-alias "DATE" - ::add/desired-alias "DATE" - ::add/position 1}] - [:field %date {:temporal-unit :month - ::nest-query/outer-select true - ::add/source-table ::add/source - ::add/source-alias "DATE" - ::add/desired-alias "DATE_2" - ::add/position 2}]] - :limit 1}) - (nest-expressions - (mt/mbql-query checkins - {:expressions {"double_id" [:* $id 2]} - :fields [[:expression "double_id"] - !day.date - !month.date] - :limit 1})))))) + (is (partial= (mt/$ids checkins + {:source-query {:source-table $$checkins + :expressions {"double_id" [:* + [:field %checkins.id {::add/source-table $$checkins + ::add/source-alias "ID" + ::add/desired-alias "ID" + ::add/position 0}] + 2]} + :fields [[:field %id {::add/source-table $$checkins + ::add/source-alias "ID" + ::add/desired-alias "ID" + ::add/position 0}] + [:field %date {:temporal-unit :default + ::add/source-table $$checkins + ::add/source-alias "DATE" + ::add/desired-alias "DATE" + ::add/position 1}] + [:field %user_id {::add/source-table $$checkins + ::add/source-alias "USER_ID" + ::add/desired-alias "USER_ID" + ::add/position 2}] + [:field %venue_id {::add/source-table $$checkins + ::add/source-alias "VENUE_ID" + ::add/desired-alias "VENUE_ID" + ::add/position 3}] + [:expression "double_id" {::add/desired-alias "double_id" + ::add/position 4}]]} + :fields [[:field "double_id" {:base-type :type/Float + ::add/source-table ::add/source + ::add/source-alias "double_id" + ::add/desired-alias "double_id" + ::add/position 0}] + [:field %date {:temporal-unit :day + ::nest-query/outer-select true + ::add/source-table ::add/source + ::add/source-alias "DATE" + ::add/desired-alias "DATE" + ::add/position 1}] + [:field %date {:temporal-unit :month + ::nest-query/outer-select true + ::add/source-table ::add/source + ::add/source-alias "DATE" + ::add/desired-alias "DATE_2" + ::add/position 2}]] + :limit 1}) + (nest-expressions + (mt/mbql-query checkins + {:expressions {"double_id" [:* $id 2]} + :fields [[:expression "double_id"] + !day.date + !month.date] + :limit 1})))))) (deftest multiple-expressions-test (testing "Make sure the nested version of the query doesn't mix up expressions if we have ones that reference others" - (is (query= (mt/$ids venues - {:source-query {:source-table $$venues - :expressions {"big_price" - [:+ - [:field %price {::add/position 5 - ::add/source-table $$venues - ::add/source-alias "PRICE" - ::add/desired-alias "PRICE"}] - 2] + (is (partial= (mt/$ids venues + {:source-query {:source-table $$venues + :expressions {"big_price" + [:+ + [:field %price {::add/position 5 + ::add/source-table $$venues + ::add/source-alias "PRICE" + ::add/desired-alias "PRICE"}] + 2] - "my_cool_new_field" - [:/ - [:field %price {::add/position 5 - ::add/source-table $$venues - ::add/source-alias "PRICE" - ::add/desired-alias "PRICE"}] - [:expression "big_price" {::add/position 6 - ::add/desired-alias "big_price"}]]} - :fields [[:field %id {::add/position 0 - ::add/source-table $$venues - ::add/source-alias "ID" - ::add/desired-alias "ID"}] - [:field %name {::add/position 1 + "my_cool_new_field" + [:/ + [:field %price {::add/position 5 + ::add/source-table $$venues + ::add/source-alias "PRICE" + ::add/desired-alias "PRICE"}] + [:expression "big_price" {::add/position 6 + ::add/desired-alias "big_price"}]]} + :fields [[:field %id {::add/position 0 ::add/source-table $$venues - ::add/source-alias "NAME" - ::add/desired-alias "NAME"}] - [:field %category_id {::add/position 2 + ::add/source-alias "ID" + ::add/desired-alias "ID"}] + [:field %name {::add/position 1 + ::add/source-table $$venues + ::add/source-alias "NAME" + ::add/desired-alias "NAME"}] + [:field %category_id {::add/position 2 + ::add/source-table $$venues + ::add/source-alias "CATEGORY_ID" + ::add/desired-alias "CATEGORY_ID"}] + [:field %latitude {::add/position 3 + ::add/source-table $$venues + ::add/source-alias "LATITUDE" + ::add/desired-alias "LATITUDE"}] + [:field %longitude {::add/position 4 ::add/source-table $$venues - ::add/source-alias "CATEGORY_ID" - ::add/desired-alias "CATEGORY_ID"}] - [:field %latitude {::add/position 3 - ::add/source-table $$venues - ::add/source-alias "LATITUDE" - ::add/desired-alias "LATITUDE"}] - [:field %longitude {::add/position 4 - ::add/source-table $$venues - ::add/source-alias "LONGITUDE" - ::add/desired-alias "LONGITUDE"}] - [:field %price {::add/position 5 - ::add/source-table $$venues - ::add/source-alias "PRICE" - ::add/desired-alias "PRICE"}] - [:expression "big_price" {::add/position 6 - ::add/desired-alias "big_price"}] - [:expression "my_cool_new_field" {::add/position 7 - ::add/desired-alias "my_cool_new_field"}]]} - :fields [[:field "my_cool_new_field" {:base-type :type/Float - ::add/position 0 - ::add/source-table ::add/source - ::add/source-alias "my_cool_new_field" - ::add/desired-alias "my_cool_new_field"}]] - :order-by [[:asc [:field %id {::add/source-table ::add/source - ::add/source-alias "ID"}]]] - :limit 3}) - (nest-expressions - (mt/mbql-query venues - {:expressions {"big_price" [:+ $price 2] - "my_cool_new_field" [:/ $price [:expression "big_price"]]} - :fields [[:expression "my_cool_new_field"]] - :order-by [[:asc $id]] - :limit 3})))))) + ::add/source-alias "LONGITUDE" + ::add/desired-alias "LONGITUDE"}] + [:field %price {::add/position 5 + ::add/source-table $$venues + ::add/source-alias "PRICE" + ::add/desired-alias "PRICE"}] + [:expression "big_price" {::add/position 6 + ::add/desired-alias "big_price"}] + [:expression "my_cool_new_field" {::add/position 7 + ::add/desired-alias "my_cool_new_field"}]]} + :fields [[:field "my_cool_new_field" {:base-type :type/Float + ::add/position 0 + ::add/source-table ::add/source + ::add/source-alias "my_cool_new_field" + ::add/desired-alias "my_cool_new_field"}]] + :order-by [[:asc [:field %id {::add/source-table ::add/source + ::add/source-alias "ID"}]]] + :limit 3}) + (nest-expressions + (mt/mbql-query venues + {:expressions {"big_price" [:+ $price 2] + "my_cool_new_field" [:/ $price [:expression "big_price"]]} + :fields [[:expression "my_cool_new_field"]] + :order-by [[:asc $id]] + :limit 3})))))) (deftest nest-expressions-ignore-source-queries-test (testing (str "When 'raising' :expression clauses, only raise ones in the current level. Handle duplicate expression " @@ -213,259 +213,125 @@ :fields [$id [:expression "x"]] :limit 1})] (mt/with-native-query-testing-context query - (is (query= (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}]]}} - :limit 1}) - (nest-expressions 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}]]}} + :limit 1}) + (nest-expressions query))))))) (deftest nest-expressions-with-joins-test (testing "If there are any `:joins`, those need to be nested into the `:source-query` as well." - (is (query= (mt/$ids venues - {:source-query {:source-table $$venues - :joins [{:strategy :left-join - :condition [:= - [:field %category_id {::add/source-table $$venues - ::add/source-alias "CATEGORY_ID" - ::add/desired-alias "CATEGORY_ID" - ::add/position 2}] - [:field %category_id {:join-alias "CategoriesStats" + (is (partial= (mt/$ids venues + {:source-query {:source-table $$venues + :joins [{:strategy :left-join + :condition [:= + [:field %category_id {::add/source-table $$venues + ::add/source-alias "CATEGORY_ID" + ::add/desired-alias "CATEGORY_ID" + ::add/position 2}] + [:field %category_id {:join-alias "CategoriesStats" + ::add/source-table "CategoriesStats" + ::add/source-alias "CATEGORY_ID" + ::add/desired-alias "CategoriesStats__CATEGORY_ID" + ::add/position 7}]] + :source-query {:source-table $$venues + :aggregation [[:aggregation-options + [:max [:field %price {::add/source-table $$venues + ::add/source-alias "PRICE"}]] + {:name "MaxPrice" + ::add/desired-alias "MaxPrice" + ::add/position 1}] + [:aggregation-options + [:avg + [:field + %price + {::add/source-table $$venues + ::add/source-alias "PRICE"}]] + {:name "AvgPrice" + ::add/desired-alias "AvgPrice" + ::add/position 2}] + [:aggregation-options + [:min [:field %price {::add/source-table $$venues + ::add/source-alias "PRICE"}]] + {:name "MinPrice" + ::add/desired-alias "MinPrice" + ::add/position 3}]] + :breakout [[:field %category_id {::add/source-table $$venues + ::add/source-alias "CATEGORY_ID" + ::add/desired-alias "CATEGORY_ID" + ::add/position 0}]] + :order-by [[:asc [:field %category_id {::add/source-table $$venues + ::add/source-alias "CATEGORY_ID" + ::add/desired-alias "CATEGORY_ID" + ::add/position 0}]]]} + :alias "CategoriesStats" + :fields [[:field %category_id {:join-alias "CategoriesStats" + ::add/source-table "CategoriesStats" + ::add/source-alias "CATEGORY_ID" + ::add/desired-alias "CategoriesStats__CATEGORY_ID" + ::add/position 7}] + [:field "MaxPrice" {:base-type :type/Integer + :join-alias "CategoriesStats" ::add/source-table "CategoriesStats" - ::add/source-alias "CATEGORY_ID" - ::add/desired-alias "CategoriesStats__CATEGORY_ID" - ::add/position 7}]] - :source-query {:source-table $$venues - :aggregation [[:aggregation-options - [:max [:field %price {::add/source-table $$venues - ::add/source-alias "PRICE"}]] - {:name "MaxPrice" - ::add/desired-alias "MaxPrice" - ::add/position 1}] - [:aggregation-options - [:avg - [:field - %price - {::add/source-table $$venues - ::add/source-alias "PRICE"}]] - {:name "AvgPrice" - ::add/desired-alias "AvgPrice" - ::add/position 2}] - [:aggregation-options - [:min [:field %price {::add/source-table $$venues - ::add/source-alias "PRICE"}]] - {:name "MinPrice" - ::add/desired-alias "MinPrice" - ::add/position 3}]] - :breakout [[:field %category_id {::add/source-table $$venues - ::add/source-alias "CATEGORY_ID" - ::add/desired-alias "CATEGORY_ID" - ::add/position 0}]] - :order-by [[:asc [:field %category_id {::add/source-table $$venues - ::add/source-alias "CATEGORY_ID" - ::add/desired-alias "CATEGORY_ID" - ::add/position 0}]]]} - :alias "CategoriesStats" - :fields [[:field %category_id {:join-alias "CategoriesStats" + ::add/source-alias "MaxPrice" + ::add/desired-alias "CategoriesStats__MaxPrice" + ::add/position 8}] + [:field "AvgPrice" {:base-type :type/Integer + :join-alias "CategoriesStats" ::add/source-table "CategoriesStats" - ::add/source-alias "CATEGORY_ID" - ::add/desired-alias "CategoriesStats__CATEGORY_ID" - ::add/position 7}] - [:field "MaxPrice" {:base-type :type/Integer - :join-alias "CategoriesStats" - ::add/source-table "CategoriesStats" - ::add/source-alias "MaxPrice" - ::add/desired-alias "CategoriesStats__MaxPrice" - ::add/position 8}] - [:field "AvgPrice" {:base-type :type/Integer - :join-alias "CategoriesStats" - ::add/source-table "CategoriesStats" - ::add/source-alias "AvgPrice" - ::add/desired-alias "CategoriesStats__AvgPrice" - ::add/position 9}] - [:field "MinPrice" {:base-type :type/Integer - :join-alias "CategoriesStats" - ::add/source-table "CategoriesStats" - ::add/source-alias "MinPrice" - ::add/desired-alias "CategoriesStats__MinPrice" - ::add/position 10}]]}] - :expressions {"RelativePrice" [:/ - [:field %price {::add/source-table $$venues - ::add/source-alias "PRICE" - ::add/desired-alias "PRICE" - ::add/position 5}] - [:field "AvgPrice" {:base-type :type/Integer - :join-alias "CategoriesStats" - ::add/source-table "CategoriesStats" - ::add/source-alias "AvgPrice" - ::add/desired-alias "CategoriesStats__AvgPrice" - ::add/position 9}]]} - :fields [[:field %id {::add/source-table $$venues - ::add/source-alias "ID" - ::add/desired-alias "ID" - ::add/position 0}] - [:field %name {::add/source-table $$venues - ::add/source-alias "NAME" - ::add/desired-alias "NAME" - ::add/position 1}] - [:field %category_id {::add/source-table $$venues - ::add/source-alias "CATEGORY_ID" - ::add/desired-alias "CATEGORY_ID" - ::add/position 2}] - [:field %latitude {::add/source-table $$venues - ::add/source-alias "LATITUDE" - ::add/desired-alias "LATITUDE" - ::add/position 3}] - [:field %longitude {::add/source-table $$venues - ::add/source-alias "LONGITUDE" - ::add/desired-alias "LONGITUDE" - ::add/position 4}] - [:field %price {::add/source-table $$venues - ::add/source-alias "PRICE" - ::add/desired-alias "PRICE" - ::add/position 5}] - [:expression "RelativePrice" {::add/desired-alias "RelativePrice" - ::add/position 6}] - [:field %category_id {:join-alias "CategoriesStats" - ::add/source-table "CategoriesStats" - ::add/source-alias "CATEGORY_ID" - ::add/desired-alias "CategoriesStats__CATEGORY_ID" - ::add/position 7}] - [:field "MaxPrice" {:base-type :type/Integer - :join-alias "CategoriesStats" - ::add/source-table "CategoriesStats" - ::add/source-alias "MaxPrice" - ::add/desired-alias "CategoriesStats__MaxPrice" - ::add/position 8}] - [:field "AvgPrice" {:base-type :type/Integer - :join-alias "CategoriesStats" - ::add/source-table "CategoriesStats" - ::add/source-alias "AvgPrice" - ::add/desired-alias "CategoriesStats__AvgPrice" - ::add/position 9}] - [:field "MinPrice" {:base-type :type/Integer - :join-alias "CategoriesStats" - ::add/source-table "CategoriesStats" - ::add/source-alias "MinPrice" - ::add/desired-alias "CategoriesStats__MinPrice" - ::add/position 10}]]} - :fields [[:field %id {::add/source-table ::add/source - ::add/source-alias "ID" - ::add/desired-alias "ID" - ::add/position 0}] - [:field %name {::add/source-table ::add/source - ::add/source-alias "NAME" - ::add/desired-alias "NAME" - ::add/position 1}] - [:field %category_id {::add/source-table ::add/source - ::add/source-alias "CATEGORY_ID" - ::add/desired-alias "CATEGORY_ID" - ::add/position 2}] - [:field %latitude {::add/source-table ::add/source - ::add/source-alias "LATITUDE" - ::add/desired-alias "LATITUDE" - ::add/position 3}] - [:field %longitude {::add/source-table ::add/source - ::add/source-alias "LONGITUDE" - ::add/desired-alias "LONGITUDE" - ::add/position 4}] - [:field %price {::add/source-table ::add/source - ::add/source-alias "PRICE" - ::add/desired-alias "PRICE" - ::add/position 5}] - [:field "RelativePrice" {:base-type :type/Float - ::add/source-table ::add/source - ::add/source-alias "RelativePrice" - ::add/desired-alias "RelativePrice" - ::add/position 6}] - [:field %category_id {:join-alias "CategoriesStats" - ::add/source-alias "CategoriesStats__CATEGORY_ID" - ::add/desired-alias "CategoriesStats__CATEGORY_ID" - ::add/source-table ::add/source - ::add/position 7}] - [:field "MaxPrice" {:base-type :type/Integer - :join-alias "CategoriesStats" - ::add/source-alias "CategoriesStats__MaxPrice" - ::add/desired-alias "CategoriesStats__MaxPrice" - ::add/source-table ::add/source - ::add/position 8}] - [:field "AvgPrice" {:base-type :type/Integer - :join-alias "CategoriesStats" - ::add/source-alias "CategoriesStats__AvgPrice" - ::add/desired-alias "CategoriesStats__AvgPrice" - ::add/source-table ::add/source - ::add/position 9}] - [:field "MinPrice" {:base-type :type/Integer - :join-alias "CategoriesStats" - ::add/source-alias "CategoriesStats__MinPrice" - ::add/desired-alias "CategoriesStats__MinPrice" - ::add/source-table ::add/source - ::add/position 10}]] - :limit 3}) - (nest-expressions - (mt/mbql-query venues - {:fields [$id - $name - $category_id - $latitude - $longitude - $price - [:expression "RelativePrice"] - &CategoriesStats.category_id - &CategoriesStats.*MaxPrice/Integer - &CategoriesStats.*AvgPrice/Integer - &CategoriesStats.*MinPrice/Integer] - :expressions {"RelativePrice" [:/ $price &CategoriesStats.*AvgPrice/Integer]} - :joins [{:strategy :left-join - :condition [:= $category_id &CategoriesStats.category_id] - :source-query {:source-table $$venues - :aggregation [[:aggregation-options [:max $price] {:name "MaxPrice"}] - [:aggregation-options [:avg $price] {:name "AvgPrice"}] - [:aggregation-options [:min $price] {:name "MinPrice"}]] - :breakout [$category_id]} - :alias "CategoriesStats" - :fields :all}] - :limit 3})))))) - -(deftest nest-expressions-eliminate-duplicate-coercion-test - (testing "If coercion happens in the source query, don't do it a second time in the parent query (#12430)" - (mt/with-temp-vals-in-db Field (mt/id :venues :price) {:coercion_strategy :Coercion/UNIXSeconds->DateTime - :effective_type :type/DateTime} - (is (query= (mt/$ids venues - {:source-query {:source-table $$venues - :expressions {"test" [:* 1 1]} + ::add/source-alias "AvgPrice" + ::add/desired-alias "CategoriesStats__AvgPrice" + ::add/position 9}] + [:field "MinPrice" {:base-type :type/Integer + :join-alias "CategoriesStats" + ::add/source-table "CategoriesStats" + ::add/source-alias "MinPrice" + ::add/desired-alias "CategoriesStats__MinPrice" + ::add/position 10}]]}] + :expressions {"RelativePrice" [:/ + [:field %price {::add/source-table $$venues + ::add/source-alias "PRICE" + ::add/desired-alias "PRICE" + ::add/position 5}] + [:field "AvgPrice" {:base-type :type/Integer + :join-alias "CategoriesStats" + ::add/source-table "CategoriesStats" + ::add/source-alias "AvgPrice" + ::add/desired-alias "CategoriesStats__AvgPrice" + ::add/position 9}]]} :fields [[:field %id {::add/source-table $$venues ::add/source-alias "ID" ::add/desired-alias "ID" @@ -486,211 +352,345 @@ ::add/source-alias "LONGITUDE" ::add/desired-alias "LONGITUDE" ::add/position 4}] - [:field %price {:temporal-unit :default - ::add/source-table $$venues + [:field %price {::add/source-table $$venues ::add/source-alias "PRICE" ::add/desired-alias "PRICE" ::add/position 5}] - [:expression "test" {::add/desired-alias "test" - ::add/position 6}]]} - :fields [[:field %price {:temporal-unit :default - ::nest-query/outer-select true - ::add/source-table ::add/source - ::add/source-alias "PRICE" - ::add/desired-alias "PRICE" - ::add/position 0}] - [:field "test" {:base-type :type/Float - ::add/source-table ::add/source - ::add/source-alias "test" - ::add/desired-alias "test" - ::add/position 1}]] - :limit 1}) + [:expression "RelativePrice" {::add/desired-alias "RelativePrice" + ::add/position 6}] + [:field %category_id {:join-alias "CategoriesStats" + ::add/source-table "CategoriesStats" + ::add/source-alias "CATEGORY_ID" + ::add/desired-alias "CategoriesStats__CATEGORY_ID" + ::add/position 7}] + [:field "MaxPrice" {:base-type :type/Integer + :join-alias "CategoriesStats" + ::add/source-table "CategoriesStats" + ::add/source-alias "MaxPrice" + ::add/desired-alias "CategoriesStats__MaxPrice" + ::add/position 8}] + [:field "AvgPrice" {:base-type :type/Integer + :join-alias "CategoriesStats" + ::add/source-table "CategoriesStats" + ::add/source-alias "AvgPrice" + ::add/desired-alias "CategoriesStats__AvgPrice" + ::add/position 9}] + [:field "MinPrice" {:base-type :type/Integer + :join-alias "CategoriesStats" + ::add/source-table "CategoriesStats" + ::add/source-alias "MinPrice" + ::add/desired-alias "CategoriesStats__MinPrice" + ::add/position 10}]]} + :fields [[:field %id {::add/source-table ::add/source + ::add/source-alias "ID" + ::add/desired-alias "ID" + ::add/position 0}] + [:field %name {::add/source-table ::add/source + ::add/source-alias "NAME" + ::add/desired-alias "NAME" + ::add/position 1}] + [:field %category_id {::add/source-table ::add/source + ::add/source-alias "CATEGORY_ID" + ::add/desired-alias "CATEGORY_ID" + ::add/position 2}] + [:field %latitude {::add/source-table ::add/source + ::add/source-alias "LATITUDE" + ::add/desired-alias "LATITUDE" + ::add/position 3}] + [:field %longitude {::add/source-table ::add/source + ::add/source-alias "LONGITUDE" + ::add/desired-alias "LONGITUDE" + ::add/position 4}] + [:field %price {::add/source-table ::add/source + ::add/source-alias "PRICE" + ::add/desired-alias "PRICE" + ::add/position 5}] + [:field "RelativePrice" {:base-type :type/Float + ::add/source-table ::add/source + ::add/source-alias "RelativePrice" + ::add/desired-alias "RelativePrice" + ::add/position 6}] + [:field %category_id {:join-alias "CategoriesStats" + ::add/source-alias "CategoriesStats__CATEGORY_ID" + ::add/desired-alias "CategoriesStats__CATEGORY_ID" + ::add/source-table ::add/source + ::add/position 7}] + [:field "MaxPrice" {:base-type :type/Integer + :join-alias "CategoriesStats" + ::add/source-alias "CategoriesStats__MaxPrice" + ::add/desired-alias "CategoriesStats__MaxPrice" + ::add/source-table ::add/source + ::add/position 8}] + [:field "AvgPrice" {:base-type :type/Integer + :join-alias "CategoriesStats" + ::add/source-alias "CategoriesStats__AvgPrice" + ::add/desired-alias "CategoriesStats__AvgPrice" + ::add/source-table ::add/source + ::add/position 9}] + [:field "MinPrice" {:base-type :type/Integer + :join-alias "CategoriesStats" + ::add/source-alias "CategoriesStats__MinPrice" + ::add/desired-alias "CategoriesStats__MinPrice" + ::add/source-table ::add/source + ::add/position 10}]] + :limit 3}) (nest-expressions (mt/mbql-query venues - {:expressions {"test" ["*" 1 1]} - :fields [$price - [:expression "test"]] - :limit 1}))))))) + {:fields [$id + $name + $category_id + $latitude + $longitude + $price + [:expression "RelativePrice"] + &CategoriesStats.category_id + &CategoriesStats.*MaxPrice/Integer + &CategoriesStats.*AvgPrice/Integer + &CategoriesStats.*MinPrice/Integer] + :expressions {"RelativePrice" [:/ $price &CategoriesStats.*AvgPrice/Integer]} + :joins [{:strategy :left-join + :condition [:= $category_id &CategoriesStats.category_id] + :source-query {:source-table $$venues + :aggregation [[:aggregation-options [:max $price] {:name "MaxPrice"}] + [:aggregation-options [:avg $price] {:name "AvgPrice"}] + [:aggregation-options [:min $price] {:name "MinPrice"}]] + :breakout [$category_id]} + :alias "CategoriesStats" + :fields :all}] + :limit 3})))))) + +(deftest nest-expressions-eliminate-duplicate-coercion-test + (testing "If coercion happens in the source query, don't do it a second time in the parent query (#12430)" + (mt/with-temp-vals-in-db Field (mt/id :venues :price) {:coercion_strategy :Coercion/UNIXSeconds->DateTime + :effective_type :type/DateTime} + (is (partial= (mt/$ids venues + {:source-query {:source-table $$venues + :expressions {"test" [:* 1 1]} + :fields [[:field %id {::add/source-table $$venues + ::add/source-alias "ID" + ::add/desired-alias "ID" + ::add/position 0}] + [:field %name {::add/source-table $$venues + ::add/source-alias "NAME" + ::add/desired-alias "NAME" + ::add/position 1}] + [:field %category_id {::add/source-table $$venues + ::add/source-alias "CATEGORY_ID" + ::add/desired-alias "CATEGORY_ID" + ::add/position 2}] + [:field %latitude {::add/source-table $$venues + ::add/source-alias "LATITUDE" + ::add/desired-alias "LATITUDE" + ::add/position 3}] + [:field %longitude {::add/source-table $$venues + ::add/source-alias "LONGITUDE" + ::add/desired-alias "LONGITUDE" + ::add/position 4}] + [:field %price {:temporal-unit :default + ::add/source-table $$venues + ::add/source-alias "PRICE" + ::add/desired-alias "PRICE" + ::add/position 5}] + [:expression "test" {::add/desired-alias "test" + ::add/position 6}]]} + :fields [[:field %price {:temporal-unit :default + ::nest-query/outer-select true + ::add/source-table ::add/source + ::add/source-alias "PRICE" + ::add/desired-alias "PRICE" + ::add/position 0}] + [:field "test" {:base-type :type/Float + ::add/source-table ::add/source + ::add/source-alias "test" + ::add/desired-alias "test" + ::add/position 1}]] + :limit 1}) + (nest-expressions + (mt/mbql-query venues + {:expressions {"test" ["*" 1 1]} + :fields [$price + [:expression "test"]] + :limit 1}))))))) (deftest multiple-joins-with-expressions-test (testing "We should be able to compile a complicated query with multiple joins and expressions correctly" (mt/dataset sample-dataset - (is (query= (mt/$ids orders - (merge {:source-query (let [id [:field %id {::add/source-table $$orders - ::add/source-alias "ID" - ::add/desired-alias "ID" - ::add/position 0}] - user-id [:field %user_id {::add/source-table $$orders - ::add/source-alias "USER_ID" - ::add/desired-alias "USER_ID" - ::add/position 1}] - product-id [:field %product_id {::add/source-table $$orders - ::add/source-alias "PRODUCT_ID" - ::add/desired-alias "PRODUCT_ID" - ::add/position 2}] - subtotal [:field %subtotal {::add/source-table $$orders - ::add/source-alias "SUBTOTAL" - ::add/desired-alias "SUBTOTAL" - ::add/position 3}] - tax [:field %tax {::add/source-table $$orders - ::add/source-alias "TAX" - ::add/desired-alias "TAX" - ::add/position 4}] - total [:field %total {::add/source-table $$orders - ::add/source-alias "TOTAL" - ::add/desired-alias "TOTAL" - ::add/position 5}] - discount [:field %discount {::add/source-table $$orders - ::add/source-alias "DISCOUNT" - ::add/desired-alias "DISCOUNT" - ::add/position 6}] - created-at [:field %created_at {:temporal-unit :default - ::add/source-table $$orders - ::add/source-alias "CREATED_AT" - ::add/desired-alias "CREATED_AT" - ::add/position 7}] - quantity [:field %quantity {::add/source-table $$orders - ::add/source-alias "QUANTITY" - ::add/desired-alias "QUANTITY" - ::add/position 8}] - pivot-grouping [:expression "pivot-grouping" {::add/desired-alias "pivot-grouping" - ::add/position 9}] - products-category [:field %products.category {:join-alias "PRODUCTS__via__PRODUCT_ID" - ::add/source-table "PRODUCTS__via__PRODUCT_ID" - ::add/source-alias "CATEGORY" - ::add/desired-alias "PRODUCTS__via__PRODUCT_ID__CATEGORY" - ::add/position 10}] - products-id [:field %products.id {:join-alias "PRODUCTS__via__PRODUCT_ID" - ::add/source-table "PRODUCTS__via__PRODUCT_ID" - ::add/source-alias "ID" - ::add/desired-alias "PRODUCTS__via__PRODUCT_ID__ID" - ::add/position 11}]] - {:source-table $$orders - :joins [{:source-table $$products - :alias "PRODUCTS__via__PRODUCT_ID" - :condition [:= product-id products-id] - :strategy :left-join - :fk-field-id %product_id}] - :expressions {"pivot-grouping" [:abs 0]} - :fields [id - user-id - product-id - subtotal - tax - total - discount - created-at - quantity - pivot-grouping - products-category - products-id]})} - (let [products-category [:field %products.category {:join-alias "PRODUCTS__via__PRODUCT_ID" + (is (partial= (mt/$ids orders + (merge {:source-query (let [id [:field %id {::add/source-table $$orders + ::add/source-alias "ID" + ::add/desired-alias "ID" + ::add/position 0}] + user-id [:field %user_id {::add/source-table $$orders + ::add/source-alias "USER_ID" + ::add/desired-alias "USER_ID" + ::add/position 1}] + product-id [:field %product_id {::add/source-table $$orders + ::add/source-alias "PRODUCT_ID" + ::add/desired-alias "PRODUCT_ID" + ::add/position 2}] + subtotal [:field %subtotal {::add/source-table $$orders + ::add/source-alias "SUBTOTAL" + ::add/desired-alias "SUBTOTAL" + ::add/position 3}] + tax [:field %tax {::add/source-table $$orders + ::add/source-alias "TAX" + ::add/desired-alias "TAX" + ::add/position 4}] + total [:field %total {::add/source-table $$orders + ::add/source-alias "TOTAL" + ::add/desired-alias "TOTAL" + ::add/position 5}] + discount [:field %discount {::add/source-table $$orders + ::add/source-alias "DISCOUNT" + ::add/desired-alias "DISCOUNT" + ::add/position 6}] + created-at [:field %created_at {:temporal-unit :default + ::add/source-table $$orders + ::add/source-alias "CREATED_AT" + ::add/desired-alias "CREATED_AT" + ::add/position 7}] + quantity [:field %quantity {::add/source-table $$orders + ::add/source-alias "QUANTITY" + ::add/desired-alias "QUANTITY" + ::add/position 8}] + pivot-grouping [:expression "pivot-grouping" {::add/desired-alias "pivot-grouping" + ::add/position 9}] + products-category [:field %products.category {:join-alias "PRODUCTS__via__PRODUCT_ID" + ::add/source-table "PRODUCTS__via__PRODUCT_ID" + ::add/source-alias "CATEGORY" + ::add/desired-alias "PRODUCTS__via__PRODUCT_ID__CATEGORY" + ::add/position 10}] + products-id [:field %products.id {:join-alias "PRODUCTS__via__PRODUCT_ID" + ::add/source-table "PRODUCTS__via__PRODUCT_ID" + ::add/source-alias "ID" + ::add/desired-alias "PRODUCTS__via__PRODUCT_ID__ID" + ::add/position 11}]] + {:source-table $$orders + :joins [{:source-table $$products + :alias "PRODUCTS__via__PRODUCT_ID" + :condition [:= product-id products-id] + :strategy :left-join + :fk-field-id %product_id}] + :expressions {"pivot-grouping" [:abs 0]} + :fields [id + user-id + product-id + subtotal + tax + total + discount + created-at + quantity + pivot-grouping + products-category + products-id]})} + (let [products-category [:field %products.category {:join-alias "PRODUCTS__via__PRODUCT_ID" + ::add/source-table ::add/source + ::add/source-alias "PRODUCTS__via__PRODUCT_ID__CATEGORY" + ::add/desired-alias "PRODUCTS__via__PRODUCT_ID__CATEGORY" + ::add/position 0}] + created-at [:field %created_at {:temporal-unit :year + ::nest-query/outer-select true + ::add/source-table ::add/source + ::add/source-alias "CREATED_AT" + ::add/desired-alias "CREATED_AT" + ::add/position 1}] + pivot-grouping [:field "pivot-grouping" {:base-type :type/Float ::add/source-table ::add/source - ::add/source-alias "PRODUCTS__via__PRODUCT_ID__CATEGORY" - ::add/desired-alias "PRODUCTS__via__PRODUCT_ID__CATEGORY" - ::add/position 0}] - created-at [:field %created_at {:temporal-unit :year - ::nest-query/outer-select true - ::add/source-table ::add/source - ::add/source-alias "CREATED_AT" - ::add/desired-alias "CREATED_AT" - ::add/position 1}] - pivot-grouping [:field "pivot-grouping" {:base-type :type/Float - ::add/source-table ::add/source - ::add/source-alias "pivot-grouping" - ::add/desired-alias "pivot-grouping" - ::add/position 2}]] - {:breakout [products-category created-at pivot-grouping] - :aggregation [[:aggregation-options [:count] {:name "count" - ::add/desired-alias "count" - ::add/position 3}]] - :order-by [[:asc products-category] - [:asc created-at] - [:asc pivot-grouping]]}))) - (nest-expressions - (mt/mbql-query orders - {:aggregation [[:aggregation-options [:count] {:name "count"}]] - :breakout [&PRODUCTS__via__PRODUCT_ID.products.category - !year.created_at - [:expression "pivot-grouping"]] - :expressions {"pivot-grouping" [:abs 0]} - :order-by [[:asc &PRODUCTS__via__PRODUCT_ID.products.category] - [:asc !year.created_at] - [:asc [:expression "pivot-grouping"]]] - :joins [{:source-table $$products - :strategy :left-join - :alias "PRODUCTS__via__PRODUCT_ID" - :fk-field-id %product_id - :condition [:= $product_id &PRODUCTS__via__PRODUCT_ID.products.id]}]}))))))) + ::add/source-alias "pivot-grouping" + ::add/desired-alias "pivot-grouping" + ::add/position 2}]] + {:breakout [products-category created-at pivot-grouping] + :aggregation [[:aggregation-options [:count] {:name "count" + ::add/desired-alias "count" + ::add/position 3}]] + :order-by [[:asc products-category] + [:asc created-at] + [:asc pivot-grouping]]}))) + (nest-expressions + (mt/mbql-query orders + {:aggregation [[:aggregation-options [:count] {:name "count"}]] + :breakout [&PRODUCTS__via__PRODUCT_ID.products.category + !year.created_at + [:expression "pivot-grouping"]] + :expressions {"pivot-grouping" [:abs 0]} + :order-by [[:asc &PRODUCTS__via__PRODUCT_ID.products.category] + [:asc !year.created_at] + [:asc [:expression "pivot-grouping"]]] + :joins [{:source-table $$products + :strategy :left-join + :alias "PRODUCTS__via__PRODUCT_ID" + :fk-field-id %product_id + :condition [:= $product_id &PRODUCTS__via__PRODUCT_ID.products.id]}]}))))))) (deftest uniquify-aliases-test (driver/with-driver :h2 (mt/dataset sample-dataset (mt/with-everything-store - (is (query= (mt/$ids products - {:source-query {:source-table $$products - :expressions {"CATEGORY" [:concat - [:field %category {::add/source-table $$products - ::add/source-alias "CATEGORY" - ::add/desired-alias "CATEGORY" - ::add/position 3}] - "2"]} - :fields [[:field %id {::add/source-table $$products - ::add/source-alias "ID" - ::add/desired-alias "ID" - ::add/position 0}] - [:field %ean {::add/source-table $$products - ::add/source-alias "EAN" - ::add/desired-alias "EAN" - ::add/position 1}] - [:field %title {::add/source-table $$products - ::add/source-alias "TITLE" - ::add/desired-alias "TITLE" - ::add/position 2}] - [:field %category {::add/source-table $$products - ::add/source-alias "CATEGORY" - ::add/desired-alias "CATEGORY" - ::add/position 3}] - [:field %vendor {::add/source-table $$products - ::add/source-alias "VENDOR" - ::add/desired-alias "VENDOR" - ::add/position 4}] - [:field %price {::add/source-table $$products - ::add/source-alias "PRICE" - ::add/desired-alias "PRICE" - ::add/position 5}] - [:field %rating {::add/source-table $$products - ::add/source-alias "RATING" - ::add/desired-alias "RATING" - ::add/position 6}] - [:field %created_at {:temporal-unit :default - ::add/source-table $$products - ::add/source-alias "CREATED_AT" - ::add/desired-alias "CREATED_AT" - ::add/position 7}] - [:expression "CATEGORY" {::add/desired-alias "CATEGORY_2" - ::add/position 8}]]} - :breakout [[:field "CATEGORY_2" {:base-type :type/Text - ::add/source-table ::add/source - ::add/source-alias "CATEGORY_2" - ::add/desired-alias "CATEGORY_2" - ::add/position 0}]] - :aggregation [[:aggregation-options [:count] {:name "count" - ::add/desired-alias "count" - ::add/position 1}]] - :order-by [[:asc [:field "CATEGORY_2" {:base-type :type/Text - ::add/source-table ::add/source - ::add/source-alias "CATEGORY_2" - ::add/desired-alias "CATEGORY_2" - ::add/position 0}]]] - :limit 1}) - (-> (mt/mbql-query products - {:expressions {"CATEGORY" [:concat $category "2"]} - :breakout [:expression"CATEGORY"] - :aggregation [[:count]] - :order-by [[:asc [:expression"CATEGORY"]]] - :limit 1}) - qp/query->preprocessed - add/add-alias-info - :query - nest-query/nest-expressions))))))) + (is (partial= (mt/$ids products + {:source-query {:source-table $$products + :expressions {"CATEGORY" [:concat + [:field %category {::add/source-table $$products + ::add/source-alias "CATEGORY" + ::add/desired-alias "CATEGORY" + ::add/position 3}] + "2"]} + :fields [[:field %id {::add/source-table $$products + ::add/source-alias "ID" + ::add/desired-alias "ID" + ::add/position 0}] + [:field %ean {::add/source-table $$products + ::add/source-alias "EAN" + ::add/desired-alias "EAN" + ::add/position 1}] + [:field %title {::add/source-table $$products + ::add/source-alias "TITLE" + ::add/desired-alias "TITLE" + ::add/position 2}] + [:field %category {::add/source-table $$products + ::add/source-alias "CATEGORY" + ::add/desired-alias "CATEGORY" + ::add/position 3}] + [:field %vendor {::add/source-table $$products + ::add/source-alias "VENDOR" + ::add/desired-alias "VENDOR" + ::add/position 4}] + [:field %price {::add/source-table $$products + ::add/source-alias "PRICE" + ::add/desired-alias "PRICE" + ::add/position 5}] + [:field %rating {::add/source-table $$products + ::add/source-alias "RATING" + ::add/desired-alias "RATING" + ::add/position 6}] + [:field %created_at {:temporal-unit :default + ::add/source-table $$products + ::add/source-alias "CREATED_AT" + ::add/desired-alias "CREATED_AT" + ::add/position 7}] + [:expression "CATEGORY" {::add/desired-alias "CATEGORY_2" + ::add/position 8}]]} + :breakout [[:field "CATEGORY_2" {:base-type :type/Text + ::add/source-table ::add/source + ::add/source-alias "CATEGORY_2" + ::add/desired-alias "CATEGORY_2" + ::add/position 0}]] + :aggregation [[:aggregation-options [:count] {:name "count" + ::add/desired-alias "count" + ::add/position 1}]] + :order-by [[:asc [:field "CATEGORY_2" {:base-type :type/Text + ::add/source-table ::add/source + ::add/source-alias "CATEGORY_2" + ::add/desired-alias "CATEGORY_2" + ::add/position 0}]]] + :limit 1}) + (-> (mt/mbql-query products + {:expressions {"CATEGORY" [:concat $category "2"]} + :breakout [:expression"CATEGORY"] + :aggregation [[:count]] + :order-by [[:asc [:expression"CATEGORY"]]] + :limit 1}) + qp/query->preprocessed + add/add-alias-info + :query + nest-query/nest-expressions))))))) diff --git a/test/metabase/query_processor_test/failure_test.clj b/test/metabase/query_processor_test/failure_test.clj index b001094d4b010e40efc31f11d1cb04f3c7ab82e7..6fd872a32f4c794605a00a2bc6c5c98f3af77ae3 100644 --- a/test/metabase/query_processor_test/failure_test.clj +++ b/test/metabase/query_processor_test/failure_test.clj @@ -7,6 +7,10 @@ [metabase.util.schema :as su] [schema.core :as s])) +(use-fixtures :each (fn [thunk] + (mt/with-log-level :fatal + (thunk)))) + (defn- bad-query [] {:database (mt/id) :type :query @@ -24,7 +28,8 @@ :type (s/eq :query) :query {:source-table (s/eq (mt/id :venues)) :fields (s/eq [[:field (mt/id :venues :id) {:temporal-unit :month}]]) - :limit (s/eq qp.i/absolute-max-results)} + :limit (s/eq qp.i/absolute-max-results) + s/Keyword s/Any} (s/optional-key :driver) (s/eq :h2) s/Keyword s/Any}) diff --git a/test/metabase/query_processor_test/query_to_native_test.clj b/test/metabase/query_processor_test/query_to_native_test.clj index 7f291eb316a3db2774e7fe21a14dff6a8f59a266..072db411b39a882835852d43b37a5478fc13cc5e 100644 --- a/test/metabase/query_processor_test/query_to_native_test.clj +++ b/test/metabase/query_processor_test/query_to_native_test.clj @@ -5,8 +5,7 @@ [metabase.models.permissions :as perms] [metabase.query-processor :as qp] [metabase.test :as mt] - [metabase.util :as u] - [schema.core :as s])) + [metabase.util :as u])) (deftest query->native-test (testing "Can we convert an MBQL query to a native query?" @@ -46,35 +45,28 @@ ;; use `query->native` (defn- query->native-with-user-perms [{database-id :database, {source-table-id :source-table} :query, :as query} {:keys [object-perms? native-perms?]}] - (try - (binding [api/*current-user-id* Integer/MAX_VALUE - api/*current-user-permissions-set* (delay (cond-> #{} - object-perms? (conj (perms/data-perms-path database-id "PUBLIC" source-table-id)) - native-perms? (conj (perms/adhoc-native-query-path database-id))))] - (qp/query->native query)) - (catch clojure.lang.ExceptionInfo e - (merge {:error (.getMessage e)} - (ex-data e))))) + (binding [api/*current-user-id* Integer/MAX_VALUE + api/*current-user-permissions-set* (delay (cond-> #{} + object-perms? (conj (perms/data-perms-path database-id "PUBLIC" source-table-id)) + native-perms? (conj (perms/adhoc-native-query-path database-id))))] + (qp/query->native query))) (deftest permissions-test - (testing "If user permissions are bound, we should do permissions checking when you call `query->native`" + (testing "If user permissions are bound, we should still NOT do permissions checking when you call `query->native`" (testing "Should work if you have the right perms" - (is (= true - (boolean - (query->native-with-user-perms - (mt/mbql-query venues) - {:object-perms? true, :native-perms? true}))))) - (testing "If you don't have MBQL permissions for the original query it should throw an error" - (is (schema= {:error (s/eq "You do not have permissions to run this query.") - s/Any s/Any} - (query->native-with-user-perms - (mt/mbql-query venues) - {:object-perms? false, :native-perms? true})))))) + (is (query->native-with-user-perms + (mt/mbql-query venues) + {:object-perms? true, :native-perms? true}))) + (testing "Should still work even WITHOUT the right perms" + (is (query->native-with-user-perms + (mt/mbql-query venues) + {:object-perms? false, :native-perms? true}))))) (deftest error-test (testing "If the query is bad in some way it should return a relevant error (?)" - (is (schema= {:error (s/eq (format "Database %d does not exist." Integer/MAX_VALUE)) - s/Any s/Any} - (query->native-with-user-perms - {:database Integer/MAX_VALUE, :type :query, :query {:source-table Integer/MAX_VALUE}} - {:object-perms? true, :native-perms? true}))))) + (is (thrown-with-msg? + clojure.lang.ExceptionInfo + #"Database \d+ does not exist" + (query->native-with-user-perms + {:database Integer/MAX_VALUE, :type :query, :query {:source-table Integer/MAX_VALUE}} + {:object-perms? true, :native-perms? true})))))