From 5872cd6171592f5a9a74a65747963f6a7b6fc36f Mon Sep 17 00:00:00 2001
From: Cam Saul <cammsaul@gmail.com>
Date: Tue, 25 Jun 2019 14:25:11 -0700
Subject: [PATCH] Display metric name instead of underlying aggregation name in
QP results (#10251)
* Display metric name instead of underlying aggregation name in QP results
[ci drivers]
Resolves #1851
* Test fixes :wrench:
---
src/metabase/models/common.clj | 1 +
src/metabase/models/interface.clj | 2 +-
.../middleware/expand_macros.clj | 73 ++++++++++++------
test/expectation_options.clj | 4 +-
test/metabase/api/embed_test.clj | 3 +-
test/metabase/api/public_test.clj | 9 +--
.../middleware/expand_macros_test.clj | 77 +++++++++----------
7 files changed, 95 insertions(+), 74 deletions(-)
diff --git a/src/metabase/models/common.clj b/src/metabase/models/common.clj
index f244680c732..bb6c07e2bef 100644
--- a/src/metabase/models/common.clj
+++ b/src/metabase/models/common.clj
@@ -1,5 +1,6 @@
(ns metabase.models.common)
+;; TODO - why is this here? Doesn't seem like the right place for it
(def timezones
"The different timezones supported by Metabase.
Presented as options for the `report-timezone` Setting in the admin panel."
diff --git a/src/metabase/models/interface.clj b/src/metabase/models/interface.clj
index c51aa4cf1eb..3ed34892693 100644
--- a/src/metabase/models/interface.clj
+++ b/src/metabase/models/interface.clj
@@ -127,7 +127,7 @@
:out (comp encryption/maybe-decrypt u/jdbc-clob->str))
(defn decompress
- "Decompress COMPRESSED-BYTES."
+ "Decompress `compressed-bytes`."
[compressed-bytes]
(if (instance? Blob compressed-bytes)
(recur (.getBytes ^Blob compressed-bytes 0 (.length ^Blob compressed-bytes)))
diff --git a/src/metabase/query_processor/middleware/expand_macros.clj b/src/metabase/query_processor/middleware/expand_macros.clj
index a51212a3902..486e19a2fe7 100644
--- a/src/metabase/query_processor/middleware/expand_macros.clj
+++ b/src/metabase/query_processor/middleware/expand_macros.clj
@@ -13,13 +13,12 @@
[metabase.models
[metric :refer [Metric]]
[segment :refer [Segment]]]
- [metabase.query-processor.interface :as i]
[metabase.util :as u]
- [puppetlabs.i18n.core :refer [tru]]
+ [metabase.util.schema :as su]
+ [puppetlabs.i18n.core :refer [trs tru]]
[schema.core :as s]
[toucan.db :as db]))
-
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | SEGMENTS |
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -51,34 +50,67 @@
;; metrics won't be in a native query but they could be in source-query or aggregation clause
(mbql.u/match query [:metric (_ :guard (complement mbql.u/ga-id?))]))
-(defn- metric-clauses->id->definition [metric-clauses]
- (db/select-id->field :definition Metric, :id [:in (set (map second metric-clauses))]))
-
-(defn- add-metrics-filters [query metric-id->definition]
- (let [filters (filter identity (map :filter (vals metric-id->definition)))]
+(def ^:private MetricInfo
+ {:id su/IntGreaterThanZero
+ :name su/NonBlankString
+ :definition {:aggregation [(s/one mbql.s/Aggregation "aggregation clause")]
+ (s/optional-key :filter) (s/maybe mbql.s/Filter)
+ s/Keyword s/Any}})
+
+(def ^:private ^{:arglists '([metric-info])} metric-info-validation-errors (s/checker MetricInfo))
+
+(s/defn ^:private metric-clauses->id->info :- {su/IntGreaterThanZero MetricInfo}
+ [metric-clauses :- [mbql.s/metric]]
+ (when (seq metric-clauses)
+ (u/key-by :id (for [metric (db/select [Metric :id :name :definition] :id [:in (set (map second metric-clauses))])
+ :let [errors (u/prog1 (metric-info-validation-errors metric)
+ (when <>
+ (log/warn (trs "Invalid metric: {0} reason: {1}" metric <>))))]
+ :when (not errors)]
+ metric))))
+
+(defn- add-metrics-filters [query metric-id->info]
+ (let [filters (for [{{filter-clause :filter} :definition} (vals metric-id->info)
+ :when filter-clause]
+ filter-clause)]
(reduce mbql.u/add-filter-clause query filters)))
-(defn- replace-metrics-aggregations [query metric-id->definition]
- (mbql.u/replace-in query [:query]
- [:metric (metric-id :guard (complement mbql.u/ga-id?))]
- (or (first (:aggregation (metric-id->definition metric-id)))
- (throw (IllegalArgumentException.
- (str (tru "Metric {0} does not exist, or is invalid." metric-id)))))))
+(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))
+ aggregation
+ [:named aggregation 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))))))]
+ (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]
+
+ [:metric (metric-id :guard (complement mbql.u/ga-id?))]
+ (metric-info->ag-clause (metric metric-id) {:wrap-in-named? true}))))
(defn- add-metrics-clauses
"Add appropriate `filter` and `aggregation` clauses for a sequence of Metrics.
(add-metrics-clauses {:query {}} [[:metric 10]])
;; -> {:query {:aggregation [[:count]], :filter [:= [:field-id 10] 20]}}"
- [query metric-id->definition]
+ [query metric-id->info]
(-> query
- (add-metrics-filters metric-id->definition)
- (replace-metrics-aggregations metric-id->definition)))
+ (add-metrics-filters metric-id->info)
+ (replace-metrics-aggregations metric-id->info)))
(s/defn ^:private expand-metrics :- mbql.s/Query
[query :- mbql.s/Query]
(if-let [metrics (metrics query)]
- (add-metrics-clauses query (metric-clauses->id->definition metrics))
+ (add-metrics-clauses query (metric-clauses->id->info metrics))
query))
@@ -97,10 +129,7 @@
[{query-type :type, :as query}]
(if-not (= query-type :query)
query
- (u/prog1 (expand-metrics-and-segments query)
- (when (and (not i/*disable-qp-logging*)
- (not= <> query))
- (log/debug (u/format-color 'cyan "\n\nMACRO/SUBSTITUTED: %s\n%s" (u/emoji "😻") (u/pprint-to-str <>)))))))
+ (expand-metrics-and-segments query)))
(defn expand-macros
"Middleware that looks for `:metric` and `:segment` macros in an unexpanded MBQL query and substitute the macros for
diff --git a/test/expectation_options.clj b/test/expectation_options.clj
index 8364ad8da4b..0525a642dae 100644
--- a/test/expectation_options.clj
+++ b/test/expectation_options.clj
@@ -31,9 +31,9 @@
:actual-message (when-let [in-a (first (data/diff a e))]
(format "\nin actual, not expected:\n%s" (u/pprint-to-str 'red in-a)))
:raw [str-e str-a]
- :result ["\nexpected:\n"
+ :result [(format "\nexpected: %s\n" (class e))
(u/pprint-to-str 'green e)
- "\nwas:\n"
+ (format "\nwas: %s\n" (class a))
(u/pprint-to-str 'red a)]})
(defmethod expectations/compare-expr :expectations/maps [e a str-e str-a]
diff --git a/test/metabase/api/embed_test.clj b/test/metabase/api/embed_test.clj
index 1ee4e028201..23976b26b50 100644
--- a/test/metabase/api/embed_test.clj
+++ b/test/metabase/api/embed_test.clj
@@ -325,7 +325,8 @@
(tt/with-temp Card [card (card-with-date-field-filter)]
;; make sure the URL doesn't include /api/ at the beginning like it normally would
(binding [http/*url-prefix* (str/replace http/*url-prefix* #"/api/$" "/")]
- (http/client :get 200 (str "embed/question/" (card-token card) ".csv?date=Q1-2014"))))))
+ (tu/with-temporary-setting-values [site-url http/*url-prefix*]
+ (http/client :get 200 (str "embed/question/" (card-token card) ".csv?date=Q1-2014")))))))
;;; ---------------------------------------- GET /api/embed/dashboard/:token -----------------------------------------
diff --git a/test/metabase/api/public_test.clj b/test/metabase/api/public_test.clj
index 627487a0510..35497a3c59e 100644
--- a/test/metabase/api/public_test.clj
+++ b/test/metabase/api/public_test.clj
@@ -220,16 +220,11 @@
(tt/with-temp Card [{uuid :public_uuid} (card-with-date-field-filter)]
;; make sure the URL doesn't include /api/ at the beginning like it normally would
(binding [http/*url-prefix* (str/replace http/*url-prefix* #"/api/$" "/")]
- (try
+ (tu/with-temporary-setting-values [site-url http/*url-prefix*]
(http/client :get 200 (str "public/question/" uuid ".csv")
:parameters (json/encode [{:type :date/quarter-year
:target [:dimension [:template-tag :date]]
- :value "Q1-2014"}]))
- ;; this test is failing a lot recently with ConnectExceptions. This is here to debug it
- (catch java.net.ConnectException e
- (println "Connection to " http/*url-prefix* "public/question/" uuid ".csv refused.")
- (println (.getMessage e))
- (throw e)))))))
+ :value "Q1-2014"}])))))))
;; make sure we include all the relevant fields like `:insights`
(defn- card-with-trendline []
diff --git a/test/metabase/query_processor/middleware/expand_macros_test.clj b/test/metabase/query_processor/middleware/expand_macros_test.clj
index 4423dd365af..86937437b33 100644
--- a/test/metabase/query_processor/middleware/expand_macros_test.clj
+++ b/test/metabase/query_processor/middleware/expand_macros_test.clj
@@ -1,15 +1,15 @@
(ns metabase.query-processor.middleware.expand-macros-test
- (:require [expectations :refer :all]
+ (:require [expectations :refer [expect]]
[metabase
[query-processor :as qp]
- [query-processor-test :refer :all]
+ [query-processor-test :as qp.test]
[util :as u]]
[metabase.models
[database :refer [Database]]
[metric :refer [Metric]]
[segment :refer [Segment]]
[table :refer [Table]]]
- [metabase.query-processor.middleware.expand-macros :as expand-macros :refer :all]
+ [metabase.query-processor.middleware.expand-macros :as expand-macros]
[metabase.test.data :as data]
[metabase.test.data.datasets :as datasets]
[toucan.util.test :as tt]))
@@ -52,30 +52,10 @@
[:> [:field-id 4] 1]]]
:breakout [[:field-id 17]]}))))
-;; Does expansion work if :and isn't capitalized? (MBQL is case-insensitive!) (#5706, #5530)
-(expect
- (mbql-query
- {:filter [:and
- [:= [:field-id 5] "abc"]
- [:is-null [:field-id 7]]]
- :breakout [[:field-id 17]]})
- (tt/with-temp* [Database [{database-id :id}]
- Table [{table-id :id} {:db_id database-id}]
- Segment [{segment-1-id :id} {:table_id table-id
- :definition {:filter [:= [:field-id 5] "abc"]}}]
- Segment [{segment-2-id :id} {:table_id table-id
- :definition {:filter [:is-null [:field-id 7]]}}]]
- (#'expand-macros/expand-metrics-and-segments
- (mbql-query
- {:filter [:and
- [:segment segment-1-id]
- [:segment segment-2-id]]
- :breakout [[:field-id 17]]}))))
-
;; just a metric (w/out nested segments)
(expect
(mbql-query
- {:aggregation [[:count]]
+ {:aggregation [[:named [:count] "Toucans in the rainforest"]]
:filter [:and
[:> [:field-id 4] 1]
[:= [:field-id 5] "abc"]]
@@ -83,7 +63,8 @@
:order-by [[:asc [:field-id 1]]]})
(tt/with-temp* [Database [{database-id :id}]
Table [{table-id :id} {:db_id database-id}]
- Metric [{metric-1-id :id} {:table_id table-id
+ Metric [{metric-1-id :id} {:name "Toucans in the rainforest"
+ :table_id table-id
:definition {:aggregation [[:count]]
:filter [:and [:= [:field-id 5] "abc"]]}}]]
(#'expand-macros/expand-metrics-and-segments
@@ -97,13 +78,14 @@
(expect
(mbql-query
{:source-table 1000
- :aggregation [[:count]]
+ :aggregation [[:named [:count] "ABC Fields"]]
:filter [:= [:field-id 5] "abc"]
:breakout [[:field-id 17]]
:order-by [[:asc [:field-id 1]]]})
(tt/with-temp* [Database [{database-id :id}]
Table [{table-id :id} {:db_id database-id}]
- Metric [{metric-1-id :id} {:table_id table-id
+ Metric [{metric-1-id :id} {:name "ABC Fields"
+ :table_id table-id
:definition {:aggregation [[:count]]
:filter [:and [:= [:field-id 5] "abc"]]}}]]
(#'expand-macros/expand-metrics-and-segments
@@ -116,13 +98,14 @@
;; metric w/ no filter definition
(expect
(mbql-query
- {:aggregation [[:count]]
+ {:aggregation [[:named [:count] "My Metric"]]
:filter [:= [:field-id 5] "abc"]
:breakout [[:field-id 17]]
:order-by [[:asc [:field-id 1]]]})
(tt/with-temp* [Database [{database-id :id}]
Table [{table-id :id} {:db_id database-id}]
- Metric [{metric-1-id :id} {:table_id table-id
+ Metric [{metric-1-id :id} {:name "My Metric"
+ :table_id table-id
:definition {:aggregation [[:count]]}}]]
(#'expand-macros/expand-metrics-and-segments
(mbql-query
@@ -135,7 +118,7 @@
(expect
(mbql-query
{:source-table 1000
- :aggregation [[:sum [:field-id 18]]]
+ :aggregation [[:named [:sum [:field-id 18]] "My Metric"]]
:filter [:and
[:> [:field-id 4] 1]
[:is-null [:field-id 7]]
@@ -149,7 +132,8 @@
:definition {:filter [:and [:between [:field-id 9] 0 25]]}}]
Segment [{segment-2-id :id} {:table_id table-id
:definition {:filter [:and [:is-null [:field-id 7]]]}}]
- Metric [{metric-1-id :id} {:table_id table-id
+ Metric [{metric-1-id :id} {:name "My Metric"
+ :table_id table-id
:definition {:aggregation [[:sum [:field-id 18]]]
:filter [:and
[:= [:field-id 5] "abc"]
@@ -165,20 +149,21 @@
:order-by [[:asc [:field-id 1]]]}))))
;; Check that a metric w/ multiple aggregation syntax (nested vector) still works correctly
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expression-aggregations)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expression-aggregations)
[[2 118]
[3 39]
[4 24]]
(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]}}]
- (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 [[:field-id (data/id :venues :price)]]}})))))
+ (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)]]}})))))
;; make sure that we don't try to expand GA "metrics" (#6104)
(expect
@@ -208,11 +193,21 @@
;; make sure we can name a :metric (ick)
(expect
(mbql-query
- {:aggregation [[:named [:sum [:field-id 20]] "My Cool Metric"]]
+ {:aggregation [[:named [:sum [:field-id 20]] "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)] "My Cool Metric"]]
+ (mbql-query {:aggregation [[:named [:metric (u/get-id metric)] "Named Metric"]]
+ :breakout [[:field-id 10]]}))))
+
+;; a Metric whose :aggregation is already named should not get wrapped in a `:named` clause
+(expect
+ (mbql-query
+ {:aggregation [[:named [:sum [:field-id 20]] "My Cool Aggregation"]]
+ :breakout [[:field-id 10]]})
+ (tt/with-temp Metric [metric {:definition {:aggregation [[:named [:sum [:field-id 20]] "My Cool Aggregation"]]}}]
+ (#'expand-macros/expand-metrics-and-segments
+ (mbql-query {:aggregation [[:metric (u/get-id metric)]]
:breakout [[:field-id 10]]}))))
--
GitLab