Replace :named aggregation clause with more general `:aggregation-o…

…ptions` (#7)

project.clj

@@ -1,4 +1,4 @@
-(defproject metabase/mbql "1.2.0"
+(defproject metabase/mbql "1.3.0"
   :description "Shared things used across several Metabase projects, such as i18n and config."
   :url "https://github.com/metabase/mbql"
   :min-lein-version "2.5.0"
@@ -19,8 +19,8 @@
   :dependencies
   [[org.clojure/core.match "0.3.0"]
    [medley "1.2.0"]
-   [metabase/common "1.0.2"]
-   [metabase/schema-util "1.0.1"]
+   [metabase/common "1.0.4"]
+   [metabase/schema-util "1.0.2"]
    [prismatic/schema "1.1.11"]]
 
   :profiles

src/metabase/mbql/normalize.clj

@@ -353,9 +353,20 @@
     [:rows & _]
     nil
 
-    ;; For named aggregations (`[:named <ag> <name>]`) we want to leave as-is and just canonicalize the ag it names
-    [:named wrapped-ag & more]
-    (into [:named (canonicalize-aggregation-subclause wrapped-ag)] more)
+    ;; for aggregations wrapped in aggregation-options we can leave it as-is and just canonicalize the subclause
+    [:aggregation-options wrapped-ag options]
+    [:aggregation-options (canonicalize-aggregation-subclause wrapped-ag) options]
+
+    ;; for legacy `:named` aggregations convert them to a new-style `:aggregation-options` clause.
+    ;;
+    ;; 99.99% of clauses should have no options, however if they do and `:use-as-display-name?` is false (default is
+    ;; true) then generate options to change `:name` rather than `:display-name`
+    [:named wrapped-ag ag-name & more]
+    (canonicalize-aggregation-subclause
+     [:aggregation-options wrapped-ag (let [[{:keys [use-as-display-name?]}] more]
+                                        (if (false? use-as-display-name?)
+                                          {:name ag-name}
+                                          {:display-name ag-name}))])
 
     [(ag-type :guard #{:+ :- :* :/}) & args]
     (apply

src/metabase/mbql/schema.clj

@@ -513,27 +513,21 @@
 (def ^:private UnnamedAggregation
   (s/recursive #'UnnamedAggregation*))
 
-;; any sort of aggregation can be wrapped in a `[:named <ag> <custom-name>]` clause, but you cannot wrap a `:named` in
-;; a `:named`
-
-(def NamedAggregationOptions
-  "Schema for optional options map for the `:named` aggregation clause."
-  ;; whether the supplied name should be used as the user-facing display name of the aggregation as well. `true` by
-  ;; default -- this is what we want to do if the `:named` clause was generated in the FE with a user-supplied name.
-  ;; If this clause is used internally to deduplicate/uniquify aggregation names (e.g. `sum` and `sum_2`, we should
-  ;; set this to `false` so we can try to come up with a good name for the aggregation it wraps (e.g. `sum of My
-  ;; Field`)
-  {(s/optional-key :use-as-display-name?) s/Bool}) ; default true
-
-(defclause named
-  aggregation      UnnamedAggregation
-  aggregation-name su/NonBlankString
-  options          (optional NamedAggregationOptions))
+(def AggregationOptions
+  "Additional options for any aggregation clause when wrapping it in `:aggregation-options`."
+  {;; name to use for this aggregation in the native query instead of the default name (e.g. `count`)
+   (s/optional-key :name)         su/NonBlankString
+   ;; user-facing display name for this aggregation instead of the default one
+   (s/optional-key :display-name) su/NonBlankString})
+
+(defclause aggregation-options
+  aggregation UnnamedAggregation
+  options     AggregationOptions)
 
 (def Aggregation
   "Schema for anything that is a valid `:aggregation` clause."
-  (s/if (partial is-clause? :named)
-    named
+  (s/if (partial is-clause? :aggregation-options)
+    aggregation-options
     UnnamedAggregation))
 
 

src/metabase/mbql/util.clj

@@ -482,39 +482,58 @@
   [names :- [s/Str]]
   (map (unique-name-generator) names))
 
-(def ^:private NamedAggregationsWithUniqueNames
-  (s/constrained [mbql.s/named] #(distinct? (map last %)) "sequence of named aggregations with unique names"))
-
-(s/defn uniquify-named-aggregations :- NamedAggregationsWithUniqueNames
+(def ^:private NamedAggregation
+  (s/constrained
+   mbql.s/aggregation-options
+   :name
+   "`:aggregation-options` with a `:name`"))
+
+(def ^:private UniquelyNamedAggregations
+  (s/constrained
+   [NamedAggregation]
+   (fn [clauses]
+     (distinct? (for [[_ _ {ag-name :name}] clauses]
+                  ag-name)))
+   "sequence of named aggregations with unique names"))
+
+(s/defn uniquify-named-aggregations :- UniquelyNamedAggregations
   "Make the names of a sequence of named aggregations unique by adding suffixes such as `_2`."
-  [named-aggregations :- [mbql.s/named]]
-  (map (fn [original-clause unique-name]
-         (assoc (vec original-clause) 2 unique-name))
-       named-aggregations
-       (uniquify-names
-        (map #(nth % 2) named-aggregations))))
-
-(s/defn pre-alias-aggregations :- [mbql.s/named]
-  "Wrap every aggregation clause in a `:named` clause, using the name returned by `(aggregation->name-fn ag-clause)`
-  as names for any clauses that are not already wrapped in `:name`.
-
-    (pre-alias-aggregations annotate/aggregation-name [[:count] [:count] [:named [:sum] \"Sum-41\"]])
-    ;; -> [[:named [:count] \"count\"]
-           [:named [:count] \"count\"]
-           [:named [:sum [:field-id 1]] \"Sum-41\"]]
+  [named-aggregations :- [NamedAggregation]]
+  (let [unique-names (uniquify-names
+                      (for [[_ wrapped-ag {ag-name :name}] named-aggregations]
+                        ag-name))]
+    (map
+     (fn [[_ wrapped-ag options] unique-name]
+       [:aggregation-options wrapped-ag (assoc options :name unique-name)])
+     named-aggregations
+     unique-names)))
+
+(s/defn pre-alias-aggregations :- [NamedAggregation]
+  "Wrap every aggregation clause in an `:aggregation-options` clause, using the name returned
+  by `(aggregation->name-fn ag-clause)` as names for any clauses that do not already have a `:name` in
+  `:aggregation-options`.
+
+    (pre-alias-aggregations annotate/aggregation-name
+     [[:count] [:count] [:aggregation-options [:sum [:field-id 1] {:name \"Sum-41\"}]])
+    ;; -> [[:aggregation-options [:count] {:name \"count\"}]
+           [:aggregation-options [:count] {:name \"count\"}]
+           [:aggregation-options [:sum [:field-id 1]] {:name \"Sum-41\"}]]
 
   Most often, `aggregation->name-fn` will be something like `annotate/aggregation-name`, but for purposes of keeping
   the `metabase.mbql` module seperate from the `metabase.query-processor` code we'll let you pass that in yourself."
   {:style/indent 1}
   [aggregation->name-fn :- (s/pred fn?), aggregations :- [mbql.s/Aggregation]]
   (replace aggregations
-    :named
+    [:aggregation-options _ (_ :guard :name)]
     &match
 
+    [:aggregation-options wrapped-ag options]
+    [:aggregation-options wrapped-ag (assoc options :name (aggregation->name-fn wrapped-ag))]
+
     [(_ :guard keyword?) & _]
-    [:named &match (aggregation->name-fn &match) {:use-as-display-name? false}]))
+    [:aggregation-options &match {:name (aggregation->name-fn &match)}]))
 
-(s/defn pre-alias-and-uniquify-aggregations :- NamedAggregationsWithUniqueNames
+(s/defn pre-alias-and-uniquify-aggregations :- UniquelyNamedAggregations
   "Wrap every aggregation clause in a `:named` clause with a unique name. Combines `pre-alias-aggregations` with
   `uniquify-named-aggregations`."
   {:style/indent 1}

test/metabase/mbql/normalize_test.clj

@@ -105,7 +105,7 @@
   {:query {:aggregation [:count [:sum 10] [:count 20] :count]}}
   (#'normalize/normalize-tokens {:query {:aggregation ["count" ["sum" 10] ["count" 20] "count"]}}))
 
-;; try an ag that is named
+;; try an ag that is named using legacy `:named` clause
 (expect
   {:query {:aggregation [:named [:sum 10] "My COOL AG"]}}
   (#'normalize/normalize-tokens {:query {:aggregation ["named" ["SuM" 10] "My COOL AG"]}}))
@@ -115,6 +115,12 @@
   (#'normalize/normalize-tokens
    {:query {:aggregation ["named" ["SuM" 10] "My COOL AG" {:use-as-display-name? false}]}}))
 
+;; try w/ `:aggregation-options`, the replacement for `:named`
+(expect
+ {:query {:aggregation [:aggregation-options [:sum 10] {:display-name "My COOL AG"}]}}
+ (#'normalize/normalize-tokens
+  {:query {:aggregation ["aggregation_options" ["SuM" 10] {"display_name" "My COOL AG"}]}}))
+
 ;; try an expression ag
 (expect
  {:query {:aggregation [:+ [:sum 10] [:* [:sum 20] 3]]}}
@@ -456,14 +462,19 @@
   {:query {:aggregation [[:count] [:sum [:field-id 10]] [:count [:field-id 20]] [:count]]}}
   (#'normalize/canonicalize {:query {:aggregation [:count [:sum 10] [:count 20] :count]}}))
 
-;; make sure we can deal with *named* aggregations!
+;; legacy `:named` aggregation clauses should get converted to `:aggregation-options`
+(expect
+ {:query {:aggregation [[:aggregation-options [:sum [:field-id 10]] {:display-name "Sum *TEN*"}]]}}
+ (#'normalize/canonicalize {:query {:aggregation [:named [:sum 10] "Sum *TEN*"]}}))
+
 (expect
-  {:query {:aggregation [[:named [:sum [:field-id 10]] "Sum *TEN*"]]}}
-  (#'normalize/canonicalize {:query {:aggregation [:named [:sum 10] "Sum *TEN*"]}}))
+ {:query {:aggregation [[:aggregation-options [:sum [:field-id 10]] {:name "Sum *TEN*"}]]}}
+ (#'normalize/canonicalize {:query {:aggregation [:named [:sum 10] "Sum *TEN*" {:use-as-display-name? false}]}}))
 
+;; subclauses of `:aggregation-options` should get canonicalized correctly
 (expect
-  {:query {:aggregation [[:named [:sum [:field-id 10]] "Sum *TEN*" {:use-as-display-name? false}]]}}
-  (#'normalize/canonicalize {:query {:aggregation [:named [:sum 10] "Sum *TEN*" {:use-as-display-name? false}]}}))
+ {:query {:aggregation [[:aggregation-options] [:sum [:field-id 10]]]}}
+ (#'normalize/canonicalize {:query {:aggregation [:aggregation-options [:sum 10] {}]}}))
 
 ;; make sure expression aggregations work correctly
 (expect

test/metabase/mbql/util_test.clj

@@ -585,27 +585,29 @@
   (mbql.u/uniquify-names ["count" "sum" "count" "count"]))
 
 (expect
-  [[:named [:count] "count"]
-   [:named [:sum [:field-id 1]] "sum"]
-   [:named [:count] "count_2"]
-   [:named [:count] "count_3"]]
-  (mbql.u/uniquify-named-aggregations [[:named [:count] "count"]
-                                       [:named [:sum [:field-id 1]] "sum"]
-                                       [:named [:count] "count"]
-                                       [:named [:count] "count"]]))
+ [[:aggregation-options [:count] {:name "count"}]
+  [:aggregation-options [:sum [:field-id 1]] {:name "sum"}]
+  [:aggregation-options [:count] {:name "count_2"}]
+  [:aggregation-options [:count] {:name "count_3"}]]
+ (mbql.u/uniquify-named-aggregations
+  [[:aggregation-options [:count] {:name "count"}]
+   [:aggregation-options [:sum [:field-id 1]] {:name "sum"}]
+   [:aggregation-options [:count] {:name "count"}]
+   [:aggregation-options [:count] {:name "count"}]]))
 
 ;; what if we try to trick it by using a name it would have generated?
 (expect
-  ["count" "count_2" "count_2_2"]
-  (mbql.u/uniquify-names ["count" "count" "count_2"]))
+ ["count" "count_2" "count_2_2"]
+ (mbql.u/uniquify-names ["count" "count" "count_2"]))
 
 (expect
-  [[:named [:count] "count"]
-   [:named [:count] "count_2"]
-   [:named [:count] "count_2_2"]]
-  (mbql.u/uniquify-named-aggregations [[:named [:count] "count"]
-                                       [:named [:count] "count"]
-                                       [:named [:count] "count_2"]]))
+ [[:aggregation-options [:count] {:name "count"}]
+  [:aggregation-options [:count] {:name "count_2"}]
+  [:aggregation-options [:count] {:name "count_2_2"}]]
+ (mbql.u/uniquify-named-aggregations
+  [[:aggregation-options [:count] {:name "count"}]
+   [:aggregation-options [:count] {:name "count"}]
+   [:aggregation-options [:count] {:name "count_2"}]]))
 
 ;; for wacky DBMSes like SQLServer that return blank column names sometimes let's make sure we handle those without
 ;; exploding
@@ -618,44 +620,44 @@
   (name ag-name))
 
 (expect
-  [[:named [:sum [:field-id 1]]   "sum"   {:use-as-display-name? false}]
-   [:named [:count [:field-id 1]] "count" {:use-as-display-name? false}]
-   [:named [:sum [:field-id 1]]   "sum"   {:use-as-display-name? false}]
-   [:named [:avg [:field-id 1]]   "avg"   {:use-as-display-name? false}]
-   [:named [:sum [:field-id 1]]   "sum"   {:use-as-display-name? false}]
-   [:named [:min [:field-id 1]]   "min"   {:use-as-display-name? false}]]
-  (mbql.u/pre-alias-aggregations simple-ag->name
-    [[:sum [:field-id 1]]
-     [:count [:field-id 1]]
-     [:sum [:field-id 1]]
-     [:avg [:field-id 1]]
-     [:sum [:field-id 1]]
-     [:min [:field-id 1]]]))
+ [[:aggregation-options [:sum [:field-id 1]]   {:name "sum"}]
+  [:aggregation-options [:count [:field-id 1]] {:name "count"}]
+  [:aggregation-options [:sum [:field-id 1]]   {:name "sum"}]
+  [:aggregation-options [:avg [:field-id 1]]   {:name "avg"}]
+  [:aggregation-options [:sum [:field-id 1]]   {:name "sum"}]
+  [:aggregation-options [:min [:field-id 1]]   {:name "min"}]]
+ (mbql.u/pre-alias-aggregations simple-ag->name
+   [[:sum [:field-id 1]]
+    [:count [:field-id 1]]
+    [:sum [:field-id 1]]
+    [:avg [:field-id 1]]
+    [:sum [:field-id 1]]
+    [:min [:field-id 1]]]))
 
 ;; we shouldn't change the name of ones that are already named
 (expect
-  [[:named [:sum [:field-id 1]]   "sum"   {:use-as-display-name? false}]
-   [:named [:count [:field-id 1]] "count" {:use-as-display-name? false}]
-   [:named [:sum [:field-id 1]]   "sum"   {:use-as-display-name? false}]
-   [:named [:avg [:field-id 1]]   "avg"   {:use-as-display-name? false}]
-   [:named [:sum [:field-id 1]]   "sum_2"]
-   [:named [:min [:field-id 1]]   "min"   {:use-as-display-name? false}]]
-  (mbql.u/pre-alias-aggregations simple-ag->name
-    [[:sum [:field-id 1]]
-     [:count [:field-id 1]]
-     [:sum [:field-id 1]]
-     [:avg [:field-id 1]]
-     [:named [:sum [:field-id 1]] "sum_2"]
-     [:min [:field-id 1]]]))
+ [[:aggregation-options [:sum [:field-id 1]]   {:name "sum"}]
+  [:aggregation-options [:count [:field-id 1]] {:name "count"}]
+  [:aggregation-options [:sum [:field-id 1]]   {:name "sum"}]
+  [:aggregation-options [:avg [:field-id 1]]   {:name "avg"}]
+  [:aggregation-options [:sum [:field-id 1]]   {:name "sum_2"}]
+  [:aggregation-options [:min [:field-id 1]]   {:name "min"}]]
+ (mbql.u/pre-alias-aggregations simple-ag->name
+   [[:sum [:field-id 1]]
+    [:count [:field-id 1]]
+    [:sum [:field-id 1]]
+    [:avg [:field-id 1]]
+    [:aggregation-options [:sum [:field-id 1]] {:name "sum_2"}]
+    [:min [:field-id 1]]]))
 
 ;; ok, can we do the same thing as the tests above but make those names *unique* at the same time?
 (expect
-  [[:named [:sum [:field-id 1]]   "sum"   {:use-as-display-name? false}]
-   [:named [:count [:field-id 1]] "count" {:use-as-display-name? false}]
-   [:named [:sum [:field-id 1]]   "sum_2" {:use-as-display-name? false}]
-   [:named [:avg [:field-id 1]]   "avg"   {:use-as-display-name? false}]
-   [:named [:sum [:field-id 1]]   "sum_3" {:use-as-display-name? false}]
-   [:named [:min [:field-id 1]]   "min"   {:use-as-display-name? false}]]
+ [[:aggregation-options [:sum [:field-id 1]]   {:name "sum"}  ]
+  [:aggregation-options [:count [:field-id 1]] {:name "count"}]
+  [:aggregation-options [:sum [:field-id 1]]   {:name "sum_2"}]
+  [:aggregation-options [:avg [:field-id 1]]   {:name "avg"}  ]
+  [:aggregation-options [:sum [:field-id 1]]   {:name "sum_3"}]
+  [:aggregation-options [:min [:field-id 1]]   {:name "min"}  ]]
   (mbql.u/pre-alias-and-uniquify-aggregations simple-ag->name
     [[:sum [:field-id 1]]
      [:count [:field-id 1]]
@@ -665,36 +667,42 @@
      [:min [:field-id 1]]]))
 
 (expect
-  [[:named [:sum [:field-id 1]]   "sum"     {:use-as-display-name? false}]
-   [:named [:count [:field-id 1]] "count"   {:use-as-display-name? false}]
-   [:named [:sum [:field-id 1]]   "sum_2"   {:use-as-display-name? false}]
-   [:named [:avg [:field-id 1]]   "avg"     {:use-as-display-name? false}]
-   [:named [:sum [:field-id 1]]   "sum_2_2"]
-   [:named [:min [:field-id 1]]   "min"     {:use-as-display-name? false}]]
-  (mbql.u/pre-alias-and-uniquify-aggregations simple-ag->name
-    [[:sum [:field-id 1]]
-     [:count [:field-id 1]]
-     [:sum [:field-id 1]]
-     [:avg [:field-id 1]]
-     [:named [:sum [:field-id 1]] "sum_2"]
-     [:min [:field-id 1]]]))
-
-;; `pre-alias-and-uniquify-aggregations` shouldn't stomp over existing options
-(expect
- [[:named [:sum [:field-id 1]]   "sum"     {:use-as-display-name? false}]
-  [:named [:count [:field-id 1]] "count"   {:use-as-display-name? false}]
-  [:named [:sum [:field-id 1]]   "sum_2"   {:use-as-display-name? true}]
-  [:named [:avg [:field-id 1]]   "avg"     {:use-as-display-name? true}]
-  [:named [:sum [:field-id 1]]   "sum_2_2" {:use-as-display-name? true}]
-  [:named [:min [:field-id 1]]   "min"     {:use-as-display-name? false}]]
+ [[:aggregation-options [:sum [:field-id 1]]   {:name "sum"}]
+  [:aggregation-options [:count [:field-id 1]] {:name "count"}]
+  [:aggregation-options [:sum [:field-id 1]]   {:name "sum_2"}]
+  [:aggregation-options [:avg [:field-id 1]]   {:name "avg"}]
+  [:aggregation-options [:sum [:field-id 1]]   {:name "sum_2_2"}]
+  [:aggregation-options [:min [:field-id 1]]   {:name "min"}]]
  (mbql.u/pre-alias-and-uniquify-aggregations simple-ag->name
    [[:sum [:field-id 1]]
     [:count [:field-id 1]]
-    [:named [:sum [:field-id 1]] "sum"   {:use-as-display-name? true}]
-    [:named [:avg [:field-id 1]] "avg"   {:use-as-display-name? true}]
-    [:named [:sum [:field-id 1]] "sum_2" {:use-as-display-name? true}]
+    [:sum [:field-id 1]]
+    [:avg [:field-id 1]]
+    [:aggregation-options [:sum [:field-id 1]] {:name "sum_2"}]
     [:min [:field-id 1]]]))
 
+;; if `:aggregation-options` only specifies `:display-name` it should still a new `:name`.
+;; `pre-alias-and-uniquify-aggregations` shouldn't stomp over display name
+(expect
+ [[:aggregation-options [:sum [:field-id 1]] {:name "sum"}]
+  [:aggregation-options [:sum [:field-id 1]] {:name "sum_2"}]
+  [:aggregation-options [:sum [:field-id 1]] {:display-name "Sum of Field 1", :name "sum_3"}]]
+ (mbql.u/pre-alias-and-uniquify-aggregations simple-ag->name
+   [[:sum [:field-id 1]]
+    [:sum [:field-id 1]]
+    [:aggregation-options [:sum [:field-id 1]] {:display-name "Sum of Field 1"}]]))
+
+;; if both are specified, `display-name` should still be propogated
+(expect
+ [[:aggregation-options [:sum [:field-id 1]] {:name "sum"}]
+  [:aggregation-options [:sum [:field-id 1]] {:name "sum_2"}]
+  [:aggregation-options [:sum [:field-id 1]] {:name "sum_2_2", :display_name "Sum of Field 1"}]]
+ (mbql.u/pre-alias-and-uniquify-aggregations simple-ag->name
+   [[:sum [:field-id 1]]
+    [:sum [:field-id 1]]
+    [:aggregation-options [:sum [:field-id 1]] {:name "sum_2", :display_name "Sum of Field 1"}]]))
+
+
 ;;; --------------------------------------------- query->max-rows-limit ----------------------------------------------
 
 ;; should return `:limit` if set