Skip to content

Commit

Permalink
Implement replace-join and support joins in (remove/replace)-clause (m…
Browse files Browse the repository at this point in the history 
…etabase#32110)

Fixes metabase#32026.
  • Loading branch information
@metamben
metamben authored Jul 4, 2023
1 parent b8d0579 commit 6790e90
Show file tree
Hide file tree
Showing 4 changed files with 133 additions and 37 deletions.
3 changes: 2 additions & 1 deletion src/metabase/lib/core.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,8 @@
remove-clause
remove-join
rename-join
replace-clause]
replace-clause
replace-join]
[lib.stage
append-stage
drop-stage]
Expand Down
90 changes: 64 additions & 26 deletions src/metabase/lib/remove_replace.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
[metabase.lib.equality :as lib.equality]
[metabase.lib.join :as lib.join]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.options :as lib.options]
[metabase.lib.ref :as lib.ref]
[metabase.lib.util :as lib.util]
[metabase.mbql.util.match :as mbql.match]
Expand All @@ -14,10 +15,14 @@

(defn- stage-paths
[query stage-number]
(let [join-indices (range (count (lib.join/joins query stage-number)))
(let [joins (lib.join/joins query stage-number)
join-indices (range (count joins))
join-condition-paths (for [idx join-indices]
[:joins idx :conditions])
join-field-paths (for [idx join-indices]
join-field-paths (for [idx join-indices
:let [join (nth joins idx)]
;; :fields in a join can be just :all or :none (#31858)
:when (not (keyword? (:fields join)))]
[:joins idx :fields])]
(concat [[:order-by] [:breakout] [:filters] [:fields] [:aggregation] [:expressions]]
join-field-paths
Expand Down Expand Up @@ -63,7 +68,7 @@
[query stage-number unmodified-query-for-stage location target-clause remove-replace-fn]
(let [result (lib.util/update-query-stage query stage-number
remove-replace-fn location target-clause)
target-uuid (lib.util/clause-uuid target-clause)]
target-uuid (lib.options/uuid target-clause)]
(if (not= query result)
(mbql.match/match-one location
[:expressions]
Expand Down Expand Up @@ -100,11 +105,9 @@
to-remove (mapcat
(fn [location]
(when-let [clauses (get-in stage location)]
;; :fields in a join can be just :all or :none (#31858)
(when (seqable? clauses)
(->> clauses
(keep #(mbql.match/match-one %
[target-op _ target-ref-id] [location %]))))))
(->> clauses
(keep #(mbql.match/match-one %
[target-op _ target-ref-id] [location %])))))
(stage-paths query stage-number))]
(reduce
(fn [query [location target-clause]]
Expand Down Expand Up @@ -133,7 +136,7 @@
location (m/find-first
(fn [possible-location]
(when-let [clauses (get-in stage possible-location)]
(let [target-uuid (lib.util/clause-uuid target-clause)]
(let [target-uuid (lib.options/uuid target-clause)]
(when (some (comp #{target-uuid} :lib/uuid second) clauses)
possible-location))))
(stage-paths query stage-number))
Expand Down Expand Up @@ -167,6 +170,8 @@
(remove-replace-location query stage-number query location target-clause remove-replace-fn)
query))))

(declare remove-join)

(mu/defn remove-clause :- :metabase.lib.schema/query
"Removes the `target-clause` from the stage specified by `stage-number` of `query`.
If `stage-number` is not specified, the last stage is used."
Expand All @@ -176,7 +181,11 @@
([query :- :metabase.lib.schema/query
stage-number :- :int
target-clause]
(remove-replace* query stage-number target-clause :remove nil)))
(if (and (map? target-clause) (= (:lib/type target-clause) :mbql/join))
(remove-join query stage-number target-clause)
(remove-replace* query stage-number target-clause :remove nil))))

(declare replace-join)

(mu/defn replace-clause :- :metabase.lib.schema/query
"Replaces the `target-clause` with `new-clause` in the `query` stage specified by `stage-number`.
Expand All @@ -189,7 +198,9 @@
stage-number :- :int
target-clause
new-clause]
(remove-replace* query stage-number target-clause :replace new-clause)))
(if (and (map? target-clause) (= (:lib/type target-clause) :mbql/join))
(replace-join query stage-number target-clause new-clause)
(remove-replace* query stage-number target-clause :replace new-clause))))

(defn- replace-join-alias
[a-join old-name new-name]
Expand Down Expand Up @@ -286,8 +297,8 @@
(let [stage-before (lib.util/query-stage query-before stage-number)
stage-after (lib.util/query-stage query-after stage-number)
removed-cols (set/difference
(set (lib.metadata.calculation/returned-columns query-before stage-number stage-before))
(set (lib.metadata.calculation/returned-columns query-after stage-number stage-after)))
(set (lib.metadata.calculation/visible-columns query-before stage-number stage-before))
(set (lib.metadata.calculation/visible-columns query-after stage-number stage-after)))
invalid-locs (referring-locations query-after stage-after removed-cols)
paths (stage-paths query-after stage-number)
to-remove (concat (clauses-to-remove stage-after paths (map first invalid-locs))
Expand All @@ -303,6 +314,20 @@
(map? join-spec) (:alias join-spec)
:else join-spec))

(defn- update-joins
([query stage-number join-spec f]
(if-let [join-alias (join-spec->alias query stage-number join-spec)]
(binding [mu/*enforce* false]
(let [query-after (lib.util/update-query-stage
query
stage-number
(fn [stage]
(u/assoc-dissoc stage :joins (f (:joins stage) join-alias))))]
(reduce #(remove-invalidated-refs %1 query %2)
query-after
(take-while some? (iterate #(lib.util/next-stage-number query %) stage-number)))))
query)))

(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
Expand All @@ -316,16 +341,29 @@
([query :- :metabase.lib.schema/query
stage-number :- :int
join-spec :- [:or :metabase.lib.schema.join/join :string :int]]
(if-let [join-alias (join-spec->alias query stage-number join-spec)]
(binding [mu/*enforce* false]
(let [query-after (lib.util/update-query-stage
query
stage-number
(fn [stage]
(u/assoc-dissoc stage :joins
(not-empty (filterv #(not= (:alias %) join-alias)
(:joins stage))))))]
(reduce #(remove-invalidated-refs %1 query %2)
query-after
(take-while some? (iterate #(lib.util/next-stage-number query %) stage-number)))))
query)))
(update-joins query stage-number join-spec (fn [joins join-alias]
(not-empty (filterv #(not= (:alias %) join-alias)
joins))))))

(mu/defn replace-join :- :metabase.lib.schema/query
"Replace the join specified by `join-spec` in `query` at `stage-number` with `new-join`.
If `new-join` is nil, the join is removed as if by [[remove-join]].
The join can be specified either by itself (as returned by [[joins]]), by its alias
or by its index in the list of joins as returned by [[joins]].
If `stage-number` is not provided, the last stage is used.
If the specified join cannot be found, then `query` is returned as is.
Top level clauses containing references to the removed join are removed too."
([query join-spec new-join]
(replace-join query -1 join-spec new-join))

([query :- :metabase.lib.schema/query
stage-number :- :int
join-spec :- [:or :metabase.lib.schema.join/join :string :int]
new-join :- [:maybe :metabase.lib.schema.join/join]]
(if (nil? new-join)
(remove-join query stage-number join-spec)
(update-joins query stage-number join-spec (fn [joins join-alias]
(mapv #(if (= (:alias %) join-alias)
new-join
%)
joins))))))
8 changes: 0 additions & 8 deletions src/metabase/lib/util.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,6 @@
[clause]
(clause-of-type? clause :field))

;;; TODO -- you should use [[metabase.lib.options/uuid]] instead of this, since it also handles stuff with
;;; `:lib/options` maps (e.g. joins)
(defn clause-uuid
"Returns the :lib/uuid of `clause`. Returns nil if `clause` is not a clause."
[clause]
(when (clause? clause)
(get-in clause [1 :lib/uuid])))

(defn expression-name
"Returns the :lib/expression-name of `clause`. Returns nil if `clause` is not a clause."
[clause]
Expand Down
69 changes: 67 additions & 2 deletions test/metabase/lib/remove_replace_test.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -759,6 +759,71 @@
(testing "by name"
(is (=? result
(lib/remove-join query' 0 "Users"))))
(testing "by name"
(testing "by join-clause"
(is (=? result
(lib/remove-join query' 0 (second (lib/joins query' 0))))))))))
(lib/remove-join query' 0 (second (lib/joins query' 0)))))
(testing "using remove-clause"
(is (=? result
(lib/remove-clause query' 0 (second (lib/joins query' 0)))))))))))

(deftest ^:parallel replace-join-test
(let [query (lib.tu/query-with-join)
expected-original {:stages [{:joins [{:lib/type :mbql/join, :alias "Cat", :fields :all}]}]}
[original-join] (lib/joins query)
new-join (lib/with-join-fields original-join :none)]
(is (=? expected-original
query))
(testing "danglig join-spec leads to no change"
(are [join-spec] (=? expected-original
(lib/replace-join query 0 join-spec new-join))
-1 1 "missing-alias"))
(testing "replace using index"
(is (=? {:stages [{:joins [{:lib/type :mbql/join, :alias "Cat", :fields :none}]}]}
(lib/replace-join query 0 new-join))))
(testing "replace using alias"
(is (=? {:stages [{:joins [{:lib/type :mbql/join, :alias "Cat", :fields :none}]}]}
(lib/replace-join query "Cat" new-join))))
(testing "replace using replace-clause"
(is (=? {:stages [{:joins [{:lib/type :mbql/join, :alias "Cat", :fields :none}]}]}
(lib/replace-clause query original-join new-join))))
(let [join-alias "alias"
price-name (str join-alias "__PRICE")
joined-column (-> (meta/field-metadata :venues :id)
(lib/with-join-alias join-alias))
users-alias "Users"
last-login-name (str users-alias "__LAST_LOGIN")
breakout-field [:field {:base-type :type/DateTime} last-login-name]
query (-> (lib/query meta/metadata-provider (meta/table-metadata :checkins))
(lib/join (-> (lib/join-clause
(meta/table-metadata :venues)
[(lib/= (meta/field-metadata :checkins :venue-id)
(-> (meta/field-metadata :venues :id)
(lib/with-join-alias join-alias)))])
(lib/with-join-fields :all)
(lib/with-join-alias join-alias)))
(lib/filter (lib/> joined-column 3))
(lib/join (-> (lib/join-clause
(meta/table-metadata :users)
[(lib/= (meta/field-metadata :checkins :user-id)
(-> (meta/field-metadata :users :id)
(lib/with-join-alias users-alias)))])
(lib/with-join-fields :all)
(lib/with-join-alias users-alias)))
lib/append-stage
(lib/filter (lib/< (lib.options/ensure-uuid [:field {:base-type :type/Integer} price-name]) 3))
(lib/filter (lib/not-null (lib.options/ensure-uuid breakout-field)))
(lib/breakout (lib.options/ensure-uuid breakout-field)))
[join0 join1] (lib/joins query 0)]
(testing "effects are reflected in subsequent stages"
(is (=? {:stages [{:joins [{:fields :none, :alias join-alias}
{:fields :all, :alias users-alias}]
:filters [[:> {} [:field {:join-alias join-alias} (meta/id :venues :id)] 3]]}
{:filters [[:not-null {} [:field {} last-login-name]]]
:breakout [[:field {} last-login-name]]}]}
(lib/replace-clause query 0 join0 (lib/with-join-fields join0 :none)))))
(testing "replacing with nil removes the join"
(is (=? {:stages
[{:joins [{:fields :all, :alias join-alias}]
:filters [[:> {} [:field {:join-alias join-alias} (meta/id :venues :id)] 3]]}
{:filters [[:< {} [:field {} price-name] 3]]}]}
(lib/replace-clause query 0 join1 nil)))))))

0 comments on commit 6790e90

Please sign in to comment.