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 

src/metabase/models/common.clj

 (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."

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)))

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

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]

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 -----------------------------------------

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 []

test/metabase/query_processor/middleware/expand_macros_test.clj

 (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]]}))))