diff --git a/frontend/test/metabase/scenarios/joins/joins.cy.spec.js b/frontend/test/metabase/scenarios/joins/joins.cy.spec.js
index 78efb86e6714eba070398d4af716e054d096ba94..4c14144622533c8f23d1f99a9194448af480c799 100644
--- a/frontend/test/metabase/scenarios/joins/joins.cy.spec.js
+++ b/frontend/test/metabase/scenarios/joins/joins.cy.spec.js
@@ -14,7 +14,7 @@ describe("scenarios > question > joined questions", () => {
cy.signInAsAdmin();
});
- it.skip("joining on a question with remapped values should work (metabase#15578)", () => {
+ it("joining on a question with remapped values should work (metabase#15578)", () => {
cy.intercept("POST", "/api/dataset").as("dataset");
// Remap display value
cy.request("POST", `/api/field/${ORDERS.PRODUCT_ID}/dimension`, {
diff --git a/src/metabase/models/params/chain_filter.clj b/src/metabase/models/params/chain_filter.clj
index 344c2e0a4d704057747cd3d9dbf593634629ef6c..8be8735a42ce89694b302fe654ba1fa812091bc4 100644
--- a/src/metabase/models/params/chain_filter.clj
+++ b/src/metabase/models/params/chain_filter.clj
@@ -391,7 +391,8 @@
;; but sort by [remapped-value]
:order-by [[:asc [:field field-id nil]]]}))
(add-joins source-table-id joins)
- (add-filters source-table-id joined-table-ids constraints)))})
+ (add-filters source-table-id joined-table-ids constraints)))
+ :middleware {:disable-remaps? true}})
;;; ------------------------ Chain filter (powers GET /api/dashboard/:id/params/:key/values) -------------------------
diff --git a/src/metabase/query_processor/middleware/add_dimension_projections.clj b/src/metabase/query_processor/middleware/add_dimension_projections.clj
index 84ef1ad9e31d47612fef5d19f1e945636ad8b59d..b98722037218e0f34541cd3d79fd367ee8fe9b25 100644
--- a/src/metabase/query_processor/middleware/add_dimension_projections.clj
+++ b/src/metabase/query_processor/middleware/add_dimension_projections.clj
@@ -25,6 +25,8 @@
`name` is `:remapped_from` `:category_id`."
(:require [clojure.data :as data]
[clojure.tools.logging :as log]
+ [clojure.walk :as walk]
+ [medley.core :as m]
[metabase.mbql.schema :as mbql.s]
[metabase.mbql.util :as mbql.u]
[metabase.models.dimension :refer [Dimension]]
@@ -166,31 +168,18 @@
{:remaps (s/maybe (su/distinct [ExternalRemappingDimension]))
:query mbql.s/Query})
-(declare add-fk-remaps)
-
-(s/defn ^:private source-query-remaps :- QueryAndRemaps
- [{{:keys [source-query]} :query, :as query}]
- (if (and source-query (not (:native source-query)))
- ;; Only do lifting if source is MBQL query
- (let [{source-query-remaps :remaps, source-query :query} (add-fk-remaps (assoc query :query source-query))]
- {:remaps source-query-remaps
- :query (assoc-in query [:query :source-query] (:query source-query))})
- {:remaps nil, :query query}))
-
-(s/defn ^:private add-fk-remaps :- QueryAndRemaps
- "Add any Fields needed for `:external` remappings to the `:fields` clause of the query, and update `:order-by` and
- `breakout` clauses as needed. Returns a map with `:query` (the updated query) and `:remaps` (a sequence
- of [[ExternalRemappingDimension]] information maps)."
- [{{:keys [fields order-by breakout]} :query, :as query} :- mbql.s/Query]
- (let [{source-query-remaps :remaps, query :query} (source-query-remaps query)]
+(defn- add-fk-remaps-one-level
+ [{:keys [fields order-by breakout], {source-query-remaps ::remaps} :source-query, :as query}]
+ (let [query (m/dissoc-in query [:source-query ::remaps])]
;; fetch remapping column pairs if any exist...
(if-let [infos (not-empty (remap-column-infos (concat fields breakout)))]
;; if they do, update `:fields`, `:order-by` and `:breakout` clauses accordingly and add to the query
(let [ ;; make a map of field-id-clause -> fk-clause from the tuples
original->remapped (into {} (map (juxt :original-field-clause :new-field-clause)) infos)
existing-fields (add-fk-remaps-rewrite-existing-fields infos fields)
- ;; don't add any new entries for fields that already exist. Use [[mbql.u/remove-namespaced-options]] here so we don't
- ;; add new entries even if the existing Field has some extra info e.g. extra unknown namespaced keys.
+ ;; don't add any new entries for fields that already exist. Use [[mbql.u/remove-namespaced-options]] here so
+ ;; we don't add new entries even if the existing Field has some extra info e.g. extra unknown namespaced
+ ;; keys.
existing-normalized-fields-set (into #{} (map mbql.u/remove-namespaced-options) existing-fields)
new-fields (into
existing-fields
@@ -198,15 +187,32 @@
(remove (comp existing-normalized-fields-set mbql.u/remove-namespaced-options)))
infos)
new-breakout (add-fk-remaps-rewrite-breakout original->remapped breakout)
- new-order-by (add-fk-remaps-rewrite-order-by original->remapped order-by)]
+ new-order-by (add-fk-remaps-rewrite-order-by original->remapped order-by)
+ remaps (into [] (comp cat (distinct)) [source-query-remaps (map :dimension infos)])]
;; return the Dimensions we are using and the query
- {:remaps (into [] (comp cat (distinct)) [source-query-remaps (map :dimension infos)])
- :query (cond-> query
- (seq fields) (assoc-in [:query :fields] new-fields)
- (seq order-by) (assoc-in [:query :order-by] new-order-by)
- (seq breakout) (assoc-in [:query :breakout] new-breakout))})
+ (cond-> query
+ (seq fields) (assoc :fields new-fields)
+ (seq order-by) (assoc :order-by new-order-by)
+ (seq breakout) (assoc :breakout new-breakout)
+ (seq remaps) (assoc ::remaps remaps)))
;; otherwise return query as-is
- {:remaps source-query-remaps, :query query})))
+ (cond-> query
+ (seq source-query-remaps) (assoc ::remaps source-query-remaps)))))
+
+(s/defn ^:private add-fk-remaps :- QueryAndRemaps
+ "Add any Fields needed for `:external` remappings to the `:fields` clause of the query, and update `:order-by` and
+ `breakout` clauses as needed. Returns a map with `:query` (the updated query) and `:remaps` (a sequence
+ of [[ExternalRemappingDimension]] information maps)."
+ [query]
+ (let [query (walk/postwalk
+ (fn [form]
+ (if (and (map? form)
+ ((some-fn :source-table :source-query) form)
+ (not (:condition form)))
+ (add-fk-remaps-one-level form)
+ form))
+ query)]
+ {:query (m/dissoc-in query [:query ::remaps]), :remaps (get-in query [:query ::remaps])}))
(defn add-remapped-columns
"Pre-processing middleware. For columns that have remappings to other columns (FK remaps), rewrite the query to
diff --git a/test/metabase/query_processor/middleware/add_dimension_projections_test.clj b/test/metabase/query_processor/middleware/add_dimension_projections_test.clj
index ea7baf2c005ac987c3d08ced6f14dd61b757bc7d..d3a8ea18b7faf158a72970c0e0f004915825582f 100644
--- a/test/metabase/query_processor/middleware/add_dimension_projections_test.clj
+++ b/test/metabase/query_processor/middleware/add_dimension_projections_test.clj
@@ -444,3 +444,36 @@
{:display_name "Sender ID [external remap]", :name "NAME", :remapped_from "SENDER_ID"}
{:display_name "Receiver ID [external remap]", :name "NAME_2", :remapped_from "RECEIVER_ID"}]}
(#'add-dim-projections/add-remapped-to-and-from-metadata metadata remap-info nil)))))))))))
+
+(deftest add-remappings-inside-joins-test
+ (testing "Remappings should work inside joins (#15578)"
+ (mt/dataset sample-dataset
+ (mt/with-column-remappings [orders.product_id products.title]
+ (is (partial (mt/mbql-query products
+ {:joins [{:source-query {:source-table $$orders}
+ :alias "Q1"
+ :fields [&Q1.orders.id
+ &Q1.orders.product_id
+ &PRODUCTS__via__PRODUCT_ID.orders.product_id->title]
+ :condition [:= $id &Q1.orders.product_id]
+ :strategy :left-join}
+ {:source-table $$products
+ :alias "PRODUCTS__via__PRODUCT_ID"
+ :condition [:= $orders.product_id &PRODUCTS__via__PRODUCT_ID.products.id]
+ :strategy :left-join
+ :fk-field-id %orders.product_id}]
+ :fields [&Q1.orders.id
+ &Q1.orders.product_id
+ &PRODUCTS__via__PRODUCT_ID.orders.product_id->products.title]
+ :limit 2})
+ (:query
+ (#'add-dim-projections/add-fk-remaps
+ (mt/mbql-query products
+ {:joins [{:strategy :left-join
+ :source-query {:source-table $$orders}
+ :alias "Q1"
+ :condition [:= $id &Q1.orders.product_id]
+ :fields [&Q1.orders.id
+ &Q1.orders.product_id]}]
+ :fields [&Q1.orders.id &Q1.orders.product_id]
+ :limit 2})))))))))
diff --git a/test/metabase/query_processor_test/remapping_test.clj b/test/metabase/query_processor_test/remapping_test.clj
index 96a3be26bf82d806cbfd324f11ccfe5ed1479dd0..4db050ba34b3c0993fe14e77d9ef47daad6cfc3a 100644
--- a/test/metabase/query_processor_test/remapping_test.clj
+++ b/test/metabase/query_processor_test/remapping_test.clj
@@ -1,6 +1,8 @@
(ns metabase.query-processor-test.remapping-test
"Tests for the remapping results"
(:require [clojure.test :refer :all]
+ [metabase.driver :as driver]
+ [metabase.models.card :refer [Card]]
[metabase.models.dimension :refer [Dimension]]
[metabase.models.field :refer [Field]]
[metabase.query-processor :as qp]
@@ -257,3 +259,64 @@
[2 8 3 "Bip bip bip bip" "Annie Albatross" "Peter Pelican"]
[3 3 2 "Coo" "Peter Pelican" "Lucky Pigeon"]]
(mt/rows results))))))))))
+
+(deftest remapped-columns-in-joined-source-queries-test
+ (mt/test-drivers (mt/normal-drivers-with-feature :nested-queries :left-join)
+ (testing "Remapped columns in joined source queries should work (#15578)"
+ (mt/dataset sample-dataset
+ (mt/with-bigquery-fks #{:bigquery :bigquery-cloud-sdk}
+ (mt/with-column-remappings [orders.product_id products.title]
+ (let [query (mt/mbql-query products
+ {:joins [{:source-query {:source-table $$orders
+ :breakout [$orders.product_id]
+ :aggregation [[:sum $orders.quantity]]}
+ :alias "Orders"
+ :condition [:= $id &Orders.orders.product_id]
+ :fields [&Orders.title
+ &Orders.*sum/Integer]}]
+ :fields [$title $category]
+ :order-by [[:asc $id]]
+ :limit 3})]
+ (mt/with-native-query-testing-context query
+ (let [results (qp/process-query query)]
+ (when (= driver/*driver* :h2)
+ (testing "Metadata"
+ (is (= [["TITLE" "Title"]
+ ["CATEGORY" "Category"]
+ ["TITLE_2" "Orders → Title"]
+ ["sum" "Orders → Sum"]]
+ (map (juxt :name :display_name) (mt/cols results))))))
+ (is (= [["Rustic Paper Wallet" "Gizmo" "Rustic Paper Wallet" 347]
+ ["Small Marble Shoes" "Doohickey" "Small Marble Shoes" 352]
+ ["Synergistic Granite Chair" "Doohickey" "Synergistic Granite Chair" 286]]
+ (mt/formatted-rows [str str str int]
+ results))))))))))))
+
+(deftest inception-style-nested-query-with-joins-test
+ (testing "source query > source query > query with join (with remappings) should work (#14724)"
+ ;; this error only seems to be triggered when actually using Cards as sources (and include the source metadata)
+ (mt/dataset sample-dataset
+ (mt/with-column-remappings [orders.product_id products.title]
+ ;; this is only triggered when using the results metadata from the Card itself -- see #19895
+ (let [q1 (mt/mbql-query orders
+ {:fields [$id $product_id]
+ :joins [{:source-table $$products
+ :alias "Products"
+ :condition [:= $product_id &Products.products.id]
+ :fields [$id
+ &Products.products.title]}]
+ :order-by [[:asc $id]]
+ :limit 3})]
+ (mt/with-temp Card [{card-1-id :id} {:dataset_query q1
+ :result_metadata (get-in (qp/process-query q1)
+ [:data :results_metadata :columns])}]
+ (let [q2 (mt/mbql-query nil {:source-table (format "card__%d" card-1-id)})]
+ (mt/with-temp Card [{card-2-id :id} {:dataset_query q2
+ :result_metadata (get-in (qp/process-query q2)
+ [:data :results_metadata :columns])}]
+ (let [q3 (mt/mbql-query nil {:source-table (format "card__%d" card-2-id)})]
+ (mt/with-native-query-testing-context q3
+ (is (= [[1 14 "Awesome Concrete Shoes" "Awesome Concrete Shoes"]
+ [2 123 "Mediocre Wooden Bench" "Mediocre Wooden Bench"]
+ [3 105 "Fantastic Wool Shirt" "Fantastic Wool Shirt"]]
+ (mt/rows (qp/process-query q3))))))))))))))