MLv2 metadata calculation (#28921)

* MLv2 metadata overhaul, second try

* Stricter linting for MLv2

* Address PR feedback

* Revert fancy fn schema for now

* Remove unused import

* Test fixes 

* Oops fix bad :require form

* MLv2 mega metadata overhaul 2 of infinity

* Test fixes

* Cljc humanization

* Misc MLv2 improvements from #28921

* Fix indentation

* expression.non-integer-real

* Remove debugging stuff

* More cleanup

* Add method for `:advanced`

* non-integer-real

* Test fixes 

* Test fixes: ::ref/field shouldn't fail if checked against a non-sequential object

* MLv2 `define-mbql-clause` and `type-of` calculation

* Continued [ci skip]

* MLv2 overhaul :flex:

* Address PR feedback

* Merge type-of overhaul

* Address PR feedback

shared/src/metabase/shared/parameters/parameters.cljc

@@ -66,6 +66,7 @@
            (formatted-value :date/single end locale))
       "")))
 
+;;; TODO -- sorta duplicated with [[metabase.lib.metadata.calculate.display-name/interval-display-name]] but not exactly
 (defn- translated-interval
   [interval n]
   (case interval

shared/src/metabase/shared/util/i18n.clj

@@ -34,10 +34,22 @@
     :cljs
     `(js-i18n ~format-string ~@args)))
 
+(defmacro trun
+  "i18n a string with both singular and plural forms, using the current user's locale. The appropriate plural form will
+  be returned based on the value of `n`. `n` can be interpolated into the format strings using the `{0}`
+  syntax. (Other placeholders are not supported)."
+  [format-string format-string-pl n]
+  (macros/case
+    :clj
+    `(i18n/trun ~format-string ~format-string-pl ~n)
+
+    :cljs
+    `(js-i18n-n ~format-string ~format-string-pl ~n)))
+
 (defmacro trsn
   "i18n a string with both singular and plural forms, using the site's locale. The appropriate plural form will be
   returned based on the value of `n`. `n` can be interpolated into the format strings using the `{0}` syntax. (Other
-  placeholders are not supported). "
+  placeholders are not supported)."
   [format-string format-string-pl n]
   (macros/case
     :clj

src/metabase/lib/core.cljc

@@ -4,6 +4,7 @@
   (:refer-clojure :exclude [remove replace =])
   (:require
    [metabase.lib.dev :as lib.dev]
+   [metabase.lib.field :as lib.field]
    [metabase.lib.filter :as lib.filter]
    [metabase.lib.join :as lib.join]
    [metabase.lib.order-by :as lib.order-by]
@@ -12,6 +13,7 @@
    [metabase.shared.util.namespaces :as shared.ns]))
 
 (comment lib.dev/keep-me
+         lib.field/keep-me
          lib.filter/keep-me
          lib.join/keep-me
          lib.order-by/keep-me
@@ -22,6 +24,8 @@
  [lib.dev
   field
   query-for-table-name]
+ [lib.field
+  with-join-alias]
  [lib.filter
   =]
  [lib.join

src/metabase/lib/field.cljc

@@ -47,3 +47,11 @@
    (->field query -1 x))
   ([query stage-number x]
    (->field query stage-number x)))
+
+(defn with-join-alias
+  "Update a `field` so that it has `join-alias`."
+  [field-or-fn join-alias]
+  (if (fn? field-or-fn)
+    (fn [query stage-number]
+      (with-join-alias (field-or-fn query stage-number) join-alias))
+    (lib.options/update-options field-or-fn assoc :join-alias join-alias)))

src/metabase/lib/join.cljc

@@ -17,6 +17,10 @@
 ;; TODO -- should the default implementation call [[metabase.lib.query/query]]? That way if we implement a method to
 ;; create an MBQL query from a `Table`, then we'd also get [[join]] support for free?
 
+(defmethod ->join-clause :mbql/join
+  [_query _stage-number a-join-clause]
+  a-join-clause)
+
 (defmethod ->join-clause :mbql/query
   [_query _stage-number another-query]
   (-> {:lib/type :mbql/join
@@ -29,6 +33,14 @@
        :stages   [mbql-stage]}
       lib.options/ensure-uuid))
 
+(defmethod ->join-clause :metadata/table
+  [query stage-number table-metadata]
+  (->join-clause query
+                 stage-number
+                 {:lib/type     :mbql.stage/mbql
+                  :lib/options  {:lib/uuid (str (random-uuid))}
+                  :source-table (:id table-metadata)}))
+
 (defmethod ->join-clause :dispatch-type/fn
   [query stage-number f]
   (->join-clause query stage-number (f query stage-number)))
@@ -83,6 +95,9 @@
   `condition` is currently required, but in the future I think we should make this smarter and try to infer a sensible
   default condition for things, e.g. when joining a Table B from Table A, if there is an FK relationship between A and
   B, join via that relationship. Not yet implemented!"
+  ([query a-join-clause]
+   (join query -1 a-join-clause (:condition a-join-clause)))
+
   ([query x condition]
    (join query -1 x condition))
 

src/metabase/lib/metadata/calculate.cljc

+(ns metabase.lib.metadata.calculate
+  (:refer-clojure :exclude [ref])
+  (:require
+   [metabase.lib.dispatch :as lib.dispatch]
+   [metabase.lib.metadata :as lib.metadata]
+   [metabase.lib.metadata.calculate.names :as calculate.names]
+   [metabase.lib.metadata.calculate.resolve :as calculate.resolve]
+   [metabase.lib.schema :as lib.schema]
+   [metabase.lib.schema.aggregation :as lib.schema.aggregation]
+   [metabase.lib.schema.common :as lib.schema.common]
+   [metabase.lib.schema.expression :as lib.schema.expresssion]
+   [metabase.lib.schema.id :as lib.schema.id]
+   [metabase.lib.schema.join :as lib.schema.join]
+   [metabase.lib.util :as lib.util]
+   [metabase.mbql.util :as mbql.u]
+   [metabase.util :as u]
+   [metabase.util.malli :as mu]))
+
+(declare stage-metadata)
+
+(mu/defn ^:private add-parent-column-metadata
+  "If this is a nested column, add metadata about the parent column."
+  [query    :- ::lib.schema/query
+   metadata :- lib.metadata/ColumnMetadata]
+  (let [parent-metadata     (lib.metadata/field query (:parent_id metadata))
+        {parent-name :name} (cond->> parent-metadata
+                              (:parent_id parent-metadata) (add-parent-column-metadata query))]
+    (update metadata :name (fn [field-name]
+                             (str parent-name \. field-name)))))
+
+(defmulti ^:private metadata-for-ref
+  {:arglists '([query stage-number ref])}
+  (fn [_query _stage-number ref]
+    (lib.dispatch/dispatch-value ref)))
+
+(defmethod metadata-for-ref :field
+  [query stage-number [_tag opts :as field-ref]]
+  (let [metadata (merge
+                  {:lib/type  :metadata/field
+                   :field_ref field-ref}
+                  (calculate.resolve/field-metadata query stage-number field-ref)
+                  {:display_name (calculate.names/display-name query stage-number field-ref)}
+                  (when (:base-type opts)
+                    {:base_type (:base-type opts)}))]
+    (cond->> metadata
+      (:parent_id metadata) (add-parent-column-metadata query))))
+
+(defmethod metadata-for-ref :expression
+  [query stage-number [_expression opts expression-name, :as expression-ref]]
+  (let [expression (calculate.resolve/expression query stage-number expression-name)]
+    {:lib/type     :metadata/field
+     :field_ref    expression-ref
+     :name         expression-name
+     :display_name (calculate.names/display-name query stage-number expression-ref)
+     :base_type    (or (:base-type opts)
+                       (lib.schema.expresssion/type-of expression))}))
+
+(mu/defn ^:private metadata-for-aggregation :- lib.metadata/ColumnMetadata
+  [query        :- ::lib.schema/query
+   stage-number :- :int
+   aggregation  :- ::lib.schema.aggregation/aggregation
+   index        :- ::lib.schema.common/int-greater-than-or-equal-to-zero]
+  (let [display-name (calculate.names/display-name query stage-number aggregation)]
+    {:lib/type     :metadata/field
+     :source       :aggregation
+     :base_type    (lib.schema.expresssion/type-of aggregation)
+     :field_ref    [:aggregation {:lib/uuid (str (random-uuid))} index]
+     :name         (calculate.names/column-name query stage-number aggregation)
+     :display_name display-name}))
+
+(defmethod metadata-for-ref :aggregation
+  [query stage-number [_ag opts index, :as aggregation-ref]]
+  (let [aggregation (calculate.resolve/aggregation query stage-number index)]
+    (merge
+     (metadata-for-aggregation query stage-number aggregation index)
+     {:field_ref aggregation-ref}
+     (when (:base-type opts)
+       {:base_type (:base-type opts)}))))
+
+(mu/defn ^:private breakout-columns :- [:maybe [:sequential lib.metadata/ColumnMetadata]]
+  [query        :- ::lib.schema/query
+   stage-number :- :int]
+  (when-let [{breakouts :breakout} (lib.util/query-stage query stage-number)]
+    (mapv (fn [ref]
+            (assoc (metadata-for-ref query stage-number ref) :source :breakout))
+          breakouts)))
+
+(mu/defn ^:private aggregation-columns :- [:maybe [:sequential lib.metadata/ColumnMetadata]]
+  [query        :- ::lib.schema/query
+   stage-number :- :int]
+  (when-let [{aggregations :aggregation} (lib.util/query-stage query stage-number)]
+    (map-indexed (fn [i aggregation]
+                   (metadata-for-aggregation query stage-number aggregation i))
+                 aggregations)))
+
+(mu/defn ^:private fields-columns :- [:maybe [:sequential lib.metadata/ColumnMetadata]]
+  [query        :- ::lib.schema/query
+   stage-number :- :int]
+  (when-let [{fields :fields} (lib.util/query-stage query stage-number)]
+    (mapv (fn [ref]
+            (assoc (metadata-for-ref query stage-number ref) :source :fields))
+          fields)))
+
+(defn- remove-hidden-default-fields
+  "Remove Fields that shouldn't be visible from the default Fields for a source Table.
+  See [[metabase.query-processor.middleware.add-implicit-clauses/table->sorted-fields*]]."
+  [field-metadatas]
+  (remove (fn [{visibility-type :visibility_type, active? :active, :as _field-metadata}]
+            (or (false? active?)
+                (#{:sensitive :retired} (some-> visibility-type keyword))))
+          field-metadatas))
+
+(defn- sort-default-fields
+  "Sort default Fields for a source Table. See [[metabase.models.table/field-order-rule]]."
+  [field-metadatas]
+  (sort-by (fn [{field-name :name, :keys [position], :as _field-metadata}]
+             [(or position 0) (u/lower-case-en (or field-name ""))])
+           field-metadatas))
+
+(mu/defn ^:private source-table-default-fields :- [:maybe [:sequential lib.metadata/ColumnMetadata]]
+  "Determine the Fields we'd normally return for a source Table.
+  See [[metabase.query-processor.middleware.add-implicit-clauses/add-implicit-fields]]."
+  [query    :- ::lib.schema/query
+   table-id :- ::lib.schema.id/table]
+  (when-let [field-metadatas (lib.metadata/fields query table-id)]
+    (->> field-metadatas
+         remove-hidden-default-fields
+         sort-default-fields)))
+
+(mu/defn ^:private default-join-alias :- ::lib.schema.common/non-blank-string
+  "Generate an alias for a join that doesn't already have one."
+  [query        :- ::lib.schema/query
+   stage-number :- :int
+   join         :- ::lib.schema.join/join]
+  (calculate.names/display-name query stage-number join))
+
+(def ^:private JoinsWithAliases
+  "Schema for a sequence of joins that all have aliases."
+  [:and
+   ::lib.schema.join/joins
+   [:sequential
+    [:map
+     [:alias ::lib.schema.common/non-blank-string]]]])
+
+(mu/defn ^:private ensure-all-joins-have-aliases :- JoinsWithAliases
+  "Make sure all the joins in a query have an `:alias` if they don't already have one."
+  [query        :- ::lib.schema/query
+   stage-number :- :int
+   joins        :- ::lib.schema.join/joins]
+  (let [unique-name-generator (mbql.u/unique-name-generator)]
+    (mapv (fn [join]
+            (cond-> join
+              (not (:alias join)) (assoc :alias (unique-name-generator (default-join-alias query stage-number join)))))
+          joins)))
+
+(mu/defn ^:private column-from-join-fields :- lib.metadata/ColumnMetadata
+  "For a column that comes from a join `:fields` list, add or update metadata as needed, e.g. include join name in the
+  display name."
+  [query           :- ::lib.schema/query
+   stage-number    :- :int
+   column-metadata :- lib.metadata/ColumnMetadata
+   join-alias      :- ::lib.schema.common/non-blank-string]
+  (let [[ref-type options arg] (:field_ref column-metadata)
+        ref-with-join-alias    [ref-type (assoc options :join-alias join-alias) arg]
+        column-metadata        (assoc column-metadata :source_alias join-alias)]
+    (assoc column-metadata
+           :field_ref    ref-with-join-alias
+           :display_name (calculate.names/display-name query stage-number column-metadata))))
+
+(mu/defn ^:private default-columns-added-by-join :- [:sequential lib.metadata/ColumnMetadata]
+  [query                                                                     :- ::lib.schema/query
+   stage-number                                                              :- :int
+   {:keys [fields stages], join-alias :alias, :or {fields :none}, :as _join} :- ::lib.schema.join/join]
+  (when-not (= fields :none)
+    (let [field-metadatas (if (= fields :all)
+                            (stage-metadata (assoc query :stages stages))
+                            (for [field-ref fields]
+                              ;; resolve the field ref in the context of the join. Not sure if this is right.
+                              (calculate.resolve/field-metadata query stage-number field-ref)))]
+      (mapv (fn [field-metadata]
+              (column-from-join-fields query stage-number field-metadata join-alias))
+            field-metadatas))))
+
+(mu/defn ^:private default-columns-added-by-joins :- [:maybe [:sequential lib.metadata/ColumnMetadata]]
+  [query        :- ::lib.schema/query
+   stage-number :- :int]
+  (when-let [joins (not-empty (:joins (lib.util/query-stage query stage-number)))]
+    (not-empty
+     (into []
+           (mapcat (partial default-columns-added-by-join query stage-number))
+           (ensure-all-joins-have-aliases query stage-number joins)))))
+
+(mu/defn ^:private default-columns :- [:sequential {:min 1} lib.metadata/ColumnMetadata]
+  "Calculate the columns to return if `:aggregations`/`:breakout`/`:fields` are unspecified.
+
+  Formula for the so-called 'default' columns is
+
+  1a. Columns returned by the previous stage of the query (if there is one), OR
+
+  1b. Default 'visible' Fields for our `:source-table`, OR
+
+  1c. `:lib/stage-metadata` if this is a `:mbql.stage/native` stage
+
+  PLUS
+
+  2. Columns added by joins at this stage"
+  [query        :- ::lib.schema/query
+   stage-number :- :int]
+  (concat
+   ;; 1: columns from the previous stage, source table or query
+   (if-let [previous-stage-number (lib.util/previous-stage-number query stage-number)]
+     ;; 1a. columns returned by previous stage
+     (stage-metadata query previous-stage-number)
+     ;; 1b or 1c
+     (let [{:keys [source-table], :as this-stage} (lib.util/query-stage query stage-number)]
+       (if (integer? source-table)
+         ;; 1b: default visible Fields for the source Table
+         (source-table-default-fields query source-table)
+         ;; 1c: `:lib/stage-metadata` for the native query
+         (:columns (:lib/stage-metadata this-stage)))))
+   ;; 2: columns added by joins at this stage
+   (default-columns-added-by-joins query stage-number)))
+
+(mu/defn stage-metadata :- [:and
+                            [:sequential {:min 1} lib.metadata/ColumnMetadata]
+                            [:fn
+                             ;; should be dev-facing only, so don't need to i18n
+                             {:error/message "Column :names must be distinct!"}
+                             (fn [columns]
+                               (apply distinct? (map :name columns)))]]
+  "Return results metadata about the expected columns in an MBQL query stage. If the query has
+  aggregations/breakouts/fields, then return THOSE. Otherwise return the defaults based on the source Table or
+  previous stage + joins."
+  ([query :- ::lib.schema/query]
+   (stage-metadata query -1))
+
+  ([query        :- ::lib.schema/query
+    stage-number :- :int]
+   (or
+    ;; stage metadata is already present: return it as-is
+    (when-let [metadata (:lib/stage-metadata (lib.util/query-stage query stage-number))]
+      (:columns metadata))
+    ;; otherwise recursively calculate the metadata for the previous stages and add it to them, we'll need it for
+    ;; calculations for this stage and we don't have to calculate it more than once...
+    (let [query (let [previous-stage-number (lib.util/previous-stage-number query stage-number)]
+                  (cond-> query
+                    previous-stage-number
+                    (lib.util/update-query-stage previous-stage-number assoc :lib/stage-metadata {:lib/type :metadata/results
+                                                                                                  :columns  (stage-metadata query previous-stage-number)})))]
+      ;; ... then calculate metadata for this stage
+      (or
+       (not-empty (into []
+                        cat
+                        [(breakout-columns query stage-number)
+                         (aggregation-columns query stage-number)
+                         (fields-columns query stage-number)]))
+       (default-columns query stage-number))))))

src/metabase/lib/metadata/calculate/names.cljc

+(ns metabase.lib.metadata.calculate.names
+  "Logic for calculating human-friendly display names for things."
+  (:require
+   [clojure.string :as str]
+   [metabase.lib.dispatch :as lib.dispatch]
+   [metabase.lib.metadata :as lib.metadata]
+   [metabase.lib.metadata.calculate.resolve :as calculate.resolve]
+   [metabase.lib.schema :as lib.schema]
+   [metabase.lib.schema.common :as lib.schema.common]
+   [metabase.lib.schema.temporal-bucketing :as lib.schema.temporal-bucketing]
+   [metabase.shared.util.i18n :as i18n]
+   [metabase.util :as u]
+   [metabase.util.humanization :as u.humanization]
+   [metabase.util.malli :as mu]
+   #?@(:cljs ([goog.string :refer [format]]
+              [goog.string.format :as gstring.format]))))
+
+;; The formatting functionality is only loaded if you depend on goog.string.format.
+#?(:cljs (comment gstring.format/keep-me))
+
+(defmulti ^:private display-name*
+  "Impl for [[display-name]]."
+  {:arglists '([query stage-number x])}
+  (fn [_query _stage-number x]
+    (lib.dispatch/dispatch-value x)))
+
+(defn- options-when-mbql-clause
+  "If this is an MBQL clause, return its options map, if it has one."
+  [x]
+  (when (and (vector? x)
+             (keyword? (first x))
+             (map? (second x)))
+    (second x)))
+
+(mu/defn display-name :- ::lib.schema.common/non-blank-string
+  "Calculate a nice human-friendly display name for something."
+  [query                                    :- ::lib.schema/query
+   stage-number                             :- :int
+   x]
+  (or
+   ;; if this is an MBQL clause with `:display-name` in the options map, then use that rather than calculating a name.
+   (:display-name (options-when-mbql-clause x))
+   (try
+     (display-name* query stage-number x)
+     (catch #?(:clj Throwable :cljs js/Error) e
+       (throw (ex-info (i18n/tru "Error calculating display name for {0}: {1}" (pr-str x) (ex-message e))
+                       {:x            x
+                        :query        query
+                        :stage-number stage-number}
+                       e))))))
+
+(defmulti ^:private column-name*
+  "Impl for [[column-name]]."
+  {:arglists '([query stage-number x])}
+  (fn [_query _stage-number x]
+    (lib.dispatch/dispatch-value x)))
+
+(mu/defn column-name :- ::lib.schema.common/non-blank-string
+  "Calculate a database-friendly name to use for an expression."
+  [query                                    :- ::lib.schema/query
+   stage-number                             :- :int
+   x]
+  (or
+   ;; if this is an MBQL clause with `:name` in the options map, then use that rather than calculating a name.
+   (:name (options-when-mbql-clause x))
+   (try
+     (column-name* query stage-number x)
+     (catch #?(:clj Throwable :cljs js/Error) e
+       (throw (ex-info (i18n/tru "Error calculating column name for {0}: {1}" (pr-str x) (ex-message e))
+                       {:x            x
+                        :query        query
+                        :stage-number stage-number}
+                       e))))))
+
+(defn- slugify [s]
+  (-> s
+      (str/replace #"\+" (i18n/tru "plus"))
+      (str/replace #"\-" (i18n/tru "minus"))
+      (str/replace #"[\(\)]" "")
+      u/slugify))
+
+;;; default impl just takes the display name and slugifies it.
+(defmethod column-name* :default
+  [query stage-number x]
+  (slugify (display-name query stage-number x)))
+
+(defmethod display-name* :mbql/join
+  [query _stage-number {[first-stage] :stages, :as _join}]
+  (if-let [source-table (:source-table first-stage)]
+    (if (integer? source-table)
+      (:display_name (lib.metadata/table query source-table))
+      ;; handle card__<id> source tables.
+      (let [[_ card-id-str] (re-matches #"^card__(\d+)$" source-table)]
+        (i18n/tru "Saved Question #{0}" card-id-str)))
+    (i18n/tru "Native Query")))
+
+(defmethod display-name* :metadata/field
+  [query stage-number {field-display-name :display_name, field-name :name, join-alias :source_alias, :as _field-metadata}]
+  (let [field-display-name (or field-display-name
+                               (u.humanization/name->human-readable-name :simple field-name))
+        join-display-name  (when join-alias
+                             (let [join (calculate.resolve/join query stage-number join-alias)]
+                              (display-name query stage-number join)))]
+    (if join-display-name
+      (str join-display-name " → " field-display-name)
+      field-display-name)))
+
+(defmethod display-name* :field
+  [query stage-number [_field {:keys [join-alias], :as _opts} _id-or-name, :as field-clause]]
+  (let [field-metadata (cond-> (calculate.resolve/field-metadata query stage-number field-clause)
+                         join-alias (assoc :source_alias join-alias))]
+    (display-name query stage-number field-metadata)))
+
+(defmethod display-name* :expression
+  [_query _stage-number [_expression _opts expression-name]]
+  expression-name)
+
+(defmethod column-name* :expression
+  [_query _stage-number [_expression _opts expression-name]]
+  expression-name)
+
+(def ^:private ^:dynamic *nested*
+  "Whether the display name we are generated is recursively nested inside another display name. For infix math operators
+  we'll wrap the results in parentheses to make the display name more obvious."
+  false)
+
+(defn- wrap-str-in-parens-if-nested [s]
+  (if *nested*
+    (str \( s \))
+    s))
+
+(defn- infix-display-name*
+  "Generate a infix-style display name for an arithmetic expression like `:+`, e.g. `x + y`."
+  [query stage-number operator args]
+  (wrap-str-in-parens-if-nested
+   (binding [*nested* true]
+     (str/join (str \space (name operator) \space)
+               (map (partial display-name* query stage-number)
+                    args)))))
+
+(defmethod display-name* :+
+  [query stage-number [_plus _opts & args]]
+  (infix-display-name* query stage-number "+" args))
+
+(defmethod display-name* :-
+  [query stage-number [_minute _opts & args]]
+  (infix-display-name* query stage-number "-" args))
+
+(defmethod display-name* :/
+  [query stage-number [_divide _opts & args]]
+  (infix-display-name* query stage-number "÷" args))
+
+(defmethod display-name* :*
+  [query stage-number [_multiply _opts & args]]
+  (infix-display-name* query stage-number "×" args))
+
+(defn- infix-column-name*
+  [query stage-number operator-str args]
+  (str/join (str \_ operator-str \_)
+            (map (partial column-name* query stage-number)
+                 args)))
+
+(defmethod column-name* :+
+  [query stage-number [_plus _opts & args]]
+  (infix-column-name* query stage-number "plus" args))
+
+(defmethod column-name* :-
+  [query stage-number [_minute _opts & args]]
+  (infix-column-name* query stage-number "minus" args))
+
+(defmethod column-name* :/
+  [query stage-number [_divide _opts & args]]
+  (infix-column-name* query stage-number "divided_by" args))
+
+(defmethod column-name* :*
+  [query stage-number [_multiply _opts & args]]
+  (infix-column-name* query stage-number "times" args))
+
+(defmethod display-name* :count
+  [query stage-number [_count _opts x]]
+  ;; x is optional.
+  (if x
+    (i18n/tru "Count of {0}" (display-name query stage-number x))
+    (i18n/tru "Count")))
+
+(defmethod column-name* :count
+  [query stage-number [_count _opts x]]
+  (if x
+    (str "count_" (column-name query stage-number x))
+    "count"))
+
+(defmethod display-name* :case
+  [_query _stage-number _case]
+  (i18n/tru "Case"))
+
+(defmethod column-name* :case
+  [_query _stage-number _case]
+  "case")
+
+(defmethod display-name* :distinct
+  [query stage-number [_distinct _opts x]]
+  (i18n/tru "Distinct values of {0}"  (display-name query stage-number x)))
+
+(defmethod column-name* :distinct
+  [query stage-number [_distinct _opts x]]
+  (str "distinct_" (column-name query stage-number x)))
+
+(defmethod display-name* :avg
+  [query stage-number [_avg _opts x]]
+  (i18n/tru "Average of {0}" (display-name query stage-number x)))
+
+(defmethod column-name* :avg
+  [query stage-number [_avg _opts x]]
+  (str "avg_" (column-name query stage-number x)))
+
+(defmethod display-name* :cum-count
+  [query stage-number [_cum-count _opts x]]
+  (i18n/tru "Cumulative count of {0}" (display-name query stage-number x)))
+
+(defmethod column-name* :cum-count
+  [query stage-number [_avg _opts x]]
+  (str "cum_count_" (column-name query stage-number x)))
+
+(defmethod display-name* :sum
+  [query stage-number [_sum _opts x]]
+  (i18n/tru "Sum of {0}" (display-name query stage-number x)))
+
+(defmethod column-name* :sum
+  [query stage-number [_sum _opts x]]
+  (str "sum_" (column-name query stage-number x)))
+
+(defmethod display-name* :cum-sum
+  [query stage-number [_cum-sum _opts x]]
+  (i18n/tru "Cumulative sum of {0}" (display-name query stage-number x)))
+
+(defmethod column-name* :cum-sum
+  [query stage-number [_avg _opts x]]
+  (str "cum_sum_" (column-name query stage-number x)))
+
+(defmethod display-name* :stddev
+  [query stage-number [_stddev _opts x]]
+  (i18n/tru "Standard deviation of {0}" (display-name query stage-number x)))
+
+(defmethod column-name* :stddev
+  [query stage-number [_avg _opts x]]
+  (str "std_dev_" (column-name query stage-number x)))
+
+(defmethod display-name* :min
+  [query stage-number [_min _opts x]]
+  (i18n/tru "Min of {0}" (display-name query stage-number x)))
+
+(defmethod column-name* :min
+  [query stage-number [_min _opts x]]
+  (str "min_" (column-name query stage-number x)))
+
+(defmethod display-name* :max
+  [query stage-number [_max _opts x]]
+  (i18n/tru "Max of {0}" (display-name query stage-number x)))
+
+(defmethod column-name* :max
+  [query stage-number [_max _opts x]]
+  (str "max_" (column-name query stage-number x)))
+
+(defmethod display-name* :var
+  [query stage-number [_var _opts x]]
+  (i18n/tru "Variance of {0}" (display-name query stage-number x)))
+
+(defmethod column-name* :var
+  [query stage-number [_var _opts x]]
+  (str "var_" (column-name query stage-number x)))
+
+(defmethod display-name* :median
+  [query stage-number [_median _opts x]]
+  (i18n/tru "Median of {0}" (display-name query stage-number x)))
+
+(defmethod column-name* :median
+  [query stage-number [_median _opts x]]
+  (str "median_" (column-name query stage-number x)))
+
+(defmethod display-name* :percentile
+  [query stage-number [_percentile _opts x p]]
+  (i18n/tru "{0}th percentile of {1}" p (display-name query stage-number x)))
+
+(defmethod column-name* :percentile
+  [query stage-number [_percentile _opts x p]]
+  (format "p%d_%s" p (column-name query stage-number x)))
+
+;;; we don't currently have sophisticated logic for generating nice display names for filter clauses
+
+(defmethod display-name* :sum-where
+  [query stage-number [_sum-where _opts x _pred]]
+  (i18n/tru "Sum of {0} matching condition" (display-name query stage-number x)))
+
+(defmethod column-name* :sum-where
+  [query stage-number [_sum-where _opts x]]
+  (str "sum_where_" (column-name query stage-number x)))
+
+(defmethod display-name* :share
+  [_query _stage-number _share]
+  (i18n/tru "Share of rows matching condition"))
+
+(defmethod column-name* :share
+  [_query _stage-number _share]
+  "share")
+
+(defmethod display-name* :count-where
+  [_query _stage-number _count-where]
+  (i18n/tru "Count of rows matching condition"))
+
+(defmethod column-name* :count-where
+  [_query _stage-number _count-where]
+  "count-where")
+
+(mu/defn ^:private interval-display-name  :- ::lib.schema.common/non-blank-string
+  "e.g. something like \"- 2 days\""
+  [amount :- :int
+   unit   :- ::lib.schema.temporal-bucketing/unit.date-time.interval]
+  ;; TODO -- sorta duplicated with [[metabase.shared.parameters.parameters/translated-interval]], but not exactly
+  (let [unit-str (case unit
+                   :millisecond (i18n/trun "millisecond" "milliseconds" (abs amount))
+                   :second      (i18n/trun "second"      "seconds"      (abs amount))
+                   :minute      (i18n/trun "minute"      "minutes"      (abs amount))
+                   :hour        (i18n/trun "hour"        "hours"        (abs amount))
+                   :day         (i18n/trun "day"         "days"         (abs amount))
+                   :week        (i18n/trun "week"        "weeks"        (abs amount))
+                   :month       (i18n/trun "month"       "months"       (abs amount))
+                   :quarter     (i18n/trun "quarter"     "quarters"     (abs amount))
+                   :year        (i18n/trun "year"        "years"        (abs amount)))]
+    (wrap-str-in-parens-if-nested
+     (if (pos? amount)
+       (format "+ %d %s" amount       unit-str)
+       (format "- %d %s" (abs amount) unit-str)))))
+
+(defmethod display-name* :datetime-add
+  [query stage-number [_datetime-add _opts x amount unit]]
+  (str (display-name query stage-number x)
+       \space
+       (interval-display-name amount unit)))
+
+;;; for now we'll just pretend `:coalesce` isn't a present and just use the display name for the expr it wraps.
+(defmethod display-name* :coalesce
+  [query stage-number [_coalesce _opts expr _null-expr]]
+  (display-name query stage-number expr))
+
+(defmethod column-name* :coalesce
+  [query stage-number [_coalesce _opts expr _null-expr]]
+  (column-name query stage-number expr))
+
+(defmethod display-name* :dispatch-type/number
+  [_query _stage-number n]
+  (str n))
+
+(defmethod display-name* :dispatch-type/string
+  [_query _stage-number s]
+  (str \" s \"))

src/metabase/lib/metadata/calculate/resolve.cljc

+(ns metabase.lib.metadata.calculate.resolve
+  "Logic for resolving references."
+  (:refer-clojure :exclude [ref])
+  (:require
+   [medley.core :as m]
+   [metabase.lib.metadata :as lib.metadata]
+   [metabase.lib.schema :as lib.schema]
+   [metabase.lib.schema.aggregation :as lib.schema.aggregation]
+   [metabase.lib.schema.common :as lib.schema.common]
+   [metabase.lib.schema.expression :as lib.schema.expression]
+   [metabase.lib.schema.id :as lib.schema.id]
+   [metabase.lib.schema.join :as lib.schema.join]
+   [metabase.lib.schema.ref]
+   [metabase.lib.util :as lib.util]
+   [metabase.shared.util.i18n :as i18n]
+   [metabase.util.malli :as mu]))
+
+(comment metabase.lib.schema.ref/keep-me)
+
+(mu/defn ^:private resolve-field-id :- lib.metadata/ColumnMetadata
+  "Integer Field ID: get metadata from the metadata provider. This is probably not 100% the correct thing to do if
+  this isn't the first stage of the query, but we can fix that behavior in a follow-on"
+  [query          :- ::lib.schema/query
+   _stage-number  :- :int
+   field-id       :- ::lib.schema.id/field]
+  (lib.metadata/field query field-id))
+
+(mu/defn ^:private resolve-field-name :- lib.metadata/ColumnMetadata
+  "String column name: get metadata from the previous stage, if it exists, otherwise if this is the first stage and we
+  have a native query or a Saved Question source query or whatever get it from our results metadata."
+  [query         :- ::lib.schema/query
+   stage-number  :- :int
+   column-name   :- ::lib.schema.common/non-blank-string]
+  (or (some (fn [column]
+              (when (= (:name column) column-name)
+                column))
+            (if-let [previous-stage-number (lib.util/previous-stage-number query stage-number)]
+              (let [previous-stage (lib.util/query-stage query previous-stage-number)]
+                (:lib/stage-metadata previous-stage))
+              (get-in (lib.util/query-stage query stage-number) [:lib/stage-metadata :columns])))
+      (throw (ex-info (i18n/tru "Invalid :field clause: column {0} does not exist" (pr-str column-name))
+                      {:name         column-name
+                       :query        query
+                       :stage-number stage-number}))))
+
+(mu/defn field-metadata :- lib.metadata/ColumnMetadata
+  "Resolve metadata for a `:field` ref."
+  [query                                        :- ::lib.schema/query
+   stage-number                                 :- :int
+   [_field _opts id-or-name, :as _field-clause] :- :mbql.clause/field]
+  (if (integer? id-or-name)
+    (resolve-field-id query stage-number id-or-name)
+    (resolve-field-name query stage-number id-or-name)))
+
+(mu/defn join :- ::lib.schema.join/join
+  "Resolve a join with a specific `join-alias`."
+  [query        :- ::lib.schema/query
+   stage-number :- :int
+   join-alias   :- ::lib.schema.common/non-blank-string]
+  (or (m/find-first #(= (:alias %) join-alias)
+                    (:joins (lib.util/query-stage query stage-number)))
+      (throw (ex-info (i18n/tru "No join named {0}" (pr-str join-alias))
+                      {:join-alias   join-alias
+                       :query        query
+                       :stage-number stage-number}))))
+
+(mu/defn aggregation :- ::lib.schema.aggregation/aggregation
+  "Resolve an aggregation with a specific `index`."
+  [query        :- ::lib.schema/query
+   stage-number :- :int
+   index        :- ::lib.schema.common/int-greater-than-or-equal-to-zero]
+  (let [{aggregations :aggregation} (lib.util/query-stage query stage-number)]
+    (when (<= (count aggregations) index)
+      (throw (ex-info (i18n/tru "No aggregation at index {0}" index)
+                      {:index        index
+                       :query        query
+                       :stage-number stage-number})))
+    (nth aggregations index)))
+
+(mu/defn expression :- ::lib.schema.expression/expression
+  "Find the expression with `expression-name` in a given stage of a `query`, or throw an Exception if it doesn't
+  exist."
+  [query           :- ::lib.schema/query
+   stage-number    :- :int
+   expression-name :- ::lib.schema.common/non-blank-string]
+  (let [stage (lib.util/query-stage query stage-number)]
+    (or (get-in stage [:expressions expression-name])
+        (throw (ex-info (i18n/tru "No expression named {0}" (pr-str expression-name))
+                        {:expression-name expression-name
+                         :query           query
+                         :stage-number    stage-number})))))

src/metabase/lib/schema.cljc

@@ -11,6 +11,8 @@
    [metabase.lib.schema.common :as common]
    [metabase.lib.schema.expression :as expression]
    [metabase.lib.schema.expression.arithmetic]
+   [metabase.lib.schema.expression.conditional]
+   [metabase.lib.schema.expression.temporal]
    [metabase.lib.schema.filter]
    [metabase.lib.schema.id :as id]
    [metabase.lib.schema.join :as join]
@@ -20,6 +22,8 @@
    [metabase.util.malli.registry :as mr]))
 
 (comment metabase.lib.schema.expression.arithmetic/keep-me
+         metabase.lib.schema.expression.conditional/keep-me
+         metabase.lib.schema.expression.temporal/keep-me
          metabase.lib.schema.filter/keep-me
          metabase.lib.schema.literal/keep-me)
 

src/metabase/lib/schema/aggregation.cljc

 (ns metabase.lib.schema.aggregation
   (:require
-   [metabase.lib.schema.common :as common]
    [metabase.lib.schema.expression :as expression]
+   [metabase.lib.schema.mbql-clause :as mbql-clause]
    [metabase.util.malli.registry :as mr]))
 
-(mr/def ::sum
-  [:tuple
-   [:= :sum]
-   ::common/options
-   [:ref ::expression/number]])
+;; count has an optional expression arg
+(mbql-clause/define-catn-mbql-clause :count
+  [:expression [:? [:ref ::expression/number]]])
+
+(defmethod expression/type-of* :count
+  [[_tag _opts expr]]
+  (if-not expr
+    :type/Integer
+    (expression/type-of expr)))
+
+(mbql-clause/define-tuple-mbql-clause :sum
+  [:ref ::expression/number])
+
+(defmethod expression/type-of* :sum
+  [[_tag _opts expr]]
+  (expression/type-of expr))
+
+(mbql-clause/define-tuple-mbql-clause :avg :- :type/Float
+  [:ref ::expression/number])
 
 (mr/def ::aggregation
+  ;; placeholder!
   [:or
-   ::sum
-   ;;; placeholder!
+   :mbql.clause/sum
    any?])
 
 (mr/def ::aggregations

src/metabase/lib/schema/expression/arithmetic.cljc

 (ns metabase.lib.schema.expression.arithmetic
   "Arithmetic expressions like `:+`."
   (:require
+   [malli.core :as mc]
+   [metabase.lib.schema.common :as common]
    [metabase.lib.schema.expression :as expression]
    [metabase.lib.schema.mbql-clause :as mbql-clause]
-   [metabase.types :as types]))
+   [metabase.lib.schema.temporal-bucketing :as temporal-bucketing]
+   [metabase.types :as types]
+   [metabase.util.malli.registry :as mr]))
+
+(mbql-clause/define-tuple-mbql-clause :interval
+  :int
+  ::temporal-bucketing/unit.date-time.interval)
+
+(defmethod expression/type-of* :interval
+  [[_tag _opts expr _n _unit]]
+  (expression/type-of expr))
+
+(defn- valid-interval-for-type? [[_tag _opts _n unit :as _interval] expr-type]
+  (let [unit-schema (cond
+                      (isa? expr-type :type/Date)     ::temporal-bucketing/unit.date.interval
+                      (isa? expr-type :type/Time)     ::temporal-bucketing/unit.time.interval
+                      (isa? expr-type :type/DateTime) ::temporal-bucketing/unit.date-time.interval)]
+    (if unit-schema
+      (mc/validate unit-schema unit)
+      true)))
+
+(mr/def ::args.temporal
+  [:and
+   [:catn
+    [:expr      [:schema [:ref ::expression/temporal]]]
+    [:intervals [:+ :mbql.clause/interval]]]
+   [:fn
+    {:error/message "Temporal arithmetic expression with valid interval units for the expression type"}
+    (fn [[expr & intervals]]
+      (let [expr-type (expression/type-of expr)]
+        (every? #(valid-interval-for-type? % expr-type) intervals)))]])
+
+(mr/def ::args.numbers
+  [:repeat {:min 2} [:schema [:ref ::expression/number]]])
+
+(defn- plus-minus-schema [tag]
+  [:or
+   [:and
+    [:cat
+     [:= tag]
+     [:schema [:ref ::common/options]]
+     [:schema [:ref ::expression/temporal]]
+     [:repeat {:min 1} [:schema [:ref :mbql.clause/interval]]]]
+    [:fn
+     {:error/message "Temporal arithmetic expression with valid interval units for the expression type"}
+     (fn [[_tag _opts expr & intervals]]
+       (let [expr-type (expression/type-of expr)]
+         (every? #(valid-interval-for-type? % expr-type) intervals)))]]
+   [:cat
+    [:= tag]
+    [:schema [:ref ::common/options]]
+    [:repeat {:min 2} [:schema [:ref ::expression/number]]]]])
+
+(mbql-clause/define-mbql-clause :+
+  (plus-minus-schema :+))
+
+;;; TODO -- should `:-` support just a single arg (for numbers)? What about `:+`?
+(mbql-clause/define-mbql-clause :-
+  (plus-minus-schema :-))
 
 (mbql-clause/define-catn-mbql-clause :*
-  [:args [:repeat {:min 2} [:schema [:ref ::expression/number]]]])
+  [:args ::args.numbers])
 
-(defmethod expression/type-of* :*
-  [[_tag _opts & args]]
+;;; we always do non-integer real division even if all the expressions are integers, e.g.
+;;;
+;;;    [:/ <int-field>  2] => my_int_field / 2.0
+;;;
+;;; so the results are 0.5 as opposed to 0. This is what people expect division to do
+(mbql-clause/define-catn-mbql-clause :/ :- :type/Float
+  [:args ::args.numbers])
+
+(defn- type-of-arithmetic-args [args]
+  ;; Okay to use reduce without an init value here since we know we have >= 2 args
   #_{:clj-kondo/ignore [:reduce-without-init]}
   (reduce types/most-specific-common-ancestor (map expression/type-of args)))
+
+(defmethod expression/type-of* :+
+  [[_tag _opts & args]]
+  (type-of-arithmetic-args args))
+
+(defmethod expression/type-of* :-
+  [[_tag _opts & args]]
+  (type-of-arithmetic-args args))
+
+(defmethod expression/type-of* :*
+  [[_tag _opts & args]]
+  (type-of-arithmetic-args args))

src/metabase/lib/schema/expression/conditional.cljc

+(ns metabase.lib.schema.expression.conditional
+  "Conditional expressions like `:case` and `:coalesce`."
+  (:require
+   [clojure.set :as set]
+   [metabase.lib.schema.expression :as expression]
+   [metabase.lib.schema.mbql-clause :as mbql-clause]
+   [metabase.types :as types]
+   [metabase.util.malli.registry :as mr]))
+
+;;; the logic for calculating the return type of a `:case` or similar statement is not optimal nor perfect. But it
+;;; should be ok for now and errors on the side of being permissive. See this Slack thread for more info:
+;;; https://metaboat.slack.com/archives/C04DN5VRQM6/p1678325996901389
+(defn- best-return-type
+  "For expressions like `:case` and `:coalesce` that can return different possible expressions, determine the best
+  return type given all of the various options."
+  [x y]
+  (cond
+    (nil? x)
+    y
+
+    ;; if both types are keywords return their most-specific ancestor.
+    (and (keyword? x)
+         (keyword? y))
+    (types/most-specific-common-ancestor x y)
+
+    ;; if one type is a specific type but the other is an ambiguous union of possible types, return the specific
+    ;; type. A case can't possibly have multiple different return types, so if one expression has an unambiguous
+    ;; type then the whole thing has to have a compatible type.
+    (keyword? x)
+    x
+
+    (keyword? y)
+    y
+
+    ;; if both types are ambiguous unions of possible types then return the intersection of the two. But if the
+    ;; intersection is empty, return the union of everything instead. I don't really want to go down a rabbit
+    ;; hole of trying to find the intersection between the most-specific common ancestors
+    :else
+    (or (when-let [intersection (not-empty (set/intersection x y))]
+          (if (= (count intersection) 1)
+            (first intersection)
+            intersection))
+        (set/union x y))))
+
+;;; believe it or not, a `:case` clause really has the syntax [:case {} [[pred1 expr1] [pred2 expr2] ...]]
+(mr/def ::case-subclause
+  [:tuple
+   {:error/message "Valid :case [pred expr] pair"}
+   #_pred [:ref ::expression/boolean]
+   #_expr [:ref ::expression/expression]])
+
+
+(mbql-clause/define-tuple-mbql-clause :case
+  ;; TODO -- we should further constrain this so all of the exprs are of the same type
+  [:sequential {:min 1} [:ref ::case-subclause]])
+
+(defmethod expression/type-of* :case
+  [[_tag _opts pred-expr-pairs]]
+  (reduce
+   (fn [best-guess [_pred expr]]
+     (let [expr-type (expression/type-of expr)]
+       (best-return-type best-guess expr-type)))
+   nil
+   pred-expr-pairs))
+
+;;; TODO -- add constraint that these types have to be compatible
+(mbql-clause/define-tuple-mbql-clause :coalesce
+  #_expr       [:ref :metabase.lib.schema.expression/expression]
+  #_null-value [:ref :metabase.lib.schema.expression/expression])
+
+(defmethod expression/type-of* :coalesce
+  [[_tag _opts expr null-value]]
+  (best-return-type (expression/type-of expr) (expression/type-of null-value)))

src/metabase/lib/schema/expression/temporal.cljc

+(ns metabase.lib.schema.expression.temporal
+  (:require
+   [metabase.lib.schema.expression :as expression]
+   [metabase.lib.schema.mbql-clause :as mbql-clause]
+   [metabase.lib.schema.temporal-bucketing :as temporal-bucketing]))
+
+;;; TODO -- we should constrain this so that you can only use a Date unit if expr is a date, etc.
+(mbql-clause/define-tuple-mbql-clause :datetime-add
+  #_expr   [:ref ::expression/temporal]
+  #_amount :int
+  #_unit   [:ref ::temporal-bucketing/unit.date-time.interval])
+
+(defmethod expression/type-of* :datetime-add
+  [[_tag _opts expr _amount _unit]]
+  (expression/type-of expr))

src/metabase/lib/schema/filter.cljc

@@ -2,12 +2,9 @@
   "Schemas for the various types of filter clauses that you'd pass to `:filter` or use inside something else that takes
   a boolean expression."
   (:require
-   [clojure.set :as set]
    [metabase.lib.schema.common :as common]
    [metabase.lib.schema.expression :as expression]
-   [metabase.lib.schema.mbql-clause :as mbql-clause]
-   [metabase.types :as types]
-   [metabase.util.malli.registry :as mr]))
+   [metabase.lib.schema.mbql-clause :as mbql-clause]))
 
 (doseq [op [:and :or]]
   (mbql-clause/define-catn-mbql-clause op :- :type/Boolean
@@ -102,53 +99,3 @@
    [:= :segment]
    ::common/options
    [:or ::common/int-greater-than-zero ::common/non-blank-string]])
-
-;;; believe it or not, a `:case` clause really has the syntax [:case {} [[pred1 expr1] [pred2 expr2] ...]]
-(mr/def ::case-subclause
-  [:tuple
-   {:error/message "Valid :case [pred expr] pair"}
-   #_pred [:ref ::expression/boolean]
-   #_expr [:ref ::expression/expression]])
-
-;;; TODO -- this is not really a filter clause and doesn't belong in here. But where does it belong?
-(mbql-clause/define-tuple-mbql-clause :case
-  ;; TODO -- we should further constrain this so all of the exprs are of the same type
-  [:sequential {:min 1} [:ref ::case-subclause]])
-
-;;; the logic for calculating the return type of a `:case` statement is not optimal nor perfect. But it should be ok
-;;; for now and errors on the side of being permissive. See this Slack thread for more info:
-;;; https://metaboat.slack.com/archives/C04DN5VRQM6/p1678325996901389
-(defmethod expression/type-of* :case
-  [[_tag _opts pred-expr-pairs]]
-  (reduce
-   (fn [best-guess [_pred expr]]
-     (let [return-type (expression/type-of expr)]
-       (cond
-         (nil? best-guess)
-         return-type
-
-         ;; if both types are keywords return their most-specific ancestor.
-         (and (keyword? best-guess)
-              (keyword? return-type))
-         (types/most-specific-common-ancestor best-guess return-type)
-
-         ;; if one type is a specific type but the other is an ambiguous union of possible types, return the specific
-         ;; type. A case can't possibly have multiple different return types, so if one expression has an unambiguous
-         ;; type then the whole thing has to have a compatible type.
-         (keyword? best-guess)
-         best-guess
-
-         (keyword? return-type)
-         return-type
-
-         ;; if both types are ambiguous unions of possible types then return the intersection of the two. But if the
-         ;; intersection is empty, return the union of everything instead. I don't really want to go down a rabbit
-         ;; hole of trying to find the intersection between the most-specific common ancestors
-         :else
-         (or (when-let [intersection (not-empty (set/intersection best-guess return-type))]
-               (if (= (count intersection) 1)
-                 (first intersection)
-                 intersection))
-             (set/union best-guess return-type)))))
-   nil
-   pred-expr-pairs))

src/metabase/lib/schema/mbql_clause.cljc

@@ -108,7 +108,7 @@
    (into [:catn
           {:error/message (str "Valid " tag " clause")}
           [:tag [:= tag]]
-          [:options ::common/options]]
+          [:options [:schema [:ref ::common/options]]]]
          args)])
 
 (defn tuple-clause-schema
@@ -119,7 +119,7 @@
   (into [:tuple
          {:error/message (str "Valid " tag " clause")}
          [:= tag]
-         ::common/options]
+         [:ref ::common/options]]
         args))
 
 ;;;; Even more convenient functions!

src/metabase/lib/schema/ref.cljc

@@ -64,8 +64,9 @@
 (mr/def ::aggregation-options
   [:merge
    ::common/options
-   [:name {:optional true} ::common/non-blank-string]
-   [:display-name {:optional true}] ::common/non-blank-string])
+   [:map
+    [:name {:optional true} ::common/non-blank-string]
+    [:display-name {:optional true} ::common/non-blank-string]]])
 
 (mbql-clause/define-mbql-clause :aggregation
   [:tuple

src/metabase/lib/util.cljc

@@ -82,7 +82,7 @@
 (mu/defn ^:private non-negative-stage-index :- ::lib.schema.common/int-greater-than-or-equal-to-zero
   "If `stage-number` index is a negative number e.g. `-1` convert it to a positive index so we can use `nth` on
   `stages`. `-1` = the last stage, `-2` = the penultimate stage, etc."
-  [stages       :- [:sequential ::lib.schema/stage]
+  [stages       :- [:sequential [:ref ::lib.schema/stage]]
    stage-number :- :int]
   (let [stage-number' (if (neg? stage-number)
                         (+ (count stages) stage-number)

src/metabase/models/humanization.clj

@@ -10,9 +10,8 @@
   There used to also be `:advanced`, which was the default until enough customers
   complained that we first fixed it and then the fix wasn't good enough so we removed it."
   (:require
-   [clojure.string :as str]
    [metabase.models.setting :as setting :refer [defsetting]]
-   [metabase.util :as u]
+   [metabase.util.humanization :as u.humanization]
    [metabase.util.i18n :refer [deferred-tru trs tru]]
    [metabase.util.log :as log]
    [schema.core :as s]
@@ -20,55 +19,21 @@
 
 (declare humanization-strategy)
 
-(defmulti ^String name->human-readable-name
+(defn name->human-readable-name
   "Convert a name, such as `num_toucans`, to a human-readable name, such as `Num Toucans`. With one arg, this uses the
   strategy defined by the Setting `humanization-strategy`. With two args, you may specify a custom strategy (intended
   mainly for the internal implementation):
 
-     (humanization-strategy! :simple)
-     (name->human-readable-name \"cool_toucans\")                         ;-> \"Cool Toucans\"
-     ;; this is the same as:
-     (name->human-readable-name (humanization-strategy) \"cool_toucans\") ;-> \"Cool Toucans\"
-     ;; specifiy a different strategy:
-     (name->human-readable-name :none \"cool_toucans\")                   ;-> \"cool_toucans\""
-  {:arglists '([s] [strategy s])}
-  (fn
-    ([_] (keyword (humanization-strategy)))
-    ([strategy _] (keyword strategy))))
-
-(def ^:private ^:const acronyms
-  #{"id" "url" "ip" "uid" "uuid" "guid"})
-
-(defn- capitalize-word [word]
-  (if (contains? acronyms (u/lower-case-en word))
-    (u/upper-case-en word)
-    ;; We are assuming that ALL_UPPER_CASE means we should be Title Casing
-    (if (= word (u/upper-case-en word))
-      (str/capitalize word)
-      (str (str/capitalize (subs word 0 1)) (subs word 1)))))
-
-;; simple replaces hyphens and underscores with spaces and capitalizes
-(defmethod name->human-readable-name :simple
-  ([s] (name->human-readable-name :simple s))
-  ([_ ^String s]
-   ;; explode on hyphens, underscores, and spaces
-   (when (seq s)
-     (let [humanized (str/join " " (for [part  (str/split s #"[-_\s]+")
-                                         :when (not (str/blank? part))]
-                                     (capitalize-word part)))]
-       (if (str/blank? humanized)
-         s
-         humanized)))))
-
-;; actual advanced method has been excised. this one just calls out to simple
-(defmethod name->human-readable-name :advanced
-  ([s] (name->human-readable-name :simple s))
-  ([_ ^String s] (name->human-readable-name :simple s)))
-
-;; :none is just an identity implementation
-(defmethod name->human-readable-name :none
-  ([s]   s)
-  ([_ s] s))
+    (humanization-strategy! :simple)
+    (name->human-readable-name \"cool_toucans\")                         ;-> \"Cool Toucans\"
+    ;; this is the same as:
+    (name->human-readable-name (humanization-strategy) \"cool_toucans\") ;-> \"Cool Toucans\"
+    ;; specifiy a different strategy:
+    (name->human-readable-name :none \"cool_toucans\")                   ;-> \"cool_toucans\""
+  ([s]
+   (name->human-readable-name (humanization-strategy) s))
+  ([strategy s]
+   (u.humanization/name->human-readable-name strategy s)))
 
 (defn- re-humanize-names!
   "Update all non-custom display names of all instances of `model` (e.g. Table or Field)."
@@ -95,7 +60,7 @@
 (defn- set-humanization-strategy! [new-value]
   (let [new-strategy (keyword (or new-value :simple))]
     ;; check to make sure `new-strategy` is a valid strategy, or throw an Exception it is it not.
-    (when-not (get-method name->human-readable-name new-strategy)
+    (when-not (get-method u.humanization/name->human-readable-name new-strategy)
       (throw (IllegalArgumentException.
                (tru "Invalid humanization strategy ''{0}''. Valid strategies are: {1}"
                     new-strategy (keys (methods name->human-readable-name))))))
@@ -115,4 +80,11 @@
   :type       :keyword
   :default    :simple
   :visibility :settings-manager
+  :getter     (fn []
+                (let [strategy (setting/get-value-of-type :keyword :humanization-strategy)]
+                  ;; actual advanced method has been excised. Use `:simple` instead if someone had specified
+                  ;; `:advanced`.
+                  (if (= strategy :advanced)
+                    :simple
+                    strategy)))
   :setter     set-humanization-strategy!)

src/metabase/util/humanization.cljc

+(ns metabase.util.humanization
+  (:require
+   [clojure.string :as str]
+   [metabase.util :as u]))
+
+(defmulti name->human-readable-name
+  "Convert a name, such as `num_toucans`, to a human-readable name, such as `Num Toucans`.
+
+    (name->human-readable-name :simple \"cool_toucans\") ;-> \"Cool Toucans\"
+
+    ;; specifiy a different strategy:
+    (name->human-readable-name :none \"cool_toucans\") ;-> \"cool_toucans\""
+  {:arglists '([strategy s])}
+  (fn [strategy _s]
+    (keyword strategy)))
+
+(def ^:private ^:const acronyms
+  #{"id" "url" "ip" "uid" "uuid" "guid"})
+
+(defn- capitalize-word [word]
+  (if (contains? acronyms (u/lower-case-en word))
+    (u/upper-case-en word)
+    ;; We are assuming that ALL_UPPER_CASE means we should be Title Casing
+    (if (= word (u/upper-case-en word))
+      (str/capitalize word)
+      (str (str/capitalize (subs word 0 1)) (subs word 1)))))
+
+;; simple replaces hyphens and underscores with spaces and capitalizes
+(defmethod name->human-readable-name :simple
+  [_strategy s]
+  ;; explode on hyphens, underscores, and spaces
+  (when (seq s)
+    (let [humanized (str/join " " (for [part  (str/split s #"[-_\s]+")
+                                        :when (not (str/blank? part))]
+                                    (capitalize-word part)))]
+      (if (str/blank? humanized)
+        s
+        humanized))))
+
+;;; `:none` is just an identity implementation
+(defmethod name->human-readable-name :none
+  [_strategy s]
+  s)
+
+;;; `:advanced` doesn't exist anymore, it used to be super fancy and do neat things. On the off chance someone still
+;;; tries to use it, just do the same thing `:simple` does.
+(defmethod name->human-readable-name :advanced
+  [strategy s]
+  ((get-method name->human-readable-name :simple) strategy s))

test/metabase/lib/metadata/calculate/names_test.cljc

+(ns metabase.lib.metadata.calculate.names-test
+  (:require
+   [clojure.test :refer [are deftest is testing]]
+   [metabase.lib.core :as lib]
+   [metabase.lib.metadata.calculate.names :as calculate.names]
+   [metabase.lib.test-metadata :as meta]))
+
+(def ^:private venues-query
+  (lib/query meta/metadata-provider (meta/table-metadata :venues)))
+
+(defn- field-clause
+  ([table field]
+   (field-clause table field nil))
+  ([table field options]
+   [:field (merge {:lib/uuid (str (random-uuid))} options) (meta/id table field)]))
+
+(deftest ^:parallel display-name-from-name-test
+  (testing "Use the 'simple humanization' logic to calculate a display name for a Field that doesn't have one (e.g. from results metadata)"
+    (is (= "Venue ID"
+           (calculate.names/display-name venues-query -1 {:lib/type :metadata/field
+                                                          :name     "venue_id"})))))
+
+(defn- aggregation-display-name [aggregation-clause]
+  (calculate.names/display-name venues-query -1 aggregation-clause))
+
+(defn- aggregation-column-name [aggregation-clause]
+  (calculate.names/column-name venues-query -1 aggregation-clause))
+
+(deftest ^:parallel aggregation-names-test
+  (are [aggregation-clause expected] (= expected
+                                        {:column-name  (aggregation-column-name aggregation-clause)
+                                         :display-name (aggregation-display-name aggregation-clause)})
+    [:count {}]
+    {:column-name "count", :display-name "Count"}
+
+    [:distinct {} (field-clause :venues :id)]
+    {:column-name "distinct_id", :display-name "Distinct values of ID"}
+
+    [:sum {} (field-clause :venues :id)]
+    {:column-name "sum_id", :display-name "Sum of ID"}
+
+    [:+ {} [:count {}] 1]
+    {:column-name "count_plus_1", :display-name "Count + 1"}
+
+    [:+
+     {}
+     [:min {} (field-clause :venues :id)]
+     [:* {} 2 [:avg {} (field-clause :venues :price)]]]
+    {:column-name  "min_id_plus_2_times_avg_price"
+     :display-name "Min of ID + (2 × Average of Price)"}
+
+    [:+
+     {}
+     [:min {} (field-clause :venues :id)]
+     [:*
+      {}
+      2
+      [:avg {} (field-clause :venues :price)]
+      3
+      [:- {} [:max {} (field-clause :venues :category-id)] 4]]]
+    {:column-name  "min_id_plus_2_times_avg_price_times_3_times_max_category_id_minus_4"
+     :display-name "Min of ID + (2 × Average of Price × 3 × (Max of Category ID - 4))"}
+
+    ;; user-specified names
+    [:+
+     {:name "generated_name", :display-name "User-specified Name"}
+     [:min {} (field-clause :venues :id)]
+     [:* {} 2 [:avg {} (field-clause :venues :price)]]]
+    {:column-name "generated_name", :display-name "User-specified Name"}
+
+    [:+
+     {:name "generated_name"}
+     [:min {} (field-clause :venues :id)]
+     [:* {} 2 [:avg {} (field-clause :venues :price)]]]
+    {:column-name "generated_name", :display-name "Min of ID + (2 × Average of Price)"}
+
+    [:+
+     {:display-name "User-specified Name"}
+     [:min {} (field-clause :venues :id)]
+     [:* {} 2 [:avg {} (field-clause :venues :price)]]]
+    {:column-name  "min_id_plus_2_times_avg_price"
+     :display-name "User-specified Name"}))
+
+(deftest ^:parallel date-interval-test
+  (let [clause [:datetime-add
+                {}
+                (field-clause :checkins :date {:base-type :type/Date})
+                -1
+                :day]]
+    (is (= "date_minus_1_day"
+           (calculate.names/column-name venues-query -1 clause)))
+    (is (= "Date - 1 day"
+           (calculate.names/display-name venues-query -1 clause)))))
+
+(deftest ^:parallel expression-reference-test
+  (let [query (assoc-in venues-query
+                        [:stages 0 :expressions "double-price"]
+                        [:*
+                         {:lib/uuid (str (random-uuid))}
+                         (field-clause :venues :price {:base-type :type/Integer})
+                         2])
+        expr  [:sum
+               {:lib/uuid (str (random-uuid))}
+               [:expression {:lib/uuid (str (random-uuid))} "double-price"]]]
+    (is (= "Sum of double-price"
+           (calculate.names/display-name query -1 expr)))
+    (is (= "sum_double-price"
+           (calculate.names/column-name query -1 expr)))))
+
+(deftest ^:parallel coalesce-test
+  (let [clause [:coalesce {} (field-clause :venues :name) "<Venue>"]]
+    (is (= "name"
+           (calculate.names/column-name venues-query -1 clause)))
+    (is (= "Name"
+           (calculate.names/display-name venues-query -1 clause)))))