diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
index c4b2b3bbc6ee4e263e356cf3d9165346f17d12dc..1255afc7d3445440729a6845cc2fb173e7dc84f0 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
@@ -476,20 +476,17 @@
(assoc col
:id id
:table_id (mt/id :venues)
- :field_ref [:field id nil])))
- expected-outer-cols (fn []
- (map #(assoc-in % [:options :nested/outer] true)
- (expected-cols)))]
+ :field_ref [:field id nil])))]
(testing "A query with a simple attributes-based sandbox should have the same metadata"
(mt/with-gtaps {:gtaps {:venues (dissoc (venues-category-mbql-gtap-def) :query)}
:attributes {"cat" 50}}
- (is (= (expected-outer-cols)
+ (is (= (expected-cols)
(cols)))))
(testing "A query with an equivalent MBQL query sandbox should have the same metadata"
(mt/with-gtaps {:gtaps {:venues (venues-category-mbql-gtap-def)}
:attributes {"cat" 50}}
- (is (= (expected-outer-cols)
+ (is (= (expected-cols)
(cols)))))
(testing "A query with an equivalent native query sandbox should have the same metadata"
diff --git a/frontend/test/metabase/scenarios/models/reproductions/22519-casting-fails-query.cy.spec.js b/frontend/test/metabase/scenarios/models/reproductions/22519-casting-fails-query.cy.spec.js
index 4533f9ccf0555888e2dba84d6bfe9b2f1b5d94c9..89b88bb19114292d6b80d8d646322c896ab13480 100644
--- a/frontend/test/metabase/scenarios/models/reproductions/22519-casting-fails-query.cy.spec.js
+++ b/frontend/test/metabase/scenarios/models/reproductions/22519-casting-fails-query.cy.spec.js
@@ -14,7 +14,7 @@ const questionDetails = {
},
};
-describe("issue 22519", () => {
+describe.skip("issue 22519", () => {
beforeEach(() => {
cy.intercept("PUT", "/api/field/*").as("updateField");
cy.intercept("POST", "/api/dataset").as("dataset");
@@ -36,6 +36,7 @@ describe("issue 22519", () => {
turnIntoModel();
+ cy.wait("@dataset");
cy.findByText("xavier");
});
});
diff --git a/src/metabase/driver/sql/query_processor.clj b/src/metabase/driver/sql/query_processor.clj
index 6635b4d9b9963a3bcbaf52a92572200826998260..43d9a3aba87e3277b2449e7fedaa186528a91b40 100644
--- a/src/metabase/driver/sql/query_processor.clj
+++ b/src/metabase/driver/sql/query_processor.clj
@@ -370,6 +370,7 @@
(defmethod ->honeysql [:sql :field]
[driver [_ id-or-name {:keys [database-type]
+ ::nest-query/keys [outer-select]
:as options}
:as field-clause]]
(try
@@ -377,7 +378,6 @@
source-alias (field-source-alias field-clause)
field (when (integer? id-or-name)
(qp.store/field id-or-name))
- outer-select (:nested/outer options)
allow-casting? (and field
(not outer-select))
database-type (or database-type
@@ -392,7 +392,7 @@
(u/prog1
(cond->> identifier
allow-casting? (cast-field-if-needed driver field)
- database-type maybe-add-db-type ; only add type info if it wasn't added by [[cast-field-if-needed]]
+ database-type maybe-add-db-type ; only add type info if it wasn't added by [[cast-field-if-needed]]
(:temporal-unit options) (apply-temporal-bucketing driver options)
(:binning options) (apply-binning options))
(log/trace (binding [*print-meta* true]
@@ -656,12 +656,12 @@
[:field id-or-name opts]
[:field id-or-name (cond-> opts
true
- (assoc ::add/source-alias (::add/desired-alias opts)
- ::add/source-table ::add/none
+ (assoc ::add/source-alias (::add/desired-alias opts)
+ ::add/source-table ::add/none
;; sort of a HACK but this key will tell the SQL QP not to apply casting here either.
- :nested/outer true
+ ::nest-query/outer-select true
;; used to indicate that this is a forced alias
- ::forced-alias true)
+ ::forced-alias true)
;; don't want to do temporal bucketing or binning inside the order by only.
;; That happens inside the `SELECT`
;; (#22831) however, we do want it in breakout
diff --git a/src/metabase/query_processor.clj b/src/metabase/query_processor.clj
index 480c6adf77d104fbea3af33b497795b4925556cd..bfff92ebfe74bdf01fbf4be36bda05a8ed398e45 100644
--- a/src/metabase/query_processor.clj
+++ b/src/metabase/query_processor.clj
@@ -36,7 +36,6 @@
[metabase.query-processor.middleware.format-rows :as format-rows]
[metabase.query-processor.middleware.large-int-id :as large-int-id]
[metabase.query-processor.middleware.limit :as limit]
- [metabase.query-processor.middleware.mark-outer-fields :as qp.mark-outer-fields]
[metabase.query-processor.middleware.mbql-to-native :as mbql-to-native]
[metabase.query-processor.middleware.normalize-query :as normalize]
[metabase.query-processor.middleware.optimize-temporal-filters :as optimize-temporal-filters]
@@ -105,7 +104,6 @@
#'resolve-joins/resolve-joins
#'resolve-joined-fields/resolve-joined-fields
#'fix-bad-refs/fix-bad-references
- #'qp.mark-outer-fields/mark-outer-fields
#'escape-join-aliases/escape-join-aliases
;; yes, this is called a second time, because we need to handle any joins that got added
(resolve 'ee.sandbox.rows/apply-sandboxing)
diff --git a/src/metabase/query_processor/middleware/mark_outer_fields.clj b/src/metabase/query_processor/middleware/mark_outer_fields.clj
deleted file mode 100644
index 3697a63024f28758a7cda7c9080dd02e9159b3c6..0000000000000000000000000000000000000000
--- a/src/metabase/query_processor/middleware/mark_outer_fields.clj
+++ /dev/null
@@ -1,32 +0,0 @@
-(ns metabase.query-processor.middleware.mark-outer-fields
- (:require [clojure.walk :as walk]
- [medley.core :as m]
- [metabase.mbql.util :as mbql.u]))
-
-(defn- annotate-outer-fields [form source-fields-names-or-ids]
- (mbql.u/replace form
- [:field (_ :guard source-fields-names-or-ids) _] (mbql.u/assoc-field-options &match :nested/outer true)))
-
-(defn- mark-mbql-outer-fields
- [mbql-query]
- (walk/prewalk
- (fn [form]
- (let [source-fields (get-in form [:source-query :fields])]
- (if (and (map? form)
- (seqable? source-fields)
- (seq source-fields))
- (let [id-set (into #{} (map second) source-fields)]
- (-> form
- (m/update-existing :fields annotate-outer-fields id-set)
- (m/update-existing :breakout annotate-outer-fields id-set)
- (m/update-existing :order-by annotate-outer-fields id-set)))
- form)))
- mbql-query))
-
-(defn mark-outer-fields
- "Mark all Fields in the MBQL query `query` coming from a source-query as `:nested/outer` so QP implementations know
- not to apply coercion or whatever to them a second time"
- [query]
- (cond-> query
- (not= (:type query) :native)
- (update :query mark-mbql-outer-fields)))
diff --git a/src/metabase/query_processor/util/nest_query.clj b/src/metabase/query_processor/util/nest_query.clj
index a270a77af19dd455ae96d8874042d4a80e53bdc3..2f91124162c2592d2bac2e2226843841fdb42c3c 100644
--- a/src/metabase/query_processor/util/nest_query.clj
+++ b/src/metabase/query_processor/util/nest_query.clj
@@ -74,8 +74,8 @@
;; mark all Fields at the new top level as `::outer-select` so QP implementations know not to apply coercion or
;; whatever to them a second time.
- [:field _id-or-name (_opts :guard (every-pred :temporal-unit (complement :nested/outer)))]
- (recur (mbql.u/assoc-field-options &match :nested/outer true))
+ [:field _id-or-name (_opts :guard (every-pred :temporal-unit (complement ::outer-select)))]
+ (recur (mbql.u/update-field-options &match assoc ::outer-select true))
[:field id-or-name (opts :guard :join-alias)]
(let [{::add/keys [desired-alias]} (mbql.u/match-one (:source-query query)
diff --git a/test/metabase/query_processor/middleware/add_source_metadata_test.clj b/test/metabase/query_processor/middleware/add_source_metadata_test.clj
index 8ba7db019dc5cc59188f87b4557b8ef4d5a02e68..401475a58d77b0a8e894d5f7bae090ea954b1eee 100644
--- a/test/metabase/query_processor/middleware/add_source_metadata_test.clj
+++ b/test/metabase/query_processor/middleware/add_source_metadata_test.clj
@@ -331,7 +331,7 @@
;; the actual metadata this middleware should return. Doesn't have all the columns that come back from
;; `qp/query->expected-cols`
expected-metadata (for [col metadata]
- (cond-> (dissoc col :description :source :visibility_type :options)
+ (cond-> (dissoc col :description :source :visibility_type)
;; for some reason this middleware returns temporal fields with a `:default` unit,
;; whereas `query->expected-cols` does not return the unit. It ulimately makes zero
;; difference, so I haven't looked into why this is the case yet.
diff --git a/test/metabase/query_processor/middleware/fetch_source_query_test.clj b/test/metabase/query_processor/middleware/fetch_source_query_test.clj
index 43796a846276a3867ca6576910e152d875807d8a..82ea350012673498a3d87fb93cbfbe69ce11e1d4 100644
--- a/test/metabase/query_processor/middleware/fetch_source_query_test.clj
+++ b/test/metabase/query_processor/middleware/fetch_source_query_test.clj
@@ -123,10 +123,7 @@
(qp/query->expected-cols (mt/mbql-query venues)))
(assoc-in [:query :source-query :source-metadata]
(mt/derecordize (qp/query->expected-cols (mt/mbql-query venues))))
- (assoc :info {:card-id (u/the-id card-2)})
- (update-in [:query :source-metadata] (fn [fields]
- (mapv #(assoc-in % [:options :nested/outer] true)
- fields))))
+ (assoc :info {:card-id (u/the-id card-2)}))
(resolve-card-id-source-tables
(wrap-inner-query
{:source-table (str "card__" (u/the-id card-2)), :limit 25}))))))
diff --git a/test/metabase/query_processor/util/add_alias_info_test.clj b/test/metabase/query_processor/util/add_alias_info_test.clj
index e2e0d90fdad150bf39d571d13b69e7c1c0923113..21f65346ff9f0e03341cb84afd20fdb83a1d4a67 100644
--- a/test/metabase/query_processor/util/add_alias_info_test.clj
+++ b/test/metabase/query_processor/util/add_alias_info_test.clj
@@ -62,14 +62,12 @@
::add/source-table ::add/source
::add/source-alias "Cat__NAME"
::add/desired-alias "Cat__NAME"
- ::add/position 0
- :nested/outer true}]]
+ ::add/position 0}]]
:order-by [[:asc [:field %categories.name {:join-alias "Cat"
::add/source-table ::add/source
::add/source-alias "Cat__NAME"
::add/desired-alias "Cat__NAME"
- ::add/position 0
- :nested/outer true}]]]
+ ::add/position 0}]]]
:limit 1})
(add-alias-info
(mt/mbql-query venues
@@ -134,7 +132,6 @@
::add/source-table "Q2"}]]
:strategy :left-join}]
:fields [[:field %products.category {:join-alias "P1"
- :nested/outer true
::add/desired-alias "P1__CATEGORY"
::add/position 0
::add/source-alias "P1__CATEGORY"
@@ -407,8 +404,7 @@
::add/source-table ::add/source
::add/source-alias "COOL.double_price"
::add/desired-alias "COOL.COOL.double_price"
- ::add/position 0
- :nested/outer true}]]
+ ::add/position 0}]]
{:aggregation [[:aggregation-options [:count] {:name "COOL.count"
::add/position 1
::add/source-alias "count"
@@ -545,7 +541,6 @@
::add/position 0
::add/source-alias "Products_Renamed__ID"
::add/source-table ::add/source
- :nested/outer true
:join-alias "Products Renamed"}]
[:field
"CC"
@@ -553,8 +548,7 @@
::add/position 1
::add/source-alias "CC"
::add/source-table ::add/source
- :base-type :type/Float
- :nested/outer true}]]
+ :base-type :type/Float}]]
:limit 1})
(-> (mt/mbql-query orders
{:source-query {:source-table $$orders
@@ -592,13 +586,11 @@
::add/source-alias "Name"
::add/desired-alias "Name_2"
::add/position 1}]]}
- :fields [[:field name-id {:nested/outer true
- ::add/source-table ::add/source
+ :fields [[:field name-id {::add/source-table ::add/source
::add/source-alias "NAME"
::add/desired-alias "NAME"
::add/position 0}]
- [:field price-id {:nested/outer true
- ::add/source-table ::add/source
+ [:field price-id {::add/source-table ::add/source
::add/source-alias "Name_2"
::add/desired-alias "Name_2"
::add/position 1}]]
diff --git a/test/metabase/query_processor/util/nest_query_test.clj b/test/metabase/query_processor/util/nest_query_test.clj
index a83e60654de1d71b4547871bfc2991eac848ff43..abbc5010b18e29ca0164af2d469b2c431284bb73 100644
--- a/test/metabase/query_processor/util/nest_query_test.clj
+++ b/test/metabase/query_processor/util/nest_query_test.clj
@@ -117,18 +117,18 @@
::add/source-alias "double_id"
::add/desired-alias "double_id"
::add/position 0}]
- [:field %date {:temporal-unit :day
- :nested/outer true
- ::add/source-table ::add/source
- ::add/source-alias "DATE"
- ::add/desired-alias "DATE"
- ::add/position 1}]
- [:field %date {:temporal-unit :month
- :nested/outer true
- ::add/source-table ::add/source
- ::add/source-alias "DATE"
- ::add/desired-alias "DATE_2"
- ::add/position 2}]]
+ [: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
@@ -588,12 +588,12 @@
::add/position 5}]
[:expression "test" {::add/desired-alias "test"
::add/position 6}]]}
- :fields [[:field %price {:temporal-unit :default
- :nested/outer true
- ::add/source-table ::add/source
- ::add/source-alias "PRICE"
- ::add/desired-alias "PRICE"
- ::add/position 0}]
+ :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"
@@ -684,12 +684,12 @@
::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
- :nested/outer true
- ::add/source-table ::add/source
- ::add/source-alias "CREATED_AT"
- ::add/desired-alias "CREATED_AT"
- ::add/position 1}]
+ 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"
diff --git a/test/metabase/query_processor_test/nested_queries_test.clj b/test/metabase/query_processor_test/nested_queries_test.clj
index 96c92ccc0c7df05bd5c7f90b24b17e8e76814089..eca4628faa1fc7b19a553ab0409b2ece9a475364 100644
--- a/test/metabase/query_processor_test/nested_queries_test.clj
+++ b/test/metabase/query_processor_test/nested_queries_test.clj
@@ -31,9 +31,7 @@
[4 "Wurstküche" 29 33.9997 -118.465 2]
[5 "Brite Spot Family Restaurant" 20 34.0778 -118.261 2]]
:cols (mapv
- (fn [col]
- (-> (qp.test/col :venues col)
- (assoc-in [:options :nested/outer] true)))
+ (partial qp.test/col :venues)
[:id :name :category_id :latitude :longitude :price])}
(qp.test/rows-and-cols
(mt/format-rows-by :venues
@@ -70,9 +68,6 @@
[3 13]
[4 6]]
:cols [(cond-> (qp.test/breakout-col (qp.test/col :venues :price))
- (not native-source?)
- (assoc-in [:options :nested/outer] true)
-
native-source?
(-> (assoc :field_ref [:field "PRICE" {:base-type :type/Integer}]
:effective_type :type/Integer)
@@ -138,11 +133,8 @@
[1 3 13]
[1 4 8]
[1 5 10]]
- :cols [(-> (qp.test/fk-col :checkins :venue_id :venues :price)
- qp.test/breakout-col)
- (-> (qp.test/col :checkins :user_id)
- qp.test/breakout-col
- (assoc-in [:options :nested/outer] true))
+ :cols [(qp.test/breakout-col (qp.test/fk-col :checkins :venue_id :venues :price))
+ (qp.test/breakout-col (qp.test/col :checkins :user_id))
(qp.test/aggregate-col :count)]}
(qp.test/rows-and-cols
(mt/format-rows-by [int int int]
@@ -198,54 +190,6 @@
-(deftest nested-with-coerced-fields
- (testing "Coerced fields are cast only once (#22519)"
- (mt/test-drivers (mt/normal-drivers-with-feature :nested-queries)
- (mt/dataset sample-dataset
- (mt/with-temp-vals-in-db Field (mt/id :reviews :rating) {:coercion_strategy :Coercion/UNIXSeconds->DateTime
- :effective_type :type/Instant}
- (mt/with-temp Card [{card-id :id}
- {:dataset_query
- (mt/mbql-query reviews
- {:fields [$rating]
- :limit 1})}]
- (testing "with all inherited fields"
- (is (= :completed
- (:status (qp/process-query {:type :query
- :database (mt/id)
- :query {:source-table (str "card__" card-id)}})))))
- (testing "with explicit top-level fields"
- (is (= :completed
- (:status (qp/process-query {:type :query
- :database (mt/id)
- :query {:fields [(mt/id :reviews :rating)]
- :source-table (str "card__" card-id)}})))))
- (testing "with breakout and order-by"
- (is (= :completed
- (:status (qp/process-query {:type :query
- :database (mt/id)
- :query {:source-table (str "card__" card-id)
- :aggregation [:count]
- :breakout [(mt/id :reviews :rating)]}})))))))))
- (mt/test-drivers (mt/normal-drivers-with-feature :nested-queries :left-join)
- (mt/dataset sample-dataset
- (mt/with-temp-vals-in-db Field (mt/id :reviews :rating) {:coercion_strategy :Coercion/UNIXSeconds->DateTime
- :effective_type :type/Instant}
- (testing "with join"
- (mt/with-temp Card [{card-id :id}
- {:dataset_query
- (mt/mbql-query products
- {:fields [$id]
- :joins [{:source-table $$reviews
- :alias "R"
- :fields [&R.reviews.rating]
- :condition [:= $id &R.reviews.product_id]}]
- :limit 1})}]
- (is (= :completed
- (:status (qp/process-query {:type :query
- :database (mt/id)
- :query {:source-table (str "card__" card-id)}})))))))))))
-
(deftest sql-source-query-breakout-aggregation-test
(mt/test-drivers (mt/normal-drivers-with-feature :nested-queries)
(testing "make sure we can do a query with breakout and aggregation using a SQL source query"
@@ -366,9 +310,7 @@
(deftest filter-by-field-literal-test
(testing "make sure we can filter by a field literal"
(is (= {:rows [[1 "Red Medicine" 4 10.0646 -165.374 3]]
- :cols (mapv (fn [col]
- (-> (qp.test/col :venues col)
- (assoc-in [:options :nested/outer] true)))
+ :cols (mapv (partial qp.test/col :venues)
[:id :name :category_id :latitude :longitude :price])}
(qp.test/rows-and-cols
(mt/run-mbql-query venues
@@ -523,9 +465,7 @@
(deftest correct-column-metadata-test
(testing "make sure a query using a source query comes back with the correct columns metadata"
- (is (= (map (fn [col]
- (-> (qp.test/col :venues col)
- (assoc-in [:options :nested/outer] true)))
+ (is (= (map (partial qp.test/col :venues)
[:id :name :category_id :latitude :longitude :price])
;; todo: i don't know why the results don't have the information
(mt/cols
@@ -533,8 +473,7 @@
(qp/process-query (query-with-source-card card)))))))
(testing "make sure a breakout/aggregate query using a source query comes back with the correct columns metadata"
- (is (= [(qp.test/breakout-col (-> (qp.test/col :venues :price)
- (assoc-in [:options :nested/outer] true)))
+ (is (= [(qp.test/breakout-col (qp.test/col :venues :price))
(qp.test/aggregate-col :count)]
(mt/cols
(mt/with-temp Card [card (venues-mbql-card-def)]
diff --git a/test/metabase/query_processor_test/remapping_test.clj b/test/metabase/query_processor_test/remapping_test.clj
index ee204dcb0836a956d2dcd0f75b2937c3ea2ad7f5..3f38729b5693484b3aefa8ca16d57af6c91b3745 100644
--- a/test/metabase/query_processor_test/remapping_test.clj
+++ b/test/metabase/query_processor_test/remapping_test.clj
@@ -73,10 +73,8 @@
["25°" 11 "Burger"]
["33 Taps" 7 "Bar"]
["800 Degrees Neapolitan Pizzeria" 58 "Pizza"]]
- :cols [(-> (mt/col :venues :name)
- (assoc-in [:options :nested/outer] true))
+ :cols [(mt/col :venues :name)
(-> (mt/col :venues :category_id)
- (assoc-in [:options :nested/outer] true)
(assoc :remapped_to "Category ID [internal remap]"))
(#'qp.add-dimension-projections/create-remapped-col
"Category ID [internal remap]"