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

Don't add `:join-alias` to `:field` clauses for fields that came from source query (#19791) 

* Don't add `:join-alias` to `:field` clauses for fields that came from source query's source table

* Some test fixes :wrench:

* Test fixes :wrench:

* Test fix :wrench:

* Test fix :wrench:
parent 3822d2cd
Branches
Tags
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 146 additions and 67 deletions
frontend/test/metabase/scenarios/question/reproductions/18502-cannot-join-two-saved-questions-same-table.cy.spec.js
+ 1
− 1
View file @ ed5a4489
......@@ -6,7 +6,7 @@ const { PEOPLE, PEOPLE_ID } = SAMPLE_DATABASE;
const question1 = getQuestionDetails("18502#1", PEOPLE.CREATED_AT);
const question2 = getQuestionDetails("18502#2", PEOPLE.BIRTH_DATE);
describe.skip("issue 18502", () => {
describe("issue 18502", () => {
beforeEach(() => {
cy.intercept("POST", "/api/dataset").as("dataset");
restore();
......
src/metabase/query_processor/middleware/resolve_joined_fields.clj
+ 20
− 11
View file @ ed5a4489
......@@ -50,21 +50,30 @@
:joins joins
:candidates candidate-tables}))))))))
(defn- primary-source-table-id
"Get the ID of the 'primary' table towards which this query is pointing at: either the `:source-table` or indirectly
thru some number of `:source-query`s."
[{:keys [source-table source-query]}]
(or source-table
(when source-query
(recur source-query))))
(s/defn ^:private add-join-alias-to-fields-if-needed*
"Wrap Field clauses in a form that has `:joins`."
[{:keys [source-table source-query joins], :as form} :- InnerQuery]
[{:keys [source-query joins], :as form} :- InnerQuery]
;; don't replace stuff in child `:join` or `:source-query` forms -- remove these from `form` when we call `replace`
(let [form (mbql.u/replace (dissoc form :joins :source-query)
;; don't add `:join-alias` to anything that already has one
[:field _ (_ :guard :join-alias)]
&match
(let [source-table (primary-source-table-id form)
form (mbql.u/replace (dissoc form :joins :source-query)
;; don't add `:join-alias` to anything that already has one
[:field _ (_ :guard :join-alias)]
&match
;; otherwise for any other `:field` whose table isn't the source Table, attempt to wrap it.
[:field
(field-id :guard (every-pred integer?
#(not= (:table_id (qp.store/field %)) source-table)))
_]
(add-join-alias (qp.store/field field-id) form &match))
;; otherwise for any other `:field` whose table isn't the source Table, attempt to wrap it.
[:field
(field-id :guard (every-pred integer?
#(not= (:table_id (qp.store/field %)) source-table)))
_]
(add-join-alias (qp.store/field field-id) form &match))
;; add :joins and :source-query back which we removed above.
form (cond-> form
(seq joins) (assoc :joins joins)
......
test/metabase/driver/sql/query_processor_test.clj
+ 6
− 5
View file @ ed5a4489
......@@ -822,11 +822,12 @@
(deftest join-table-on-itself-with-custom-column-test
(testing "Should be able to join a source query against itself using an expression (#17770)"
(mt/dataset sample-dataset
(is (= '{:select [Q1.CATEGORY AS Q1__CATEGORY
source.count AS count
source.CC AS CC
Q1.count AS Q1__count
Q1.CC AS Q1__CC]
(is (= '{:select [source.CATEGORY AS CATEGORY
source.count AS count
source.CC AS CC
Q1.CATEGORY AS Q1__CATEGORY
Q1.count AS Q1__count
Q1.CC AS Q1__CC]
:from [{:select [source.CATEGORY AS CATEGORY
source.count AS count
(1 + 1) AS CC]
......
test/metabase/query_processor/middleware/resolve_joined_fields_test.clj
+ 42
− 0
View file @ ed5a4489
......@@ -214,3 +214,45 @@
:aggregation [[:count]]
:breakout [$products.id]
:limit 5})))))))
(deftest do-not-rewrite-top-level-clauses-if-field-is-from-source-table-or-query
(testing (str "Do not add `:join-alias` to top-level `:field` clauses if the Field could come from the "
"`:source-table` or `:source-query` (#18502)")
(mt/dataset sample-dataset
(is (query= (mt/mbql-query people
{:source-query {:source-table $$people
:breakout [!month.created_at]
:aggregation [[:count]]
:order-by [[:asc !month.created_at]]}
:joins [{:source-query {:source-table $$people
:breakout [!month.birth_date]
:aggregation [[:count]]
:order-by [[:asc !month.birth_date]]}
:alias "Q2"
:condition [:= !month.created_at !month.&Q2.birth_date]
:fields [&Q2.birth_date &Q2.*count/BigInteger]
:strategy :left-join}]
:fields [!default.created_at
*count/BigInteger
&Q2.birth_date
&Q2.*count/BigInteger]
:limit 3})
(wrap-joined-fields
(mt/mbql-query people
{:source-query {:source-table $$people
:breakout [!month.created_at]
:aggregation [[:count]]
:order-by [[:asc !month.created_at]]}
:joins [{:source-query {:source-table $$people
:breakout [!month.birth_date]
:aggregation [[:count]]
:order-by [[:asc !month.birth_date]]}
:alias "Q2"
:condition [:= !month.created_at !month.&Q2.birth_date]
:fields [&Q2.birth_date &Q2.*count/BigInteger]
:strategy :left-join}]
:fields [!default.created_at
*count/BigInteger
&Q2.birth_date
&Q2.*count/BigInteger]
:limit 3})))))))
test/metabase/query_processor/util/nest_query_test.clj
+ 46
− 45
View file @ ed5a4489
......@@ -205,51 +205,52 @@
(deftest nest-expressions-ignore-source-queries-test
(testing (str "When 'raising' :expression clauses, only raise ones in the current level. Handle duplicate expression "
"names correctly.")
(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
(mt/mbql-query venues
{:source-query {:source-table $$venues
:expressions {:x [:* $price 2]}
:fields [$id [:expression "x"]]}
:expressions {:x [:* $price 4]}
:fields [$id [:expression "x"]]
:limit 1}))))))
(let [query (mt/mbql-query venues
{:source-query {:source-table $$venues
:expressions {:x [:* $price 2]}
:fields [$id [:expression "x"]]}
:expressions {:x [:* $price 4]}
: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)))))))
(deftest nest-expressions-with-joins-test
(testing "If there are any `:joins`, those need to be nested into the `:source-query` as well."
......
test/metabase/query_processor_test/explicit_joins_test.clj
+ 26
− 2
View file @ ed5a4489
......@@ -579,8 +579,9 @@
[:field "count" {:base-type :type/BigInteger}]]}
:limit 2})]
(mt/with-native-query-testing-context query
(is (= [[4 89 0.46 41]]
(mt/formatted-rows [int int 2.0 int]
;; source.product_id, source.count, source.expr, source.Q2__product_id, source.Q2__count
(is (= [[4 89 0.46 4 41]]
(mt/formatted-rows [int int 2.0 int int]
(qp/process-query query)))))))))))
(deftest join-against-saved-question-with-sort-test
......@@ -630,3 +631,26 @@
(is (= [[1 448] [1 493]]
(mt/formatted-rows [int int]
(qp/process-query query)))))))))))
(deftest join-against-same-table-as-source-query-source-table-test
(testing "Joining against the same table as the source table of the source query should work (#18502)"
(mt/test-drivers (mt/normal-drivers-with-feature :nested-queries :left-join)
(mt/dataset sample-dataset
(let [query (mt/mbql-query people
{:source-query {:source-table $$people
:breakout [!month.created_at]
:aggregation [[:count]]}
:joins [{:source-query {:source-table $$people
:breakout [!month.birth_date]
:aggregation [[:count]]}
:alias "Q2"
:condition [:= !month.created_at !month.&Q2.birth_date]
:fields :all}]
:order-by [[:asc !month.created_at]]
:limit 3})]
(mt/with-native-query-testing-context query
(is (= [["2016-04-01T00:00:00Z" 26 nil nil]
["2016-05-01T00:00:00Z" 77 nil nil]
["2016-06-01T00:00:00Z" 82 nil nil]]
(mt/formatted-rows [str int str int]
(qp/process-query query))))))))))
test/metabase/query_processor_test/expressions_test.clj
+ 5
− 3
View file @ ed5a4489
......@@ -472,9 +472,11 @@
[:field "CC" {:base-type :type/Integer, :join-alias "Q1"}]]
:fields :all}]
:order-by [[:asc $products.category]
[:desc [:field "count" {:base-type :type/Integer}]]]
[:desc [:field "count" {:base-type :type/Integer}]]
[:asc &Q1.products.category]]
:limit 1})]
(mt/with-native-query-testing-context query
(is (= [["Doohickey" 54 2 42 2]]
(mt/formatted-rows [str int int int int]
;; source.category, source.count, source.CC, Q1.category, Q1.count, Q1.CC
(is (= [["Doohickey" 42 2 "Doohickey" 42 2]]
(mt/formatted-rows [str int int str int int]
(qp/process-query query))))))))))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment