diff --git a/src/metabase/lib/aggregation.cljc b/src/metabase/lib/aggregation.cljc
index 4288c3b99085d4ed98dc6bef87308e5e2995c882..5e09a68844d72df755eb54b6b75fae7cc502c26f 100644
--- a/src/metabase/lib/aggregation.cljc
+++ b/src/metabase/lib/aggregation.cljc
@@ -66,20 +66,34 @@
[query stage-number [_tag _opts index] style]
(lib.metadata.calculation/display-name query stage-number (resolve-aggregation query stage-number index) style))
-(defmethod lib.metadata.calculation/display-name-method :count
- [query stage-number [_count _opts x] style]
+(lib.hierarchy/derive ::count-aggregation ::aggregation)
+
+;;; count and cumulative count can both be used either with no args (count of rows) or with one arg (count of X, which
+;;; I think means count where X is not NULL or something like that. Basically `count(x)` in SQL)
+(doseq [tag [:count
+ :cum-count]]
+ (lib.hierarchy/derive tag ::count-aggregation))
+
+(defmethod lib.metadata.calculation/display-name-method ::count-aggregation
+ [query stage-number [tag _opts x] style]
;; x is optional.
(if x
- (i18n/tru "Count of {0}" (lib.metadata.calculation/display-name query stage-number x style))
- (i18n/tru "Count")))
+ (let [x-display-name (lib.metadata.calculation/display-name query stage-number x style)]
+ (case tag
+ :count (i18n/tru "Count of {0}" x-display-name)
+ :cum-count (i18n/tru "Cumulative count of {0}" x-display-name)))
+ (case tag
+ :count (i18n/tru "Count")
+ :cum-count (i18n/tru "Cumulative count"))))
(defmethod lib.metadata.calculation/column-name-method :count
- [query stage-number [_count _opts x]]
- (if x
- (str "count_" (lib.metadata.calculation/column-name query stage-number x))
- "count"))
-
-(lib.hierarchy/derive :count ::aggregation)
+ [query stage-number [tag _opts x]]
+ (let [prefix (case tag
+ :count "count"
+ :cum-count "cum_count")]
+ (if x
+ (str prefix \_ (lib.metadata.calculation/column-name query stage-number x))
+ prefix)))
(defmethod lib.metadata.calculation/display-name-method :case
[_query _stage-number _case _style]
@@ -89,10 +103,11 @@
[_query _stage-number _case]
"case")
+;;; TODO - Should `:case` derive from `::aggregation` as well???
+
(lib.hierarchy/derive ::unary-aggregation ::aggregation)
(doseq [tag [:avg
- :cum-count
:cum-sum
:distinct
:max
@@ -109,7 +124,6 @@
(str
(case tag
:avg "avg_"
- :cum-count "cum_count_"
:cum-sum "cum_sum_"
:distinct "distinct_"
:max "max_"
@@ -125,7 +139,6 @@
(let [arg (lib.metadata.calculation/display-name query stage-number arg style)]
(case tag
:avg (i18n/tru "Average of {0}" arg)
- :cum-count (i18n/tru "Cumulative count of {0}" arg)
:cum-sum (i18n/tru "Cumulative sum of {0}" arg)
:distinct (i18n/tru "Distinct values of {0}" arg)
:max (i18n/tru "Max of {0}" arg)
diff --git a/src/metabase/lib/core.cljc b/src/metabase/lib/core.cljc
index 55274b9142dd5ebabc80892ded5e300bbd84a7b4..246b26ad9364f3be790d919773e7f1fc9e535e2e 100644
--- a/src/metabase/lib/core.cljc
+++ b/src/metabase/lib/core.cljc
@@ -73,7 +73,9 @@
stddev
sum
sum-where
- var]
+ var
+ cum-count
+ cum-sum]
[lib.binning
available-binning-strategies
binning
diff --git a/test/metabase/lib/aggregation_test.cljc b/test/metabase/lib/aggregation_test.cljc
index c089d9ff81045539e4ad05a313cd00201fe6f600..68a1ff4426df33b89ad97e775da544d43df4bc4a 100644
--- a/test/metabase/lib/aggregation_test.cljc
+++ b/test/metabase/lib/aggregation_test.cljc
@@ -245,147 +245,151 @@
(is (= :type/Integer
(lib/type-of query (first (lib/aggregations-metadata query)))))))
+(def ^:private op-test-summable-cols
+ [{:display-name "Latitude"
+ :effective-type :type/Float
+ :semantic-type :type/Latitude
+ :lib/source :source/table-defaults}
+ {:display-name "Longitude"
+ :effective-type :type/Float
+ :semantic-type :type/Longitude
+ :lib/source :source/table-defaults}
+ {:display-name "Price"
+ :effective-type :type/Integer
+ :semantic-type :type/Category
+ :lib/source :source/table-defaults}
+ {:display-name "double-price"
+ :effective-type :type/Integer
+ :lib/source :source/expressions}])
+
+(def ^:private op-test-all-cols
+ [{:display-name "ID"
+ :effective-type :type/BigInteger
+ :semantic-type :type/PK
+ :lib/source :source/table-defaults}
+ {:display-name "Name"
+ :effective-type :type/Text
+ :semantic-type :type/Name
+ :lib/source :source/table-defaults}
+ {:display-name "Category ID"
+ :effective-type :type/Integer
+ :semantic-type :type/FK
+ :lib/source :source/table-defaults}
+ {:display-name "Latitude"
+ :effective-type :type/Float
+ :semantic-type :type/Latitude
+ :lib/source :source/table-defaults}
+ {:display-name "Longitude"
+ :effective-type :type/Float
+ :semantic-type :type/Longitude
+ :lib/source :source/table-defaults}
+ {:display-name "Price"
+ :effective-type :type/Integer
+ :semantic-type :type/Category
+ :lib/source :source/table-defaults}
+ {:display-name "double-price"
+ :effective-type :type/Integer
+ :lib/source :source/expressions}
+ {:display-name "budget?"
+ :effective-type :type/Boolean
+ :lib/source :source/expressions}
+ {:display-name "ID"
+ :effective-type :type/BigInteger
+ :semantic-type :type/PK
+ :lib/source :source/implicitly-joinable}
+ {:display-name "Name"
+ :effective-type :type/Text
+ :semantic-type :type/Name
+ :lib/source :source/implicitly-joinable}])
+
(deftest ^:parallel aggregation-operator-test
(let [query (-> (lib/query-for-table-name meta/metadata-provider "VENUES")
(lib/expression "double-price" (lib/* (lib/field (meta/id :venues :price)) 2))
(lib/expression "budget?" (lib/< (lib/field (meta/id :venues :price)) 2))
(lib/aggregate (lib/sum [:expression {:lib/uuid (str (random-uuid))} "double-price"])))
- summable-cols [{:display-name "Latitude",
- :effective-type :type/Float,
- :semantic-type :type/Latitude,
- :lib/source :source/table-defaults}
- {:display-name "Longitude",
- :effective-type :type/Float,
- :semantic-type :type/Longitude,
- :lib/source :source/table-defaults}
- {:display-name "Price",
- :effective-type :type/Integer,
- :semantic-type :type/Category,
- :lib/source :source/table-defaults}
- {:display-name "double-price",
- :effective-type :type/Integer
- :lib/source :source/expressions}]
- all-cols [{:display-name "ID"
- :effective-type :type/BigInteger
- :semantic-type :type/PK
- :lib/source :source/table-defaults}
- {:display-name "Name"
- :effective-type :type/Text
- :semantic-type :type/Name
- :lib/source :source/table-defaults}
- {:display-name "Category ID",
- :effective-type :type/Integer,
- :semantic-type :type/FK,
- :lib/source :source/table-defaults}
- {:display-name "Latitude",
- :effective-type :type/Float,
- :semantic-type :type/Latitude,
- :lib/source :source/table-defaults}
- {:display-name "Longitude",
- :effective-type :type/Float,
- :semantic-type :type/Longitude,
- :lib/source :source/table-defaults}
- {:display-name "Price",
- :effective-type :type/Integer,
- :semantic-type :type/Category,
- :lib/source :source/table-defaults}
- {:display-name "double-price"
- :effective-type :type/Integer
- :lib/source :source/expressions}
- {:display-name "budget?"
- :effective-type :type/Boolean
- :lib/source :source/expressions}
- {:display-name "ID",
- :effective-type :type/BigInteger,
- :semantic-type :type/PK,
- :lib/source :source/implicitly-joinable}
- {:display-name "Name",
- :effective-type :type/Text,
- :semantic-type :type/Name,
- :lib/source :source/implicitly-joinable}]
- scope-cols all-cols
+ scope-cols op-test-all-cols
aggregation-operators (lib/available-aggregation-operators query)
count-op (first aggregation-operators)
sum-op (second aggregation-operators)]
(testing "available aggregation operators"
- (is (=? [{:short :count,
+ (is (=? [{:short :count
:requires-column? false}
- {:short :sum,
- :requires-column? true,
- :columns summable-cols}
- {:short :avg,
- :requires-column? true,
- :columns summable-cols}
- {:short :distinct,
- :requires-column? true,
- :columns all-cols}
- {:short :cum-sum,
- :requires-column? true,
- :columns summable-cols}
- {:short :cum-count,
+ {:short :sum
+ :requires-column? true
+ :columns op-test-summable-cols}
+ {:short :avg
+ :requires-column? true
+ :columns op-test-summable-cols}
+ {:short :distinct
+ :requires-column? true
+ :columns op-test-all-cols}
+ {:short :cum-sum
+ :requires-column? true
+ :columns op-test-summable-cols}
+ {:short :cum-count
:requires-column? false}
- {:short :stddev,
- :requires-column? true,
- :columns summable-cols}
- {:short :min,
- :requires-column? true,
+ {:short :stddev
+ :requires-column? true
+ :columns op-test-summable-cols}
+ {:short :min
+ :requires-column? true
:columns scope-cols}
- {:short :max,
- :requires-column? true,
+ {:short :max
+ :requires-column? true
:columns scope-cols}]
aggregation-operators)))
(testing "aggregation operator display info"
- (is (=? [{:display-name "Count of rows",
- :column-name "Count",
- :description "Total number of rows in the answer.",
- :short-name "count",
+ (is (=? [{:display-name "Count of rows"
+ :column-name "Count"
+ :description "Total number of rows in the answer."
+ :short-name "count"
:requires-column false}
- {:display-name "Sum of ...",
- :column-name "Sum",
- :description "Sum of all the values of a column.",
- :short-name "sum",
+ {:display-name "Sum of ..."
+ :column-name "Sum"
+ :description "Sum of all the values of a column."
+ :short-name "sum"
:requires-column true}
- {:display-name "Average of ...",
- :column-name "Average",
- :description "Average of all the values of a column",
- :short-name "avg",
+ {:display-name "Average of ..."
+ :column-name "Average"
+ :description "Average of all the values of a column"
+ :short-name "avg"
:requires-column true}
- {:display-name "Number of distinct values of ...",
- :column-name "Distinct values",
- :description "Number of unique values of a column among all the rows in the answer.",
- :short-name "distinct",
+ {:display-name "Number of distinct values of ..."
+ :column-name "Distinct values"
+ :description "Number of unique values of a column among all the rows in the answer."
+ :short-name "distinct"
:requires-column true}
- {:display-name "Cumulative sum of ...",
- :column-name "Sum",
- :description "Additive sum of all the values of a column.\ne.x. total revenue over time.",
- :short-name "cum-sum",
+ {:display-name "Cumulative sum of ..."
+ :column-name "Sum"
+ :description "Additive sum of all the values of a column.\ne.x. total revenue over time."
+ :short-name "cum-sum"
:requires-column true}
- {:display-name "Cumulative count of rows",
- :column-name "Count",
- :description "Additive count of the number of rows.\ne.x. total number of sales over time.",
- :short-name "cum-count",
+ {:display-name "Cumulative count of rows"
+ :column-name "Count"
+ :description "Additive count of the number of rows.\ne.x. total number of sales over time."
+ :short-name "cum-count"
:requires-column false}
- {:display-name "Standard deviation of ...",
- :column-name "SD",
- :description "Number which expresses how much the values of a column vary among all rows in the answer.",
- :short-name "stddev",
+ {:display-name "Standard deviation of ..."
+ :column-name "SD"
+ :description "Number which expresses how much the values of a column vary among all rows in the answer."
+ :short-name "stddev"
:requires-column true}
- {:display-name "Minimum of ...",
- :column-name "Min",
- :description "Minimum value of a column",
- :short-name "min",
+ {:display-name "Minimum of ..."
+ :column-name "Min"
+ :description "Minimum value of a column"
+ :short-name "min"
:requires-column true}
- {:display-name "Maximum of ...",
- :column-name "Max",
- :description "Maximum value of a column",
- :short-name "max",
+ {:display-name "Maximum of ..."
+ :column-name "Max"
+ :description "Maximum value of a column"
+ :short-name "max"
:requires-column true}]
(map #(lib/display-info query %) aggregation-operators))))
(testing "display name"
(is (= "Count of rows" (lib/display-name query (first aggregation-operators)))))
(testing "testing getting the available columns for an aggregation operator"
(is (nil? (lib/aggregation-operator-columns count-op)))
- (is (=? summable-cols (lib/aggregation-operator-columns sum-op))))
+ (is (=? op-test-summable-cols (lib/aggregation-operator-columns sum-op))))
(testing "aggregation operators can be added as aggregates"
(let [price-col (-> sum-op lib/aggregation-operator-columns pop peek)
agg-query (-> query
@@ -393,8 +397,8 @@
(lib/aggregate (lib/aggregation-clause sum-op price-col)))]
(is (=? {:lib/type :mbql/query
:stages
- [{:lib/type :mbql.stage/mbql,
- :source-table int?,
+ [{:lib/type :mbql.stage/mbql
+ :source-table int?
:expressions
{"double-price"
[:* {} [:field {:base-type :type/Integer, :effective-type :type/Integer} int?] 2]
@@ -405,21 +409,21 @@
[:count {}]
[:sum {} [:field {:base-type :type/Integer, :effective-type :type/Integer} int?]]]}]}
agg-query))
- (is (=? [{:lib/type :metadata/field,
- :effective-type :type/Integer,
- :name "sum_double-price",
- :display-name "Sum of double-price",
+ (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/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",
+ {: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)))))))
@@ -432,136 +436,83 @@
(lib/aggregate (lib/count)))
aggregations (lib/aggregations query)
aggregation-operators (lib/available-aggregation-operators query)
- summable-cols [{:display-name "Latitude",
- :effective-type :type/Float,
- :semantic-type :type/Latitude,
- :lib/source :source/table-defaults}
- {:display-name "Longitude",
- :effective-type :type/Float,
- :semantic-type :type/Longitude,
- :lib/source :source/table-defaults}
- {:display-name "Price",
- :effective-type :type/Integer,
- :semantic-type :type/Category,
- :lib/source :source/table-defaults}
- {:display-name "double-price",
- :effective-type :type/Integer
- :lib/source :source/expressions}]
- all-cols [{:display-name "ID"
- :effective-type :type/BigInteger
- :semantic-type :type/PK
- :lib/source :source/table-defaults}
- {:display-name "Name"
- :effective-type :type/Text
- :semantic-type :type/Name
- :lib/source :source/table-defaults}
- {:display-name "Category ID",
- :effective-type :type/Integer,
- :semantic-type :type/FK,
- :lib/source :source/table-defaults}
- {:display-name "Latitude",
- :effective-type :type/Float,
- :semantic-type :type/Latitude,
- :lib/source :source/table-defaults}
- {:display-name "Longitude",
- :effective-type :type/Float,
- :semantic-type :type/Longitude,
- :lib/source :source/table-defaults}
- {:display-name "Price",
- :effective-type :type/Integer,
- :semantic-type :type/Category,
- :lib/source :source/table-defaults}
- {:display-name "double-price"
- :effective-type :type/Integer
- :lib/source :source/expressions}
- {:display-name "budget?"
- :effective-type :type/Boolean
- :lib/source :source/expressions}
- {:display-name "ID",
- :effective-type :type/BigInteger,
- :semantic-type :type/PK,
- :lib/source :source/implicitly-joinable}
- {:display-name "Name",
- :effective-type :type/Text,
- :semantic-type :type/Name,
- :lib/source :source/implicitly-joinable}]
- scope-cols all-cols]
+ scope-cols op-test-all-cols]
(testing "aggregations"
(is (=? [[:sum {} [:expression {} "double-price"]]
[:count {}]]
aggregations)))
(testing "selected-aggregation-operators w/o column"
- (is (=? [{:short :count,
+ (is (=? [{:short :count
:requires-column? false
:selected? true}
- {:short :sum,
- :requires-column? true,
- :columns summable-cols}
- {:short :avg,
- :requires-column? true,
- :columns summable-cols}
- {:short :distinct,
- :requires-column? true,
- :columns all-cols}
- {:short :cum-sum,
- :requires-column? true,
- :columns summable-cols}
- {:short :cum-count,
+ {:short :sum
+ :requires-column? true
+ :columns op-test-summable-cols}
+ {:short :avg
+ :requires-column? true
+ :columns op-test-summable-cols}
+ {:short :distinct
+ :requires-column? true
+ :columns op-test-all-cols}
+ {:short :cum-sum
+ :requires-column? true
+ :columns op-test-summable-cols}
+ {:short :cum-count
:requires-column? false}
- {:short :stddev,
- :requires-column? true,
- :columns summable-cols}
- {:short :min,
- :requires-column? true,
+ {:short :stddev
+ :requires-column? true
+ :columns op-test-summable-cols}
+ {:short :min
+ :requires-column? true
:columns scope-cols}
- {:short :max,
- :requires-column? true,
+ {:short :max
+ :requires-column? true
:columns scope-cols}]
(lib/selected-aggregation-operators aggregation-operators (second aggregations)))))
(testing "selected-aggregation-operators w/ column"
(let [selected-operators (lib/selected-aggregation-operators
aggregation-operators (first aggregations))]
- (is (=? [{:short :count,
+ (is (=? [{:short :count
:requires-column? false}
- {:short :sum,
- :requires-column? true,
+ {:short :sum
+ :requires-column? true
:columns (mapv (fn [col]
(cond-> col
(= (:display-name col) "double-price")
(assoc :selected? true)))
- summable-cols)
+ op-test-summable-cols)
:selected? true}
- {:short :avg,
- :requires-column? true,
- :columns summable-cols}
- {:short :distinct,
- :requires-column? true,
- :columns all-cols}
- {:short :cum-sum,
- :requires-column? true,
- :columns summable-cols}
- {:short :cum-count,
+ {:short :avg
+ :requires-column? true
+ :columns op-test-summable-cols}
+ {:short :distinct
+ :requires-column? true
+ :columns op-test-all-cols}
+ {:short :cum-sum
+ :requires-column? true
+ :columns op-test-summable-cols}
+ {:short :cum-count
:requires-column? false}
- {:short :stddev,
- :requires-column? true,
- :columns summable-cols}
- {:short :min,
- :requires-column? true,
+ {:short :stddev
+ :requires-column? true
+ :columns op-test-summable-cols}
+ {:short :min
+ :requires-column? true
:columns scope-cols}
- {:short :max,
- :requires-column? true,
+ {:short :max
+ :requires-column? true
:columns scope-cols}]
selected-operators))
- (is (= [{:display-name "Count of rows",
- :column-name "Count",
- :description "Total number of rows in the answer.",
- :short-name "count",
+ (is (= [{:display-name "Count of rows"
+ :column-name "Count"
+ :description "Total number of rows in the answer."
+ :short-name "count"
:requires-column false}
- {:display-name "Sum of ...",
- :column-name "Sum",
- :description "Sum of all the values of a column.",
- :short-name "sum",
- :requires-column true,
+ {:display-name "Sum of ..."
+ :column-name "Sum"
+ :description "Sum of all the values of a column."
+ :short-name "sum"
+ :requires-column true
:selected true}]
(take 2 (map #(lib/display-info query %) selected-operators))))
(is (=? [{:display-name "Latitude",}
@@ -691,3 +642,30 @@
:lib/source-column-alias "sum_case"
:lib/desired-column-alias "sum_case"}]
(lib.metadata.calculation/metadata query)))))
+
+(deftest ^:parallel count-display-name-test
+ (testing "#31255"
+ (doseq [{:keys [k f expected]} [{:k :cum-count
+ :f lib/cum-count
+ :expected {:with-field "Cumulative count of ID"
+ :without-field "Cumulative count"}}
+ {:k :count
+ :f lib/count
+ :expected {:with-field "Count of ID"
+ :without-field "Count"}}]]
+ (testing k
+ (doseq [field? [true false]]
+ (testing (if field? "with field" "without field")
+ (let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :venues))
+ (lib/aggregate (if field?
+ (f (meta/field-metadata :venues :id))
+ (f))))]
+ (is (=? {:stages [{:aggregation [(if field?
+ [k {} [:field {} integer?]]
+ [k {}])]}]}
+ query)
+ "query")
+ (is (= [(expected (if field? :with-field :without-field))]
+ (map (partial lib/display-name query)
+ (lib.metadata.calculation/metadata query)))
+ "display name"))))))))