diff --git a/src/metabase/driver/druid/query_processor.clj b/src/metabase/driver/druid/query_processor.clj
index abcbc0fbd723d2d0766f5677c01155fe542a3a0d..c617e1a7b7328c02ffba3c49237d8187a4ea2e4b 100644
--- a/src/metabase/driver/druid/query_processor.clj
+++ b/src/metabase/driver/druid/query_processor.clj
@@ -218,34 +218,35 @@
(ag:count output-name))))
-(defn- handle-aggregation [query-type {ag-type :aggregation-type, ag-field :field, output-name :output-name, :as ag} druid-query]
- (when (isa? query-type ::ag-query)
- (merge-with concat
- druid-query
- (let [ag-type (when-not (= ag-type :rows) ag-type)]
- (match [ag-type ag-field]
- ;; For 'distinct values' queries (queries with a breakout by no aggregation) just aggregate by count, but name it :___count so it gets discarded automatically
- [nil nil] {:aggregations [(ag:count (or output-name :___count))]}
-
- [:count nil] {:aggregations [(ag:count (or output-name :count))]}
-
- [:count _] {:aggregations [(ag:count ag-field (or output-name :count))]}
-
- [:avg _] (let [count-name (name (gensym "___count_"))
- sum-name (name (gensym "___sum_"))]
- {:aggregations [(ag:count ag-field count-name)
- (ag:doubleSum ag-field sum-name)]
- :postAggregations [{:type :arithmetic
- :name (or output-name :avg)
- :fn :/
- :fields [{:type :fieldAccess, :fieldName sum-name}
- {:type :fieldAccess, :fieldName count-name}]}]})
- [:distinct _] {:aggregations [{:type :cardinality
- :name (or output-name :distinct___count)
- :fieldNames [(->rvalue ag-field)]}]}
- [:sum _] {:aggregations [(ag:doubleSum ag-field (or output-name :sum))]}
- [:min _] {:aggregations [(ag:doubleMin ag-field (or output-name :min))]}
- [:max _] {:aggregations [(ag:doubleMax ag-field (or output-name :max))]})))))
+(defn- handle-aggregation [query-type {ag-type :aggregation-type, ag-field :field, output-name :output-name, custom-name :custom-name, :as ag} druid-query]
+ (let [output-name (or custom-name output-name)]
+ (when (isa? query-type ::ag-query)
+ (merge-with concat
+ druid-query
+ (let [ag-type (when-not (= ag-type :rows) ag-type)]
+ (match [ag-type ag-field]
+ ;; For 'distinct values' queries (queries with a breakout by no aggregation) just aggregate by count, but name it :___count so it gets discarded automatically
+ [nil nil] {:aggregations [(ag:count (or output-name :___count))]}
+
+ [:count nil] {:aggregations [(ag:count (or output-name :count))]}
+
+ [:count _] {:aggregations [(ag:count ag-field (or output-name :count))]}
+
+ [:avg _] (let [count-name (name (gensym "___count_"))
+ sum-name (name (gensym "___sum_"))]
+ {:aggregations [(ag:count ag-field count-name)
+ (ag:doubleSum ag-field sum-name)]
+ :postAggregations [{:type :arithmetic
+ :name (or output-name :avg)
+ :fn :/
+ :fields [{:type :fieldAccess, :fieldName sum-name}
+ {:type :fieldAccess, :fieldName count-name}]}]})
+ [:distinct _] {:aggregations [{:type :cardinality
+ :name (or output-name :distinct___count)
+ :fieldNames [(->rvalue ag-field)]}]}
+ [:sum _] {:aggregations [(ag:doubleSum ag-field (or output-name :sum))]}
+ [:min _] {:aggregations [(ag:doubleMin ag-field (or output-name :min))]}
+ [:max _] {:aggregations [(ag:doubleMax ag-field (or output-name :max))]}))))))
(defn- add-expression-aggregation-output-names [args]
(for [arg args]
diff --git a/src/metabase/driver/generic_sql/query_processor.clj b/src/metabase/driver/generic_sql/query_processor.clj
index 98df73108f4d91546485229d74a43c1ed72f4522..7841d353e6e0ecec2ad6832e9cde59dd106094f0 100644
--- a/src/metabase/driver/generic_sql/query_processor.clj
+++ b/src/metabase/driver/generic_sql/query_processor.clj
@@ -153,24 +153,21 @@
(h/merge-select honeysql-form [(expression-aggregation->honeysql driver expression)
(hx/escape-dots (annotate/expression-aggregation-name expression))]))
+(defn- apply-single-aggregation [driver honeysql-form {:keys [aggregation-type field], :as aggregation}]
+ (h/merge-select honeysql-form [(aggregation->honeysql driver aggregation-type field)
+ (hx/escape-dots (annotate/expression-aggregation-name aggregation))]))
+
(defn apply-aggregation
"Apply a `aggregation` clauses to HONEYSQL-FORM. Default implementation of `apply-aggregation` for SQL drivers."
- ([driver honeysql-form {aggregations :aggregation}]
- (loop [form honeysql-form, [ag & more] aggregations]
- (let [form (if (instance? Expression ag)
- (apply-expression-aggregation driver form ag)
- (let [{:keys [aggregation-type field]} ag]
- (apply-aggregation driver form aggregation-type field)))]
- (if-not (seq more)
- form
- (recur form more)))))
-
- ([driver honeysql-form aggregation-type field]
- (h/merge-select honeysql-form [(aggregation->honeysql driver aggregation-type field)
- ;; the column alias is always the same as the ag type except for `:distinct` with is called `:count` (WHY?)
- (if (= aggregation-type :distinct)
- :count
- aggregation-type)])))
+ [driver honeysql-form {aggregations :aggregation}]
+ (loop [form honeysql-form, [ag & more] aggregations]
+ (let [form (if (instance? Expression ag)
+ (apply-expression-aggregation driver form ag)
+ (apply-single-aggregation driver form ag))]
+ (if-not (seq more)
+ form
+ (recur form more)))))
+
(defn apply-breakout
"Apply a `breakout` clause to HONEYSQL-FORM. Default implementation of `apply-breakout` for SQL drivers."
diff --git a/src/metabase/query_processor/annotate.clj b/src/metabase/query_processor/annotate.clj
index eb7cde1a58ce3a7b43ffdcf2067acaf538bc8c3d..fe6e778d5d501001083edeadd71489478413ff18 100644
--- a/src/metabase/query_processor/annotate.clj
+++ b/src/metabase/query_processor/annotate.clj
@@ -94,18 +94,24 @@
:count
ag-type))
+;; TODO - rename to something like `aggregation-name` or `aggregation-subclause-name` now that this handles any sort of aggregation
(defn expression-aggregation-name
- "Return an appropriate name for an expression aggregation, e.g. `sum + count`."
- ^String [ag]
+ "Return an appropriate name for an `:aggregation` subclause (an aggregation or expression)."
+ ^String [{custom-name :custom-name, aggregation-type :aggregation-type, :as ag}]
(cond
+ ;; if a custom name was provided use it
+ custom-name custom-name
+ ;; for unnamed expressions, just compute a name like "sum + count"
(instance? Expression ag) (let [{:keys [operator args]} ag]
(str/join (str " " (name operator) " ")
(for [arg args]
(if (instance? Expression arg)
(str "(" (expression-aggregation-name arg) ")")
(expression-aggregation-name arg)))))
- (:aggregation-type ag) (name (:aggregation-type ag))
- :else ag))
+ ;; for unnamed normal aggregations, the column alias is always the same as the ag type except for `:distinct` with is called `:count` (WHY?)
+ aggregation-type (if (= (keyword aggregation-type) :distinct)
+ "count"
+ (name aggregation-type))))
(defn- expression-aggregate-field-info [expression]
(let [ag-name (expression-aggregation-name expression)]
diff --git a/src/metabase/query_processor/expand.clj b/src/metabase/query_processor/expand.clj
index 11dcaf31528bb4434baa8e33b7db953014fe8b45..7c08ac74c45b439df87b418962363aa787aff1ed 100644
--- a/src/metabase/query_processor/expand.clj
+++ b/src/metabase/query_processor/expand.clj
@@ -59,6 +59,13 @@
(field-id f))
f))
+(s/defn ^:ql ^:always-validate named :- i/Aggregation
+ "Specify a CUSTOM-NAME to use for a top-level AGGREGATION-OR-EXPRESSION in the results.
+ (This will probably be extended to support Fields in the future, but for now, only the `:aggregation` clause is supported.)"
+ {:added "0.22.0"}
+ [aggregation-or-expression :- i/Aggregation, custom-name :- su/NonBlankString]
+ (assoc aggregation-or-expression :custom-name custom-name))
+
(s/defn ^:ql ^:always-validate datetime-field :- FieldPlaceholder
"Reference to a `DateTimeField`. This is just a `Field` reference with an associated datetime UNIT."
([f _ unit] (log/warn (u/format-color 'yellow (str "The syntax for datetime-field has changed in MBQL '98. [:datetime-field <field> :as <unit>] is deprecated. "
@@ -122,11 +129,11 @@
(if (number? arg)
arg
(field-or-expression arg))))
- ;; otherwise if it's not an Expression it's a a
+ ;; otherwise if it's not an Expression it's a Field
(field f)))
(s/defn ^:private ^:always-validate ag-with-field :- i/Aggregation [ag-type f]
- (i/strict-map->AggregationWithField {:aggregation-type ag-type, :field (field-or-expression f)}))
+ (i/map->AggregationWithField {:aggregation-type ag-type, :field (field-or-expression f)}))
(def ^:ql ^{:arglists '([f])} avg "Aggregation clause. Return the average value of F." (partial ag-with-field :avg))
(def ^:ql ^{:arglists '([f])} distinct "Aggregation clause. Return the number of distinct values of F." (partial ag-with-field :distinct))
@@ -144,13 +151,13 @@
(s/defn ^:ql ^:always-validate count :- i/Aggregation
"Aggregation clause. Return total row count (e.g., `COUNT(*)`). If F is specified, only count rows where F is non-null (e.g. `COUNT(f)`)."
- ([] (i/strict-map->AggregationWithoutField {:aggregation-type :count}))
+ ([] (i/map->AggregationWithoutField {:aggregation-type :count}))
([f] (ag-with-field :count f)))
(s/defn ^:ql ^:always-validate cum-count :- i/Aggregation
"Aggregation clause. Return the cumulative row count (presumably broken out in some way)."
[]
- (i/strict-map->AggregationWithoutField {:aggregation-type :cumulative-count}))
+ (i/map->AggregationWithoutField {:aggregation-type :cumulative-count}))
(defn ^:ql ^:deprecated rows
"Bare rows aggregation. This is the default behavior, so specifying it is deprecated."
@@ -405,10 +412,10 @@
(s/defn ^:private ^:always-validate expression-fn :- Expression
[k :- s/Keyword, & args]
- (i/strict-map->Expression {:operator k, :args (vec (for [arg args]
- (if (number? arg)
- (float arg) ; convert args to floats so things like 5 / 10 -> 0.5 instead of 0
- arg)))}))
+ (i/map->Expression {:operator k, :args (vec (for [arg args]
+ (if (number? arg)
+ (float arg) ; convert args to floats so things like 5 / 10 -> 0.5 instead of 0
+ arg)))}))
(def ^:ql ^{:arglists '([rvalue1 rvalue2 & more]), :added "0.17.0"} + "Arithmetic addition function." (partial expression-fn :+))
(def ^:ql ^{:arglists '([rvalue1 rvalue2 & more]), :added "0.17.0"} - "Arithmetic subtraction function." (partial expression-fn :-))
diff --git a/src/metabase/query_processor/interface.clj b/src/metabase/query_processor/interface.clj
index 0e95c8e4e8fb027ea8c0ab33ee01f03ee896a048..526e887800f025b620fb5d0c0d7102f16fd88bba 100644
--- a/src/metabase/query_processor/interface.clj
+++ b/src/metabase/query_processor/interface.clj
@@ -181,9 +181,10 @@
(def ^:private ExpressionOperator (s/named (s/enum :+ :- :* :/) "Valid expression operator"))
-(s/defrecord Expression [operator :- ExpressionOperator
- args :- [(s/cond-pre (s/recursive #'RValue)
- (s/recursive #'Aggregation))]])
+(s/defrecord Expression [operator :- ExpressionOperator
+ args :- [(s/cond-pre (s/recursive #'RValue)
+ (s/recursive #'Aggregation))]
+ custom-name :- (s/maybe su/NonBlankString)])
(def AnyFieldOrExpression
"Schema for a `FieldPlaceholder`, `AgRef`, or `Expression`."
@@ -241,12 +242,14 @@
;;; # ------------------------------------------------------------ CLAUSE SCHEMAS ------------------------------------------------------------
(s/defrecord AggregationWithoutField [aggregation-type :- (s/named (s/enum :count :cumulative-count)
- "Valid aggregation type")])
+ "Valid aggregation type")
+ custom-name :- (s/maybe su/NonBlankString)])
(s/defrecord AggregationWithField [aggregation-type :- (s/named (s/enum :avg :count :cumulative-sum :distinct :max :min :stddev :sum)
"Valid aggregation type")
field :- (s/cond-pre FieldPlaceholderOrExpressionRef
- Expression)])
+ Expression)
+ custom-name :- (s/maybe su/NonBlankString)])
(defn- valid-aggregation-for-driver? [{:keys [aggregation-type]}]
(when (= aggregation-type :stddev)
diff --git a/src/metabase/query_processor/macros.clj b/src/metabase/query_processor/macros.clj
index 429cfc30daafafd29b3bd64eca1d43f6de7132d0..dfe291fc13f9d56804b35e9925a8d6872564464e 100644
--- a/src/metabase/query_processor/macros.clj
+++ b/src/metabase/query_processor/macros.clj
@@ -1,5 +1,8 @@
(ns metabase.query-processor.macros
+ "TODO - this namespace is ancient and written with MBQL '95 in mind, e.g. it is case-sensitive.
+ At some point this ought to be reworked to be case-insensitive and cleaned up."
(:require [clojure.core.match :refer [match]]
+ [clojure.walk :as walk]
[metabase.db :as db]
[metabase.util :as u]))
@@ -18,6 +21,9 @@
~@match-forms
form# (throw (Exception. (format ~(format "%s failed: invalid clause: %%s" fn-name) form#)))))))
+
+;;; ------------------------------------------------------------ Segments ------------------------------------------------------------
+
(defparser segment-parse-filter-subclause
["SEGMENT" (segment-id :guard integer?)] (:filter (db/select-one-field :definition 'Segment, :id segment-id))
subclause subclause)
@@ -40,50 +46,50 @@
(seq addtl) addtl
:else []))
-(defn- merge-aggregation [aggregations new-ag]
- (if (map? aggregations)
- (recur [aggregations] new-ag)
- (conj aggregations new-ag)))
-(defn- merge-aggregations {:style/indent 0} [query-dict [aggregation & more]]
- (if-not aggregation
- ;; no more aggregations? we're done
- query-dict
- ;; otherwise determine if this aggregation is a METRIC and recur
- (let [metric-def (match aggregation
- ["METRIC" (metric-id :guard integer?)] (db/select-one-field :definition 'Metric, :id metric-id)
- _ nil)]
- (recur (if-not metric-def
- ;; not a metric, move to next aggregation
- query-dict
- ;; it *is* a metric, insert it into the query appropriately
- (-> query-dict
- (update-in [:query :aggregation] merge-aggregation (:aggregation metric-def))
- (update-in [:query :filter] merge-filter-clauses (:filter metric-def))))
- more))))
-
-(defn- remove-metrics [aggregations]
- (if-not (and (sequential? aggregations)
- (every? coll? aggregations))
- (recur [aggregations])
- (vec (for [ag aggregations
- :when (match ag
- ["METRIC" (_ :guard integer?)] false
- _ true)]
- ag))))
+;;; ------------------------------------------------------------ Metrics ------------------------------------------------------------
+
+(defn- metric? [aggregation]
+ (match aggregation
+ ["METRIC" (_ :guard integer?)] true
+ _ false))
+
+(defn- metric-id [metric]
+ (when (metric? metric)
+ (second metric)))
+
+(defn- expand-metric [metric-clause filter-clauses-atom]
+ (let [{filter-clause :filter, ag-clause :aggregation} (db/select-one-field :definition 'Metric, :id (metric-id metric-clause))]
+ (when filter-clause
+ (swap! filter-clauses-atom conj filter-clause))
+ ag-clause))
+
+(defn- expand-metrics-in-ag-clause [query-dict filter-clauses-atom]
+ (walk/postwalk (fn [form]
+ (if-not (metric? form)
+ form
+ (expand-metric form filter-clauses-atom)))
+ query-dict))
+
+(defn- add-metrics-filter-clauses [query-dict filter-clauses]
+ (update-in query-dict [:query :filter] merge-filter-clauses (if (> (count filter-clauses) 1)
+ (cons "AND" filter-clauses)
+ (first filter-clauses))))
+
+(defn- expand-metrics [query-dict]
+ (let [filter-clauses-atom (atom [])
+ query-dict (expand-metrics-in-ag-clause query-dict filter-clauses-atom)]
+ (add-metrics-filter-clauses query-dict @filter-clauses-atom)))
(defn- macroexpand-metric [{{aggregations :aggregation} :query, :as query-dict}]
(if-not (seq aggregations)
;; :aggregation is empty, so no METRIC to expand
query-dict
- ;; we have an aggregation clause, so lets see if we are using a METRIC
- ;; (since `:aggregation` can be either single or multiple, wrap single ones so `merge-aggregations` can always assume input is multiple)
- (merge-aggregations
- (update-in query-dict [:query :aggregation] remove-metrics)
- (if (and (sequential? aggregations)
- (every? coll? aggregations))
- aggregations
- [aggregations]))))
+ ;; otherwise walk the query dict and expand METRIC clauses
+ (expand-metrics query-dict)))
+
+
+;;; ------------------------------------------------------------ Middleware ------------------------------------------------------------
(defn expand-macros "Expand the macros (SEGMENT, METRIC) in a QUERY-DICT."
[query-dict]
diff --git a/test/metabase/api/dataset_test.clj b/test/metabase/api/dataset_test.clj
index 24d491c85f756657fcdec68657272bd3e7eb07d5..c6f2459457da028576d2a26ab9d4dea8fac4e6b8 100644
--- a/test/metabase/api/dataset_test.clj
+++ b/test/metabase/api/dataset_test.clj
@@ -64,7 +64,7 @@
(query checkins
(ql/aggregation (ql/count))))
(assoc :type "query")
- (assoc-in [:query :aggregation] [{:aggregation-type "count"}])
+ (assoc-in [:query :aggregation] [{:aggregation-type "count", :custom-name nil}])
(assoc :constraints query-constraints))
:started_at true
:finished_at true
@@ -80,7 +80,7 @@
(query checkins
(ql/aggregation (ql/count))))
(assoc :type "query")
- (assoc-in [:query :aggregation] [{:aggregation-type "count"}])
+ (assoc-in [:query :aggregation] [{:aggregation-type "count", :custom-name nil}])
(assoc :constraints query-constraints))
:started_at true
:finished_at true
diff --git a/test/metabase/driver/druid_test.clj b/test/metabase/driver/druid_test.clj
index 3cb08f21eb73411193c1aa1ecb05a10dea137b13..58d803d0cbda2d2ae1aee8e50f1183a504194183 100644
--- a/test/metabase/driver/druid_test.clj
+++ b/test/metabase/driver/druid_test.clj
@@ -1,13 +1,15 @@
(ns metabase.driver.druid-test
(:require [cheshire.core :as json]
[expectations :refer :all]
+ [metabase.models.metric :refer [Metric]]
[metabase.query-processor :as qp]
[metabase.query-processor.expand :as ql]
- [metabase.query-processor-test :refer [rows]]
+ [metabase.query-processor-test :refer [rows rows+column-names]]
[metabase.test.data :as data]
[metabase.test.data.datasets :as datasets, :refer [expect-with-engine]]
+ [metabase.test.util :as tu]
[metabase.timeseries-query-processor-test :as timeseries-qp-test]
- [metabase.query :as q]))
+ [metabase.util :as u]))
(def ^:const ^:private ^String native-query-1
(json/generate-string
@@ -68,12 +70,15 @@
;;; | EXPRESSION AGGREGATIONS |
;;; +------------------------------------------------------------------------------------------------------------------------+
+(defmacro ^:private druid-query {:style/indent 0} [& body]
+ `(timeseries-qp-test/with-flattened-dbdef
+ (qp/process-query {:database (data/id)
+ :type :query
+ :query (data/query ~'checkins
+ ~@body)})))
+
(defmacro ^:private druid-query-returning-rows {:style/indent 0} [& body]
- `(rows (timeseries-qp-test/with-flattened-dbdef
- (qp/process-query {:database (data/id)
- :type :query
- :query (data/query ~'checkins
- ~@body)}))))
+ `(rows (druid-query ~@body)))
;; sum, *
(expect-with-engine :druid
@@ -202,3 +207,43 @@
(druid-query-returning-rows
(ql/aggregation (ql/sum (ql/+ $venue_price 1)))
(ql/breakout $venue_price)))
+
+;; check that we can name an expression aggregation w/ aggregation at top-level
+(expect-with-engine :druid
+ {:rows [["1" 442.0]
+ ["2" 1845.0]
+ ["3" 460.0]
+ ["4" 245.0]]
+ :columns ["venue_price"
+ "New Price"]}
+ (rows+column-names
+ (druid-query
+ (ql/aggregation (ql/named (ql/sum (ql/+ $venue_price 1)) "New Price"))
+ (ql/breakout $venue_price))))
+
+;; check that we can name an expression aggregation w/ expression at top-level
+(expect-with-engine :druid
+ {:rows [["1" 180.0]
+ ["2" 1189.0]
+ ["3" 304.0]
+ ["4" 155.0]]
+ :columns ["venue_price" "Sum-41"]}
+ (rows+column-names
+ (druid-query
+ (ql/aggregation (ql/named (ql/- (ql/sum $venue_price) 41) "Sum-41"))
+ (ql/breakout $venue_price))))
+
+;; check that we can handle METRICS (ick) inside expression aggregation clauses
+(expect-with-engine :druid
+ [["2" 1231.0]
+ ["3" 346.0]
+ ["4" 197.0]]
+ (timeseries-qp-test/with-flattened-dbdef
+ (tu/with-temp Metric [metric {:definition {:aggregation [:sum [:field-id (data/id :checkins :venue_price)]]
+ :filter [:> [:field-id (data/id :checkins :venue_price)] 1]}}]
+ (rows (qp/process-query
+ {:database (data/id)
+ :type :query
+ :query {:source-table (data/id :checkins)
+ :aggregation [:+ ["METRIC" (u/get-id metric)] 1]
+ :breakout [(ql/breakout (ql/field-id (data/id :checkins :venue_price)))]}})))))
diff --git a/test/metabase/events/activity_feed_test.clj b/test/metabase/events/activity_feed_test.clj
index 70231b803d729694b16aadcfdefc0775b40c2a65..0a541ffe77216282fff7e8273da2267b66a1746a 100644
--- a/test/metabase/events/activity_feed_test.clj
+++ b/test/metabase/events/activity_feed_test.clj
@@ -271,7 +271,7 @@
:model "segment"
:model_id (:id segment)
:database_id (id)
- :table_id (id :venues)
+ :table_id (id :checkins)
:details {:name (:name segment)
:description (:description segment)}}
(with-temp-activities
@@ -288,7 +288,7 @@
:model "segment"
:model_id (:id segment)
:database_id (id)
- :table_id (id :venues)
+ :table_id (id :checkins)
:details {:name (:name segment)
:description (:description segment)
:revision_message "update this mofo"}}
@@ -310,7 +310,7 @@
:model "segment"
:model_id (:id segment)
:database_id (id)
- :table_id (id :venues)
+ :table_id (id :checkins)
:details {:name (:name segment)
:description (:description segment)
:revision_message "deleted"}}
diff --git a/test/metabase/query_processor/expand_resolve_test.clj b/test/metabase/query_processor/expand_resolve_test.clj
index 8b7381575bf50db5fdb41bba0b15da97757df89c..cb381f32c1628d74451e7952b93522aee0c199ef 100644
--- a/test/metabase/query_processor/expand_resolve_test.clj
+++ b/test/metabase/query_processor/expand_resolve_test.clj
@@ -222,9 +222,10 @@
:type :query
:query {:source-table (id :checkins)
:aggregation [{:aggregation-type :sum
- :field {:field-id (id :venues :price)
- :fk-field-id (id :checkins :venue_id)
- :datetime-unit nil}}]
+ :custom-name nil
+ :field {:field-id (id :venues :price)
+ :fk-field-id (id :checkins :venue_id)
+ :datetime-unit nil}}]
:breakout [{:field-id (id :checkins :date)
:fk-field-id nil
:datetime-unit :day-of-week}]}}
@@ -235,20 +236,21 @@
:name "CHECKINS"
:id (id :checkins)}
:aggregation [{:aggregation-type :sum
- :field {:description nil
- :base-type :type/Integer
- :parent nil
- :table-id (id :venues)
- :special-type :type/Category
- :field-name "PRICE"
- :field-display-name "Price"
- :parent-id nil
- :visibility-type :normal
- :position nil
- :field-id (id :venues :price)
- :fk-field-id (id :checkins :venue_id)
- :table-name "VENUES__via__VENUE_ID"
- :schema-name nil}}]
+ :custom-name nil
+ :field {:description nil
+ :base-type :type/Integer
+ :parent nil
+ :table-id (id :venues)
+ :special-type :type/Category
+ :field-name "PRICE"
+ :field-display-name "Price"
+ :parent-id nil
+ :visibility-type :normal
+ :position nil
+ :field-id (id :venues :price)
+ :fk-field-id (id :checkins :venue_id)
+ :table-name "VENUES__via__VENUE_ID"
+ :schema-name nil}}]
:breakout [{:field {:description nil
:base-type :type/Date
:parent nil
@@ -275,7 +277,7 @@
:fk-field-ids #{(id :checkins :venue_id)}
:table-ids #{(id :venues) (id :checkins)}}]
(let [expanded-form (ql/expand (wrap-inner-query (query checkins
- (ql/aggregation (ql/sum $venue_id->venues.price))
- (ql/breakout (ql/datetime-field $checkins.date :day-of-week)))))]
+ (ql/aggregation (ql/sum $venue_id->venues.price))
+ (ql/breakout (ql/datetime-field $checkins.date :day-of-week)))))]
(mapv obj->map [expanded-form
(resolve/resolve expanded-form)])))
diff --git a/test/metabase/query_processor/macros_test.clj b/test/metabase/query_processor/macros_test.clj
index 8cc3bf487dbd0a7dcf0daff9ca7ad809ba6434a2..5c39a816075be96050860a83775e4ef56fa3a643 100644
--- a/test/metabase/query_processor/macros_test.clj
+++ b/test/metabase/query_processor/macros_test.clj
@@ -14,7 +14,7 @@
(expect
{:database 1
:type :query
- :query {:aggregation [["rows"]]
+ :query {:aggregation ["rows"]
:filter ["AND" [">" 4 1]]
:breakout [17]}}
(expand-macros {:database 1
@@ -27,8 +27,10 @@
(expect
{:database 1
:type :query
- :query {:aggregation [["rows"]]
- :filter ["AND" ["AND" ["=" 5 "abc"]] ["OR" ["AND" ["IS_NULL" 7]] [">" 4 1]]]
+ :query {:aggregation ["rows"]
+ :filter ["AND" ["AND" ["=" 5 "abc"]]
+ ["OR" ["AND" ["IS_NULL" 7]]
+ [">" 4 1]]]
:breakout [17]}}
(tu/with-temp* [Database [{database-id :id}]
Table [{table-id :id} {:db_id database-id}]
@@ -46,8 +48,9 @@
(expect
{:database 1
:type :query
- :query {:aggregation [["count"]]
- :filter ["AND" ["AND" [">" 4 1]] ["AND" ["=" 5 "abc"]]]
+ :query {:aggregation ["count"]
+ :filter ["AND" ["AND" [">" 4 1]]
+ ["AND" ["=" 5 "abc"]]]
:breakout [17]
:order_by [[1 "ASC"]]}}
(tu/with-temp* [Database [{database-id :id}]
@@ -66,7 +69,7 @@
(expect
{:database 1
:type :query
- :query {:aggregation [["count"]]
+ :query {:aggregation ["count"]
:filter ["AND" ["=" 5 "abc"]]
:breakout [17]
:order_by [[1 "ASC"]]}}
@@ -86,7 +89,7 @@
(expect
{:database 1
:type :query
- :query {:aggregation [["count"]]
+ :query {:aggregation ["count"]
:filter ["AND" ["=" 5 "abc"]]
:breakout [17]
:order_by [[1 "ASC"]]}}
@@ -105,7 +108,7 @@
(expect
{:database 1
:type :query
- :query {:aggregation [["sum" 18]]
+ :query {:aggregation ["sum" 18]
:filter ["AND" ["AND" [">" 4 1] ["AND" ["IS_NULL" 7]]] ["AND" ["=" 5 "abc"] ["AND" ["BETWEEN" 9 0 25]]]]
:breakout [17]
:order_by [[1 "ASC"]]}}
diff --git a/test/metabase/query_processor_test.clj b/test/metabase/query_processor_test.clj
index 1f1327c551fbf52aa48a9319b5f17268f49b65ea..c0fc9b26eb11338f3e78bca177f10cb82f914ae6 100644
--- a/test/metabase/query_processor_test.clj
+++ b/test/metabase/query_processor_test.clj
@@ -288,15 +288,22 @@
(defn rows
- "Return the result rows from query results, or throw an Exception if they're missing."
+ "Return the result rows from query RESULTS, or throw an Exception if they're missing."
{:style/indent 0}
[results]
- (vec (or (-> results :data :rows)
+ (vec (or (get-in results [:data :rows])
(println (u/pprint-to-str 'red results))
(throw (Exception. "Error!")))))
+(defn rows+column-names
+ "Return the result rows and column names from query RESULTS, or throw an Exception if they're missing."
+ {:style/indent 0}
+ [results]
+ {:rows (rows results)
+ :columns (get-in results [:data :columns])})
+
(defn first-row
- "Return the first row in the results of a query, or throw an Exception if they're missing."
+ "Return the first row in the RESULTS of a query, or throw an Exception if they're missing."
{:style/indent 0}
[results]
(first (rows results)))
diff --git a/test/metabase/query_processor_test/expression_aggregations_test.clj b/test/metabase/query_processor_test/expression_aggregations_test.clj
index b0e855f6735b0e9d187505d08cef8ac25e189715..d69018fe9d9f913f15b9441ec4e7245f3e083951 100644
--- a/test/metabase/query_processor_test/expression_aggregations_test.clj
+++ b/test/metabase/query_processor_test/expression_aggregations_test.clj
@@ -1,10 +1,13 @@
(ns metabase.query-processor-test.expression-aggregations-test
"Tests for expression aggregations."
(:require [expectations :refer :all]
+ [metabase.models.metric :refer [Metric]]
+ [metabase.query-processor :as qp]
[metabase.query-processor.expand :as ql]
[metabase.query-processor-test :refer :all]
[metabase.test.data :as data]
[metabase.test.data.datasets :as datasets, :refer [*engine*]]
+ [metabase.test.util :as tu]
[metabase.util :as u]))
;; sum, *
@@ -156,3 +159,45 @@
(rows (data/run-query venues
(ql/aggregation (ql/sum (ql/+ $price 1)))
(ql/breakout $price)))))
+
+;; check that we can name an expression aggregation w/ aggregation at top-level
+(datasets/expect-with-engines (engines-that-support :expression-aggregations)
+ {:rows [[1 44]
+ [2 177]
+ [3 52]
+ [4 30]]
+ :columns [(data/format-name "price")
+ (if (= *engine* :redshift) "new price" "New Price")]} ; Redshift annoyingly always lowercases column aliases
+ (format-rows-by [int int]
+ (rows+column-names (data/run-query venues
+ (ql/aggregation (ql/named (ql/sum (ql/+ $price 1)) "New Price"))
+ (ql/breakout $price)))))
+
+;; check that we can name an expression aggregation w/ expression at top-level
+(datasets/expect-with-engines (engines-that-support :expression-aggregations)
+ {:rows [[1 -19]
+ [2 77]
+ [3 -2]
+ [4 -17]]
+ :columns [(data/format-name "price")
+ (if (= *engine* :redshift) "sum-41" "Sum-41")]}
+ (format-rows-by [int int]
+ (rows+column-names (data/run-query venues
+ (ql/aggregation (ql/named (ql/- (ql/sum $price) 41) "Sum-41"))
+ (ql/breakout $price)))))
+
+;; check that we can handle METRICS (ick) inside expression aggregation clauses
+(datasets/expect-with-engines (engines-that-support :expression-aggregations)
+ [[2 119]
+ [3 40]
+ [4 25]]
+ (tu/with-temp Metric [metric {:table_id (data/id :venues)
+ :definition {:aggregation [:sum [:field-id (data/id :venues :price)]]
+ :filter [:> [:field-id (data/id :venues :price)] 1]}}]
+ (format-rows-by [int int]
+ (rows (qp/process-query
+ {:database (data/id)
+ :type :query
+ :query {:source-table (data/id :venues)
+ :aggregation [:+ ["METRIC" (u/get-id metric)] 1]
+ :breakout [(ql/breakout (ql/field-id (data/id :venues :price)))]}})))))
diff --git a/test/metabase/test/data.clj b/test/metabase/test/data.clj
index 5f23a1336ad52f1bd38e331d0cd3268968f0864e..7f54967b1982bcc27be41981e10a0c46bdb4b76d 100644
--- a/test/metabase/test/data.clj
+++ b/test/metabase/test/data.clj
@@ -105,6 +105,7 @@
"Call `driver/process-query` on expanded inner QUERY, looking up the `Database` ID for the `source-table.`
(run-query* (query (source-table 5) ...))"
+ {:style/indent 0}
[query :- qi/Query]
(qp/process-query (wrap-inner-query query)))
diff --git a/test/metabase/test/util.clj b/test/metabase/test/util.clj
index a91f7567950d78c395b07b64a2b7df46d16489b2..3ee0d3fc00ad06dbe339b998862b70ef1a3d22d9 100644
--- a/test/metabase/test/util.clj
+++ b/test/metabase/test/util.clj
@@ -124,7 +124,7 @@
{:with-temp-defaults (fn [_] {:base_type :type/Text
:name (random-name)
:position 1
- :table_id (data/id :venues)})})
+ :table_id (data/id :checkins)})})
(u/strict-extend (class Metric)
WithTempDefaults
@@ -132,7 +132,7 @@
:definition {}
:description "Lookin' for a blueberry"
:name "Toucans in the rainforest"
- :table_id (data/id :venues)})})
+ :table_id (data/id :checkins)})})
(u/strict-extend (class PermissionsGroup)
WithTempDefaults
@@ -172,7 +172,7 @@
:definition {}
:description "Lookin' for a blueberry"
:name "Toucans in the rainforest"
- :table_id (data/id :venues)})})
+ :table_id (data/id :checkins)})})
;; TODO - `with-temp` doesn't return `Sessions`, probably because their ID is a string?