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

Fix resolution of implicit joins inside explicit join conditions (#20697) 

* Fix #20519

* Fix busted indentation
parent 97e2b7d3
Branches
Tags
No related merge requests found
Hide whitespace changes
Inline Side-by-side
frontend/test/metabase/scenarios/joins/reproductions/20519-cannot-join-on-aggregation-with-implicit-joins-and-nested-query.cy.spec.js
+ 1
− 1
View file @ 8b2877bd
......@@ -33,7 +33,7 @@ const questionDetails = {
},
};
describe.skip("issue 20519", () => {
describe("issue 20519", () => {
beforeEach(() => {
restore();
cy.signInAsAdmin();
......
src/metabase/query_processor/middleware/add_implicit_joins.clj
+ 64
− 11
View file @ 8b2877bd
......@@ -3,6 +3,7 @@
in the options and adds `:join-alias` info to those `:field` clauses."
(:refer-clojure :exclude [alias])
(:require [clojure.set :as set]
[clojure.walk :as walk]
[medley.core :as m]
[metabase.db.util :as mdb.u]
[metabase.driver :as driver]
......@@ -145,27 +146,79 @@
add-condition-fields-to-source
(add-referenced-fields-to-source reused-joins))))))
(defn- join-dependencies
"Get a set of join aliases that `join` has an immediate dependency on."
[join]
(set
(mbql.u/match (:condition join)
[:field _ (opts :guard :join-alias)]
(let [{:keys [join-alias]} opts]
(when-not (= join-alias (:alias join))
join-alias)))))
(defn- topologically-sort-joins
"Sort `joins` by topological dependency order: joins that are referenced by the `:condition` of another will be sorted
first. If no dependencies exist between joins, preserve the existing order."
[joins]
(let [ ;; make a map of join alias -> immediate dependencies
join->immediate-deps (into {}
(map (fn [join]
[(:alias join) (join-dependencies join)]))
joins)
;; make a map of join alias -> immediate and transient dependencies
all-deps (fn all-deps [join-alias]
(let [immediate-deps (set (get join->immediate-deps join-alias))]
(into immediate-deps
(mapcat all-deps)
immediate-deps)))
join->all-deps (into {}
(map (fn [[join-alias]]
[join-alias (all-deps join-alias)]))
join->immediate-deps)
;; now we can create a function to decide if one join depends on another
depends-on? (fn [join-1 join-2]
(contains? (join->all-deps (:alias join-1))
(:alias join-2)))]
(->> ;; add a key to each join to record its original position
(map-indexed (fn [i join]
(assoc join ::original-position i)) joins)
;; sort the joins by topological order falling back to preserving original position
(sort (fn [join-1 join-2]
(cond
(depends-on? join-1 join-2) 1
(depends-on? join-2 join-1) -1
:else (compare (::original-position join-1)
(::original-position join-2)))))
;; remove the keys we used to record original position
(mapv (fn [join]
(dissoc join ::original-position))))))
(defn- resolve-implicit-joins-this-level
"Add new `:joins` for tables referenced by `:field` forms with a `:source-field`. Add `:join-alias` info to those
`:fields`. Add additional `:fields` to source query if needed to perform the join."
[form]
(let [implicitly-joined-fields (implicitly-joined-fields form)
new-joins (implicitly-joined-fields->joins implicitly-joined-fields)
required-joins (remove (partial already-has-join? form) new-joins)
reused-joins (set/difference (set new-joins) (set required-joins))]
(let [implicitly-joined-fields (implicitly-joined-fields form)
new-joins (implicitly-joined-fields->joins implicitly-joined-fields)
required-joins (remove (partial already-has-join? form) new-joins)
reused-joins (set/difference (set new-joins) (set required-joins))]
(cond-> form
(seq required-joins) (update :joins (fn [existing-joins]
(m/distinct-by
:alias
(concat existing-joins required-joins))))
true add-join-alias-to-fields-with-source-field
true (add-fields-to-source reused-joins))))
true (add-fields-to-source reused-joins)
(seq required-joins) (update :joins topologically-sort-joins))))
(defn- resolve-implicit-joins [{:keys [source-query joins], :as inner-query}]
(let [recursively-resolved (cond-> inner-query
source-query (update :source-query resolve-implicit-joins)
(seq joins) (update :joins (partial map resolve-implicit-joins)))]
(resolve-implicit-joins-this-level recursively-resolved)))
(defn- resolve-implicit-joins [query]
(walk/postwalk
(fn [form]
(if (and (map? form)
((some-fn :source-query :source-table) form)
(not (:condition form)))
(resolve-implicit-joins-this-level form)
form))
query))
;;; +----------------------------------------------------------------------------------------------------------------+
......@@ -181,7 +234,7 @@
[query]
(if (mbql.u/match-one (:query query) [:field _ (_ :guard (every-pred :source-field (complement :join-alias)))])
(do
(when-not (driver/supports? driver/*driver* :foreign-keys)
(when-not (driver/database-supports? driver/*driver* :foreign-keys (qp.store/database))
(throw (ex-info (tru "{0} driver does not support foreign keys." driver/*driver*)
{:driver driver/*driver*
:type error-type/unsupported-feature})))
......
test/metabase/query_processor/middleware/add_implicit_joins_test.clj
+ 127
− 0
View file @ 8b2877bd
......@@ -363,6 +363,51 @@
:breakout [$venue_id->venues.price]
:order-by [[:asc $venue_id->venues.price]]}))))))
(deftest topologically-sort-joins-test
(let [parent {:alias "Parent"
:condition [:=
[:field 2 nil]
[:field 1 {:join-alias "Parent"}]]}
child-1 {:alias "Child 1"
:condition [:=
[:field 1 {:join-alias "Parent"}]
[:field 1 {:join-alias "Child 1"}]]}
child-2 {:alias "Child 2"
:condition [:=
[:field 1 {:join-alias "Parent"}]
[:field 1 {:join-alias "Child 2"}]]}
child-1-child {:alias "Child 1 Child"
:condition [:=
[:field 1 {:join-alias "Child 1"}]
[:field 1 {:join-alias "Child 1 Child"}]]}]
(testing "Join dependencies"
(is (= #{}
(#'add-implicit-joins/join-dependencies parent)))
(is (= #{"Parent"}
(#'add-implicit-joins/join-dependencies child-1)
(#'add-implicit-joins/join-dependencies child-2)))
(is (= #{"Child 1"}
(#'add-implicit-joins/join-dependencies child-1-child))))
(testing "Sort by dependency order"
(let [alias->join (u/key-by :alias [parent child-1 child-2 child-1-child])]
(doseq [[original expected] {["Parent" "Child 1" "Child 2"] ["Parent" "Child 1" "Child 2"]
["Child 1" "Parent" "Child 2"] ["Parent" "Child 1" "Child 2"]
["Child 1" "Child 2" "Parent"] ["Parent" "Child 1" "Child 2"]
;; should preserve order if there are no dependencies.
["Child 1" "Child 2"] ["Child 1" "Child 2"]
["Child 2" "Child 1"] ["Child 2" "Child 1"]
["Parent" "Child 2" "Child 1"] ["Parent" "Child 2" "Child 1"]
["Child 2" "Parent" "Child 1"] ["Parent" "Child 2" "Child 1"]
["Child 2" "Child 1" "Parent"] ["Parent" "Child 2" "Child 1"]
;; should handle dependencies of dependencies
["Parent" "Child 1" "Child 2" "Child 1 Child"] ["Parent" "Child 1" "Child 2" "Child 1 Child"]
["Parent" "Child 1" "Child 1 Child" "Child 2"] ["Parent" "Child 1" "Child 1 Child" "Child 2"]
["Parent" "Child 1 Child" "Child 1" "Child 2"] ["Parent" "Child 1" "Child 1 Child" "Child 2"]
["Child 1 Child" "Parent" "Child 1" "Child 2"] ["Parent" "Child 1" "Child 1 Child" "Child 2"]}]
(testing (format "Sort %s" original)
(is (= expected
(mapv :alias (#'add-implicit-joins/topologically-sort-joins (mapv alias->join original)))))))))))
(deftest mix-implicit-and-explicit-joins-test
(testing "Test that adding implicit joins still works correctly if the query also contains explicit joins"
(is (= (mt/mbql-query checkins
......@@ -468,3 +513,85 @@
{:source-field %product_id, ::namespaced true}]]})
add-implicit-joins
(m/dissoc-in [:query :source-metadata])))))))
(deftest resolve-implicit-joins-in-join-conditions-test
(testing "Should be able to resolve implicit joins inside a join `:condition`"
(mt/dataset sample-dataset
(is (query= (mt/mbql-query orders
{:source-query {:source-table $$orders
:fields [$product_id]}
:joins [{:source-table $$products
:alias "PRODUCTS__via__PRODUCT_ID"
:condition [:=
$product_id
[:field
%products.id
{:join-alias "PRODUCTS__via__PRODUCT_ID"}]]
:fields :none
:strategy :left-join
:fk-field-id %product_id}
{:source-table $$products
:alias "Products"
:condition [:=
[:field
%products.category
{:join-alias "PRODUCTS__via__PRODUCT_ID"
:source-field %product_id}]
&Products.products.category]
:fields :none}]})
(add-implicit-joins
(mt/mbql-query orders
{:source-query {:source-table $$orders
:fields [$product_id]}
:joins [{:source-table $$products
:alias "Products"
:condition [:=
$product_id->products.category
&Products.products.category]
:fields :none}]})))))))
(deftest use-source-query-implicit-joins-for-join-conditions-test
(testing "Implicit join inside a join `:condition` should use implicit join from source query if available (#20519)"
(mt/dataset sample-dataset
(is (query= (mt/mbql-query orders
{:source-query {:source-table $$orders
:joins [{:source-table $$products
:alias "PRODUCTS__via__PRODUCT_ID"
:condition [:=
$product_id
[:field
%products.id
{:join-alias "PRODUCTS__via__PRODUCT_ID"}]]
:fields :none
:strategy :left-join
:fk-field-id %product_id}]
:breakout [[:field
%products.category
{:join-alias "PRODUCTS__via__PRODUCT_ID"
:source-field %product_id}]]
:aggregation [[:count]]}
:joins [{:source-table $$products
:alias "Products"
:condition [:=
[:field
%products.category
{:join-alias "PRODUCTS__via__PRODUCT_ID"
:source-field %product_id}]
&Products.products.category]
:fields :none}]
:fields [[:field
%products.category
{:join-alias "PRODUCTS__via__PRODUCT_ID"
:source-field %product_id}]]})
(add-implicit-joins
(mt/mbql-query orders
{:source-query {:source-table $$orders
:breakout [$product_id->products.category]
:aggregation [[:count]]}
:joins [{:source-table $$products
:alias "Products"
:condition [:=
$product_id->products.category
&Products.products.category]
:fields :none}]
:fields [$product_id->products.category]})))))))
test/metabase/query_processor_test/explicit_joins_test.clj
+ 23
− 0
View file @ 8b2877bd
......@@ -848,3 +848,26 @@
[2 "Stout Burgers & Beers" "Burger" "Burger"]]
(mt/formatted-rows [int str str str]
(qp/process-query query)))))))))))
(deftest join-against-implicit-join-test
(testing "Should be able to explicitly join against an implicit join (#20519)"
(mt/test-drivers (mt/normal-drivers-with-feature :left-join :expressions :basic-aggregations)
(mt/with-bigquery-fks #{:bigquery-cloud-sdk}
(mt/dataset sample-dataset
(let [query (mt/mbql-query orders
{:source-query {:source-table $$orders
:breakout [$product_id->products.category]
:aggregation [[:count]]}
:joins [{:source-table $$products
:alias "Products"
:condition [:= *products.category &Products.products.category]
:fields [&Products.products.id
&Products.products.title]}]
:expressions {"CC" [:+ 1 1]}
:order-by [[:asc &Products.products.id]]
:limit 2})]
(mt/with-native-query-testing-context query
(is (= [["Gizmo" 4784 2 1 "Rustic Paper Wallet"]
["Doohickey" 3976 2 2 "Small Marble Shoes"]]
(mt/formatted-rows [str int int int str]
(qp/process-query query)))))))))))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment