Skip to content
Snippets Groups Projects
Unverified Commit f00918c7 authored by Braden Shepherdson's avatar Braden Shepherdson Committed by GitHub
Browse files

[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.
parent 5b64eafb
No related merge requests found
......@@ -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]
......
......@@ -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)))))
......@@ -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)]}
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment