From 64b48da522b048ab0d20911a80c8136b0bbbf5b5 Mon Sep 17 00:00:00 2001
From: metamben <103100869+metamben@users.noreply.github.com>
Date: Mon, 29 May 2023 19:35:34 +0300
Subject: [PATCH] Ensure aggregation metadata has effective-type (#31100)
---
src/metabase/lib/aggregation.cljc | 8 ++--
src/metabase/lib/stage.cljc | 13 +++---
src/metabase/util.cljc | 8 ++++
test/metabase/lib/aggregation_test.cljc | 58 +++++++++++++++----------
4 files changed, 55 insertions(+), 32 deletions(-)
diff --git a/src/metabase/lib/aggregation.cljc b/src/metabase/lib/aggregation.cljc
index 45f77a06fc1..c9f1977c443 100644
--- a/src/metabase/lib/aggregation.cljc
+++ b/src/metabase/lib/aggregation.cljc
@@ -237,9 +237,11 @@
stage-number :- :int]
(some->> (not-empty (:aggregation (lib.util/query-stage query stage-number)))
(into [] (map (fn [aggregation]
- (-> (lib.metadata.calculation/metadata query stage-number aggregation)
- (assoc :lib/source :source/aggregations
- :lib/source-uuid (:lib/uuid (second aggregation))))))))))
+ (let [metadata (lib.metadata.calculation/metadata query stage-number aggregation)]
+ (-> metadata
+ (u/assoc-default :effective-type (or (:base-type metadata) :type/*))
+ (assoc :lib/source :source/aggregations
+ :lib/source-uuid (:lib/uuid (second aggregation)))))))))))
(def ^:private OperatorWithColumns
[:merge
diff --git a/src/metabase/lib/stage.cljc b/src/metabase/lib/stage.cljc
index bc436c49f72..c53fab0a037 100644
--- a/src/metabase/lib/stage.cljc
+++ b/src/metabase/lib/stage.cljc
@@ -16,6 +16,7 @@
[metabase.lib.schema.id :as lib.schema.id]
[metabase.lib.util :as lib.util]
[metabase.shared.util.i18n :as i18n]
+ [metabase.util :as u]
[metabase.util.malli :as mu]))
(declare stage-metadata)
@@ -155,13 +156,11 @@
(not-empty
(for [expression (lib.expression/expressions-metadata query stage-number)]
(let [base-type (:base-type expression)]
- (cond-> (assoc expression
- :lib/source :source/expressions
- :lib/source-column-alias (:name expression)
- :lib/desired-column-alias (unique-name-fn (:name expression)))
- (and (not (:effective-type expression))
- base-type)
- (assoc :effective-type base-type))))))
+ (-> (assoc expression
+ :lib/source :source/expressions
+ :lib/source-column-alias (:name expression)
+ :lib/desired-column-alias (unique-name-fn (:name expression)))
+ (u/assoc-default :effective-type (or base-type :type/*)))))))
;;; Calculate the columns to return if `:aggregations`/`:breakout`/`:fields` are unspecified.
;;;
diff --git a/src/metabase/util.cljc b/src/metabase/util.cljc
index 8e012a9e47d..97f269a807e 100644
--- a/src/metabase/util.cljc
+++ b/src/metabase/util.cljc
@@ -784,3 +784,11 @@
(if (some? v)
(assoc m k v)
(dissoc m k)))
+
+(defn assoc-default
+ "Called like `(assoc m k v)`, this does [[assoc]] iff `m` does not contain `k`
+ and `v` is not nil."
+ [m k v]
+ (if (or (nil? v) (contains? m k))
+ m
+ (assoc m k v)))
diff --git a/test/metabase/lib/aggregation_test.cljc b/test/metabase/lib/aggregation_test.cljc
index d6d3b1a4068..85f75de8db7 100644
--- a/test/metabase/lib/aggregation_test.cljc
+++ b/test/metabase/lib/aggregation_test.cljc
@@ -10,6 +10,7 @@
[metabase.lib.schema.expression :as lib.schema.expression]
[metabase.lib.test-metadata :as meta]
[metabase.lib.test-util :as lib.tu]
+ [metabase.lib.types.isa :as lib.types.isa]
[metabase.lib.util :as lib.util]))
#?(:cljs (comment metabase.test-runner.assert-exprs.approximately-equal/keep-me))
@@ -404,22 +405,22 @@
[:count {}]
[:sum {} [:field {:base-type :type/Integer, :effective-type :type/Integer} int?]]]}]}
agg-query))
- (is (=? [{:lib/type :metadata/field,
- :base-type :type/Integer,
- :name "sum_double-price",
- :display-name "Sum of double-price",
- :lib/source :source/aggregations}
- {:lib/type :metadata/field,
- :base-type :type/Integer,
- :name "count",
- :display-name "Count",
- :lib/source :source/aggregations}
- {:settings {:is_priceless true},
- :lib/type :metadata/field,
- :base-type :type/Integer,
- :name "sum_PRICE",
- :display-name "Sum of Price",
- :lib/source :source/aggregations}]
+ (is (=? [{:lib/type :metadata/field,
+ :effective-type :type/Integer,
+ :name "sum_double-price",
+ :display-name "Sum of double-price",
+ :lib/source :source/aggregations}
+ {:lib/type :metadata/field,
+ :effective-type :type/Integer,
+ :name "count",
+ :display-name "Count",
+ :lib/source :source/aggregations}
+ {:settings {:is_priceless true},
+ :lib/type :metadata/field,
+ :effective-type :type/Integer,
+ :name "sum_PRICE",
+ :display-name "Sum of Price",
+ :lib/source :source/aggregations}]
(lib/aggregations-metadata agg-query)))))))
(deftest ^:parallel selected-aggregation-operator-test
@@ -574,14 +575,27 @@
(testing "Aggregation metadata should return the `:settings` for the field being aggregated, for some reason."
(let [query (-> (lib/query-for-table-name meta/metadata-provider "VENUES")
(lib/aggregate (lib/sum (lib/field (meta/id :venues :price)))))]
- (is (=? {:settings {:is_priceless true}
- :lib/type :metadata/field
- :base-type :type/Integer
- :name "sum_PRICE"
- :display-name "Sum of Price"
- :lib/source :source/aggregations}
+ (is (=? {:settings {:is_priceless true}
+ :lib/type :metadata/field
+ :effective-type :type/Integer
+ :name "sum_PRICE"
+ :display-name "Sum of Price"
+ :lib/source :source/aggregations}
(lib.metadata.calculation/metadata query (first (lib/aggregations-metadata query -1))))))))
+(deftest ^:parallel count-aggregation-type-test
+ (testing "Count aggregation should produce numeric columns"
+ (let [query (-> (lib/query-for-table-name meta/metadata-provider "VENUES")
+ (lib/aggregate (lib/count)))
+ count-meta (first (lib/aggregations-metadata query -1))]
+ (is (=? {:lib/type :metadata/field
+ :effective-type :type/Integer
+ :name "count"
+ :display-name "Count"
+ :lib/source :source/aggregations}
+ count-meta))
+ (is (lib.types.isa/numeric? count-meta)))))
+
(deftest ^:parallel var-test
(let [query (-> (lib/query-for-table-name meta/metadata-provider "VENUES")
(lib/aggregate (lib/var (lib/field (meta/id :venues :price)))))]
--
GitLab