Skip to content

Commit

Permalink
[Metrics] Remove support for joining on metrics
Browse files Browse the repository at this point in the history 
Effectively a revert of #42222 and #42300.

Fixes #42471.
  • Loading branch information
@bshepherdson
bshepherdson committed May 10, 2024
1 parent eed2514 commit 99a7185
Show file tree
Hide file tree
Showing 5 changed files with 23 additions and 180 deletions.
52 changes: 13 additions & 39 deletions src/metabase/lib/join.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
[metabase.lib.join.util :as lib.join.util]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.metric.basics :as lib.metric.basics]
[metabase.lib.options :as lib.options]
[metabase.lib.query :as lib.query]
[metabase.lib.ref :as lib.ref]
Expand Down Expand Up @@ -793,10 +792,7 @@
"Return suggested default join conditions when constructing a join against `joinable`, e.g. a Table, Saved
Question, or another query. Suggested conditions will be returned if the source Table has a foreign key to the
primary key of the thing we're joining (see #31175 for more info); otherwise this will return `nil` if no default
conditions are suggested.
For queries on metrics, the suggested join condition will be the first breakout of the source and join target,
provided that both are metrics with at least one breakout, and the types of these first breakouts are compatible."
conditions are suggested."
([query joinable]
(suggested-join-conditions query -1 joinable nil))

Expand Down Expand Up @@ -835,40 +831,18 @@
(let [x (dissoc x ::lib.card/force-broken-id-refs)
y (dissoc y ::lib.card/force-broken-id-refs)]
(lib.filter/filter-clause (lib.filter.operator/operator-def :=) x y)))]

(if-let [source (lib.metric.basics/source-metric query (lib.util/query-stage query stage-number))]
;; Metrics: if both the query stage and the joinable are metrics with at least one breakout, and those
;; breakouts have the same type, suggest joining on those breakout columns.
;; But if the `source` is a metric and the joinable not, then we don't want to return other join conditions.
(when-let [target (lib.metric.basics/join-metric query joinable)]
;; Both are metrics, now check that each has a breakout.
(let [source-breakout (some->> source (lib.metric.basics/primary-breakout unjoined))
target-breakout (some->> target (lib.metric.basics/primary-breakout unjoined))
source-type (:base-type source-breakout)
target-type (:base-type target-breakout)]
(when (and source-breakout target-breakout
source-type target-type
(= source-type target-type))
;; Everything lines up, so return the suggested condition.
;; If the source has a temporal bucket applied, overwrite the target's bucket to match.
(let [source-bucket (lib.temporal-bucket/temporal-bucket source-breakout)
target-breakout (cond-> target-breakout
source-bucket (lib.temporal-bucket/with-temporal-bucket source-bucket))]
[(filter-clause source-breakout target-breakout)]))))

;; Source isn't a metric, so run the normal table cases.
(or
;; find cases where we have FK(s) pointing to joinable. Our column goes on the LHS.
(when-let [fks (fks stage joinable)]
(mapv (fn [fk]
(filter-clause fk (::target fk)))
fks))
;; find cases where the `joinable` has FK(s) pointing to us. Note our column is the target this time around --
;; keep in on the LHS.
(when-let [fks (fks joinable stage)]
(mapv (fn [fk]
(filter-clause (::target fk) fk))
fks))))))))
(or
;; find cases where we have FK(s) pointing to joinable. Our column goes on the LHS.
(when-let [fks (fks stage joinable)]
(mapv (fn [fk]
(filter-clause fk (::target fk)))
fks))
;; find cases where the `joinable` has FK(s) pointing to us. Note our column is the target this time around --
;; keep in on the LHS.
(when-let [fks (fks joinable stage)]
(mapv (fn [fk]
(filter-clause (::target fk) fk))
fks)))))))

(defn- xform-add-join-alias [metadata-providerable a-join]
(let [join-alias (lib.join.util/current-join-alias a-join)]
Expand Down
33 changes: 10 additions & 23 deletions src/metabase/lib/metric.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
[metabase.lib.join :as lib.join]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.metric.basics :as lib.metric.basics]
[metabase.lib.options :as lib.options]
[metabase.lib.query :as lib.query]
[metabase.lib.ref :as lib.ref]
Expand Down Expand Up @@ -92,33 +91,23 @@
(lib.metadata.calculation/column-name query stage-number metric-metadata))
"metric"))

(defmethod lib.metadata.calculation/returned-columns-method :metadata/metric
[query stage-number metric options]
(lib.metadata.calculation/returned-columns-method query stage-number (assoc metric :lib/type :metadata/card) options))

(defn- metrics-for-all-joins
[query stage-number]
(for [join (lib.join/joins query stage-number)]
(lib.metric.basics/join-metric query join)))

(defn- joined-metrics
[query stage-number]
(->> (metrics-for-all-joins query stage-number)
(filter identity)))
(defn- source-metric
"Returns the `:metadata/metric` for the given stage, or nil if this stage is not based on a metric."
[metadata-providerable stage]
(some->> stage :source-card (lib.metadata/metric metadata-providerable)))

(mu/defn metric-based? :- :boolean
"Returns true if this MBQL `query` is based on metrics.
This is always false for stages other than 0, but accepting the parameter means consumers of the API don't need to
know about that.
Being \"based on metrics\" means the source is a metric, and so are the sources of all the joins in stage 0."
Being \"based on metrics\" means the source is a metric."
[query :- ::lib.schema/query
stage-number :- :int]
(and (zero? (lib.util/canonical-stage-index query stage-number))
(not (lib.query/native? query))
(lib.metric.basics/source-metric query (lib.util/query-stage query stage-number))
(every? identity (metrics-for-all-joins query 0))))
(source-metric query (lib.util/query-stage query stage-number))))

(mu/defn available-metrics :- [:maybe [:sequential {:min 1} ::lib.schema.metadata/metric]]
"Get a list of Metrics that you may consider using as aggregations for a query."
Expand All @@ -133,16 +122,14 @@
(:join-alias (lib.options/options aggregation-clause))]
index])))
(lib.aggregation/aggregations query stage-number))
s-metric (lib.metric.basics/source-metric query (lib.util/query-stage query stage-number))
metrics (cond->> (joined-metrics query stage-number)
s-metric (cons s-metric))]
(when (seq metrics)
s-metric (source-metric query (lib.util/query-stage query stage-number))]
(when s-metric
(if (empty? metric-aggregations)
(vec metrics)
[s-metric]
(mapv (fn [metric-metadata]
(let [aggregation-pos (-> metric-metadata
((juxt :id ::lib.join/join-alias))
metric-aggregations)]
(cond-> metric-metadata
aggregation-pos (assoc :aggregation-position aggregation-pos))))
metrics))))))
[s-metric]))))))
60 changes: 0 additions & 60 deletions src/metabase/lib/metric/basics.cljc
View file

This file was deleted.

48 changes: 0 additions & 48 deletions test/metabase/lib/join_test.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1470,51 +1470,3 @@
(testing "but when editing the first join, Orders.USER_ID is not visible and no condition is suggested"
(is (=? nil
(lib/suggested-join-conditions query -1 (meta/table-metadata :people) 0)))))))))

(deftest ^:parallel suggested-join-conditions-joining-metrics-test
(testing "joining metrics should suggest a condition on the primary breakouts"
(let [->metric (fn [id label inner-query]
{:name label
:id id
:database-id (meta/id)
:dataset-query {:database (meta/id)
:type :query
:query inner-query}})
subtotals-no-breakout (->metric 1 "All time revenue"
{:source-table (meta/id :orders)
:aggregation [[:sum [:field (meta/id :orders :subtotal) nil]]]})
subtotals-monthly (->metric 2 "Monthly revenue"
{:source-table (meta/id :orders)
:aggregation [[:sum [:field (meta/id :orders :subtotal) nil]]]
:breakout [[:field (meta/id :orders :created-at) {:temporal-unit :month}]]})
taxes-annually (->metric 3 "Annual tax totals"
{:source-table (meta/id :orders)
:aggregation [[:sum [:field (meta/id :orders :tax) nil]]]
:breakout [[:field (meta/id :orders :created-at) {:temporal-unit :year}]]})
subtotals-by-category (->metric 4 "Revenue by product category"
{:source-table (meta/id :orders)
:aggregation [[:sum [:field (meta/id :orders :subtotal) nil]]]
:breakout [[:field (meta/id :products :category)
{:fk-field-id (meta/id :orders :product-id)}]]})
provider (lib.tu/metadata-provider-with-mock-metrics
[subtotals-no-breakout subtotals-monthly taxes-annually subtotals-by-category])]
(testing "only if query is metric-based and joinable is a metric"
;; TODO: Enable creating a query directly from a metric rather than pretending it's a card.
(let [metric-query (lib/query provider (lib.metadata/card provider 2))
table-query (lib/query provider (meta/table-metadata :orders))]
(is (empty? (lib.join/suggested-join-conditions table-query (lib.metadata/metric provider 3))))
(is (empty? (lib.join/suggested-join-conditions metric-query (meta/table-metadata :products))))))

(testing "if query and joinable both have breakouts"
(let [metric-query (lib/query provider (lib.metadata/card provider 2))]
(testing "with matching types"
(is (=? [[:= {}
;; NOTE: No join aliases yet; those are added to the conditions only when the join is attached to
;; the query.
;; NOTE: Also, the temporal unit is copied from the source metric to the joined metric.
[:field {:temporal-unit :month} (meta/id :orders :created-at)]
[:field {:temporal-unit :month} (meta/id :orders :created-at)]]]
(lib.join/suggested-join-conditions metric-query (lib.metadata/metric provider 3)))))
(testing "- but not if their types don't match"
;; Breaking by product category, not time.
(is (empty? (lib.join/suggested-join-conditions metric-query (lib.metadata/metric provider 4))))))))))
10 changes: 0 additions & 10 deletions test/metabase/lib/test_util.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,16 +86,6 @@
(providers.mock/mock-metadata-provider
(assoc-in cards [:cards 0 :type] :metric))))

(defn metadata-provider-with-mock-metrics
([metrics]
(metadata-provider-with-mock-metrics meta/metadata-provider metrics))
([metadata-provider metrics]
(lib/composed-metadata-provider
metadata-provider
(mock-metadata-provider
;; Yes, cards, since v2 metrics are cards with `:type :metric`.
{:cards (mapv #(assoc % :type :metric) metrics)}))))

(def query-with-source-card
"A query against `:source-card 1`, with a metadata provider that has that Card. Card's name is `My Card`. Card
'exports' two columns, `USER_ID` and `count`."
Expand Down

0 comments on commit 99a7185

Please sign in to comment.