diff --git a/frontend/src/metabase-lib/lib/Dimension.js b/frontend/src/metabase-lib/lib/Dimension.js
index 89d272cc074ba528a5ca17b01d41161b9ea9de4b..4cc2a0f1ba2f49984a31a4496d78fadf17aca720 100644
--- a/frontend/src/metabase-lib/lib/Dimension.js
+++ b/frontend/src/metabase-lib/lib/Dimension.js
@@ -639,7 +639,9 @@ export class AggregationDimension extends Dimension {
const aggregation =
this._query && this._query.aggregations()[this.aggregationIndex()];
if (aggregation) {
- return aggregation[0] === "named" ? aggregation[1] : aggregation;
+ return aggregation[0] === "aggregation-options"
+ ? aggregation[1]
+ : aggregation;
}
return null;
}
@@ -649,13 +651,15 @@ export class AggregationDimension extends Dimension {
this._query && this._query.aggregations()[this.aggregationIndex()];
if (aggregation) {
// FIXME: query lib
- if (aggregation[0] === "named") {
- return aggregation[2];
- } else {
- const short = aggregation[0];
- // NOTE: special case for "distinct"
- return short === "distinct" ? "count" : short;
+ if (aggregation[0] === "aggregation-options") {
+ const { "display-name": displayName } = aggregation[2];
+ if (displayName) {
+ return displayName;
+ }
}
+ const short = aggregation[0];
+ // NOTE: special case for "distinct"
+ return short === "distinct" ? "count" : short;
}
return null;
}
diff --git a/frontend/src/metabase/lib/query.js b/frontend/src/metabase/lib/query.js
index 3db02d1b56d92d020caaae7b39a1c5667eb921ea..7b8b6663518f51587af823021e7fcc38a64b0053 100644
--- a/frontend/src/metabase/lib/query.js
+++ b/frontend/src/metabase/lib/query.js
@@ -824,22 +824,30 @@ for (const prop in Q) {
import { isMath } from "metabase/lib/expressions";
export const NamedClause = {
+ hasOptions(clause) {
+ return Array.isArray(clause) && clause[0] === "aggregation-options";
+ },
+ getOptions(clause) {
+ return NamedClause.hasOptions(clause) ? clause[2] : {};
+ },
isNamed(clause) {
- return Array.isArray(clause) && clause[0] === "named";
+ return NamedClause.getOptions(clause)["display-name"];
},
getName(clause) {
- return NamedClause.isNamed(clause) ? clause[2] : null;
+ return NamedClause.getOptions(clause)["display-name"];
},
getContent(clause) {
- return NamedClause.isNamed(clause) ? clause[1] : clause;
+ return NamedClause.hasOptions(clause) ? clause[1] : clause;
},
setName(clause, name) {
- return ["named", NamedClause.getContent(clause), name];
+ return [
+ "aggregation-options",
+ NamedClause.getContent(clause),
+ { "display-name": name, ...NamedClause.getOptions(clause) },
+ ];
},
setContent(clause, content) {
- return NamedClause.isNamed(clause)
- ? ["named", content, NamedClause.getName(clause)]
- : content;
+ return ["aggregation-options", content, NamedClause.getOptions(clause)];
},
};
diff --git a/frontend/test/metabase-lib/lib/queries/StructuredQuery.unit.spec.js b/frontend/test/metabase-lib/lib/queries/StructuredQuery.unit.spec.js
index 3551660a0bf5be57af3387d221f74f9bfe5d5407..d645c11f2e32988135034cd1378e5bc09e13547e 100644
--- a/frontend/test/metabase-lib/lib/queries/StructuredQuery.unit.spec.js
+++ b/frontend/test/metabase-lib/lib/queries/StructuredQuery.unit.spec.js
@@ -275,9 +275,9 @@ describe("StructuredQuery unit tests", () => {
it("returns a named expression name", () => {
expect(
makeQueryWithAggregation([
- "named",
+ "aggregation-options",
["sum", ["field-id", ORDERS_TOTAL_FIELD_ID]],
- "Named",
+ { "display-name": "Named" },
]).aggregationName(),
).toBe("Named");
});
diff --git a/frontend/test/metabase/lib/query.unit.spec.js b/frontend/test/metabase/lib/query.unit.spec.js
index e2f1551aa69f7320cad4f3070e4cd2879f56f837..174434e9fc741b230bea40189c702b8cb8fb2e8d 100644
--- a/frontend/test/metabase/lib/query.unit.spec.js
+++ b/frontend/test/metabase/lib/query.unit.spec.js
@@ -344,7 +344,13 @@ describe("generateQueryDescription", () => {
expect(
Query.generateQueryDescription(mockTableMetadata, {
"source-table": 1,
- aggregation: [["named", ["sum", ["field-id", 1]], "Revenue"]],
+ aggregation: [
+ [
+ "aggregation-options",
+ ["sum", ["field-id", 1]],
+ { "display-name": "Revenue" },
+ ],
+ ],
}),
).toEqual("Orders, Revenue");
});
diff --git a/modules/drivers/bigquery/test/metabase/driver/bigquery_test.clj b/modules/drivers/bigquery/test/metabase/driver/bigquery_test.clj
index 272e5dbbf058a29ea62c282c77d8fe9b29b67543..3320e24aaffedf36a00da8041a6c1ba79f76227a 100644
--- a/modules/drivers/bigquery/test/metabase/driver/bigquery_test.clj
+++ b/modules/drivers/bigquery/test/metabase/driver/bigquery_test.clj
@@ -77,22 +77,10 @@
:type :native
:database (data/id)})))
-;; make sure that the bigquery driver can handle named columns with characters that aren't allowed in BQ itself
-(datasets/expect-with-driver :bigquery
- {:rows [[113]]
- :columns ["User_ID_Plus_Venue_ID"]}
- (qp.test/rows+column-names
- (qp/process-query {:database (data/id)
- :type "query"
- :query {:source-table (data/id :checkins)
- :aggregation [["named" ["max" ["+" ["field-id" (data/id :checkins :user_id)]
- ["field-id" (data/id :checkins :venue_id)]]]
- "User ID Plus Venue ID"]]}})))
-
-;; ok, make sure we actually wrap all of our ag clauses in `:named` clauses with unique names
+;; ok, make sure we actually wrap all of our ag clauses in `:aggregation-options` clauses with unique names
(defn- aggregation-names [query]
(mbql.u/match (-> query :query :aggregation)
- [:named _ ag-name] ag-name))
+ [:aggregation-options _ {:name ag-name}] ag-name))
;; make sure queries with two or more of the same aggregation type still work. Aggregations used to be deduplicated
;; here in the BigQuery driver; now they are deduplicated as part of the main QP middleware, but no reason not to keep
diff --git a/modules/drivers/druid/src/metabase/driver/druid/query_processor.clj b/modules/drivers/druid/src/metabase/driver/druid/query_processor.clj
index b05fecd24d25b4b63aab74ac2f32d62188503264..d48440140bafccdc1b246f4cbf94f084291d2581 100644
--- a/modules/drivers/druid/src/metabase/driver/druid/query_processor.clj
+++ b/modules/drivers/druid/src/metabase/driver/druid/query_processor.clj
@@ -10,7 +10,9 @@
[clojure.tools.logging :as log]
[flatland.ordered.map :as ordered-map]
[metabase.driver.druid.js :as js]
- [metabase.mbql.util :as mbql.u]
+ [metabase.mbql
+ [schema :as mbql.s]
+ [util :as mbql.u]]
[metabase.query-processor
[interface :as i]
[store :as qp.store]]
@@ -81,7 +83,7 @@
(= ag-type :distinct)
:distinct___count
- (= ag-type :named)
+ (= ag-type :aggregation-options)
(recur (second ag))
ag-type
@@ -139,18 +141,23 @@
(dimension-or-metric? field))
-(def ^:private ^:const query-type->default-query
- (let [defaults {:intervals ["1900-01-01/2100-01-01"]
- :granularity :all
- :context {:timeout 60000
- :queryId (str (java.util.UUID/randomUUID))}}]
- {::select (merge defaults {:queryType :select
- :pagingSpec {:threshold i/absolute-max-results}})
- ::total (merge defaults {:queryType :timeseries})
- ::grouped-timeseries (merge defaults {:queryType :timeseries})
- ::topN (merge defaults {:queryType :topN
- :threshold topN-max-results})
- ::groupBy (merge defaults {:queryType :groupBy})}))
+(defn- random-query-id []
+ (str (java.util.UUID/randomUUID)))
+
+(defn- query-type->default-query [query-type]
+ (merge
+ {:intervals ["1900-01-01/2100-01-01"]
+ :granularity :all
+ :context {:timeout 60000
+ :queryId (random-query-id)}}
+ (case query-type
+ ::select {:queryType :select
+ :pagingSpec {:threshold i/absolute-max-results}}
+ ::total {:queryType :timeseries}
+ ::grouped-timeseries {:queryType :timeseries}
+ ::topN {:queryType :topN
+ :threshold topN-max-results}
+ ::groupBy {:queryType :groupBy})))
@@ -582,12 +589,12 @@
[:max _] [[(or output-name-kwd :max)]
{:aggregations [(ag:doubleMax ag-field (or output-name :max))]}])))
-(defn- handle-aggregation
- [query-type ag-clause updated-query]
+(s/defn ^:private handle-aggregation
+ [query-type, ag-clause :- mbql.s/Aggregation, updated-query]
(let [output-name (annotate/aggregation-name ag-clause)
[ag-type ag-field & args] (mbql.u/match-one ag-clause
- [:named ag & _] (recur ag)
- [_ _ & _] &match)]
+ [:aggregation-options ag & _] (recur ag)
+ _ &match)]
(if-not (isa? query-type ::ag-query)
updated-query
(let [[projections ag-clauses] (create-aggregation-clause output-name ag-type ag-field args)]
@@ -595,49 +602,61 @@
(update :projections #(vec (concat % projections)))
(update :query #(merge-with concat % ag-clauses)))))))
-(defn- add-expression-aggregation-output-names
- [[operator & args :as expression]]
- (if (mbql.u/is-clause? :named expression)
- (update (vec expression) 1 add-expression-aggregation-output-names)
- (into [operator]
- (for [arg args]
- (cond
- (number? arg)
- arg
+(defn- deduplicate-aggregation-options [expression]
+ (mbql.u/replace expression
+ [:aggregation-options [:aggregation-options ag options-1] options-2]
+ [:aggregation-options ag (merge options-1 options-2)]))
- (mbql.u/is-clause? :named arg)
- arg
+(def ^:private ^:dynamic *query-unique-identifier-counter*
+ "Counter used for generating unique identifiers for use in the query. Bound to `(atom 0)` and incremented on each use
+ as the MBQL query is compiled."
+ nil)
- (mbql.u/is-clause? #{:count :avg :distinct :stddev :sum :min :max} arg)
- [:named arg (name (gensym (str "___" (name (first arg)) "_")))]
+(defn- aggregation-unique-identifier [clause]
+ (format "__%s_%d" (name clause) (first (swap-vals! *query-unique-identifier-counter* inc))))
+
+(defn- add-expression-aggregation-output-names
+ [expression]
+ (mbql.u/replace expression
+ [:aggregation-options ag options]
+ (deduplicate-aggregation-options [:aggregation-options (add-expression-aggregation-output-names ag) options])
- (mbql.u/is-clause? #{:+ :- :/ :*} arg)
- (add-expression-aggregation-output-names arg))))))
+ [(clause :guard #{:count :avg :distinct :stddev :sum :min :max}) & _]
+ [:aggregation-options &match {:name (aggregation-unique-identifier clause)}]))
(defn- expression-post-aggregation
[[operator & args, :as expression]]
- (if (mbql.u/is-clause? :named expression)
+ (mbql.u/match-one expression
;; If it's a named expression, we want to preserve the included name, so recurse, but merge in the name
+ [:aggregation-options ag _]
(merge (expression-post-aggregation (second expression))
- {:name (annotate/aggregation-name expression {:top-level? true})})
+ {:name (annotate/aggregation-name expression)})
+
+ _
{:type :arithmetic
- :name (annotate/aggregation-name expression {:top-level? true})
+ :name (annotate/aggregation-name expression)
:fn operator
- :fields (for [arg args]
- (cond
- (number? arg)
- {:type :constant, :name (str arg), :value arg}
+ :fields (vec (for [arg args]
+ (mbql.u/match-one arg
+ number?
+ {:type :constant, :name (str &match), :value &match}
+
+ [:aggregation-options _ (options :guard :name)]
+ {:type :fieldAccess, :fieldName (:name options)}
+
+ #{:+ :- :/ :*}
+ (expression-post-aggregation &match)
- (mbql.u/is-clause? :named arg)
- {:type :fieldAccess, :fieldName (last arg)}
+ ;; we should never get here unless our code is B U S T E D
+ _
+ (throw (ex-info (str (tru "Expected :aggregation-options, constant, or expression."))
+ {:type :bug, :input arg})))))}))
- (mbql.u/is-clause? #{:+ :- :/ :*} arg)
- (expression-post-aggregation arg)))}))
(declare handle-aggregations)
(defn- expression->actual-ags
- "Return a flattened list of actual aggregations that are needed for EXPRESSION."
+ "Return a flattened list of actual aggregations that are needed for `expression`."
[[_ & args]]
(apply concat (for [arg args
:when (not (number? arg))]
@@ -647,7 +666,7 @@
(defn- unwrap-name
[x]
- (if (mbql.u/is-clause? :named x)
+ (if (mbql.u/is-clause? :aggregation-options x)
(second x)
x))
@@ -662,24 +681,23 @@
post-agg (expression-post-aggregation expression)]
(-> updated-query
(update :projections conj (keyword (:name post-agg)))
- (update :query #(merge-with concat % {:postAggregations [post-agg]})))))
+ (update-in [:query :postAggregations] concat [post-agg]))))
(defn- handle-aggregations
[query-type {aggregations :aggregation} updated-query]
- (loop [[ag & more] aggregations, query updated-query]
- (cond
- (and (mbql.u/is-clause? :named ag)
- (mbql.u/is-clause? #{:+ :- :/ :*} (second ag)))
- (handle-expression-aggregation query-type ag query)
-
- (mbql.u/is-clause? #{:+ :- :/ :*} ag)
- (handle-expression-aggregation query-type ag query)
+ (reduce
+ (fn [updated-query aggregation]
+ (mbql.u/match-one aggregation
+ [:aggregation-options [(_ :guard #{:+ :- :/ :*}) & _] _]
+ (handle-expression-aggregation query-type &match updated-query)
- (not ag)
- query
+ #{:+ :- :/ :*}
+ (handle-expression-aggregation query-type &match updated-query)
- :else
- (recur more (handle-aggregation query-type ag query)))))
+ _
+ (handle-aggregation query-type &match updated-query)))
+ updated-query
+ aggregations))
;;; ------------------------------------------------ handle-breakout -------------------------------------------------
@@ -871,7 +889,7 @@
:distinct
:distinct___count
- [:named wrapped-ag & _]
+ [:aggregation-options wrapped-ag _]
(recur wrapped-ag)
[(ag-type :guard keyword?) & _]
@@ -1139,7 +1157,8 @@
;; Merge `:settings` into the inner query dict so the QP has access to it
(let [query (assoc (:query query)
:settings (:settings query))]
- (binding [*query* query]
+ (binding [*query* query
+ *query-unique-identifier-counter* (atom 0)]
(build-druid-query query))))
@@ -1188,7 +1207,7 @@
(json/parse-string query keyword)
query)
query-type (or query-type
- (keyword "metabase.driver.druid.query-processor" (name (:queryType query))))
+ (keyword (namespace ::query) (name (:queryType query))))
post-proc-map (->> query
(do-query details)
(post-process query-type projections
diff --git a/modules/drivers/druid/test/metabase/driver/druid/query_processor_test.clj b/modules/drivers/druid/test/metabase/driver/druid/query_processor_test.clj
new file mode 100644
index 0000000000000000000000000000000000000000..7eb078516f614cec7edce40cb2c92bbad68d1831
--- /dev/null
+++ b/modules/drivers/druid/test/metabase/driver/druid/query_processor_test.clj
@@ -0,0 +1,48 @@
+(ns metabase.driver.druid.query-processor-test
+ "Some tests to make sure the Druid Query Processor is generating sane Druid queries when compiling MBQL."
+ (:require [metabase
+ [driver :as driver]
+ [query-processor :as qp]]
+ [metabase.driver.druid.query-processor :as druid.qp]
+ [metabase.test.data :as data]
+ [metabase.test.data.datasets :as datasets]
+ [metabase.timeseries-query-processor-test.util :as tqpt]))
+
+(defn- do-query->native [query]
+ (driver/with-driver :druid
+ (tqpt/with-flattened-dbdef
+ (with-redefs [druid.qp/random-query-id (constantly "<Query ID>")]
+ (qp/query->native
+ query)))))
+
+(defmacro ^:private query->native [query]
+ `(do-query->native
+ (data/mbql-query ~'checkins
+ ~query)))
+
+(datasets/expect-with-driver :druid
+ {:projections [:venue_price :__count_0 :expression]
+ :query {:queryType :topN
+ :threshold 1000
+ :granularity :all
+ :dataSource "checkins"
+ :dimension "venue_price"
+ :context {:timeout 60000, :queryId "<Query ID>"}
+ :postAggregations [{:type :arithmetic
+ :name "expression"
+ :fn :*
+ :fields [{:type :fieldAccess, :fieldName "__count_0"}
+ {:type :constant, :name "10", :value 10}]}]
+ :intervals ["1900-01-01/2100-01-01"]
+ :metric {:type :alphaNumeric}
+ :aggregations
+ [{:type :filtered
+ :filter
+ {:type :not
+ :field {:type :selector, :dimension "id", :value nil}}
+ :aggregator {:type :count, :name "__count_0"}}]}
+ :query-type ::druid.qp/topN
+ :mbql? true}
+ (query->native
+ {:aggregation [[:* [:count $id] 10]]
+ :breakout [$venue_price]}))
diff --git a/modules/drivers/druid/test/metabase/driver/druid_test.clj b/modules/drivers/druid/test/metabase/driver/druid_test.clj
index 2f37d59eded1e1268f85849332d0f9793b6e6481..609023cfb3970ff2ec01d517c02cbad21085501c 100644
--- a/modules/drivers/druid/test/metabase/driver/druid_test.clj
+++ b/modules/drivers/druid/test/metabase/driver/druid_test.clj
@@ -325,7 +325,7 @@
["4" 245.0]]
(qp.test/rows
(druid-query
- {:aggregation [[:named [:sum [:+ $venue_price 1]] "New Price"]]
+ {:aggregation [[:aggregation-options [:sum [:+ $venue_price 1]] {:name "New Price"}]]
:breakout [$venue_price]})))
;; check that we can name an expression aggregation w/ expression at top-level
@@ -337,7 +337,7 @@
:columns ["venue_price" "Sum-41"]}
(qp.test/rows+column-names
(druid-query
- {:aggregation [[:named [:- [:sum $venue_price] 41] "Sum-41"]]
+ {:aggregation [[:aggregation-options [:- [:sum $venue_price] 41] {:name "Sum-41"}]]
:breakout [$venue_price]})))
;; check that we can handle METRICS (ick) inside expression aggregation clauses
diff --git a/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj b/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj
index 82e4271ebab71bc8cce26bc322826ad50fd5e174..b1b809dc0a7805b3dfe14b4274d9de108e418995 100644
--- a/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj
+++ b/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj
@@ -390,29 +390,49 @@
;;; -------------------------------------------------- aggregation ---------------------------------------------------
-(defn- aggregation->rvalue [[aggregation-type arg & args]]
- {:pre [(keyword? aggregation-type)]}
- (if-not arg
- (case aggregation-type
- :count {$sum 1})
- (case aggregation-type
- :named (recur arg)
- :avg {$avg (->rvalue arg)}
- :count {$sum {$cond {:if (->rvalue arg)
- :then 1
- :else 0}}}
- :distinct {$addToSet (->rvalue arg)}
- :sum {$sum (->rvalue arg)}
- :min {$min (->rvalue arg)}
- :max {$max (->rvalue arg)}
- :sum-where (let [[pred] args]
- {$sum {$cond {:if (parse-cond pred)
- :then (->rvalue arg)
- :else 0}}})
- :count-where (recur [:sum-where [:value 1] arg]))))
+(defn- aggregation->rvalue [ag]
+ (mbql.u/match-one ag
+ [:aggregation-options ag _]
+ (recur ag)
+
+ [:count]
+ {$sum 1}
+
+ [:count arg]
+ {$sum {$cond {:if (->rvalue arg)
+ :then 1
+ :else 0}}}
+ [:avg arg]
+ {$avg (->rvalue arg)}
+
+
+ [:distinct arg]
+ {$addToSet (->rvalue arg)}
+
+ [:sum arg]
+ {$sum (->rvalue arg)}
+
+ [:min arg]
+ {$min (->rvalue arg)}
+
+ [:max arg]
+ {$max (->rvalue arg)}
+
+ [:sum-where arg pred]
+ {$sum {$cond {:if (parse-cond pred)
+ :then (->rvalue arg)
+ :else 0}}}
+
+ [:count-where pred]
+ (recur [:sum-where [:value 1] pred])
+
+ :else
+ (throw
+ (ex-info (str (tru "Don't know how to handle aggregation {0}" ag))
+ {:type :invalid-query, :clause ag}))))
(defn- unwrap-named-ag [[ag-type arg :as ag]]
- (if (= ag-type :named)
+ (if (= ag-type :aggregation-options)
(recur arg)
ag))
diff --git a/project.clj b/project.clj
index 51fac8c50c9a0502b0ccfa00ce9bead9078aa74c..22a3847376969c2e597e1e1dc68eb009ef107b94 100644
--- a/project.clj
+++ b/project.clj
@@ -99,7 +99,7 @@
com.sun.jmx/jmxri]]
[medley "1.2.0"] ; lightweight lib of useful functions
[metabase/connection-pool "1.0.2"] ; simple wrapper around C3P0. JDBC connection pools
- [metabase/mbql "1.2.0"] ; MBQL language schema & util fns
+ [metabase/mbql "1.3.1"] ; MBQL language schema & util fns
[metabase/throttle "1.0.1"] ; Tools for throttling access to API endpoints and other code pathways
[javax.xml.bind/jaxb-api "2.4.0-b180830.0359"] ; add the `javax.xml.bind` classes which we're still using but were removed in Java 11
[net.sf.cssbox/cssbox "4.12" :exclusions [org.slf4j/slf4j-api]] ; HTML / CSS rendering
diff --git a/src/metabase/automagic_dashboards/core.clj b/src/metabase/automagic_dashboards/core.clj
index 58c2995c8ac4658ad86ebce585e2d74e29c526bb..2249a19195ea83ca395cb9ffdaef9109f68a8d84 100644
--- a/src/metabase/automagic_dashboards/core.clj
+++ b/src/metabase/automagic_dashboards/core.clj
@@ -89,7 +89,7 @@
(complement mbql.u/ga-metric-or-segment?)))
(def ^:private ^{:arglists '([metric])} custom-expression?
- (partial mbql.u/is-clause? :named))
+ (partial mbql.u/is-clause? :aggregation-options))
(def ^:private ^{:arglists '([metric])} adhoc-metric?
(complement (some-fn saved-metric? custom-expression?)))
diff --git a/src/metabase/driver.clj b/src/metabase/driver.clj
index a53dd8679f93ce5ce9cc047d331bf01e32942b68..0037187d8c4f1bac9b9d0ffdef03450c824d19aa 100644
--- a/src/metabase/driver.clj
+++ b/src/metabase/driver.clj
@@ -52,6 +52,15 @@
below. The QP binds the driver this way in the `bind-driver` middleware."
nil)
+(defn do-with-driver
+ "Impl for `with-driver`."
+ [driver f]
+ ;; it's substantially faster not to call `binding` in the first place if it's already bound
+ (if (= *driver* driver)
+ (f)
+ (binding [*driver* driver]
+ (f))))
+
(defmacro with-driver
"Bind current driver to `driver` and execute `body`.
@@ -59,8 +68,7 @@
...)"
{:style/indent 1}
[driver & body]
- `(binding [*driver* ~driver]
- ~@body))
+ `(do-with-driver ~driver (fn [] ~@body)))
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -537,12 +545,24 @@
(defmethod supports? [::driver :case-sensitivity-string-filter-options] [_ _] true)
-(defmulti format-custom-field-name
- "Return the custom name passed via an MBQL `:named` clause so it matches the way it is returned in the results. This
- is used by the post-processing annotation stage to find the correct metadata to include with fields in the results.
- The default implementation is `identity`, meaning the resulting field will have exactly the same name as passed to
- the `:named` clause. Certain drivers like Redshift always lowercase these names, so this method is provided for
- those situations."
+(defmulti ^:deprecated format-custom-field-name
+ "Prior to Metabase 0.33.0, you could specifiy custom names for aggregations in MBQL by wrapping the clause in a
+ `:named` clause:
+
+ [:named [:count] \"My Count\"]
+
+ This name was used for both the `:display_name` in the query results, and for the `:name` used as an alias in the
+ query (e.g. the right-hand side of a SQL `AS` expression). Because some custom display names weren't allowed by some
+ drivers, or would be transformed in some way (for example, Redshift always lowercases custom aliases), this method
+ was needed so we could match the name we had given the column with the one in the query results.
+
+ In 0.33.0, we started using `:named` internally to alias aggregations in middleware in *all* queries to prevent
+ issues with referring to multiple aggregations of the same type when that query was used as a source query.
+ See [#9767](https://github.com/metabase/metabase/issues/9767) for more details. After this change, it became
+ desirable to differentiate between such internally-generated aliases and display names, which need not be used in
+ the query at all; thus in MBQL 1.3.0 [`:named` was replaced by the more general
+ `:aggregation-options`](https://github.com/metabase/mbql/pull/7). Because user-generated names are no longer used as
+ aliases in native queries themselves, this method is no longer needed and will be removed in a future release."
{:arglists '([driver custom-field-name])}
dispatch-on-initialized-driver
:hierarchy #'hierarchy)
diff --git a/src/metabase/driver/sql/query_processor.clj b/src/metabase/driver/sql/query_processor.clj
index 12afab53945d873ef093ee20bb6c9eb880a77ba7..9f9df2324b396d75eb9b88a7a1343e85d87ec26f 100644
--- a/src/metabase/driver/sql/query_processor.clj
+++ b/src/metabase/driver/sql/query_processor.clj
@@ -273,15 +273,18 @@
(hsql/call :/ (->honeysql driver [:count-where pred]) :%count.*))
;; actual handling of the name is done in the top-level clause handler for aggregations
-(defmethod ->honeysql [:sql :named] [driver [_ ag ag-name]]
+(defmethod ->honeysql [:sql :aggregation-options] [driver [_ ag]]
(->honeysql driver ag))
;; aggregation REFERENCE e.g. the ["aggregation" 0] fields we allow in order-by
(defmethod ->honeysql [:sql :aggregation]
[driver [_ index]]
(mbql.u/match-one (mbql.u/aggregation-at-index *query* index *nested-query-level*)
- [:named _ ag-name & _]
- (->honeysql driver (hx/identifier :field-alias ag-name))
+ [:aggregation-options ag (options :guard :name)]
+ (->honeysql driver (hx/identifier :field-alias (:name options)))
+
+ [:aggregation-options ag _]
+ (recur ag)
;; For some arcane reason we name the results of a distinct aggregation "count", everything else is named the
;; same as the aggregation
diff --git a/src/metabase/query_processor/middleware/annotate.clj b/src/metabase/query_processor/middleware/annotate.clj
index 36f621f6710e2491e9420007c546cf5bbb3ae26f..2194441b46dcf54e168e17916ad7ced95db7b2d2 100644
--- a/src/metabase/query_processor/middleware/annotate.clj
+++ b/src/metabase/query_processor/middleware/annotate.clj
@@ -175,13 +175,7 @@
;;; ---------------------------------------------- Aggregate Field Info ----------------------------------------------
-(def ^:private arithmetic-op->text
- {:+ "add"
- :- "sub"
- :/ "div"
- :* "mul"})
-
-(defn- expression-ag-arg->name
+(defn- expression-arg-display-name
"Generate an appropriate name for an `arg` in an expression aggregation."
[recursive-name-fn arg]
(mbql.u/match-one arg
@@ -199,29 +193,24 @@
_ &match))
(s/defn aggregation-name :- su/NonBlankString
- "Return an appropriate field *and* display name for an `:aggregation` subclause (an aggregation or
- expression). Takes an options map as schema won't support passing keypairs directly as a varargs. `{:top-level?
- true}` will cause a name to be generated that will appear in the results, other names with a leading `__` will be
- trimmed on some backends.
+ "Return an appropriate aggregation name/alias *used inside a query* for an `:aggregation` subclause (an aggregation
+ or expression). Takes an options map as schema won't support passing keypairs directly as a varargs.
These names are also used directly in queries, e.g. in the equivalent of a SQL `AS` clause."
- [ag-clause :- mbql.s/Aggregation & [{:keys [top-level? recursive-name-fn], :or {recursive-name-fn aggregation-name}}]]
+ [ag-clause :- mbql.s/Aggregation & [{:keys [recursive-name-fn], :or {recursive-name-fn aggregation-name}}]]
(when-not driver/*driver*
(throw (Exception. (str (tru "*driver* is unbound.")))))
(mbql.u/match-one ag-clause
- ;; if a custom name was provided use it. Some drivers have limits on column names, so call
- ;; `format-custom-field-name` so they can modify it as needed.
- [:named _ ag-name & _]
- (driver/format-custom-field-name driver/*driver* ag-name)
+ [:aggregation-options _ (options :guard :name)]
+ (:name options)
+
+ [:aggregation-options ag _]
+ (recur ag)
;; For unnamed expressions, just compute a name like "sum + count"
;; Top level expressions need a name without a leading __ as those are automatically removed from the results
[(operator :guard #{:+ :- :/ :*}) & args]
- (str (when top-level?
- (str (arithmetic-op->text operator)
- "__"))
- (str/join (str " " (name operator) " ")
- (map (partial expression-ag-arg->name recursive-name-fn) args)))
+ "expression"
;; for historic reasons a distinct count clause is still named "count". Don't change this or you might break FE
;; stuff that keys off of aggregation names.
@@ -253,18 +242,15 @@
"Return an appropriate user-facing display name for an aggregation clause."
[ag-clause]
(mbql.u/match-one ag-clause
- ;; if the `named` clause has options, and *explicity* specifies {:use-as-display-name? false}, generate a name based
- ;; on the wrapped clause instead
- [:named wrapped-clause _ (_ :guard #(false? (:use-as-display-name? %)))]
- (aggregation-display-name wrapped-clause)
+ [:aggregation-options _ (options :guard :display-name)]
+ (:display-name options)
- ;; otherwise for a `named` aggregation use the supplied name as the display name
- [:named _ ag-name & _]
- ag-name
+ [:aggregation-options ag _]
+ (recur ag)
[(operator :guard #{:+ :- :/ :*}) & args]
(str/join (format " %s " (name operator))
- (map (partial expression-ag-arg->name aggregation-arg-display-name) args))
+ (map (partial expression-arg-display-name aggregation-arg-display-name) args))
[:count]
(str (tru "count"))
@@ -301,8 +287,8 @@
; `match` pattern for field clauses below
[inner-query :- su/Map, clause]
(mbql.u/match-one clause
- ;; ok, if this is a named aggregation recurse so we can get information about the ag we are naming
- [:named ag & _]
+ ;; ok, if this is a aggregation w/ options recurse so we can get information about the ag it wraps
+ [:aggregation-options ag _]
(merge
(col-info-for-aggregation-clause inner-query ag)
(ag->name-info &match))
@@ -338,14 +324,14 @@
;; hardcoding these types is fine; In the future when we extend Expressions to handle more functionality
;; we'll want to introduce logic that associates a return type with a given expression. But this will work
;; for the purposes of a patch release.
- [(_ :guard #{:expression :+ :- :/ :*}) & _]
+ #{:expression :+ :- :/ :*}
(merge
(infer-expression-type &match)
(when (mbql.preds/Aggregation? &match)
(ag->name-info &match)))
;; get name/display-name of this ag
- [(_ :guard keyword?) arg & args]
+ [(_ :guard keyword?) arg & _]
(merge
(col-info-for-aggregation-clause inner-query arg)
(ag->name-info &match))))
diff --git a/src/metabase/query_processor/middleware/expand_macros.clj b/src/metabase/query_processor/middleware/expand_macros.clj
index 486e19a2fe7a7a8ebbaf5a27e753daa75969463b..df2cfb9cb44fc71e88d5b6cfa071a5302ba66ad4 100644
--- a/src/metabase/query_processor/middleware/expand_macros.clj
+++ b/src/metabase/query_processor/middleware/expand_macros.clj
@@ -78,24 +78,42 @@
(s/defn ^:private metric-info->ag-clause :- mbql.s/Aggregation
"Return an appropriate aggregation clause from `metric-info`."
[{{[aggregation] :aggregation} :definition, metric-name :name} :- MetricInfo
- {:keys [wrap-in-named?]} :- {:wrap-in-named? s/Bool}]
- ;; try to give the resulting aggregation the name of the Metric it came from, unless it's already `:named` in
- ;; which case keep that name
- (if (or (not wrap-in-named?) (mbql.u/is-clause? :named aggregation))
+ {:keys [use-metric-name-as-display-name?]} :- {:use-metric-name-as-display-name? s/Bool}]
+ (if-not use-metric-name-as-display-name?
aggregation
- [:named aggregation metric-name]))
+ ;; try to give the resulting aggregation the name of the Metric it came from, unless it already has a display
+ ;; name in which case keep that name
+ (mbql.u/match-one aggregation
+ [:aggregation-options _ (_ :guard :display-name)]
+ &match
+
+ [:aggregation-options ag options]
+ [:aggregation-options ag (assoc options :display-name metric-name)]
+
+ _
+ [:aggregation-options &match {:display-name metric-name}])))
(defn- replace-metrics-aggregations [query metric-id->info]
(let [metric (fn [metric-id]
(or (get metric-id->info metric-id)
- (throw (IllegalArgumentException.
- (str (tru "Metric {0} does not exist, or is invalid." metric-id))))))]
+ (throw (ex-info (str (tru "Metric {0} does not exist, or is invalid." metric-id))
+ {:type :invalid-query, :metric metric-id}))))]
(mbql.u/replace-in query [:query]
- [:named [:metric (metric-id :guard (complement mbql.u/ga-id?))] metric-name]
- [:named (metric-info->ag-clause (metric metric-id) {:wrap-in-named? false}) metric-name]
-
+ ;; if metric is wrapped in aggregation options that give it a display name, expand the metric but do not name it
+ [:aggregation-options [:metric (metric-id :guard (complement mbql.u/ga-id?))] (options :guard :display-name)]
+ [:aggregation-options
+ (metric-info->ag-clause (metric metric-id) {:use-metric-name-as-display-name? false})
+ options]
+
+ ;; if metric is wrapped in aggregation options that *do not* give it a display name, expand the metric and then
+ ;; merge the options
+ [:aggregation-options [:metric (metric-id :guard (complement mbql.u/ga-id?))] options]
+ (let [[_ ag ag-options] (metric-info->ag-clause (metric metric-id) {:use-metric-name-as-display-name? true})]
+ [:aggregation-options ag (merge ag-options options)])
+
+ ;; otherwise for unwrapped metrics expand them in-place
[:metric (metric-id :guard (complement mbql.u/ga-id?))]
- (metric-info->ag-clause (metric metric-id) {:wrap-in-named? true}))))
+ (metric-info->ag-clause (metric metric-id) {:use-metric-name-as-display-name? true}))))
(defn- add-metrics-clauses
"Add appropriate `filter` and `aggregation` clauses for a sequence of Metrics.
diff --git a/test/metabase/driver/sql/query_processor_test.clj b/test/metabase/driver/sql/query_processor_test.clj
index 61e2a77440503b7e4ad260685f15a91a4dd56d9f..2cc90efbed657dec92124fe9104c7b9fae05d966 100644
--- a/test/metabase/driver/sql/query_processor_test.clj
+++ b/test/metabase/driver/sql/query_processor_test.clj
@@ -143,18 +143,18 @@
:fields :none}]})))))
-;; This query tests that named aggregations are handled correctly
+;; Check that named aggregations are handled correctly
(expect
{:select [[(id :field "PUBLIC" "VENUES" "PRICE") (id :field-alias "PRICE")]
- [(hsql/call :avg (id :field "PUBLIC" "VENUES" "CATEGORY_ID")) (id :field-alias "my_average")]]
+ [(hsql/call :avg (id :field "PUBLIC" "VENUES" "CATEGORY_ID")) (id :field-alias "avg_2")]]
:from [(id :table "PUBLIC" "VENUES")]
- :group-by [(id :field "PUBLIC" "VENUES" "PRICE")],
- :order-by [[(id :field-alias "my_average") :asc]]}
+ :group-by [(id :field "PUBLIC" "VENUES" "PRICE")]
+ :order-by [[(id :field-alias "avg_2") :asc]]}
(qp.test-util/with-everything-store
(metabase.driver/with-driver :h2
(#'sql.qp/mbql->honeysql
::id-swap
(data/mbql-query venues
- {:aggregation [[:named [:avg $category_id] "my_average"]]
- :breakout [$price]
- :order-by [[:asc [:aggregation 0]]]})))))
+ {:aggregation [[:aggregation-options [:avg $category_id] {:name "avg_2"}]]
+ :breakout [$price]
+ :order-by [[:asc [:aggregation 0]]]})))))
diff --git a/test/metabase/query_processor/middleware/add_source_metadata_test.clj b/test/metabase/query_processor/middleware/add_source_metadata_test.clj
index 3f44f68956bc3739b7cab27e1efd03d7bbb1b88a..00775833423677ff1557d42332d1e5b487402e47 100644
--- a/test/metabase/query_processor/middleware/add_source_metadata_test.clj
+++ b/test/metabase/query_processor/middleware/add_source_metadata_test.clj
@@ -93,25 +93,58 @@
:aggregation [[:avg $id]]
:breakout [$price]}})))
-;; Can we add source metadata for a source query that has a named aggregation?
+;; Can we add source metadata for a source query that has a named aggregation? (w/ `:name` and `:display-name`)
(expect
(data/mbql-query venues
{:source-query {:source-table $$venues
- :aggregation [[:named [:avg $id] "my_cool_aggregation"]]
+ :aggregation [[:aggregation-options
+ [:avg $id]
+ {:name "some_generated_name", :display-name "My Cool Ag"}]]
:breakout [$price]}
:source-metadata (concat
(venues-source-metadata :price)
- [{:name "my_cool_aggregation"
- :display_name "my_cool_aggregation"
+ [{:name "some_generated_name"
+ :display_name "My Cool Ag"
:base_type :type/BigInteger
:special_type :type/PK
:settings nil}])})
(add-source-metadata
(data/mbql-query venues
{:source-query {:source-table $$venues
- :aggregation [[:named [:avg $id] "my_cool_aggregation"]]
+ :aggregation [[:aggregation-options
+ [:avg $id]
+ {:name "some_generated_name", :display-name "My Cool Ag"}]]
:breakout [$price]}})))
+(defn- source-metadata [query]
+ (get-in query [:query :source-metadata] query))
+
+;; Can we add source metadata for a source query that has a named aggregation? (w/ `:name` only)
+(expect
+ [{:name "some_generated_name"
+ :display_name "average of ID"
+ :base_type :type/BigInteger
+ :special_type :type/PK
+ :settings nil}]
+ (source-metadata
+ (add-source-metadata
+ (data/mbql-query venues
+ {:source-query {:source-table $$venues
+ :aggregation [[:aggregation-options [:avg $id] {:name "some_generated_name"}]]}}))))
+
+;; Can we add source metadata for a source query that has a named aggregation? (w/ `:display-name` only)
+(expect
+ [{:name "avg"
+ :display_name "My Cool Ag"
+ :base_type :type/BigInteger
+ :special_type :type/PK
+ :settings nil}]
+ (source-metadata
+ (add-source-metadata
+ (data/mbql-query venues
+ {:source-query {:source-table $$venues
+ :aggregation [[:aggregation-options [:avg $id] {:display-name "My Cool Ag"}]]}}))))
+
;; Can we automatically add source metadata to the parent level of a query? If the source query has a source query
;; with source metadata
(expect
diff --git a/test/metabase/query_processor/middleware/annotate_test.clj b/test/metabase/query_processor/middleware/annotate_test.clj
index 6cc37387195bb888d3c7a26940a431a4c73526ee..0020e444162da55e5961563b14ea8e65fd5879d1 100644
--- a/test/metabase/query_processor/middleware/annotate_test.clj
+++ b/test/metabase/query_processor/middleware/annotate_test.clj
@@ -264,11 +264,11 @@
(aggregation-names [:sum [:field-id (data/id :venues :id)]]))
(expect
- {:name "count + 1", :display_name "count + 1"}
+ {:name "expression", :display_name "count + 1"}
(aggregation-names [:+ [:count] 1]))
(expect
- {:name "min + (2 * avg)"
+ {:name "expression"
:display_name "minimum value of ID + (2 * average of Price)"}
(aggregation-names
[:+
@@ -276,7 +276,7 @@
[:* 2 [:avg [:field-id (data/id :venues :price)]]]]))
(expect
- {:name "min + (2 * avg * 3 * (max - 4))"
+ {:name "expression"
:display_name "minimum value of ID + (2 * average of Price * 3 * (maximum value of Category ID - 4))"}
(aggregation-names
[:+
@@ -289,16 +289,29 @@
[:max [:field-id (data/id :venues :category_id)]]
4]]]))
+;; `aggregation-options` (`:name` and `:display-name`)
(expect
- {:name "x", :display_name "x"}
+ {:name "generated_name", :display_name "User-specified Name"}
(aggregation-names
- [:named
+ [:aggregation-options
[:+ [:min [:field-id (data/id :venues :id)]] [:* 2 [:avg [:field-id (data/id :venues :price)]]]]
- "x"]))
+ {:name "generated_name", :display-name "User-specified Name"}]))
+;; `aggregation-options` (`:name` only)
(expect
- {:name "My Cool Aggregation", :display_name "My Cool Aggregation"}
- (aggregation-names [:named [:avg [:field-id (data/id :venues :price)]] "My Cool Aggregation"]))
+ {:name "generated_name", :display_name "minimum value of ID + (2 * average of Price)"}
+ (aggregation-names
+ [:aggregation-options
+ [:+ [:min [:field-id (data/id :venues :id)]] [:* 2 [:avg [:field-id (data/id :venues :price)]]]]
+ {:name "generated_name"}]))
+
+;; `aggregation-options` (`:display-name` only)
+(expect
+ {:name "expression", :display_name "User-specified Name"}
+ (aggregation-names
+ [:aggregation-options
+ [:+ [:min [:field-id (data/id :venues :id)]] [:* 2 [:avg [:field-id (data/id :venues :price)]]]]
+ {:display-name "User-specified Name"}]))
;; make sure custom aggregation names get included in the col info
(defn- col-info-for-aggregation-clause [clause]
@@ -308,7 +321,7 @@
(expect
{:base_type :type/Float
:special_type :type/Number
- :name "count / 2"
+ :name "expression"
:display_name "count / 2"}
(col-info-for-aggregation-clause [:/ [:count] 2]))
@@ -321,31 +334,41 @@
(data/$ids venues
(col-info-for-aggregation-clause [:sum [:+ $price 1]]))))
-;; if a `:named` aggregation supplies optional `:use-as-display-name?` `options` we should respect that
-;; `use-as-disply-name?` is `true` by default, e.g. in cases where the user supplies the names themselves
+;; col info for `:aggregation-options` (`:name` and `:display-name`)
(expect
{:base_type :type/Integer
:special_type :type/Category
:settings nil
- :name "sum"
- :display_name "sum"}
+ :name "sum_2"
+ :display_name "My custom name"}
(qp.test-util/with-everything-store
(data/$ids venues
- (col-info-for-aggregation-clause [:named [:sum $price] "sum" {:use-as-display-name? true}]))))
+ (col-info-for-aggregation-clause
+ [:aggregation-options [:sum $price] {:name "sum_2", :display-name "My custom name"}]))))
-;; `use-as-display-name?` will normally be `false` when the `:named` clause is generated automatically, e.g. by the
-;; `pre-alias-aggregations` middleware. In this case we want to use the name internally in the query to prevent
-;; duplicate column names, but do not want to use them as display names. See
-;; https://github.com/metabase/mbql/releases/tag/1.2.0 for detailed explanation.
+;; col info for `:aggregation-options` (`:name` only)
(expect
{:base_type :type/Integer
:special_type :type/Category
:settings nil
- :name "sum"
+ :name "sum_2"
:display_name "sum of Price"}
(qp.test-util/with-everything-store
(data/$ids venues
- (col-info-for-aggregation-clause [:named [:sum $price] "sum" {:use-as-display-name? false}]))))
+ (col-info-for-aggregation-clause
+ [:aggregation-options [:sum $price] {:name "sum_2"}]))))
+
+;; col info for `:aggregation-options` (`:display-name` only)
+(expect
+ {:base_type :type/Integer
+ :special_type :type/Category
+ :settings nil
+ :name "sum"
+ :display_name "My Custom Name"}
+ (qp.test-util/with-everything-store
+ (data/$ids venues
+ (col-info-for-aggregation-clause
+ [:aggregation-options [:sum $price] {:display-name "My Custom Name"}]))))
;; if a driver is kind enough to supply us with some information about the `:cols` that come back, we should include
;; that information in the results. Their information should be preferred over ours
@@ -394,22 +417,24 @@
:display_name "count_2"
:source :aggregation
:field_ref [:aggregation 3]}]}
- (binding [driver/*driver* :h2]
- ((annotate/add-column-info (constantly {:cols [{:name "count"
- :display_name "count"
- :base_type :type/Number}
- {:name "sum"
- :display_name "sum"
- :base_type :type/Number}
- {:name "count"
- :display_name "count"
- :base_type :type/Number}
- {:name "count_2"
- :display_name "count_2"
- :base_type :type/Number}]
- :columns ["count" "sum" "count" "count_2"]}))
+ (driver/with-driver :h2
+ ((annotate/add-column-info (constantly {:cols [{:name "count"
+ :display_name "count"
+ :base_type :type/Number}
+ {:name "sum"
+ :display_name "sum"
+ :base_type :type/Number}
+ {:name "count"
+ :display_name "count"
+ :base_type :type/Number}
+ {:name "count_2"
+ :display_name "count_2"
+ :base_type :type/Number}]}))
(data/mbql-query venues
- {:aggregation [[:count] [:sum] [:count] [:named [:count] "count_2"]]}))))
+ {:aggregation [[:count]
+ [:sum]
+ [:count]
+ [:aggregation-options [:count] {:display-name "count_2"}]]}))))
;; make sure expressions come back with the right set of keys, including `:expression_name` (#8854)
(expect
@@ -423,12 +448,36 @@
(-> (qp.test-util/with-everything-store
((annotate/add-column-info (constantly {}))
(data/mbql-query venues
- {:expressions {:discount_price [:* 0.9 [:field-id $price]]}
+ {:expressions {"discount_price" [:* 0.9 $price]}
:fields [$name [:expression "discount_price"]]
:limit 10})))
:cols
second))
+
+;; make sure multiple expressions come back with deduplicated names
+(expect
+ [{:base_type :type/Float
+ :special_type :type/Number
+ :name "expression"
+ :display_name "0.9 * average of Price"
+ :source :aggregation
+ :field_ref [:aggregation 0]}
+ {:base_type :type/Float
+ :special_type :type/Number
+ :name "expression_2"
+ :display_name "0.8 * average of Price"
+ :source :aggregation
+ :field_ref [:aggregation 1]}]
+ (-> (driver/with-driver :h2
+ (qp.test-util/with-everything-store
+ ((annotate/add-column-info (constantly {}))
+ (data/mbql-query venues
+ {:aggregation [[:* 0.9 [:avg $price]]
+ [:* 0.8 [:avg $price]]]
+ :limit 10}))))
+ :cols))
+
(expect
{:name "prev_month"
:display_name "prev_month"
diff --git a/test/metabase/query_processor/middleware/expand_macros_test.clj b/test/metabase/query_processor/middleware/expand_macros_test.clj
index 86937437b33d72e0bdb5baa408980ba2df3c541d..4c98f2ebf4f5ae656b43abe768f8be56459179ba 100644
--- a/test/metabase/query_processor/middleware/expand_macros_test.clj
+++ b/test/metabase/query_processor/middleware/expand_macros_test.clj
@@ -1,7 +1,6 @@
(ns metabase.query-processor.middleware.expand-macros-test
(:require [expectations :refer [expect]]
[metabase
- [query-processor :as qp]
[query-processor-test :as qp.test]
[util :as u]]
[metabase.models
@@ -55,7 +54,7 @@
;; just a metric (w/out nested segments)
(expect
(mbql-query
- {:aggregation [[:named [:count] "Toucans in the rainforest"]]
+ {:aggregation [[:aggregation-options [:count] {:display-name "Toucans in the rainforest"}]]
:filter [:and
[:> [:field-id 4] 1]
[:= [:field-id 5] "abc"]]
@@ -78,7 +77,7 @@
(expect
(mbql-query
{:source-table 1000
- :aggregation [[:named [:count] "ABC Fields"]]
+ :aggregation [[:aggregation-options [:count] {:display-name "ABC Fields"}]]
:filter [:= [:field-id 5] "abc"]
:breakout [[:field-id 17]]
:order-by [[:asc [:field-id 1]]]})
@@ -98,7 +97,7 @@
;; metric w/ no filter definition
(expect
(mbql-query
- {:aggregation [[:named [:count] "My Metric"]]
+ {:aggregation [[:aggregation-options [:count] {:display-name "My Metric"}]]
:filter [:= [:field-id 5] "abc"]
:breakout [[:field-id 17]]
:order-by [[:asc [:field-id 1]]]})
@@ -118,7 +117,7 @@
(expect
(mbql-query
{:source-table 1000
- :aggregation [[:named [:sum [:field-id 18]] "My Metric"]]
+ :aggregation [[:aggregation-options [:sum [:field-id 18]] {:display-name "My Metric"}]]
:filter [:and
[:> [:field-id 4] 1]
[:is-null [:field-id 7]]
@@ -158,12 +157,9 @@
:filter [:> [:field-id (data/id :venues :price)] 1]}}]
(qp.test/format-rows-by [int int]
(qp.test/rows
- (qp/process-query
- {:database (data/id)
- :type :query
- :query {:source-table (data/id :venues)
- :aggregation [[:metric (u/get-id metric)]]
- :breakout [[:field-id (data/id :venues :price)]]}})))))
+ (data/run-mbql-query venues
+ {:aggregation [[:metric (u/get-id metric)]]
+ :breakout [$price]})))))
;; make sure that we don't try to expand GA "metrics" (#6104)
(expect
@@ -193,19 +189,48 @@
;; make sure we can name a :metric (ick)
(expect
(mbql-query
- {:aggregation [[:named [:sum [:field-id 20]] "Named Metric"]]
+ {:aggregation [[:aggregation-options [:sum [:field-id 20]] {:display-name "Named Metric"}]]
:breakout [[:field-id 10]]})
(tt/with-temp Metric [metric {:definition {:aggregation [[:sum [:field-id 20]]]}}]
(#'expand-macros/expand-metrics-and-segments
- (mbql-query {:aggregation [[:named [:metric (u/get-id metric)] "Named Metric"]]
+ (mbql-query {:aggregation [[:aggregation-options [:metric (u/get-id metric)] {:display-name "Named Metric"}]]
:breakout [[:field-id 10]]}))))
-;; a Metric whose :aggregation is already named should not get wrapped in a `:named` clause
+;; if the `:metric` is wrapped in aggregation options that do *not* give it a display name, `:display-name` should be
+;; added to the options
(expect
(mbql-query
- {:aggregation [[:named [:sum [:field-id 20]] "My Cool Aggregation"]]
+ {:aggregation [[:aggregation-options
+ [:sum [:field-id 20]]
+ {:name "auto_generated_name", :display-name "Toucans in the rainforest"}]]
:breakout [[:field-id 10]]})
- (tt/with-temp Metric [metric {:definition {:aggregation [[:named [:sum [:field-id 20]] "My Cool Aggregation"]]}}]
+ (tt/with-temp Metric [metric {:definition {:aggregation [[:sum [:field-id 20]]]}}]
+ (#'expand-macros/expand-metrics-and-segments
+ (mbql-query {:aggregation [[:aggregation-options [:metric (u/get-id metric)] {:name "auto_generated_name"}]]
+ :breakout [[:field-id 10]]}))))
+
+;; a Metric whose :aggregation is already named should not get wrapped in an `:aggregation-options` clause
+(expect
+ (mbql-query
+ {:aggregation [[:aggregation-options [:sum [:field-id 20]] {:display-name "My Cool Aggregation"}]]
+ :breakout [[:field-id 10]]})
+ (tt/with-temp Metric [metric {:definition {:aggregation [[:aggregation-options
+ [:sum [:field-id 20]]
+ {:display-name "My Cool Aggregation"}]]}}]
+ (#'expand-macros/expand-metrics-and-segments
+ (mbql-query {:aggregation [[:metric (u/get-id metric)]]
+ :breakout [[:field-id 10]]}))))
+
+;; ...but if it's wrapped in `:aggregation-options`, but w/o given a display name, we should merge the options
+(expect
+ (mbql-query
+ {:aggregation [[:aggregation-options
+ [:sum [:field-id 20]]
+ {:name "auto_generated_name", :display-name "Toucans in the rainforest"}]]
+ :breakout [[:field-id 10]]})
+ (tt/with-temp Metric [metric {:definition {:aggregation [[:aggregation-options
+ [:sum [:field-id 20]]
+ {:name "auto_generated_name"}]]}}]
(#'expand-macros/expand-metrics-and-segments
(mbql-query {:aggregation [[:metric (u/get-id metric)]]
:breakout [[:field-id 10]]}))))
diff --git a/test/metabase/query_processor/middleware/pre_alias_aggregations_test.clj b/test/metabase/query_processor/middleware/pre_alias_aggregations_test.clj
index 8bc40efaa1cae782a659811d660973cacecb3e40..970bab79e30ab9ed3f59adc0d5b5dc44067b99fd 100644
--- a/test/metabase/query_processor/middleware/pre_alias_aggregations_test.clj
+++ b/test/metabase/query_processor/middleware/pre_alias_aggregations_test.clj
@@ -2,8 +2,7 @@
"Tests for the `pre-alias-aggregations` middleware. For the most part we don't need to test the actual pre-alias
logic, as that comes from the MBQL library and is tested thoroughly there -- we just need to test that it gets
applied in the correct places."
- (:require [clojure.string :as str]
- [expectations :refer [expect]]
+ (:require [expectations :refer [expect]]
[metabase.driver :as driver]
[metabase.query-processor.middleware.pre-alias-aggregations :as pre-alias-aggregations]
[metabase.test.data :as data]))
@@ -16,8 +15,8 @@
(expect
(data/mbql-query checkins
{:source-table $$checkins
- :aggregation [[:named [:sum $user_id] "sum" {:use-as-display-name? false}]
- [:named [:sum $venue_id] "sum_2" {:use-as-display-name? false}]]})
+ :aggregation [[:aggregation-options [:sum $user_id] {:name "sum"} ]
+ [:aggregation-options [:sum $venue_id] {:name "sum_2"} ]]})
(pre-alias
(data/mbql-query checkins
{:source-table $$checkins
@@ -27,21 +26,21 @@
(expect
(data/mbql-query checkins
{:source-table $$checkins
- :aggregation [[:named [:sum $user_id] "sum" {:use-as-display-name? false}]
- [:named [:sum $venue_id] "sum_2"]]})
+ :aggregation [[:aggregation-options [:sum $user_id] {:name "sum"}]
+ [:aggregation-options [:sum $venue_id] {:name "sum_2"}]]})
(pre-alias
(data/mbql-query checkins
{:source-table $$checkins
:aggregation [[:sum $user_id]
- [:named [:sum $venue_id] "sum"]]})))
+ [:aggregation-options [:sum $venue_id] {:name "sum"}]]})))
;; do aggregations inside source queries get pre-aliased?
(expect
(data/mbql-query checkins
{:source-query {:source-table $$checkins
- :aggregation [[:named [:sum $user_id] "sum" {:use-as-display-name? false}]
- [:named [:sum $venue_id] "sum_2" {:use-as-display-name? false}]]}
- :aggregation [[:named [:count] "count" {:use-as-display-name? false}]]})
+ :aggregation [[:aggregation-options [:sum $user_id] {:name "sum"}]
+ [:aggregation-options [:sum $venue_id] {:name "sum_2"}]]}
+ :aggregation [[:aggregation-options [:count] {:name "count"}]]})
(pre-alias
(data/mbql-query checkins
{:source-query {:source-table $$checkins
@@ -52,10 +51,10 @@
(expect
(data/mbql-query checkins
{:source-table $$venues
- :aggregation [[:named [:count] "count" {:use-as-display-name? false}]]
+ :aggregation [[:aggregation-options [:count] {:name "count"}]]
:joins [{:source-query {:source-table $$checkins
- :aggregation [[:named [:sum $user_id] "sum" {:use-as-display-name? false}]
- [:named [:sum $venue_id] "sum_2" {:use-as-display-name? false}]]
+ :aggregation [[:aggregation-options [:sum $user_id] {:name "sum"}]
+ [:aggregation-options [:sum $venue_id] {:name "sum_2"}]]
:breakout [$venue_id]}
:alias "checkins"
:condition [:= &checkins.venue_id $venues.id]}]})
@@ -74,28 +73,48 @@
{:database 1
:type :query
:query {:source-table 1
- :aggregation [[:named [:+ 20 [:sum [:field-id 2]]] "20 + sum" {:use-as-display-name? false}]]}}
+ :aggregation [[:aggregation-options [:+ 20 [:sum [:field-id 2]]] {:name "expression"}]]}}
(pre-alias
{:database 1
:type :query
:query {:source-table 1
:aggregation [[:+ 20 [:sum [:field-id 2]]]]}}))
+(expect
+ {:database 1
+ :type :query
+ :query {:source-table 1
+ :aggregation [[:aggregation-options
+ [:+ 20 [:sum [:field-id 2]]]
+ {:name "expression"}]
+ [:aggregation-options
+ [:- 20 [:sum [:field-id 2]]]
+ {:name "expression_2"}]]}}
+ (pre-alias
+ {:database 1
+ :type :query
+ :query {:source-table 1
+ :aggregation [[:+ 20 [:sum [:field-id 2]]]
+ [:- 20 [:sum [:field-id 2]]]]}}))
+
;; we should use `driver/format-custom-field-name` on the generated aggregation names in case the drivers need to
-;; tweak the default names we generate
+;; tweak the default names we generate.
+;;
+;; TODO - `format-custom-field-name` is officially deprecated because it is no longer needed now that custom
+;; aggregation names are not used in queries themselves. It can be removed soon, and we can remove this test
(driver/register! ::test-driver, :abstract? true, :parent :sql)
-;; this implementation prepends an underscore to any name that starts with a number. Some DBs, such as BigQuery, do
-;; not allow aliases that start with an underscore
+;; this implementation prepends an underscore to any all names. Some DBs, such as BigQuery, do ;; not allow aliases
+;; that start with an number for example
(defmethod driver/format-custom-field-name ::test-driver [_ custom-field-name]
- (str/replace custom-field-name #"(^\d)" "_$1"))
+ (str \_ custom-field-name))
(expect
{:database 1
:type :query
:query {:source-table 1
- :aggregation [[:named [:+ 20 [:sum [:field-id 2]]] "_20 + sum" {:use-as-display-name? false}]
- [:named [:count] "count" {:use-as-display-name? false}]]}}
+ :aggregation [[:aggregation-options [:+ 20 [:sum [:field-id 2]]] {:name "_expression"}]
+ [:aggregation-options [:count] {:name "_count"}]]}}
(driver/with-driver ::test-driver
(pre-alias
{:database 1
diff --git a/test/metabase/query_processor_test/expression_aggregations_test.clj b/test/metabase/query_processor_test/expression_aggregations_test.clj
index 482ad89d1f91d38ac540e9744dc3511e8188ce87..76ccc21af31d7d1fcf50d3924596df2a7afb3541 100644
--- a/test/metabase/query_processor_test/expression_aggregations_test.clj
+++ b/test/metabase/query_processor_test/expression_aggregations_test.clj
@@ -184,12 +184,11 @@
[3 52]
[4 30]]
:columns [(data/format-name "price")
- ;; Redshift annoyingly always lowercases column aliases
- (driver/format-custom-field-name driver/*driver* "New Price")]}
+ "sum_of_price"]}
(qp.test/format-rows-by [int int]
(qp.test/rows+column-names
(data/run-mbql-query venues
- {:aggregation [[:named [:sum [:+ $price 1]] "New Price"]]
+ {:aggregation [[:aggregation-options [:sum [:+ $price 1]] {:name "sum_of_price"}]]
:breakout [$price]}))))
;; check that we can name an expression aggregation w/ expression at top-level
@@ -199,14 +198,14 @@
[3 -2]
[4 -17]]
:columns [(data/format-name "price")
- (driver/format-custom-field-name driver/*driver* "Sum-41")]}
+ "sum_41"]}
(qp.test/format-rows-by [int int]
(qp.test/rows+column-names
(data/run-mbql-query venues
- {:aggregation [[:named [:- [:sum $price] 41] "Sum-41"]]
+ {:aggregation [[:aggregation-options [:- [:sum $price] 41] {:name "sum_41"}]]
:breakout [$price]}))))
-;; check that we can handle METRICS (ick) inside expression aggregation clauses
+;; check that we can handle Metrics inside expression aggregation clauses
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expression-aggregations)
[[2 119]
[3 40]
@@ -220,29 +219,30 @@
{:aggregation [:+ [:metric (u/get-id metric)] 1]
:breakout [[:field-id $price]]})))))
-;; check that we can handle METRICS (ick) inside a NAMED clause
+;; check that we can handle Metrics inside an `:aggregation-options` clause
+;; TODO - Pretty sure this test doesn't belong here (?)
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expression-aggregations)
{:rows [[2 118]
[3 39]
[4 24]]
:columns [(data/format-name "price")
- (driver/format-custom-field-name driver/*driver* "My Cool Metric")]}
+ "auto_generated_name"]}
(tt/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]}}]
(qp.test/format-rows-by [int int]
(qp.test/rows+column-names
(data/run-mbql-query venues
- {:aggregation [[:named [:metric (u/get-id metric)] "My Cool Metric"]]
+ {:aggregation [[:aggregation-options [:metric (u/get-id metric)] {:name "auto_generated_name"}]]
:breakout [[:field-id $price]]})))))
-;; check that METRICS (ick) with a nested aggregation still work inside a NAMED clause
+;; check that Metrics with a nested aggregation still work inside an `:aggregation-options` clause
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expression-aggregations)
{:rows [[2 118]
[3 39]
[4 24]]
:columns [(data/format-name "price")
- (driver/format-custom-field-name driver/*driver* "My Cool Metric")]}
+ "auto_generated_name"]}
(tt/with-temp Metric [metric (data/$ids venues
{:table_id $$venues
:definition {:aggregation [[:sum $price]]
@@ -250,16 +250,16 @@
(qp.test/format-rows-by [int int]
(qp.test/rows+column-names
(data/run-mbql-query venues
- {:aggregation [[:named [:metric (u/get-id metric)] "My Cool Metric"]]
+ {:aggregation [[:aggregation-options [:metric (u/get-id metric)] {:name "auto_generated_name"}]]
:breakout [[:field-id $price]]})))))
;; check that named aggregations come back with the correct column metadata (#4002)
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expression-aggregations)
(assoc (qp.test/aggregate-col :count)
- :name (driver/format-custom-field-name driver/*driver* "Count of Things")
+ :name "auto_generated_name"
:display_name "Count of Things")
(-> (data/run-mbql-query venues
- {:aggregation [[:named ["COUNT"] "Count of Things"]]})
+ {:aggregation [[:aggregation-options ["COUNT"] {:name "auto_generated_name", :display-name "Count of Things"}]]})
qp.test/cols
first))
diff --git a/test/metabase/query_processor_test/expressions_test.clj b/test/metabase/query_processor_test/expressions_test.clj
index 47238370288d3bed6de83f0003a3158036669122..bc6fd61ca22e9e752d8f6a53c97bd0003038b4f0 100644
--- a/test/metabase/query_processor_test/expressions_test.clj
+++ b/test/metabase/query_processor_test/expressions_test.clj
@@ -106,10 +106,10 @@
:snowflake :type/Number
:type/Integer)})
(set (map #(select-keys % [:name :base_type])
- (-> (data/run-mbql-query venues
- {:aggregation [:named [:sum [:* $price -1]] "x"]
- :breakout [$category_id]})
- (get-in [:data :cols])))))
+ (qp.test/cols
+ (data/run-mbql-query venues
+ {:aggregation [[:aggregation-options [:sum [:* $price -1]] {:name "x"}]]
+ :breakout [$category_id]})))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | HANDLING NULLS AND ZEROES |
diff --git a/test/metabase/test/data/datasets.clj b/test/metabase/test/data/datasets.clj
index 8742bc08bdb825e416dccaa04a297a24f1f8be10..cfa852dc563aee7e44da59161364c07e43597ec6 100644
--- a/test/metabase/test/data/datasets.clj
+++ b/test/metabase/test/data/datasets.clj
@@ -2,7 +2,7 @@
"Interface + implementations for loading test datasets for different drivers, and getting information about the
dataset's tables, fields, etc.
- TODO - rename this to `metabase.driver.test-extensions.expect` or something like that"
+ TODO - rename this to `metabase.driver.test-extensions.expect` or `metabase.driver.test` or something like that"
(:require [expectations :refer [expect]]
[metabase.driver :as driver]
[metabase.test.data.env :as tx.env]))