Fix issues with column matching for multiple breakouts of the same column (#46979)

* Fix binning matching

* Add tests

* Add tests

* Add tests

* typo

* Update src/metabase/lib/equality.cljc

Co-authored-by: Braden Shepherdson <braden@metabase.com>

* Fix alignment

* Fix alignment

* Update src/metabase/lib/js.cljs

Co-authored-by: Braden Shepherdson <braden@metabase.com>

---------

Co-authored-by: Braden Shepherdson <braden@metabase.com>

frontend/src/metabase-lib/comparison.unit.spec.ts

+import * as Lib from "metabase-lib";
+
+import { createQueryWithClauses } from "./test-helpers";
+
+describe("findColumnIndexesFromLegacyRefs", () => {
+  const stageIndex = -1;
+
+  it("should match columns that differ only by temporal buckets", () => {
+    const query = createQueryWithClauses({
+      breakouts: [
+        {
+          tableName: "ORDERS",
+          columnName: "CREATED_AT",
+          temporalBucketName: "Year",
+        },
+        {
+          tableName: "ORDERS",
+          columnName: "CREATED_AT",
+          temporalBucketName: "Month",
+        },
+      ],
+    });
+    const columns = Lib.returnedColumns(query, stageIndex);
+    const columnIndexes = Lib.findColumnIndexesFromLegacyRefs(
+      query,
+      stageIndex,
+      columns,
+      columns.map(column => Lib.legacyRef(query, stageIndex, column)),
+    );
+    expect(columnIndexes).toEqual([0, 1]);
+  });
+
+  it("should match columns that differ only by binning", () => {
+    const query = createQueryWithClauses({
+      breakouts: [
+        {
+          tableName: "ORDERS",
+          columnName: "TOTAL",
+          binningStrategyName: "10 bins",
+        },
+        {
+          tableName: "ORDERS",
+          columnName: "TOTAL",
+          binningStrategyName: "50 bins",
+        },
+      ],
+    });
+    const columns = Lib.returnedColumns(query, stageIndex);
+    const columnIndexes = Lib.findColumnIndexesFromLegacyRefs(
+      query,
+      stageIndex,
+      columns,
+      columns.map(column => Lib.legacyRef(query, stageIndex, column)),
+    );
+    expect(columnIndexes).toEqual([0, 1]);
+  });
+});

src/metabase/lib/binning.cljc

@@ -130,13 +130,22 @@
            (when (= :default (:strategy binning-value))
              {:default true}))))
 
+(mu/defn binning= :- boolean?
+  "Given binning values (as returned by [[binning]]), check if they match."
+  [x :- [:maybe ::lib.schema.binning/binning]
+   y :- [:maybe ::lib.schema.binning/binning]]
+  (let [binning-keys (case (:strategy x)
+                       :num-bins  [:strategy :num-bins]
+                       :bin-width [:strategy :bin-width]
+                       [:strategy])]
+    (= (select-keys x binning-keys) (select-keys y binning-keys))))
+
 (mu/defn strategy= :- boolean?
   "Given a binning option (as returned by [[available-binning-strategies]]) and the binning value (possibly nil) from
   a column, check if they match."
   [binning-option :- ::lib.schema.binning/binning-option
    column-binning :- [:maybe ::lib.schema.binning/binning]]
-  (= (:mbql binning-option)
-     (select-keys column-binning [:strategy :num-bins :bin-width])))
+  (binning= (:mbql binning-option) column-binning))
 
 (mu/defn resolve-bin-width :- [:maybe [:map
                                        [:bin-width ::lib.schema.binning/bin-width]

src/metabase/lib/equality.cljc

@@ -4,6 +4,7 @@
   (:require
    #?@(:clj ([metabase.util.log :as log]))
    [medley.core :as m]
+   [metabase.lib.binning :as lib.binning]
    [metabase.lib.card :as lib.card]
    [metabase.lib.convert :as lib.convert]
    [metabase.lib.dispatch :as lib.dispatch]
@@ -188,6 +189,18 @@
       #?(:cljs (js/console.warn (ambiguous-match-error a-ref columns))
          :clj  (log/warn (ambiguous-match-error a-ref columns)))))
 
+(mu/defn- disambiguate-matches-find-match-with-same-binning :- [:maybe ::lib.schema.metadata/column]
+  "If there are multiple matching columns and `a-ref` has a binning value, check if only one column has that same
+  binning."
+  [a-ref   :- ::lib.schema.ref/ref
+   columns :- [:sequential {:min 2} ::lib.schema.metadata/column]]
+  (or (when-let [binning (lib.binning/binning a-ref)]
+        (let [matching-columns (filter #(-> % lib.binning/binning (lib.binning/binning= binning))
+                                       columns)]
+          (when (= (count matching-columns) 1)
+            (first matching-columns))))
+      (disambiguate-matches-dislike-field-refs-to-expressions a-ref columns)))
+
 (mu/defn- disambiguate-matches-find-match-with-same-temporal-bucket :- [:maybe ::lib.schema.metadata/column]
   "If there are multiple matching columns and `a-ref` has a temporal bucket, check if only one column has that same
   unit."
@@ -199,7 +212,7 @@
                                        columns)]
           (when (= (count matching-columns) 1)
             (first matching-columns))))
-      (disambiguate-matches-dislike-field-refs-to-expressions a-ref columns)))
+      (disambiguate-matches-find-match-with-same-binning a-ref columns)))
 
 (mu/defn- disambiguate-matches-prefer-explicit :- [:maybe ::lib.schema.metadata/column]
   "Prefers table-default or explicitly joined columns over implicitly joinable ones."

src/metabase/lib/js.cljs

@@ -1240,6 +1240,7 @@
   (-> a-legacy-ref
       (js->clj :keywordize-keys true)
       (update 0 keyword)
+      (->> (mbql.normalize/normalize-fragment nil))
       lib.convert/->pMBQL))
 
 (defn- ->column-or-ref [column]

test/metabase/lib/binning_test.cljc

 (ns metabase.lib.binning-test
   (:require
    #?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))
-   [clojure.test :refer [deftest is testing]]
+   [clojure.test :refer [are deftest is testing]]
    [medley.core :as m]
    [metabase.lib.binning :as lib.binning]
    [metabase.lib.core :as lib]
@@ -65,3 +65,24 @@
       (is (=?
            {:metabase.lib.field/temporal-unit :minute}
            (m/find-first (comp #{"CREATED_AT"} :name) (lib/returned-columns query)))))))
+
+(deftest ^:parallel binning-equality-test
+  (testing "should compare 'default' binning values"
+    (are [expected x y] (= expected (lib.binning/binning= x y) (lib.binning/binning= y x))
+      true  {:strategy :default} {:strategy :default}
+      false {:strategy :default} {:strategy :num-bins, :num-bins 10}
+      false {:strategy :default} {:strategy :bin-width, :bin-width 10}))
+  (testing "should compare 'num-bins' binning values"
+    (are [expected x y] (= expected (lib.binning/binning= x y) (lib.binning/binning= y x))
+      true  {:strategy :num-bins, :num-bins 10}               {:strategy :num-bins, :num-bins 10}
+      true  {:strategy :num-bins, :num-bins 10, :bin-width 3} {:strategy :num-bins, :num-bins 10}
+      true  {:strategy :num-bins, :num-bins 10, :bin-width 3} {:strategy :num-bins, :num-bins 10, :bin-width 4}
+      true  {:strategy :num-bins, :num-bins 10}               {:strategy :num-bins, :num-bins 10, :metadata-fn (fn [] nil)}
+      false {:strategy :num-bins, :num-bins 10}               {:strategy :num-bins, :num-bins 20}))
+  (testing "should compare 'bin-width' binning values"
+    (are [expected x y] (= expected (lib.binning/binning= x y) (lib.binning/binning= y x))
+      true  {:strategy :bin-width, :bin-width 10}              {:strategy :bin-width, :bin-width 10}
+      true  {:strategy :bin-width, :bin-width 10, :num-bins 3} {:strategy :bin-width, :bin-width 10}
+      true  {:strategy :bin-width, :bin-width 10, :num-bins 3} {:strategy :bin-width, :bin-width 10, :num-bins 4}
+      true  {:strategy :bin-width, :bin-width 10}              {:strategy :bin-width, :bin-width 10, :metadata-fn (fn [] nil)}
+      false {:strategy :bin-width, :bin-width 10}              {:strategy :bin-width, :bin-width 20})))

test/metabase/lib/equality_test.cljc

@@ -629,3 +629,25 @@
               (lib/ref col)
               [created-at-month
                created-at-year]))))))
+
+(deftest ^:parallel disambiguate-matches-using-binning-if-needed-test
+  (testing "'bin-width' binning strategy"
+    (let [latitude-10 (lib/with-binning (meta/field-metadata :people :latitude) {:strategy :bin-width, :bin-width 10})
+          latitude-20 (lib/with-binning (meta/field-metadata :people :latitude) {:strategy :bin-width, :bin-width 20})]
+      (doseq [col [latitude-10
+                   latitude-20]]
+        (is (= col
+               (lib.equality/find-matching-column
+                (lib/ref col)
+                [latitude-10
+                 latitude-20]))))))
+  (testing "'num-bins' binning strategy"
+    (let [total-10 (lib/with-binning (meta/field-metadata :orders :total) {:strategy :num-bins, :num-bins 10})
+          total-20 (lib/with-binning (meta/field-metadata :orders :total) {:strategy :num-bins, :num-bins 20})]
+      (doseq [col [total-10
+                   total-20]]
+        (is (= col
+               (lib.equality/find-matching-column
+                (lib/ref col)
+                [total-10
+                 total-20])))))))