diff --git a/src/metabase/query_processor/middleware/annotate.clj b/src/metabase/query_processor/middleware/annotate.clj
index 31440e1236739bafa0be0630f7b4fc81107fcb74..7d8d5386d2923cb0e2347f331489939a45bc9312 100644
--- a/src/metabase/query_processor/middleware/annotate.clj
+++ b/src/metabase/query_processor/middleware/annotate.clj
@@ -164,15 +164,19 @@
:field_ref &match)
[:expression expression-name]
- (merge
- ;; There's some inconsistency when expression names are keywords and when strings.
- ;; TODO: remove this duality once https://github.com/metabase/mbql/issues/5 is resolved.
- (infer-expression-type (some expressions ((juxt identity keyword) expression-name)))
- {:name expression-name
- :display_name expression-name
- ;; provided so the FE can add easily add sorts and the like when someone clicks a column header
- :expression_name expression-name
- :field_ref &match})
+ (if-let [matching-expression (when (seq expressions)
+ (some expressions ((juxt keyword u/keyword->qualified-name) expression-name)))]
+ (merge
+ ;; There's some inconsistency when expression names are keywords and when strings.
+ ;; TODO: remove this duality once https://github.com/metabase/mbql/issues/5 is resolved.
+ (infer-expression-type matching-expression)
+ {:name expression-name
+ :display_name expression-name
+ ;; provided so the FE can add easily add sorts and the like when someone clicks a column header
+ :expression_name expression-name
+ :field_ref &match})
+ (throw (ex-info (str (tru "No expression named {0} found. Found: {1}" expression-name (keys expressions)))
+ {:type :invalid-query, :clause &match, :expressions expressions})))
[:field-id id]
(let [{parent-id :parent_id, :as field} (dissoc (qp.store/field id) :database_type)]
@@ -247,15 +251,15 @@
(s/defn ^:private aggregation-arg-display-name :- su/NonBlankString
"Name to use for an aggregation clause argument such as a Field when constructing the complete aggregation name."
- [ag-arg :- Object]
+ [inner-query, ag-arg :- Object]
(or (when (mbql.preds/Field? ag-arg)
- (when-let [info (col-info-for-field-clause {} ag-arg)]
+ (when-let [info (col-info-for-field-clause inner-query ag-arg)]
(some info [:display_name :name])))
- (aggregation-display-name ag-arg)))
+ (aggregation-display-name inner-query ag-arg)))
(s/defn aggregation-display-name :- su/NonBlankString
"Return an appropriate user-facing display name for an aggregation clause."
- [ag-clause]
+ [inner-query ag-clause]
(mbql.u/match-one ag-clause
[:aggregation-options _ (options :guard :display-name)]
(:display-name options)
@@ -265,36 +269,37 @@
[(operator :guard #{:+ :- :/ :*}) & args]
(str/join (format " %s " (name operator))
- (map (partial expression-arg-display-name aggregation-arg-display-name) args))
+ (for [arg args]
+ (expression-arg-display-name (partial aggregation-arg-display-name inner-query) arg)))
[:count]
(str (tru "count"))
- [:distinct arg] (str (tru "distinct count of {0}" (aggregation-arg-display-name arg)))
- [:count arg] (str (tru "count of {0}" (aggregation-arg-display-name arg)))
- [:avg arg] (str (tru "average of {0}" (aggregation-arg-display-name arg)))
+ [:distinct arg] (str (tru "distinct count of {0}" (aggregation-arg-display-name inner-query arg)))
+ [:count arg] (str (tru "count of {0}" (aggregation-arg-display-name inner-query arg)))
+ [:avg arg] (str (tru "average of {0}" (aggregation-arg-display-name inner-query arg)))
;; cum-count and cum-sum get names for count and sum, respectively (see explanation in `aggregation-name`)
- [:cum-count arg] (str (tru "count of {0}" (aggregation-arg-display-name arg)))
- [:cum-sum arg] (str (tru "sum of {0}" (aggregation-arg-display-name arg)))
- [:stddev arg] (str (tru "standard deviation of {0}" (aggregation-arg-display-name arg)))
- [:sum arg] (str (tru "sum of {0}" (aggregation-arg-display-name arg)))
- [:min arg] (str (tru "minimum value of {0}" (aggregation-arg-display-name arg)))
- [:max arg] (str (tru "maximum value of {0}" (aggregation-arg-display-name arg)))
+ [:cum-count arg] (str (tru "count of {0}" (aggregation-arg-display-name inner-query arg)))
+ [:cum-sum arg] (str (tru "sum of {0}" (aggregation-arg-display-name inner-query arg)))
+ [:stddev arg] (str (tru "standard deviation of {0}" (aggregation-arg-display-name inner-query arg)))
+ [:sum arg] (str (tru "sum of {0}" (aggregation-arg-display-name inner-query arg)))
+ [:min arg] (str (tru "minimum value of {0}" (aggregation-arg-display-name inner-query arg)))
+ [:max arg] (str (tru "maximum value of {0}" (aggregation-arg-display-name inner-query arg)))
;; until we have a way to generate good names for filters we'll just have to say 'matching condition' for now
- [:sum-where arg _] (str (tru "sum of {0} matching condition" (aggregation-arg-display-name arg)))
+ [:sum-where arg _] (str (tru "sum of {0} matching condition" (aggregation-arg-display-name inner-query arg)))
[:share _] (str (tru "share of rows matching condition"))
[:count-where _] (str (tru "count of rows matching condition"))
(_ :guard mbql.preds/Field?)
- (:display_name (col-info-for-field-clause nil ag-clause))
+ (:display_name (col-info-for-field-clause inner-query ag-clause))
_
- (aggregation-name ag-clause {:recursive-name-fn aggregation-arg-display-name})))
+ (aggregation-name ag-clause {:recursive-name-fn (partial aggregation-arg-display-name inner-query)})))
-(defn- ag->name-info [ag]
+(defn- ag->name-info [inner-query ag]
{:name (aggregation-name ag)
- :display_name (aggregation-display-name ag)})
+ :display_name (aggregation-display-name inner-query ag)})
(s/defn col-info-for-aggregation-clause
"Return appropriate column metadata for an `:aggregation` clause."
@@ -306,7 +311,7 @@
[:aggregation-options ag _]
(merge
(col-info-for-aggregation-clause inner-query ag)
- (ag->name-info &match))
+ (ag->name-info inner-query &match))
;; Always treat count or distinct count as an integer even if the DB in question returns it as something
;; wacky like a BigDecimal or Float
@@ -315,20 +320,20 @@
(col-info-for-aggregation-clause inner-query args)
{:base_type :type/Integer
:special_type :type/Number}
- (ag->name-info &match))
+ (ag->name-info inner-query &match))
; TODO - should we be doing this for `:sum-where` as well?
[:count-where _]
(merge
{:base_type :type/Integer
:special_type :type/Number}
- (ag->name-info &match))
+ (ag->name-info inner-query &match))
[:share _]
(merge
{:base_type :type/Float
:special_type :type/Number}
- (ag->name-info &match))
+ (ag->name-info inner-query &match))
;; get info from a Field if we can (theses Fields are matched when ag clauses recursively call
;; `col-info-for-ag-clause`, and this info is added into the results)
@@ -343,13 +348,13 @@
(merge
(infer-expression-type &match)
(when (mbql.preds/Aggregation? &match)
- (ag->name-info &match)))
+ (ag->name-info inner-query &match)))
;; get name/display-name of this ag
[(_ :guard keyword?) arg & _]
(merge
(col-info-for-aggregation-clause inner-query arg)
- (ag->name-info &match))))
+ (ag->name-info inner-query &match))))
;;; ----------------------------------------- Putting it all together (MBQL) -----------------------------------------
diff --git a/test/metabase/query_processor/middleware/annotate_test.clj b/test/metabase/query_processor/middleware/annotate_test.clj
index 9f97366e3369f40cb357d653f5902092b30c8c8e..6e83e05cc8e54e0da5b0d6cbe0177e3f3a4986f1 100644
--- a/test/metabase/query_processor/middleware/annotate_test.clj
+++ b/test/metabase/query_processor/middleware/annotate_test.clj
@@ -288,17 +288,51 @@
{:name "sum", :display_name "sum of User ID", :base_type :type/Integer, :special_type :type/FK}]}
[:field-literal "sum" :type/Integer])))
+;; col info for an `expression` should work as expected
+(expect
+ {:base_type :type/Float
+ :special_type :type/Number
+ :name "double-price"
+ :display_name "double-price"
+ :expression_name "double-price"
+ :field_ref [:expression "double-price"]}
+ (qp.test-util/with-everything-store
+ (data/$ids venues
+ (#'annotate/col-info-for-field-clause
+ {:expressions {"double-price" [:* $price 2]}}
+ [:expression "double-price"]))))
+
+;; if there is no matching expression it should give a meaningful error message
+(expect
+ {:message "No expression named double-price found. Found: (\"one-hundred\")"
+ :data {:type :invalid-query
+ :clause [:expression "double-price"]
+ :expressions {"one-hundred" 100}}}
+ (try
+ (qp.test-util/with-everything-store
+ (data/$ids venues
+ (#'annotate/col-info-for-field-clause
+ {:expressions {"one-hundred" 100}}
+ [:expression "double-price"])))
+ (catch Throwable e
+ {:message (.getMessage e)
+ :data (ex-data e)})))
+
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | (MBQL) Col info for Aggregation clauses |
;;; +----------------------------------------------------------------------------------------------------------------+
;; test that added information about aggregations looks the way we'd expect
-(defn- aggregation-names [ag-clause]
- (binding [driver/*driver* :h2]
- (qp.test-util/with-everything-store
- {:name (annotate/aggregation-name ag-clause)
- :display_name (annotate/aggregation-display-name ag-clause)})))
+(defn- aggregation-names
+ ([ag-clause]
+ (aggregation-names {} ag-clause))
+
+ ([inner-query ag-clause]
+ (binding [driver/*driver* :h2]
+ (qp.test-util/with-everything-store
+ {:name (annotate/aggregation-name ag-clause)
+ :display_name (annotate/aggregation-display-name inner-query ag-clause)}))))
(expect
{:name "count", :display_name "count"}
@@ -363,9 +397,13 @@
{:display-name "User-specified Name"}]))
;; make sure custom aggregation names get included in the col info
-(defn- col-info-for-aggregation-clause [clause]
- (binding [driver/*driver* :h2]
- (#'annotate/col-info-for-aggregation-clause {} clause)))
+(defn- col-info-for-aggregation-clause
+ ([clause]
+ (col-info-for-aggregation-clause {} clause))
+
+ ([inner-query clause]
+ (binding [driver/*driver* :h2]
+ (#'annotate/col-info-for-aggregation-clause inner-query clause))))
(expect
{:base_type :type/Float
@@ -435,6 +473,18 @@
(data/mbql-query venues
{:aggregation [[:metric "ga:totalEvents"]]}))))
+;; col info for an `expression` aggregation w/ a named expression should work as expected
+(expect
+ {:base_type :type/Float
+ :special_type :type/Number
+ :name "sum"
+ :display_name "sum of double-price"}
+ (qp.test-util/with-everything-store
+ (data/$ids venues
+ (col-info-for-aggregation-clause
+ {:expressions {"double-price" [:* $price 2]}}
+ [:sum [:expression "double-price"]]))))
+
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Other MBQL col info tests |
diff --git a/test/metabase/query_processor_test/expression_aggregations_test.clj b/test/metabase/query_processor_test/expression_aggregations_test.clj
index 76ccc21af31d7d1fcf50d3924596df2a7afb3541..d51238f4b21b7e33dfe5cc3de531fc8c895e8727 100644
--- a/test/metabase/query_processor_test/expression_aggregations_test.clj
+++ b/test/metabase/query_processor_test/expression_aggregations_test.clj
@@ -270,3 +270,12 @@
(qp.test/rows
(data/run-mbql-query venues
{:aggregation [["*" ["cum_count"] 10]]}))))
+
+;; can we use named expressions inside expression aggregations?
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expression-aggregations)
+ [[406]]
+ (qp.test/format-rows-by [int]
+ (qp.test/rows
+ (data/run-mbql-query venues
+ {:aggregation [[:sum [:expression "double-price"]]]
+ :expressions {"double-price" [:* $price 2]}}))))