From a0667fd3dbc012fef3a4bda503daa57300202b17 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Thu, 19 Oct 2023 11:50:36 -0700
Subject: [PATCH] Distribution drill should not apply binning to FK columns
(#34814)
---
frontend/src/metabase-lib/drills.unit.spec.ts | 24 +++++++++----------
src/metabase/lib/drill_thru/distribution.cljc | 18 ++++++++++----
.../lib/drill_thru/distribution_test.cljc | 15 ++++++++++++
3 files changed, 41 insertions(+), 16 deletions(-)
diff --git a/frontend/src/metabase-lib/drills.unit.spec.ts b/frontend/src/metabase-lib/drills.unit.spec.ts
index ae876800213..85986a56455 100644
--- a/frontend/src/metabase-lib/drills.unit.spec.ts
+++ b/frontend/src/metabase-lib/drills.unit.spec.ts
@@ -1789,18 +1789,18 @@ describe("drillThru", () => {
},
},
- // FIXME: distribution drill result for FK columns creates extra binning, which is wrong (metabase#34343)
- // {
- // drillType: "drill-thru/distribution",
- // clickType: "header",
- // columnName: "USER_ID",
- // queryType: "unaggregated",
- // expectedQuery: {
- // aggregation: [["count"]],
- // breakout: [["field", ORDERS.USER_ID, null]],
- // "source-table": ORDERS_ID,
- // },
- // },
+ // distribution drill result for FK columns creates extra binning, which is wrong (metabase#34343)
+ {
+ drillType: "drill-thru/distribution",
+ clickType: "header",
+ columnName: "USER_ID",
+ queryType: "unaggregated",
+ expectedQuery: {
+ aggregation: [["count"]],
+ breakout: [["field", ORDERS.USER_ID, { "base-type": "type/Integer" }]],
+ "source-table": ORDERS_ID,
+ },
+ },
{
drillType: "drill-thru/distribution",
clickType: "header",
diff --git a/src/metabase/lib/drill_thru/distribution.cljc b/src/metabase/lib/drill_thru/distribution.cljc
index 29de9829415..068abd53544 100644
--- a/src/metabase/lib/drill_thru/distribution.cljc
+++ b/src/metabase/lib/drill_thru/distribution.cljc
@@ -34,15 +34,25 @@
:type :drill-thru/distribution
:column column}))
+(defn- add-temporal-bucketing-or-binning
+ [column]
+ (cond
+ (lib.types.isa/temporal? column)
+ (lib.temporal-bucket/with-temporal-bucket column :month)
+
+ (and (lib.types.isa/numeric? column)
+ (not (lib.types.isa/foreign-key? column)))
+ (lib.binning/with-binning column (lib.binning/default-auto-bin))
+
+ :else
+ column))
+
(mu/defmethod lib.drill-thru.common/drill-thru-method :drill-thru/distribution :- ::lib.schema/query
[query :- ::lib.schema/query
stage-number :- :int
{:keys [column] :as _drill-thru} :- ::lib.schema.drill-thru/drill-thru.distribution]
(when (lib.drill-thru.common/mbql-stage? query stage-number)
- (let [breakout (cond
- (lib.types.isa/temporal? column) (lib.temporal-bucket/with-temporal-bucket column :month)
- (lib.types.isa/numeric? column) (lib.binning/with-binning column (lib.binning/default-auto-bin))
- :else column)]
+ (let [breakout (add-temporal-bucketing-or-binning column)]
(-> query
;; Remove most of the target stage.
(lib.util/update-query-stage stage-number dissoc :aggregation :breakout :limit :order-by)
diff --git a/test/metabase/lib/drill_thru/distribution_test.cljc b/test/metabase/lib/drill_thru/distribution_test.cljc
index 4c1f3b2ccf7..15e8041633f 100644
--- a/test/metabase/lib/drill_thru/distribution_test.cljc
+++ b/test/metabase/lib/drill_thru/distribution_test.cljc
@@ -46,3 +46,18 @@
:query-type :unaggregated
:column-name "QUANTITY"
:expected {:type :drill-thru/distribution}}))
+
+(deftest ^:parallel apply-to-fk-column-test
+ (testing "do not apply binning to FK columns (#34343)"
+ (lib.drill-thru.tu/test-drill-application
+ {:click-type :header
+ :column-name "USER_ID"
+ :query-type :unaggregated
+ :drill-type :drill-thru/distribution
+ :expected {:type :drill-thru/distribution
+ :column {:name "USER_ID"}}
+ :expected-query {:stages [{:source-table (meta/id :orders)
+ :aggregation [[:count {}]]
+ :breakout [[:field
+ {:binning (symbol "nil #_\"key is not present.\"")}
+ (meta/id :orders :user-id)]]}]}})))
--
GitLab