Merge pull request #3955 from metabase/add-test-for-name-metrics

Properly handle METRICs defined with nested ag syntax

src/metabase/query_processor/macros.clj

@@ -58,11 +58,22 @@
   (when (metric? metric)
     (second metric)))
 
+(defn- maybe-unnest-ag-clause
+  "Unnest AG-CLAUSE if it's wrapped in a vector (i.e. if it is using the \"multiple-aggregation\" syntax).
+   (This is provided merely as a convenience to facilitate implementation of the Query Builder, so it can use the same UI for
+   normal aggregations and Metric creation. *METRICS DO NOT SUPPORT MULTIPLE AGGREGATIONS,* so if nested syntax is used, any
+   aggregation after the first will be ignored.)"
+  [ag-clause]
+  (if (and (coll? ag-clause)
+           (every? coll? ag-clause))
+    (first ag-clause)
+    ag-clause))
+
 (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))
+    (maybe-unnest-ag-clause ag-clause)))
 
 (defn- expand-metrics-in-ag-clause [query-dict filter-clauses-atom]
   (walk/postwalk (fn [form]

test/metabase/query_processor/macros_test.clj

@@ -4,9 +4,15 @@
             [metabase.models.metric :refer [Metric]]
             [metabase.models.segment :refer [Segment]]
             [metabase.models.table :refer [Table]]
-            [metabase.query-processor.macros :refer :all]
-            [metabase.test.data.users :refer :all]
-            [metabase.test.util :as tu]))
+            [metabase.query-processor :as qp]
+            (metabase.query-processor [expand :as ql]
+                                      [macros :refer :all])
+            [metabase.query-processor-test :refer :all]
+            [metabase.test.data :as data]
+            (metabase.test.data [datasets :as datasets]
+                                [users :refer :all])
+            [metabase.test.util :as tu]
+            [metabase.util :as u]))
 
 ;; expand-macros
 
@@ -127,3 +133,19 @@
                                :filter      ["AND" [">" 4 1] ["SEGMENT" segment-2-id]]
                                :breakout    [17]
                                :order_by    [[1 "ASC"]]}})))
+
+;; Check that a metric w/ multiple aggregation syntax (nested vector) still works correctly
+(datasets/expect-with-engines (engines-that-support :expression-aggregations)
+  [[2 118]
+   [3  39]
+   [4  24]]
+  (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)]]
+                          :breakout     [(ql/breakout (ql/field-id (data/id :venues :price)))]}})))))

test/metabase/query_processor_test/expression_aggregations_test.clj

@@ -201,3 +201,39 @@
                :query    {:source-table (data/id :venues)
                           :aggregation  [:+ ["METRIC" (u/get-id metric)] 1]
                           :breakout     [(ql/breakout (ql/field-id (data/id :venues :price)))]}})))))
+
+;; check that we can handle METRICS (ick) inside a NAMED clause
+(datasets/expect-with-engines (engines-that-support :expression-aggregations)
+  {:rows    [[2 118]
+             [3  39]
+             [4  24]]
+   :columns [(data/format-name "price")
+             (if (= *engine* :redshift) "my cool metric" "My Cool Metric")]}
+  (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+column-names (qp/process-query
+                           {:database (data/id)
+                            :type     :query
+                            :query    {:source-table (data/id :venues)
+                                       :aggregation  [[:named ["METRIC" (u/get-id metric)] "My Cool Metric"]]
+                                       :breakout     [(ql/breakout (ql/field-id (data/id :venues :price)))]}})))))
+
+;; check that METRICS (ick) with a nested aggregation still work inside a NAMED clause
+(datasets/expect-with-engines (engines-that-support :expression-aggregations)
+  {:rows    [[2 118]
+             [3  39]
+             [4  24]]
+   :columns [(data/format-name "price")
+             (if (= *engine* :redshift) "my cool metric" "My Cool Metric")]}
+  (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+column-names (qp/process-query
+                           {:database (data/id)
+                            :type     :query
+                            :query    {:source-table (data/id :venues)
+                                       :aggregation  [[:named ["METRIC" (u/get-id metric)] "My Cool Metric"]]
+                                       :breakout     [(ql/breakout (ql/field-id (data/id :venues :price)))]}})))))