diff --git a/src/metabase/query_processor/macros.clj b/src/metabase/query_processor/macros.clj index dfe291fc13f9d56804b35e9925a8d6872564464e..3b923679a87a72caa49e1053836cf9b6b0e907ed 100644 --- a/src/metabase/query_processor/macros.clj +++ b/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] diff --git a/test/metabase/query_processor/macros_test.clj b/test/metabase/query_processor/macros_test.clj index 5c39a816075be96050860a83775e4ef56fa3a643..9b0ca09ed012eafb9d474a4595117aca0eabae1e 100644 --- a/test/metabase/query_processor/macros_test.clj +++ b/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)))]}}))))) diff --git a/test/metabase/query_processor_test/expression_aggregations_test.clj b/test/metabase/query_processor_test/expression_aggregations_test.clj index d69018fe9d9f913f15b9441ec4e7245f3e083951..66a30e1a2721db11a17faedb286c062f3c217c2c 100644 --- a/test/metabase/query_processor_test/expression_aggregations_test.clj +++ b/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)))]}})))))