From a13940ac3ef3645b7b1abdf697f442ac85907a56 Mon Sep 17 00:00:00 2001
From: metamben <103100869+metamben@users.noreply.github.com>
Date: Tue, 15 Aug 2023 07:32:06 +0300
Subject: [PATCH] Fix alias of LHS from join (#33103)
Porting fix alias of joined column LHS of standard join conditions (#32987)
* Check if the LHS has the alias of the current join before stripping its alias.
* Remove dependent joins when removing a join
---
src/metabase/lib/join.cljc | 10 ++--
src/metabase/lib/remove_replace.cljc | 39 +++++++++------
test/metabase/lib/join_test.cljc | 58 ++++++++++++++++++----
test/metabase/lib/remove_replace_test.cljc | 20 ++++++++
4 files changed, 98 insertions(+), 29 deletions(-)
diff --git a/src/metabase/lib/join.cljc b/src/metabase/lib/join.cljc
index 4513a17e16f..992ebebc254 100644
--- a/src/metabase/lib/join.cljc
+++ b/src/metabase/lib/join.cljc
@@ -483,19 +483,19 @@
;; so we break ties by looking at the poisition of the field reference.
(mbql.u.match/replace condition
[op op-opts (lhs :guard lib.util/field-clause?) (rhs :guard lib.util/field-clause?)]
- (let [lhs-aliased (contains? (lib.options/options lhs) :join-alias)
- rhs-aliased (contains? (lib.options/options rhs) :join-alias)]
+ (let [lhs-alias (current-join-alias lhs)
+ rhs-alias (current-join-alias rhs)]
(cond
;; no sides obviously belong to joined
- (not (or lhs-aliased rhs-aliased))
+ (not (or lhs-alias rhs-alias))
(if (lib.equality/find-closest-matching-ref metadata-providerable rhs home-refs)
[op op-opts (with-join-alias lhs join-alias) rhs]
[op op-opts lhs (with-join-alias rhs join-alias)])
;; both sides seem to belong to joined assuming this resulted from
;; overly fuzzy matching, we remove the join alias from the LHS
- ;; unless the RHS seems to belong to home too while the LHS doen't
- (and lhs-aliased rhs-aliased)
+ ;; unless the RHS seems to belong to home too while the LHS doesn't
+ (and (= lhs-alias join-alias) (= rhs-alias join-alias))
(let [bare-lhs (lib.options/update-options lhs dissoc :join-alias)
bare-rhs (lib.options/update-options rhs dissoc :join-alias)]
(if (and (nil? (lib.equality/find-closest-matching-ref metadata-providerable bare-lhs home-refs))
diff --git a/src/metabase/lib/remove_replace.cljc b/src/metabase/lib/remove_replace.cljc
index 0e0d2c3aed0..7ed858c1397 100644
--- a/src/metabase/lib/remove_replace.cljc
+++ b/src/metabase/lib/remove_replace.cljc
@@ -103,16 +103,16 @@
(defn- remove-local-references [query stage-number unmodified-query-for-stage target-op target-opts target-ref-id]
(let [stage (lib.util/query-stage query stage-number)
to-remove (mapcat
- (fn [location]
- (when-let [clauses (get-in stage location)]
- (->> clauses
- (keep (fn [clause]
- (mbql.match/match-one clause
- [target-op
- (_ :guard #(or (empty? target-opts)
- (set/subset? (set target-opts) (set %))))
- target-ref-id] [location clause]))))))
- (stage-paths query stage-number))]
+ (fn [location]
+ (when-let [clauses (get-in stage location)]
+ (->> clauses
+ (keep (fn [clause]
+ (mbql.match/match-one clause
+ [target-op
+ (_ :guard #(or (empty? target-opts)
+ (set/subset? (set target-opts) (set %))))
+ target-ref-id] [location clause]))))))
+ (stage-paths query stage-number))]
(reduce
(fn [query [location target-clause]]
(remove-replace-location query stage-number unmodified-query-for-stage location target-clause lib.util/remove-clause))
@@ -206,12 +206,15 @@
(replace-join query stage-number target-clause new-clause)
(remove-replace* query stage-number target-clause :replace new-clause))))
+(defn- field-clause-with-join-alias?
+ [field-clause join-alias]
+ (and (lib.util/field-clause? field-clause)
+ (= (lib.join/current-join-alias field-clause) join-alias)))
+
(defn- replace-join-alias
[a-join old-name new-name]
(mbql.match/replace a-join
- (field :guard (fn [field-clause]
- (and (lib.util/field-clause? field-clause)
- (= (lib.join/current-join-alias field-clause) old-name))))
+ (field :guard #(field-clause-with-join-alias? % old-name))
(lib.join/with-join-alias field new-name)))
(defn- rename-join-in-stage
@@ -313,6 +316,14 @@
(remove-invalidated-refs query-after query stage-number)))
query)))
+(defn- has-field-from-join? [form join-alias]
+ (some? (mbql.match/match-one form
+ (field :guard #(field-clause-with-join-alias? % join-alias)))))
+
+(defn- dependent-join? [join join-alias]
+ (or (= (:alias join) join-alias)
+ (has-field-from-join? join join-alias)))
+
(mu/defn remove-join :- :metabase.lib.schema/query
"Remove the join specified by `join-spec` in `query` at `stage-number`.
The join can be specified either by itself (as returned by [[joins]]), by its alias
@@ -327,7 +338,7 @@
stage-number :- :int
join-spec :- [:or :metabase.lib.schema.join/join :string :int]]
(update-joins query stage-number join-spec (fn [joins join-alias]
- (not-empty (filterv #(not= (:alias %) join-alias)
+ (not-empty (filterv #(not (dependent-join? % join-alias))
joins))))))
(mu/defn replace-join :- :metabase.lib.schema/query
diff --git a/test/metabase/lib/join_test.cljc b/test/metabase/lib/join_test.cljc
index dafa5e4fcf4..35460d82f2c 100644
--- a/test/metabase/lib/join_test.cljc
+++ b/test/metabase/lib/join_test.cljc
@@ -15,6 +15,8 @@
#?(:cljs (comment metabase.test-runner.assert-exprs.approximately-equal/keep-me))
+(def ^:private absent-key-marker (symbol "nil #_\"key is not present.\""))
+
(deftest ^:parallel resolve-join-test
(let [query lib.tu/venues-query
join-clause (-> (lib/join-clause
@@ -41,7 +43,7 @@
{:lib/uuid string?}
[:field
{:lib/uuid string?
- :join-alias (symbol "nil #_\"key is not present.\"")}
+ :join-alias absent-key-marker}
(meta/id :venues :category-id)]
[:field
{:lib/uuid string?
@@ -100,7 +102,7 @@
{:lib/uuid string?}
[:field
{:lib/uuid string?
- :join-alias (symbol "nil #_\"key is not present.\"")}
+ :join-alias absent-key-marker}
(meta/id :categories :id)]
[:field
{:lib/uuid string?
@@ -140,7 +142,7 @@
[:field
{:base-type :type/BigInteger
:lib/uuid string?
- :join-alias (symbol "nil #_\"key is not present.\"")}
+ :join-alias absent-key-marker}
"ID"]
[:field
{:lib/uuid string?
@@ -381,7 +383,7 @@
(are [strategy] (=? {:stages [{:joins [{:conditions [[:=
{}
[:field
- {:join-alias (symbol "nil #_\"key is not present.\"")}
+ {:join-alias absent-key-marker}
(meta/id :venues :category-id)]
[:field
{:join-alias "Categories"}
@@ -457,8 +459,8 @@
conditions))))]
(is (=? {:conditions [[:= {}
[:field {} (meta/id :venues :category-id)]
- [:field {:join-alias (symbol "nil #_\"key is not present.\"")} (meta/id :categories :id)]]]
- :alias (symbol "nil #_\"key is not present.\"")}
+ [:field {:join-alias absent-key-marker} (meta/id :categories :id)]]]
+ :alias absent-key-marker}
join))
(let [join' (lib/with-join-alias join "New Alias")]
(is (=? {:conditions [[:= {}
@@ -532,7 +534,7 @@
join' (lib/with-join-conditions join new-conditions)]
(is (=? [[:= {}
[:field {} (meta/id :venues :id)]
- [:field {:join-alias (symbol "nil #_\"key is not present.\"")} (meta/id :categories :id)]]]
+ [:field {:join-alias absent-key-marker} (meta/id :categories :id)]]]
(lib/join-conditions join')))
(testing "Adding an alias later with lib/with-join-alias should update conditions that are missing aliases"
(let [join'' (lib/with-join-alias join' "New Alias")]
@@ -553,7 +555,7 @@
conditions' (lib/join-conditions (lib/with-join-conditions join new-conditions))]
(is (=? [[:between {}
[:field {} (meta/id :venues :id)]
- [:field {:join-alias (symbol "nil #_\"key is not present.\"")}
+ [:field {:join-alias absent-key-marker}
(meta/id :categories :id)]
1]]
conditions'))))
@@ -569,10 +571,10 @@
(is (=? [[:and {}
[:= {}
[:field {} (meta/id :venues :id)]
- [:field {:join-alias (symbol "nil #_\"key is not present.\"")} (meta/id :categories :id)]]
+ [:field {:join-alias absent-key-marker} (meta/id :categories :id)]]
[:= {}
[:field {} (meta/id :venues :category-id)]
- [:field {:join-alias (symbol "nil #_\"key is not present.\"")} (meta/id :categories :id)]]]]
+ [:field {:join-alias absent-key-marker} (meta/id :categories :id)]]]]
conditions')))))))
(defn- test-with-join-fields [input expected]
@@ -1041,6 +1043,42 @@
products-created-at)
:year)))))
+(deftest ^:parallel default-join-alias-test
+ (testing "default join-alias set without overwriting other aliases (#32897)"
+ (let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :venues))
+ (lib/join (-> (lib/join-clause
+ (meta/table-metadata :checkins)
+ [(lib/= (meta/field-metadata :venues :id)
+ (-> (meta/field-metadata :checkins :venue-id)
+ (lib/with-join-alias "Checkins")))])
+ (lib/with-join-fields :all)
+ (lib/with-join-alias "Checkins")))
+ (lib/join (-> (lib/join-clause
+ (meta/table-metadata :users)
+ [(lib/= (-> (meta/field-metadata :checkins :user-id)
+ (lib/with-join-alias "Checkins"))
+ (meta/field-metadata :users :id))])
+ (lib/with-join-fields :all))))]
+ (is (=? [{:lib/type :mbql/join,
+ :stages [{:lib/type :mbql.stage/mbql
+ :source-table (meta/id :checkins)}]
+ :fields :all
+ :conditions [[:=
+ {}
+ [:field {:join-alias absent-key-marker} (meta/id :venues :id)]
+ [:field {:join-alias "Checkins"} (meta/id :checkins :venue-id)]]]
+ :alias "Checkins"}
+ {:lib/type :mbql/join
+ :stages [{:lib/type :mbql.stage/mbql
+ :source-table (meta/id :users)}]
+ :fields :all,
+ :conditions [[:=
+ {}
+ [:field {:join-alias "Checkins"} (meta/id :checkins :user-id)]
+ [:field {:join-alias "Users"} (meta/id :users :id)]]],
+ :alias "Users"}]
+ (lib/joins query))))))
+
(deftest ^:parallel join-condition-columns-handle-temporal-units-test
(testing "join-condition-lhs-columns and -rhs-columns should correctly mark columns regardless of :temporal-unit (#32390)\n"
(let [orders-query (lib/query meta/metadata-provider (meta/table-metadata :orders))
diff --git a/test/metabase/lib/remove_replace_test.cljc b/test/metabase/lib/remove_replace_test.cljc
index 70b575ab1b1..26ea4b33960 100644
--- a/test/metabase/lib/remove_replace_test.cljc
+++ b/test/metabase/lib/remove_replace_test.cljc
@@ -766,6 +766,26 @@
(is (=? result
(lib/remove-clause query' 0 (second (lib/joins query' 0)))))))))))
+(deftest ^:parallel remove-dependent-join
+ (let [first-join-clause (-> (lib/join-clause
+ (meta/table-metadata :checkins)
+ [(lib/= (meta/field-metadata :venues :id)
+ (-> (meta/field-metadata :checkins :venue-id)
+ (lib/with-join-alias "Checkins")))])
+ (lib/with-join-fields :all)
+ (lib/with-join-alias "Checkins"))
+ query (-> (lib/query meta/metadata-provider (meta/table-metadata :venues))
+ (lib/join first-join-clause)
+ (lib/join (-> (lib/join-clause
+ (meta/table-metadata :users)
+ [(lib/= (-> (meta/field-metadata :checkins :user-id)
+ (lib/with-join-alias "Checkins"))
+ (-> (meta/field-metadata :users :id)
+ (lib/with-join-alias "Users")))])
+ (lib/with-join-fields :all)
+ (lib/with-join-alias "Users"))))]
+ (is (nil? (lib/joins (lib/remove-clause query 0 first-join-clause))))))
+
(deftest ^:parallel replace-join-test
(let [query lib.tu/query-with-join
expected-original {:stages [{:joins [{:lib/type :mbql/join, :alias "Cat", :fields :all}]}]}
--
GitLab