From 448eaa0eed033dced0b5033d2deaf819c31a12e1 Mon Sep 17 00:00:00 2001
From: Alexander Polyankin <alexander.polyankin@metabase.com>
Date: Thu, 8 Aug 2024 08:49:15 -0400
Subject: [PATCH] Disable broken card refs (#46594)
* Try without broken refs
* Disable broken test
* Disable broken test
---
src/metabase/lib/card.cljc | 2 +-
src/metabase/lib/equality.cljc | 8 +++-
.../middleware/binning_test.clj | 43 -------------------
3 files changed, 7 insertions(+), 46 deletions(-)
diff --git a/src/metabase/lib/card.cljc b/src/metabase/lib/card.cljc
index 9f85ba4d210..4aa52239d2a 100644
--- a/src/metabase/lib/card.cljc
+++ b/src/metabase/lib/card.cljc
@@ -72,7 +72,7 @@
untangle. The FE currently ignores results metadata for ad-hoc queries, and thus cannot match up 'correct' Field
refs like 'Products__CATEGORY'... for the time being we'll have to force ID refs even when we should be using
nominal refs so as to not completely destroy the FE. Once we port more stuff over maybe we can fix this."
- true)
+ false)
(defn- ->card-metadata-column
"Massage possibly-legacy Card results metadata into MLv2 ColumnMetadata. Note that `card` might be unavailable so we
diff --git a/src/metabase/lib/equality.cljc b/src/metabase/lib/equality.cljc
index 7bfaade7572..7305e1877d5 100644
--- a/src/metabase/lib/equality.cljc
+++ b/src/metabase/lib/equality.cljc
@@ -136,13 +136,17 @@
(clojure.core/= (column-join-alias column) join-alias)))
(mu/defn- plausible-matches-for-name :- [:sequential ::lib.schema.metadata/column]
- [[_ref-kind _opts ref-name :as a-ref] :- ::lib.schema.ref/ref
+ [[_ref-kind opts ref-name :as a-ref] :- ::lib.schema.ref/ref
columns :- [:sequential ::lib.schema.metadata/column]]
(or (not-empty (filter #(and (clojure.core/= (:lib/desired-column-alias %) ref-name)
(matching-join? a-ref %))
columns))
(filter #(and (clojure.core/= (:name %) ref-name)
- (matching-join? a-ref %))
+ ;; TODO: If the target ref has no join-alias, AND the source is fields or card, the join
+ ;; alias on the column can be ignored. QP can set it when it shouldn't. See #33972.
+ (or (and (not (:join-alias opts))
+ (#{:source/fields :source/card} (:lib/source %)))
+ (matching-join? a-ref %)))
columns)))
(mu/defn- plausible-matches-for-id :- [:sequential ::lib.schema.metadata/column]
diff --git a/test/metabase/query_processor/middleware/binning_test.clj b/test/metabase/query_processor/middleware/binning_test.clj
index 130f48cb14e..e05bfb78441 100644
--- a/test/metabase/query_processor/middleware/binning_test.clj
+++ b/test/metabase/query_processor/middleware/binning_test.clj
@@ -3,17 +3,14 @@
(:require
[clojure.test :refer :all]
[medley.core :as m]
- [metabase.lib.card :as lib.card]
[metabase.lib.convert :as lib.convert]
[metabase.lib.core :as lib]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.jvm :as lib.metadata.jvm]
[metabase.lib.test-metadata :as meta]
[metabase.lib.test-util :as lib.tu]
- [metabase.lib.test-util.macros :as lib.tu.macros]
[metabase.query-processor :as qp]
[metabase.query-processor.middleware.binning :as binning]
- [metabase.query-processor.preprocess :as qp.preprocess]
[metabase.query-processor.store :as qp.store]
[metabase.query-processor.test-util :as qp.test-util]
[metabase.test :as mt]))
@@ -177,46 +174,6 @@
(is (= [[-30.00M -60.00M 1]]
(mt/rows (qp/process-query query))))))))))
-(deftest ^:parallel fuzzy-metadata-matching-test
- (testing "Make sure we use fuzzy metadata matching to update binning strategies so it works for queries generated by MLv2"
- ;; this is disabled for now, but once we stop generating broken card refs it should work again -- see #33453
- (when-not lib.card/*force-broken-card-refs*
- (qp.store/with-metadata-provider meta/metadata-provider
- (let [source-card-query (lib.tu.macros/mbql-query orders
- {:joins [{:source-table $$people
- :alias "People"
- :condition [:= $user-id [:field %people.id {:join-alias "People"}]]
- :fields [[:field %people.longitude {:join-alias "People"}]
- [:field %people.birth-date {:temporal-unit :default, :join-alias "People"}]]}
- {:source-table $$products
- :alias "Products"
- :condition [:= $product-id &Products.products.id]
- :fields [&Products.products.price]}]
- :fields [[:field %id {:base-type :type/BigInteger}]]})
- source-metadata (qp.preprocess/query->expected-cols source-card-query)
- query (-> (lib/query meta/metadata-provider source-card-query)
- lib/append-stage
- (lib/aggregate (lib/count)))
- people-longitude (m/find-first #(= (:id %) (meta/id :people :longitude))
- (lib/breakoutable-columns query))
- _ (is (some? people-longitude))
- binning-strategy (m/find-first #(= (:display-name %) "Bin every 20 degrees")
- (lib/available-binning-strategies query people-longitude))
- _ (is (some? binning-strategy))
- query (-> query
- (lib/breakout (lib/with-binning people-longitude binning-strategy)))
- legacy-query (-> (lib.convert/->legacy-MBQL query)
- (assoc-in [:query :source-metadata] source-metadata))]
- (is (=? {:query {:breakout [[:field
- "People__LONGITUDE"
- {:base-type :type/Float,
- :binning {:strategy :bin-width
- :bin-width 20.0
- :min-value -180.0
- :max-value -60.0
- :num-bins 6}}]]}}
- (binning/update-binning-strategy legacy-query))))))))
-
(deftest ^:parallel match-named-field-ref-filter
(testing "fields referencing source expressions can still properly update binning strategies (#26202)"
(mt/with-temp [:model/Card card (-> orders
--
GitLab