Support Legacy Metrics in MLv1 (#31885)

* Extra Metric tests; include `:description` in the Metric `display-info`

* Add `available-metrics` function & TS wrappers

* Support passing a Metric to `lib/aggregate`

* Include `:selected` information in Metric display info

* Fix indentation

* Revert changes to arithmetic type-of calculation

* Test fixes 

* Address PR feedback

* Address PR feedback

* Update docstring

.clj-kondo/config.edn

@@ -3,11 +3,11 @@
  {:unresolved-symbol
   {:exclude
    [instaparse.core/transform
-    (cljs.test/is [=?])
+    (cljs.test/is [=? malli=])
     (clojure.core.logic/fresh)
     (clojure.core.logic/matcha)
     (clojure.core.logic/run)
-    (clojure.test/is [query= sql= =?])
+    (clojure.test/is [query= sql= =? malli=])
     (clojure.tools.macro/macrolet)
     (metabase.async.streaming-response/streaming-response)
     (metabase.driver.druid.query-processor-test/druid-query-returning-rows)

frontend/src/metabase-lib/metrics.ts

+import * as ML from "cljs/metabase.lib.js";
+
+import type { MetricMetadata, Query } from "./types";
+
+export function availableMetrics(query: Query): MetricMetadata[] {
+  return ML.available_metrics(query);
+}

frontend/src/metabase-lib/types.ts

@@ -14,6 +14,9 @@ export type TableMetadata = unknown & { _opaque: typeof TableMetadata };
 declare const CardMetadata: unique symbol;
 export type CardMetadata = unknown & { _opaque: typeof CardMetadata };
 
+declare const MetricMetadata: unique symbol;
+export type MetricMetadata = unknown & { _opaque: typeof MetricMetadata };
+
 export type Limit = number | null;
 
 declare const BreakoutClause: unique symbol;

frontend/src/metabase-lib/v2.ts

@@ -12,6 +12,7 @@ export * from "./limit";
 export * from "./order_by";
 export * from "./filter";
 export * from "./join";
+export * from "./metrics";
 export * from "./query";
 export * from "./temporal_bucket";
 export * from "./types";

src/metabase/api/geojson.clj

@@ -107,6 +107,8 @@
                (setting/set-value-of-type! :json :custom-geojson new-value)))
   :visibility :public)
 
+(def ^:private connection-timeout-ms 8000)
+
 (defn- read-url-and-respond
   "Reads the provided URL and responds with the contents as a stream."
   [url respond]
@@ -114,8 +116,8 @@
                                        (io/reader resource)
                                        (:body (http/get url {:as                 :reader
                                                              :redirect-strategy  :none
-                                                             :socket-timeout     8000
-                                                             :connection-timeout 8000})))
+                                                             :socket-timeout     connection-timeout-ms
+                                                             :connection-timeout connection-timeout-ms})))
               is                     (ReaderInputStream. reader)]
     (respond (-> (response/response is)
                  (response/content-type "application/json")))))

src/metabase/lib/aggregation.cljc

@@ -3,6 +3,7 @@
   (:require
    [medley.core :as m]
    [metabase.lib.common :as lib.common]
+   [metabase.lib.dispatch :as lib.dispatch]
    [metabase.lib.equality :as lib.equality]
    [metabase.lib.hierarchy :as lib.hierarchy]
    [metabase.lib.metadata :as lib.metadata]
@@ -223,12 +224,25 @@
   [aggregation-clause]
   aggregation-clause)
 
+(def ^:private Aggregatable
+  "Schema for something you can pass to [[aggregate]] to add to a query as an aggregation."
+  [:or
+   ::lib.schema.aggregation/aggregation
+   ::lib.schema.common/external-op
+   lib.metadata/MetricMetadata])
+
 (mu/defn aggregate :- ::lib.schema/query
   "Adds an aggregation to query."
-  ([query an-aggregate-clause]
-   (aggregate query -1 an-aggregate-clause))
-  ([query stage-number an-aggregate-clause]
-   (lib.util/add-summary-clause query stage-number :aggregation an-aggregate-clause)))
+  ([query aggregatable]
+   (aggregate query -1 aggregatable))
+
+  ([query        :- ::lib.schema/query
+    stage-number :- :int
+    aggregatable :- Aggregatable]
+   ;; if this is a Metric metadata, convert it to `:metric` MBQL clause before adding.
+   (if (= (lib.dispatch/dispatch-value aggregatable) :metadata/metric)
+     (recur query stage-number (lib.ref/ref aggregatable))
+     (lib.util/add-summary-clause query stage-number :aggregation aggregatable))))
 
 (mu/defn aggregations :- [:maybe [:sequential ::lib.schema.aggregation/aggregation]]
   "Get the aggregations in a given stage of a query."

src/metabase/lib/core.cljc

@@ -188,6 +188,8 @@
    display-info
    suggested-name
    type-of]
+  [lib.metric
+   available-metrics]
   [lib.native
    #?@(:cljs [->TemplateTags
               TemplateTags->])

src/metabase/lib/hierarchy.cljc

@@ -7,7 +7,9 @@
 (defn derive
   "Like [[clojure.core/derive]], but affects [[hierarchy]] rather than the global hierarchy."
   [tag parent]
-  (swap! hierarchy clojure.core/derive tag parent))
+  (swap! hierarchy clojure.core/derive tag parent)
+  ;; for REPL convenience so we don't dump a lot of garbage
+  nil)
 
 (defn isa?
   "Like [[clojure.core/isa?]], but uses [[hierarchy]]."

src/metabase/lib/js.cljs

@@ -555,3 +555,9 @@
   "Returns the native query's template tags"
   [a-query]
   (lib.core/TemplateTags-> (lib.core/template-tags a-query)))
+
+(defn ^:export available-metrics
+  "Get a list of Metrics that you may consider using as aggregations for a query. Returns JS array of opaque Metric
+  metadata objects."
+  [a-query]
+  (to-array (lib.core/available-metrics a-query)))

src/metabase/lib/js/metadata.cljs

@@ -375,6 +375,12 @@
         :when              (and a-field (= (:table-id a-field) table-id))]
     a-field))
 
+(defn- metrics [metadata table-id]
+  (for [[_id metric-delay] (some-> metadata :metrics deref)
+        :let               [a-metric (some-> metric-delay deref)]
+        :when              (and a-metric (= (:table-id a-metric) table-id))]
+    a-metric))
+
 (defn metadata-provider
   "Use a `metabase-lib/metadata/Metadata` as a [[metabase.lib.metadata.protocols/MetadataProvider]]."
   [database-id unparsed-metadata]
@@ -389,6 +395,7 @@
       (card     [_this card-id]    (card     metadata card-id))
       (tables   [_this]            (tables   metadata database-id))
       (fields   [_this table-id]   (fields   metadata table-id))
+      (metrics  [_this table-id]   (metrics  metadata table-id))
 
       ;; for debugging: call [[clojure.datafy/datafy]] on one of these to parse all of our metadata and see the whole
       ;; thing at once.

src/metabase/lib/metadata.cljc

@@ -117,19 +117,27 @@
    ;; probably missing `:lib/type` and probably using `:snake_case` keys.
    [:result-metadata {:optional true} [:maybe [:sequential :map]]]])
 
-(def ^:private SegmentMetadata
+(def SegmentMetadata
   "More or less the same as a [[metabase.models.segment]], but with kebab-case keys."
   [:map
    [:lib/type [:= :metadata/segment]]
    [:id       ::lib.schema.id/segment]
    [:name     ::lib.schema.common/non-blank-string]])
 
-(def ^:private MetricMetadata
-  "More or less the same as a [[metabase.models.metric]], but with kebab-case keys."
+(def MetricMetadata
+  "Malli schema for a legacy v1 [[metabase.models.metric]], but with kebab-case keys. A Metric defines an MBQL snippet
+  with an aggregation and optionally a filter clause. You can add a `:metric` reference to the `:aggregations` in an
+  MBQL stage, and the QP treats it like a macro and expands it to the underlying clauses --
+  see [[metabase.query-processor.middleware.expand-macros]]."
   [:map
-   [:lib/type [:= :metadata/metric]]
-   [:id       ::lib.schema.id/metric]
-   [:name     ::lib.schema.common/non-blank-string]])
+   [:lib/type   [:= :metadata/metric]]
+   [:id         ::lib.schema.id/metric]
+   [:name       ::lib.schema.common/non-blank-string]
+   [:table-id   ::lib.schema.id/table]
+   ;; the MBQL snippet defining this Metric; this may still be in legacy
+   ;; format. [[metabase.lib.metric/metric-definition]] handles conversion to pMBQL if needed.
+   [:definition :map]
+   [:description {:optional true} [:maybe ::lib.schema.common/non-blank-string]]])
 
 (def TableMetadata
   "Schema for metadata about a specific [[metabase.models.table]]. More or less the same as a [[metabase.models.table]],
@@ -155,7 +163,9 @@
 
 (def MetadataProvider
   "Schema for something that satisfies the [[lib.metadata.protocols/MetadataProvider]] protocol."
-  [:fn lib.metadata.protocols/metadata-provider?])
+  [:fn
+   {:error/message "Valid MetadataProvider"}
+   #'lib.metadata.protocols/metadata-provider?])
 
 (def MetadataProviderable
   "Something that can be used to get a MetadataProvider. Either a MetadataProvider, or a map with a MetadataProvider in

src/metabase/lib/metadata/cached_provider.cljc

@@ -55,12 +55,13 @@
   lib.metadata.protocols/MetadataProvider
   (database [_this]            (get-in-cache-or-fetch cache [:metadata/database]            #(lib.metadata.protocols/database metadata-provider)))
   (table    [_this table-id]   (get-in-cache-or-fetch cache [:metadata/table table-id]      #(lib.metadata.protocols/table    metadata-provider table-id)))
-  (field    [_this field-id]   (get-in-cache-or-fetch cache [:metadata/column field-id]      #(lib.metadata.protocols/field    metadata-provider field-id)))
+  (field    [_this field-id]   (get-in-cache-or-fetch cache [:metadata/column field-id]     #(lib.metadata.protocols/field    metadata-provider field-id)))
   (card     [_this card-id]    (get-in-cache-or-fetch cache [:metadata/card card-id]        #(lib.metadata.protocols/card     metadata-provider card-id)))
   (metric   [_this metric-id]  (get-in-cache-or-fetch cache [:metadata/metric metric-id]    #(lib.metadata.protocols/metric   metadata-provider metric-id)))
   (segment  [_this segment-id] (get-in-cache-or-fetch cache [:metadata/segment segment-id]  #(lib.metadata.protocols/segment  metadata-provider segment-id)))
   (tables   [_this]            (get-in-cache-or-fetch cache [::database-tables]             #(lib.metadata.protocols/tables   metadata-provider)))
   (fields   [_this table-id]   (get-in-cache-or-fetch cache [::table-fields table-id]       #(lib.metadata.protocols/fields   metadata-provider table-id)))
+  (metrics  [_this table-id]   (get-in-cache-or-fetch cache [::table-metrics table-id]      #(lib.metadata.protocols/metrics  metadata-provider table-id)))
 
   lib.metadata.protocols/CachedMetadataProvider
   (cached-database [_this]                           (get-in-cache    cache [:metadata/database]))

src/metabase/lib/metadata/calculation.cljc

@@ -174,7 +174,12 @@
       ;; otherwise if `:base-type` is specified, we can return that.
       (:base-type options)
       ;; if none of the special cases are true, fall back to [[type-of-method]].
-      (type-of-method query stage-number x)))))
+      (let [calculated-type (type-of-method query stage-number x)]
+        ;; if calculated type is not a true type but a placeholder like `:metabase.lib.schema.expression/type.unknown`
+        ;; or a union of types then fall back to `:type/*`, an actual type.
+        (if (isa? calculated-type :type/*)
+          calculated-type
+          :type/*))))))
 
 (defmethod type-of-method :default
   [_query _stage-number expr]
@@ -358,7 +363,7 @@
 (defmethod display-info-method :metadata/table
   [query stage-number table]
   (merge (default-display-info query stage-number table)
-         {:is-source-table (= (lib.util/source-table query) (:id table))}))
+         {:is-source-table (= (lib.util/source-table-id query) (:id table))}))
 
 (def ColumnsWithUniqueAliases
   "Schema for column metadata that should be returned by [[visible-columns]]. This is mostly used

src/metabase/lib/metadata/jvm.clj

@@ -50,6 +50,11 @@
   (mapv #(instance->metadata % :metadata/column)
         (t2/select :model/Field :table_id table-id)))
 
+(defn- metrics [table-id]
+  (log/debugf "Fetching all Metrics for Table %d" table-id)
+  (mapv #(instance->metadata % :metadata/metric)
+        (t2/select :model/Metric :table_id table-id)))
+
 (p/deftype+ UncachedApplicationDatabaseMetadataProvider [database-id]
   lib.metadata.protocols/MetadataProvider
   (database [_this]
@@ -72,6 +77,9 @@
   (fields [_this table-id]
     (fields table-id))
 
+  (metrics [_this table-id]
+    (metrics table-id))
+
   lib.metadata.protocols/BulkMetadataProvider
   (bulk-metadata [_this metadata-type ids]
     (bulk-instances metadata-type ids))

src/metabase/lib/metadata/protocols.cljc

@@ -61,7 +61,11 @@
 
   (fields [metadata-provider table-id]
     "Return a sequence of Fields associated with a Table with the given `table-id`. Fields should satisfy
-  the [[metabase.lib.metadata/ColumnMetadata]] schema. If no such Table exists, this should error."))
+  the [[metabase.lib.metadata/ColumnMetadata]] schema. If no such Table exists, this should error.")
+
+  (metrics [metadata-provider table-id]
+    "Return a sequence of legacy v1 Metrics associated with a Table with the given `table-id`. Metrics should satisfy
+  the [[metabase.lib.metadata/MetricMetadata]] schema. If no such Table exists, this should error."))
 
 (defn metadata-provider?
   "Whether `x` is a valid [[MetadataProvider]]."

src/metabase/lib/metric.cljc

@@ -5,7 +5,11 @@
    [metabase.lib.convert :as lib.convert]
    [metabase.lib.metadata :as lib.metadata]
    [metabase.lib.metadata.calculation :as lib.metadata.calculation]
+   [metabase.lib.metadata.protocols :as lib.metadata.protocols]
+   [metabase.lib.ref :as lib.ref]
    [metabase.lib.schema :as lib.schema]
+   [metabase.lib.schema.expression :as lib.schema.expression]
+   [metabase.lib.schema.id :as lib.schema.id]
    [metabase.lib.util :as lib.util]
    [metabase.mbql.normalize :as mbql.normalize]
    [metabase.shared.util.i18n :as i18n]
@@ -16,7 +20,7 @@
     (lib.metadata/metric query metric-id)))
 
 (mu/defn ^:private metric-definition :- [:maybe ::lib.schema/stage.mbql]
-  [{:keys [definition], :as _metric-metadata}]
+  [{:keys [definition], :as _metric-metadata} :- lib.metadata/MetricMetadata]
   (when definition
     (if (:mbql/type definition)
       definition
@@ -28,6 +32,18 @@
         lib.convert/->pMBQL
         (lib.util/query-stage -1)))))
 
+(defmethod lib.ref/ref-method :metadata/metric
+  [{:keys [id], :as metric-metadata}]
+  (let [effective-type (or (:effective-type metric-metadata)
+                           (:base-type metric-metadata)
+                           (when-let [aggregation (first (:aggregation (metric-definition metric-metadata)))]
+                             (let [ag-effective-type (lib.schema.expression/type-of aggregation)]
+                               (when (isa? ag-effective-type :type/*)
+                                 ag-effective-type))))
+        options (cond-> {:lib/uuid (str (random-uuid))}
+                  effective-type (assoc :effective-type effective-type))]
+    [:metric options id]))
+
 (defmethod lib.metadata.calculation/type-of-method :metadata/metric
   [query stage-number metric-metadata]
   (or
@@ -41,19 +57,58 @@
         (lib.metadata.calculation/type-of query stage-number metric-metadata))
       :type/*))
 
+(defn- fallback-display-name []
+  (i18n/tru "[Unknown Metric]"))
+
 (defmethod lib.metadata.calculation/display-name-method :metadata/metric
   [_query _stage-number metric-metadata _style]
   (or ((some-fn :display-name :name) metric-metadata)
-      (i18n/tru "[Unknown Metric]")))
+      (fallback-display-name)))
 
 (defmethod lib.metadata.calculation/display-name-method :metric
   [query stage-number [_tag _opts metric-id-or-name] style]
   (or (when-let [metric-metadata (resolve-metric query metric-id-or-name)]
         (lib.metadata.calculation/display-name query stage-number metric-metadata style))
-      (i18n/tru "[Unknown Metric]")))
+      (fallback-display-name)))
+
+(mu/defn ^:private aggregating-by-metric? :- :boolean
+  "Whether a given stage of a query currently includes a `:metric` ref clause in its aggregations."
+  [query        :- ::lib.schema/query
+   stage-number :- :int
+   metric-id    :- ::lib.schema.id/metric]
+  (let [{aggregations :aggregation} (lib.util/query-stage query stage-number)]
+    (boolean
+     (some (fn [[tag :as clause]]
+             (when (= tag :metric)
+               (let [[_tag _opts id] clause]
+                 (= id metric-id))))
+           aggregations))))
+
+(defmethod lib.metadata.calculation/display-info-method :metadata/metric
+  [query stage-number {:keys [id description], :as metric-metadata}]
+  (merge
+   ((get-method lib.metadata.calculation/display-info-method :default) query stage-number metric-metadata)
+   {:description description}
+   (when (aggregating-by-metric? query stage-number id)
+     {:selected true})))
+
+(defmethod lib.metadata.calculation/display-info-method :metric
+  [query stage-number [_tag _opts metric-id-or-name]]
+  (if-let [metric-metadata (resolve-metric query metric-id-or-name)]
+    (lib.metadata.calculation/display-info query stage-number metric-metadata)
+    {:effective-type    :type/*
+     :display-name      (fallback-display-name)
+     :long-display-name (fallback-display-name)}))
 
 (defmethod lib.metadata.calculation/column-name-method :metric
   [query stage-number [_tag _opts metric-id-or-name]]
   (or (when-let [metric-metadata (resolve-metric query metric-id-or-name)]
         (lib.metadata.calculation/column-name query stage-number metric-metadata))
       "metric"))
+
+(mu/defn available-metrics :- [:maybe [:sequential {:min 1} lib.metadata/MetricMetadata]]
+  "Get a list of Metrics that you may consider using as aggregations for a query. Only Metrics that have the same
+  `table-id` as the `source-table` for this query will be suggested."
+  [query :- ::lib.schema/query]
+  (when-let [source-table-id (lib.util/source-table-id query)]
+    (not-empty (lib.metadata.protocols/metrics (lib.metadata/->metadata-provider query) source-table-id))))

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

@@ -5,7 +5,8 @@
    [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.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:
@@ -18,6 +19,11 @@
     (nil? x)
     y
 
+    ;; if the type of either x or y is unknown, then the overall type of this has to be unknown as well.
+    (or (= x ::expression/type.unknown)
+        (= y ::expression/type.unknown))
+    ::expression/type.unknown
+
     ;; if both types are keywords return their most-specific ancestor.
     (and (keyword? x)
          (keyword? y))
@@ -55,25 +61,15 @@
   [:pred-expr-pairs [:sequential {:min 1} [:ref ::case-subclause]]]
   [:default [:? [:schema [:ref ::expression/expression]]]])
 
-(defn- allow-unkown-type
-  "A hack to allow case and coalesce expressions using fields only.
-  The real fix would pass in metadata so that the types of fields
-  can be determined."
-  [expr-type]
-  (if (= expr-type ::expression/type.unknown)
-    :type/*
-    expr-type))
-
 (defmethod expression/type-of-method :case
   [[_tag _opts pred-expr-pairs default]]
-  (-> (reduce
-       (fn [best-guess [_pred expr]]
-         (let [expr-type (expression/type-of expr)]
-           (best-return-type best-guess expr-type)))
-       (when (some? default)
-         (expression/type-of default))
-       pred-expr-pairs)
-      allow-unkown-type))
+  (reduce
+   (fn [best-guess [_pred expr]]
+     (let [expr-type (expression/type-of expr)]
+       (best-return-type best-guess expr-type)))
+   (when (some? default)
+     (expression/type-of default))
+   pred-expr-pairs))
 
 ;;; TODO -- add constraint that these types have to be compatible
 (mbql-clause/define-tuple-mbql-clause :coalesce
@@ -82,5 +78,4 @@
 
 (defmethod expression/type-of-method :coalesce
   [[_tag _opts expr null-value]]
-  (-> (best-return-type (expression/type-of expr) (expression/type-of null-value))
-      allow-unkown-type))
+  (best-return-type (expression/type-of expr) (expression/type-of null-value)))

src/metabase/lib/schema/mbql_clause.cljc

@@ -85,7 +85,7 @@
 
   ([tag         :- simple-keyword?
     _arrow      :- [:= :-]
-    return-type :- [:fn {:error/message "valid base type"} #(isa? % :type/*)]
+    return-type :- ::expression/base-type
     schema]
    (define-mbql-clause tag schema)
    (defmethod expression/type-of-method tag

src/metabase/lib/schema/ref.cljc

 (ns metabase.lib.schema.ref
   "Malli schema for a Field, aggregation, or expression reference (etc.)"
   (:require
+   [clojure.string :as str]
    [metabase.lib.dispatch :as lib.dispatch]
+   [metabase.lib.hierarchy :as lib.hierarchy]
    [metabase.lib.schema.common :as common]
    [metabase.lib.schema.expression :as expression]
    [metabase.lib.schema.id :as id]
@@ -55,6 +57,8 @@
     [:dispatch-type/integer ::field.id]
     [:dispatch-type/string ::field.literal]]])
 
+(lib.hierarchy/derive :field ::ref)
+
 (defmethod expression/type-of-method :field
   [[_tag opts _id-or-name]]
   (or ((some-fn :effective-type :base-type) opts)
@@ -68,6 +72,8 @@
   (or ((some-fn :effective-type :base-type) opts)
       ::expression/type.unknown))
 
+(lib.hierarchy/derive :expression ::ref)
+
 (mr/def ::aggregation-options
   [:merge
    ::common/options
@@ -86,10 +92,19 @@
   (or ((some-fn :effective-type :base-type) opts)
       ::expression/type.unknown))
 
+(lib.hierarchy/derive :aggregation ::ref)
+
+(mbql-clause/define-tuple-mbql-clause :metric :- ::expression/type.unknown
+  #_metric-id [:schema [:ref ::id/metric]])
+
+(lib.hierarchy/derive :metric ::ref)
+
 (mr/def ::ref
   [:and
    ::mbql-clause/clause
    [:fn
-    {:error/message ":field, :expression, :or :aggregation reference"}
+    {:error/fn (fn [_ _]
+                 (str "Valid reference, must be one of these clauses: "
+                      (str/join ", " (sort (descendants @lib.hierarchy/hierarchy ::ref)))))}
     (fn [[tag :as _clause]]
-      (#{:field :expression :aggregation} tag))]])
+      (lib.hierarchy/isa? tag ::ref))]])

src/metabase/lib/util.cljc

@@ -456,7 +456,7 @@
     (when-let [[_match card-id-str] (re-find #"^card__(\d+)$" table-id)]
       (parse-long card-id-str))))
 
-(mu/defn source-table :- [:maybe ::lib.schema.id/table]
+(mu/defn source-table-id :- [:maybe ::lib.schema.id/table]
   "If this query has a `:source-table` ID, return it."
   [query]
   (-> query :stages first :source-table))