From f00918c7b70c4ba8d854e29c4265aeeecc1fd476 Mon Sep 17 00:00:00 2001
From: Braden Shepherdson <braden@metabase.com>
Date: Tue, 30 May 2023 17:42:13 -0400
Subject: [PATCH] [MLv2] Support `:case` expressions in aggregations (#31088)
Fixes #29935.
Part of the issue here was incorrect hand-rolling of the second
argument to `:case`, the default value. In the failing test case given
in #29935, it was written as `{:default 0}`. But that's accidentally
transferring for legacy MBQL works, where the default is an option and
the options come last rather than first in a clause.
This also fixes `lib.common/->op-arg` to make sure it recurses
properly. It wasn't recursing into lists or MBQL clauses, which left
unresolved `(fn [query stage] ...)` functions in eg. `:case`
alternatives, since those are nested inside lists.
---
src/metabase/lib/common.cljc | 13 +++++-
test/metabase/lib/aggregation_test.cljc | 40 +++++++++++++++++++
.../query_processor_test/test_mlv2.clj | 8 +---
3 files changed, 52 insertions(+), 9 deletions(-)
diff --git a/src/metabase/lib/common.cljc b/src/metabase/lib/common.cljc
index 284253999ad..34eec2cc3f0 100644
--- a/src/metabase/lib/common.cljc
+++ b/src/metabase/lib/common.cljc
@@ -28,8 +28,17 @@
:hierarchy lib.hierarchy/hierarchy)
(defmethod ->op-arg :default
- [_query _stage-number x]
- x)
+ [query stage-number x]
+ (if (and (vector? x)
+ (keyword? (first x)))
+ ;; MBQL clause
+ (mapv #(->op-arg query stage-number %) x)
+ ;; Something else - just return it
+ x))
+
+(defmethod ->op-arg :dispatch-type/sequential
+ [query stage-number xs]
+ (mapv #(->op-arg query stage-number %) xs))
(defmethod ->op-arg :metadata/field
[_query _stage-number field-metadata]
diff --git a/test/metabase/lib/aggregation_test.cljc b/test/metabase/lib/aggregation_test.cljc
index 85f75de8db7..c089d9ff810 100644
--- a/test/metabase/lib/aggregation_test.cljc
+++ b/test/metabase/lib/aggregation_test.cljc
@@ -651,3 +651,43 @@
first-stage (lib.util/query-stage query 0)]
(is (= 2 (count (:stages query))))
(is (contains? first-stage :order-by)))))
+
+(deftest ^:parallel aggregation-with-case-expression-metadata-test
+ (let [query (-> (lib/query-for-table-name meta/metadata-provider "VENUES")
+ (lib/limit 4)
+ (lib/breakout (lib/field (meta/id :venues :category-id)))
+ (lib/aggregate (lib/sum (lib/case [[(lib/< (lib/field (meta/id :venues :price)) 2)
+ (lib/field (meta/id :venues :price))]]
+ 0))))]
+ (is (=? [{:description nil
+ :lib/type :metadata/field
+ :table-id (meta/id :venues)
+ :name "CATEGORY_ID"
+ :base-type :type/Integer
+ :semantic-type :type/FK
+ :database-type "INTEGER"
+ :effective-type :type/Integer
+ :lib/source :source/breakouts
+ :lib/source-column-alias "CATEGORY_ID"
+ :lib/source-uuid string?
+ :fk-target-field-id (meta/id :categories :id)
+ :custom-position 0
+ :active true
+ :id (meta/id :venues :category-id)
+ :parent-id nil
+ :visibility-type :normal
+ :lib/desired-column-alias "CATEGORY_ID"
+ :display-name "Category ID"
+ :has-field-values :none
+ :target nil
+ :preview-display true
+ :fingerprint {:global {:distinct-count 28, :nil% 0.0}}}
+ {:lib/type :metadata/field
+ :base-type :type/Integer
+ :name "sum_case"
+ :display-name "Sum of Case"
+ :lib/source :source/aggregations
+ :lib/source-uuid string?
+ :lib/source-column-alias "sum_case"
+ :lib/desired-column-alias "sum_case"}]
+ (lib.metadata.calculation/metadata query)))))
diff --git a/test/metabase/query_processor_test/test_mlv2.clj b/test/metabase/query_processor_test/test_mlv2.clj
index c00bb64bc34..f20273392b4 100644
--- a/test/metabase/query_processor_test/test_mlv2.clj
+++ b/test/metabase/query_processor_test/test_mlv2.clj
@@ -42,13 +42,7 @@
#{:datetime-add :datetime-subtract :convert-timezone}
(mbql.u/match-one &match
[_tag (_literal :guard string?) & _]
- "#29910"))
- ;; #29935: metadata for an `:aggregation` with a `:case` expression not working
- (mbql.u/match-one legacy-query
- {:aggregation aggregations}
- (mbql.u/match-one aggregations
- :case
- "#29935"))))
+ "#29910"))))
(defn- test-mlv2-metadata [original-query _qp-metadata]
{:pre [(map? original-query)]}
--
GitLab