Merge pull request #8744 from metabase/make-sure-expressions-have-names

Make sure expression aggregations come back with name

src/metabase/mbql/predicates.clj

+(ns metabase.mbql.predicates
+  "Predicate functions for checking whether something is a valid instance of a given MBQL clause."
+  (:require [metabase.mbql.schema :as mbql.s]
+            [schema.core :as s]))
+
+;; This namespace only covers a few things, please add more stuff here as we write the functions so we can use them
+;; elsewhere
+
+(def ^{:arglists '([unit])} TimeUnit?
+  "Is `unit` a datetime bucketing unit referring only to time, such as `hour` or `minute`?"
+  (complement (s/checker mbql.s/TimeUnit)))
+
+(def ^{:arglists '([unit])} DatetimeFieldUnit?
+  "Is `unit` a valid datetime bucketing unit?"
+  (complement (s/checker mbql.s/DatetimeFieldUnit)))
+
+(def ^{:arglists '([ag-clause])} Aggregation?
+  "Is this a valid Aggregation clause?"
+  (complement (s/checker mbql.s/Aggregation)))

src/metabase/query_processor/interface.clj

@@ -16,7 +16,6 @@
 
 (def ^:dynamic *driver*
   "The driver that will be used to run the query we are currently parsing.
-   Used by `assert-driver-supports` and other places.
    Always bound when running queries the normal way, e.g. via `metabase.driver/process-query`.
    Not neccesarily bound when using various functions like `fk->` in the REPL."
   nil)

src/metabase/query_processor/middleware/annotate.clj

@@ -2,8 +2,11 @@
   "Middleware for annotating (adding type information to) the results of a query and sorting the columns in the
   results."
   (:require [clojure.string :as str]
-            [metabase.driver :as driver]
+            [metabase
+             [driver :as driver]
+             [util :as u]]
             [metabase.mbql
+             [predicates :as mbql.preds]
              [schema :as mbql.s]
              [util :as mbql.u]]
             [metabase.models.humanization :as humanization]
@@ -13,8 +16,7 @@
             [metabase.util
              [i18n :refer [tru]]
              [schema :as su]]
-            [schema.core :as s]
-            [metabase.util :as u]))
+            [schema.core :as s]))
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                      Adding :cols info for native queries                                      |
@@ -92,9 +94,25 @@
    :/ "div"
    :* "mul"})
 
-;; TODO - I think this might be an appropriate thing to move to either `mbql.u` or somewhere else more general we can
-;; have the function take an ag clause and optional custom-name-formatting function so it doesn't need to worry about
-;; `*driver*
+(declare aggregation-name)
+
+(defn- expression-ag-arg->name
+  "Generate an appropriate name for an `arg` in an expression aggregation."
+  [arg]
+  (mbql.u/match-one arg
+    ;; if the arg itself is a nested expression, recursively find a name for it, and wrap in parens
+    [(_ :guard #{:+ :- :/ :*}) & _]
+    (str "(" (aggregation-name &match) ")")
+
+    ;; if the arg is another aggregation, recurse to get its name. (Only aggregations, nested expressions, or numbers
+    ;; are allowed as args to expression aggregations; thus anything that's an MBQL clause, but not a nested
+    ;; expression, is a ag clause.)
+    [(_ :guard keyword?) & _]
+    (aggregation-name &match)
+
+    ;; otherwise for things like numbers just use that directly
+    _ &match))
+
 (s/defn aggregation-name :- su/NonBlankString
   "Return an appropriate field *and* display name for an `:aggregation` subclause (an aggregation or
   expression). Takes an options map as schema won't support passing keypairs directly as a varargs. `{:top-level?
@@ -115,19 +133,7 @@
            (str (arithmetic-op->text operator)
                 "__"))
          (str/join (str " " (name operator) " ")
-                   ;; for each arg...
-                   (for [arg args]
-                     (mbql.u/match-one arg
-                       ;; if the arg itself is a nested expression, recursively find a name for it, and wrap in parens
-                       [(_ :guard #{:+ :- :/ :*}) & _]
-                       (str "(" (aggregation-name &match) ")")
-
-                       ;; if the arg is another aggregation, recurse to get its name
-                       [(_ :guard keyword?) & _]
-                       (aggregation-name &match)
-
-                       ;; otherwise for things like numbers just use that directly
-                       _ &match))))
+                   (map expression-ag-arg->name args)))
 
     ;; for unnamed normal aggregations, the column alias is always the same as the ag type except for `:distinct` with
     ;; is called `:count` (WHY?)
@@ -160,7 +166,8 @@
             :special_type :type/Number}
            (ag->name-info &match))
 
-    ;; get info from a Field if we can
+    ;; get info from a Field if we can (theses Fields are matched when ag clauses recursively call
+    ;; `col-info-for-ag-clause`, and this info is added into the results)
     [(_ :guard #{:field-id :field-literal :fk-> :datetime-field :expression :binning-strategy}) & _]
     (select-keys (col-info-for-field-clause &match) [:base_type :special_type :settings])
 
@@ -169,8 +176,10 @@
     ;; we'll want to introduce logic that associates a return type with a given expression. But this will work
     ;; for the purposes of a patch release.
     [(_ :guard #{:expression :+ :- :/ :*}) & _]
-    {:base_type    :type/Float
-     :special_type :type/Number}
+    (merge {:base_type    :type/Float
+            :special_type :type/Number}
+           (when (mbql.preds/Aggregation? &match)
+             (ag->name-info &match)))
 
     ;; get name/display-name of this ag
     [(_ :guard keyword?) arg]

src/metabase/query_processor/middleware/wrap_value_literals.clj

 (ns metabase.query-processor.middleware.wrap-value-literals
   (:require [metabase.driver :as driver]
             [metabase.mbql
+             [predicates :as mbql.preds]
              [schema :as mbql.s]
              [util :as mbql.u]]
             [metabase.models.field :refer [Field]]
             [metabase.query-processor.store :as qp.store]
-            [metabase.util.date :as du]
-            [schema.core :as s])
+            [metabase.util.date :as du])
   (:import java.util.TimeZone))
 
 ;;; --------------------------------------------------- Type Info ----------------------------------------------------
@@ -60,7 +60,7 @@
   [:absolute-datetime this (get info :unit :default)])
 
 (defn- maybe-parse-as-time [datetime-str unit]
-  (when-not (s/check mbql.s/TimeUnit unit)
+  (when (mbql.preds/TimeUnit? unit)
     (du/str->time datetime-str (when-let [report-timezone (driver/report-timezone)]
                                  (TimeZone/getTimeZone ^String report-timezone)))))
 

src/metabase/sync/analyze/query_results.clj

@@ -3,7 +3,7 @@
   results. The current focus of this namespace is around column metadata from the results of a query. Going forward
   this is likely to extend beyond just metadata about columns but also about the query results as a whole and over
   time."
-  (:require [metabase.mbql.schema :as mbql.s]
+  (:require [metabase.mbql.predicates :as mbql.preds]
             [metabase.sync.analyze.classifiers.name :as classify-name]
             [metabase.sync.analyze.fingerprint
              [fingerprinters :as f]
@@ -17,7 +17,7 @@
 (def ^:private DateTimeUnitKeywordOrString
   "Schema for a valid datetime unit string like \"default\" or \"minute-of-hour\"."
   (s/constrained su/KeywordOrString
-                 #(not (s/check mbql.s/DatetimeFieldUnit (keyword %)))
+                 #(mbql.preds/DatetimeFieldUnit? (keyword %))
                  "Valid field datetime unit keyword or string"))
 
 (def ^:private ResultColumnMetadata

test/metabase/query_processor/middleware/annotate_test.clj

@@ -5,7 +5,8 @@
              [interface :as i]
              [store :as qp.store]]
             [metabase.query-processor.middleware.annotate :as annotate]
-            [metabase.test.data :as data])
+            [metabase.test.data :as data]
+            [metabase.query-processor.interface :as qp.i])
   (:import metabase.driver.h2.H2Driver))
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
@@ -149,8 +150,32 @@
                        [:max [:field-id 3]]
                        4]]]))
 
+(expect
+  "x"
+  (aggregation-name [:named [:+ [:min [:field-id 1]] [:* 2 [:avg [:field-id 2]]]] "x"]))
+
 (expect
   "My Cool Aggregation"
   (aggregation-name [:named [:avg [:field-id 2]] "My Cool Aggregation"]))
 
-;; TODO - more tests for info added for aggregations
+;; make sure custom aggregation names get included in the col info
+(defn- col-info-for-aggregation-clause [clause]
+  (binding [qp.i/*driver* (metabase.driver.h2.H2Driver.)]
+    (#'annotate/col-info-for-aggregation-clause clause)))
+
+(expect
+  {:base_type    :type/Float
+   :special_type :type/Number
+   :name         "count / 2"
+   :display_name "count / 2"}
+  (col-info-for-aggregation-clause [:/ [:count] 2]))
+
+(expect
+  {:base_type    :type/Float
+   :special_type :type/Number
+   :name         "sum"
+   :display_name "sum"}
+  (qp.store/with-store
+    (data/$ids venues
+      (qp.store/store-field! (Field $price))
+      (col-info-for-aggregation-clause [:sum [:+ [:field-id $price] 1]]))))

test/metabase/test/data.clj

@@ -119,8 +119,8 @@
 
     $$table -> (id :venues)"
   {:style/indent 1}
-  [table-name body & {:keys [wrap-field-ids?], :or {wrap-field-ids? false}}]
-  ($->id (keyword table-name) body, :wrap-field-ids? wrap-field-ids?))
+  [table-name & body]
+  ($->id (keyword table-name) `(do ~@body) :wrap-field-ids? false))
 
 
 (defn wrap-inner-mbql-query
@@ -147,7 +147,7 @@
   [table & [query]]
   `(wrap-inner-mbql-query
      ~(merge `{:source-table (id ~(keyword table))}
-             ($->id (keyword table) query))))
+             ($->id table query))))
 
 (defmacro run-mbql-query
   "Like `mbql-query`, but runs the query as well."