MLv2: Remove `metabase.lib.dev` and "unresolved" functions (#31738)

* Implement `suggested-join-condition`

* Update docstring

* OOOF fix typo in `filter-clause`

* Fix premature i18n lookup

* Address PR feedback

* MLv2 JS join function wrappers

* PR feedback from #31590

* `join-conditions` returns raw MBQL clauses!

* WIP

* Simplify `with-join-alias` and `with-join-fields`

* Simplify join and add with-join-conditions

* Un-revert dev

* Wow! Remove unresolved nonsense.

* Remove dev

* Remove dev

* Test fixes 

* PR feedback

* WIP

* defop doesn't need query or stage-number anymore

* Fix Cljs tests for base PR

* Test fixes 

* Test fix 

* Update JS wrappers

frontend/src/metabase-lib/join.ts

@@ -21,12 +21,10 @@ export function joins(query: Query, stageIndex: number): Join[] {
 }
 
 export function joinClause(
-  query: Query,
-  stageIndex: number,
   joinable: Joinable,
   conditions: FilterClause[] | ExternalOp[],
 ): Join {
-  return ML.join_clause(query, stageIndex, joinable, conditions);
+  return ML.join_clause(joinable, conditions);
 }
 
 export function join(query: Query, stageIndex: number, join: Join): Query {

frontend/src/metabase-lib/order_by.ts

@@ -28,12 +28,10 @@ export function orderBy(
 }
 
 export function orderByClause(
-  query: Query,
-  stageNumber: number,
   column: ColumnMetadata,
   direction?: OrderByDirection,
 ): OrderByClause {
-  return ML.order_by_clause(query, stageNumber, column, direction);
+  return ML.order_by_clause(column, direction);
 }
 
 export function changeDirection(query: Query, clause: OrderByClause): Query {

frontend/src/metabase-lib/order_by.unit.spec.ts

@@ -178,7 +178,7 @@ describe("order by", () => {
         orderedQuery,
         0,
         orderBys[0],
-        ML.orderByClause(orderedQuery, 0, productCategory, "desc"),
+        ML.orderByClause(productCategory, "desc"),
       );
       const nextOrderBys = ML.orderBys(nextQuery, 0);
       expect(ML.displayName(nextQuery, nextOrderBys[0])).toBe(

frontend/src/metabase/query_builder/components/notebook/steps/SortStep/SortStep.tsx

@@ -60,7 +60,7 @@ function SortStep({
     clause: Lib.OrderByClause,
     column: Lib.ColumnMetadata,
   ) => {
-    const nextClause = Lib.orderByClause(topLevelQuery, stageIndex, column);
+    const nextClause = Lib.orderByClause(column);
     const nextQuery = Lib.replaceClause(
       topLevelQuery,
       stageIndex,

src/metabase/lib/aggregation.cljc

@@ -367,3 +367,23 @@
                                  refs cols)
                            cols)))))))
             agg-operators))))
+
+(mu/defn aggregation-ref :- :mbql.clause/aggregation
+  "Find the aggregation at `ag-index` and create an `:aggregation` ref for it. Intended for use
+  when creating queries using threading macros e.g.
+
+    (-> (lib/query ...)
+        (lib/aggregate (lib/avg ...))
+        (as-> <> (lib/order-by <> (lib/aggregation-ref <> 0))))"
+  ([query ag-index]
+   (aggregation-ref query -1 ag-index))
+
+  ([query        :- ::lib.schema/query
+    stage-number :- :int
+    ag-index     :- ::lib.schema.common/int-greater-than-or-equal-to-zero]
+   (if-let [[_ {ag-uuid :lib/uuid}] (get (:aggregation (lib.util/query-stage query stage-number)) ag-index)]
+     (lib.options/ensure-uuid [:aggregation {} ag-uuid])
+     (throw (ex-info (str "Undefined aggregation " ag-index)
+                     {:aggregation-index ag-index
+                      :query             query
+                      :stage-number      stage-number})))))

src/metabase/lib/binning.cljc

@@ -16,12 +16,6 @@
   (fn [x _binning]
     (lib.dispatch/dispatch-value x)) :hierarchy lib.hierarchy/hierarchy)
 
-(defmethod with-binning-method :dispatch-type/fn
-  [f binning]
-  (fn [query stage-number]
-    (let [x (f query stage-number)]
-      (with-binning-method x binning))))
-
 (mu/defn with-binning
   "Add binning to an MBQL clause or something that can be converted to an MBQL clause.
   Eg. for a Field or Field metadata or `:field` clause, this might do something like this:

src/metabase/lib/common.cljc

@@ -23,37 +23,39 @@
 
 (defmulti ->op-arg
   "Ensures that clause arguments are properly unwrapped"
-  {:arglists '([query stage-number x])}
-  (fn [_query _stage-number x]
-    (lib.dispatch/dispatch-value x))
+  {:arglists '([x])}
+  lib.dispatch/dispatch-value
   :hierarchy lib.hierarchy/hierarchy)
 
 (defmethod ->op-arg :default
-  [query stage-number x]
+  [x]
   (if (and (vector? x)
            (keyword? (first x)))
     ;; MBQL clause
-    (mapv #(->op-arg query stage-number %) x)
+    (mapv ->op-arg x)
     ;; Something else - just return it
     x))
 
 (defmethod ->op-arg :dispatch-type/sequential
-  [query stage-number xs]
-  (mapv #(->op-arg query stage-number %) xs))
+  [xs]
+  (mapv ->op-arg xs))
 
 (defmethod ->op-arg :metadata/field
-  [_query _stage-number field-metadata]
+  [field-metadata]
   (lib.ref/ref field-metadata))
 
 (defmethod ->op-arg :lib/external-op
-  [query stage-number {:keys [operator options args] :or {options {}}}]
-  (->op-arg query stage-number (lib.options/ensure-uuid (into [(keyword operator) options]
-                                                              (map #(->op-arg query stage-number %))
-                                                              args))))
+  [{:keys [operator options args] :or {options {}}}]
+  (->op-arg (lib.options/ensure-uuid (into [(keyword operator) options]
+                                           (map ->op-arg)
+                                           args))))
 
-(defmethod ->op-arg :dispatch-type/fn
-  [query stage-number f]
-  (->op-arg query stage-number (f query stage-number)))
+(defn defop-create
+  "Impl for [[defop]]."
+  [op-name args]
+  (into [op-name {:lib/uuid (str (random-uuid))}]
+        (map ->op-arg)
+        args))
 
 #?(:clj
    (defmacro defop
@@ -63,31 +65,11 @@
      {:pre [(symbol? op-name)
             (every? vector? argvecs) (every? #(every? symbol? %) argvecs)
             (every? #(not-any? #{'query 'stage-number} %) argvecs)]}
-     (let [fn-rename #(name (get {'/ 'div} % %))]
-       `(do
-          (mu/defn ~(symbol (str (fn-rename op-name) "-clause")) :- :metabase.lib.schema.common/external-op
-            ~(format "Create a standalone clause of type `%s`." (name op-name))
-            ~@(for [argvec argvecs
-                    :let [arglist-expr (if (contains? (set argvec) '&)
-                                         (cons `list* (remove #{'&} argvec))
-                                         argvec)]]
-                `([~'query ~'stage-number ~@argvec]
-                  {:lib/type :lib/external-op
-                   :operator ~(keyword op-name)
-                   :args (mapv (fn [~'arg]
-                                 (->op-arg ~'query ~'stage-number ~'arg))
-                               ~arglist-expr)})))
-
-          (mu/defn ~op-name :- fn?
-            ~(format "Create a closure of clause of type `%s`." (name op-name))
-            ~@(for [argvec argvecs
-                    :let [varargs? (contains? (set argvec) '&)
-                          arglist-expr (if varargs?
-                                         (filterv (complement #{'&}) argvec)
-                                         argvec)]]
-                `([~@argvec]
-                  (fn ~(symbol (str (fn-rename op-name) "-closure"))
-                    [~'query ~'stage-number]
-                    ~(cond->> (concat [(symbol (str (fn-rename op-name) "-clause")) 'query 'stage-number]
-                                      arglist-expr)
-                       varargs? (cons `apply))))))))))
+     `(mu/defn ~op-name :- ~(keyword "mbql.clause" (name op-name))
+        ~(format "Create a standalone clause of type `%s`." (name op-name))
+        ~@(for [argvec argvecs
+                :let [arglist-expr (if (contains? (set argvec) '&)
+                                     (cons `list* (remove #{'&} argvec))
+                                     argvec)]]
+            `([~@argvec]
+              (defop-create ~(keyword op-name) ~arglist-expr))))))

src/metabase/lib/core.cljc

@@ -10,7 +10,6 @@
    [metabase.lib.card :as lib.card]
    [metabase.lib.column-group :as lib.column-group]
    [metabase.lib.common :as lib.common]
-   [metabase.lib.dev :as lib.dev]
    [metabase.lib.expression :as lib.expression]
    [metabase.lib.field :as lib.field]
    [metabase.lib.filter :as lib.filter]
@@ -36,7 +35,6 @@
          lib.card/keep-me
          lib.column-group/keep-me
          lib.common/keep-me
-         lib.dev/keep-me
          lib.expression/keep-me
          lib.field/keep-me
          lib.filter/keep-me
@@ -58,6 +56,7 @@
   [lib.aggregation
    aggregate
    aggregation-clause
+   aggregation-ref
    aggregation-operator-columns
    aggregations
    aggregations-metadata
@@ -92,17 +91,12 @@
    group-columns]
   [lib.common
    external-op]
-  [lib.dev
-   field
-   query-for-table-id
-   query-for-table-name
-   table
-   ref-lookup]
   [lib.expression
    expression
    expressions
    expressions-metadata
    expressionable-columns
+   expression-ref
    +
    -
    *

src/metabase/lib/dev.cljc

-(ns metabase.lib.dev
-  "Conveniences for usage in REPL and tests. Things in this namespace are not meant for normal usage in the FE client or
-  in QB code."
-  (:require
-   [metabase.lib.metadata :as lib.metadata]
-   [metabase.lib.options :as lib.options]
-   [metabase.lib.query :as lib.query]
-   [metabase.lib.ref :as lib.ref]
-   [metabase.lib.schema :as lib.schema]
-   [metabase.lib.schema.common :as lib.schema.common]
-   [metabase.lib.schema.id :as lib.schema.id]
-   [metabase.lib.util :as lib.util]
-   [metabase.util.malli :as mu]))
-
-(mu/defn field :- fn?
-  "Returns a function that can get resolved to an actual `:field` clause later."
-  ([id-or-name :- [:or
-                   ::lib.schema.id/field
-                   ::lib.schema.common/non-blank-string]]
-   (if (integer? id-or-name)
-     (let [id id-or-name]
-       (fn [query _stage-number]
-         (lib.ref/ref (lib.metadata/field query id))))
-     (let [column-name id-or-name]
-       (fn [query stage-number]
-         (lib.ref/ref (lib.metadata/stage-column query stage-number column-name))))))
-
-  ([table-name :- ::lib.schema.common/non-blank-string
-    field-name :- ::lib.schema.common/non-blank-string]
-   (field nil table-name field-name))
-
-  ([schema     :- [:maybe ::lib.schema.common/non-blank-string]
-    table-name :- ::lib.schema.common/non-blank-string
-    field-name :- ::lib.schema.common/non-blank-string]
-   (fn [query _stage-number]
-     (lib.ref/ref (lib.metadata/field query schema table-name field-name)))))
-
-(mu/defn query-for-table-name :- ::lib.schema/query
-  "Create a new query for a specific Table with a table name."
-  ([metadata-provider :- lib.metadata/MetadataProvider
-    table-name        :- ::lib.schema.common/non-blank-string]
-   (query-for-table-name metadata-provider nil table-name))
-
-  ([metadata-provider :- lib.metadata/MetadataProvider
-    schema-name       :- [:maybe ::lib.schema.common/non-blank-string]
-    table-name        :- ::lib.schema.common/non-blank-string]
-   (let [table-metadata (lib.metadata/table metadata-provider schema-name table-name)]
-     (lib.query/query metadata-provider table-metadata))))
-
-(mu/defn query-for-table-id :- ::lib.schema/query
-  "Create a new query for a specific Table with `table-id`."
-  [metadata-provider :- lib.metadata/MetadataProvider
-   table-id          :- ::lib.schema.id/table]
-  (let [table-metadata (lib.metadata/table metadata-provider table-id)]
-    (lib.query/query metadata-provider table-metadata)))
-
-(mu/defn table :- fn?
-  "Returns a function that can be resolved to Table metadata. For use with a [[lib/join]] or something like that."
-  [id :- ::lib.schema.id/table]
-  (fn [query _stage-number]
-    (lib.metadata/table query id)))
-
-(mu/defn ref-lookup
-  "Returns a function that can be resolved into an expression or aggregation reference for the arguments.
-   Useful for tests so you don't have to split up queries to get references from metadata.
-   Throws an exception if no expression with that name can be found."
-  ([expression-or-aggregation :- [:enum :aggregation :expression]
-    index-or-name :- [:or :string ::lib.schema.common/int-greater-than-or-equal-to-zero]]
-   (fn [query stage-number]
-     (case expression-or-aggregation
-       :expression
-       (if (some (comp #{index-or-name} lib.util/expression-name)
-                 (:expressions (lib.util/query-stage query stage-number)))
-         (lib.options/ensure-uuid [:expression {} index-or-name])
-         (throw (ex-info (str "Undefined expression " index-or-name)
-                         {:expression-name index-or-name
-                          :query query
-                          :stage-number stage-number})))
-       :aggregation
-       (if-let [[_ {ag-uuid :lib/uuid}] (get (:aggregation (lib.util/query-stage query stage-number)) index-or-name)]
-         (lib.options/ensure-uuid [:aggregation {} ag-uuid])
-         (throw (ex-info (str "Undefined aggregation " index-or-name)
-                         {:aggregation-index index-or-name
-                          :query query
-                          :stage-number stage-number}))))))
-  ([query
-    stage-number
-    expression-or-aggregation :- [:enum :aggregation :expression]
-    index-or-name :- [:or :string ::lib.schema.common/int-greater-than-or-equal-to-zero]]
-   ((ref-lookup expression-or-aggregation index-or-name) query stage-number)))

src/metabase/lib/expression.cljc

@@ -34,16 +34,19 @@
 (mu/defn resolve-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 (m/find-first (comp #{expression-name} lib.util/expression-name)
-                      (:expressions stage))
-        (throw (ex-info (i18n/tru "No expression named {0}" (pr-str expression-name))
-                        {:expression-name expression-name
-                         :query           query
-                         :stage-number    stage-number})))))
+  ([query expression-name]
+   (resolve-expression query -1 expression-name))
+
+  ([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 (m/find-first (comp #{expression-name} lib.util/expression-name)
+                       (:expressions stage))
+         (throw (ex-info (i18n/tru "No expression named {0}" (pr-str expression-name))
+                         {:expression-name expression-name
+                          :query           query
+                          :stage-number    stage-number}))))))
 
 (defmethod lib.metadata.calculation/type-of-method :expression
   [query stage-number [_expression _opts expression-name, :as _expression-ref]]
@@ -51,12 +54,12 @@
     (lib.metadata.calculation/type-of query stage-number expression)))
 
 (defmethod lib.metadata.calculation/metadata-method :expression
-  [query stage-number [_expression opts expression-name, :as expression-ref]]
+  [query stage-number [_expression opts expression-name, :as expression-ref-clause]]
   {:lib/type        :metadata/field
    :lib/source-uuid (:lib/uuid opts)
    :name            expression-name
-   :display-name    (lib.metadata.calculation/display-name query stage-number expression-ref)
-   :base-type       (lib.metadata.calculation/type-of query stage-number expression-ref)
+   :display-name    (lib.metadata.calculation/display-name query stage-number expression-ref-clause)
+   :base-type       (lib.metadata.calculation/type-of query stage-number expression-ref-clause)
    :lib/source      :source/expressions})
 
 (defmethod lib.metadata.calculation/display-name-method :dispatch-type/integer
@@ -192,9 +195,13 @@
 
 (mu/defn expression :- ::lib.schema/query
   "Adds an expression to query."
-  ([query expression-name an-expression-clause]
-   (expression query -1 expression-name an-expression-clause))
-  ([query stage-number expression-name an-expression-clause]
+  ([query expression-name expressionable]
+   (expression query -1 expression-name expressionable))
+
+  ([query                :- ::lib.schema/query
+    stage-number         :- [:maybe :int]
+    expression-name      :- ::lib.schema.common/non-blank-string
+    expressionable]
    (let [stage-number (or stage-number -1)]
      (when (conflicting-name? query stage-number expression-name)
        (throw (ex-info "Expression name conflicts with a column in the same query stage"
@@ -202,7 +209,7 @@
      (lib.util/update-query-stage
       query stage-number
       add-expression-to-stage
-      (-> (lib.common/->op-arg query stage-number an-expression-clause)
+      (-> (lib.common/->op-arg expressionable)
           (lib.util/named-expression-clause expression-name))))))
 
 (lib.common/defop + [x y & more])
@@ -248,6 +255,16 @@
 (lib.common/defop upper [s])
 (lib.common/defop lower [s])
 
+(mu/defn ^:private expression-metadata :- lib.metadata/ColumnMetadata
+  [query                 :- ::lib.schema/query
+   stage-number          :- :int
+   expression-definition :- ::lib.schema.expression/expression]
+  (let [expression-name (lib.util/expression-name expression-definition)]
+    (-> (lib.metadata.calculation/metadata query stage-number expression-definition)
+        (assoc :lib/source   :source/expressions
+               :name         expression-name
+               :display-name expression-name))))
+
 (mu/defn expressions-metadata :- [:maybe [:sequential lib.metadata/ColumnMetadata]]
   "Get metadata about the expressions in a given stage of a `query`."
   ([query]
@@ -256,12 +273,7 @@
   ([query        :- ::lib.schema/query
     stage-number :- :int]
    (some->> (not-empty (:expressions (lib.util/query-stage query stage-number)))
-            (mapv (fn [expression-definition]
-                    (let [expression-name (lib.util/expression-name expression-definition)]
-                      (-> (lib.metadata.calculation/metadata query stage-number expression-definition)
-                          (assoc :lib/source   :source/expressions
-                                 :name         expression-name
-                                 :display-name expression-name))))))))
+            (mapv (partial expression-metadata query stage-number)))))
 
 (mu/defn expressions :- [:maybe ::lib.schema.expression/expressions]
   "Get the expressions map from a given stage of a `query`."
@@ -311,3 +323,21 @@
      (->> columns
           (filterv unavailable-expressions)
           not-empty))))
+
+(mu/defn expression-ref :- :mbql.clause/expression
+  "Find the expression with `expression-name` using [[resolve-expression]], then create a ref for it. Intended for use
+  when creating queries using threading macros e.g.
+
+    (-> (lib/query ...)
+        (lib/expression \"My Expression\" ...)
+        (as-> <> (lib/aggregate <> (lib/avg (lib/expression-ref <> \"My Expression\")))))"
+  ([query expression-name]
+   (expression-ref query -1 expression-name))
+
+  ([query           :- ::lib.schema/query
+    stage-number    :- :int
+    expression-name :- ::lib.schema.common/non-blank-string]
+   (->> expression-name
+        (resolve-expression query stage-number)
+        (expression-metadata query stage-number)
+        lib.ref/ref)))

src/metabase/lib/field.cljc

@@ -261,6 +261,11 @@
          {:table {:name (:name card), :display-name (:name card)}})))))
 
 ;;; ---------------------------------- Temporal Bucketing ----------------------------------------
+
+;;; TODO -- it's a little silly to make this a multimethod I think since there are exactly two implementations of it,
+;;; right? Or can expression and aggregation references potentially be temporally bucketed as well? Think about
+;;; whether just making this a plain function like we did for [[metabase.lib.join/with-join-alias]] makes sense or not.
+
 (defmethod lib.temporal-bucket/temporal-bucket-method :field
   [[_tag opts _id-or-name]]
   (:temporal-unit opts))
@@ -456,12 +461,7 @@
   ([query        :- ::lib.schema/query
     stage-number :- :int
     xs]
-   (let [xs (not-empty
-             (mapv (fn [x]
-                     (lib.ref/ref (if (fn? x)
-                                    (x query stage-number)
-                                    x)))
-                   xs))
+   (let [xs (not-empty (mapv lib.ref/ref xs))
          ;; if any fields are specified, include all expressions not yet included too
          xs (some-> xs
                     (into (remove #(lib.equality/find-closest-matching-ref % xs))

src/metabase/lib/filter.cljc

@@ -156,7 +156,7 @@
     stage-number :- [:maybe :int]
     boolean-expression]
    (let [stage-number (clojure.core/or stage-number -1)
-         new-filter (lib.common/->op-arg query stage-number boolean-expression)]
+         new-filter (lib.common/->op-arg boolean-expression)]
      (lib.util/update-query-stage query stage-number update :filters (fnil conj []) new-filter))))
 
 (mu/defn filters :- [:maybe [:ref ::lib.schema/filters]]

src/metabase/lib/join.cljc

@@ -47,12 +47,12 @@
    [:ref :mbql.clause/field]
    PartialJoin])
 
-(mu/defn with-join-alias
+(mu/defn with-join-alias :- FieldOrPartialJoin
   "Add OR REMOVE a specific `join-alias` to `field-or-joinable`, which is either a `:field`/Field metadata, or something
   'joinable' like a join map or Table metadata. Does not recursively update other references (yet; we can add this in
   the future)."
   {:style/indent [:form]}
-  [field-or-joinable :- [:or FieldOrPartialJoin fn?]
+  [field-or-joinable :- FieldOrPartialJoin
    join-alias        :- [:maybe ::lib.schema.common/non-blank-string]]
   (case (lib.dispatch/dispatch-value field-or-joinable)
     :field
@@ -62,11 +62,7 @@
     (u/assoc-dissoc field-or-joinable ::join-alias join-alias)
 
     :mbql/join
-    (u/assoc-dissoc field-or-joinable :alias join-alias)
-
-    :dispatch-type/fn
-    (fn [query stage-number]
-      (with-join-alias (field-or-joinable query stage-number) join-alias))))
+    (u/assoc-dissoc field-or-joinable :alias join-alias)))
 
 (mu/defn current-join-alias :- [:maybe ::lib.schema.common/non-blank-string]
   "Get the current join alias associated with something, if it has one."
@@ -187,77 +183,55 @@
 
 (defmulti join-clause-method
   "Convert something to a join clause."
-  {:arglists '([query stage-number x])}
-  (fn [_query _stage-number x]
-    (lib.dispatch/dispatch-value x))
+  {:arglists '([joinable])}
+  lib.dispatch/dispatch-value
   :hierarchy lib.hierarchy/hierarchy)
 
 ;; 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-method :mbql/join
-  [_query _stage-number a-join-clause]
+  [a-join-clause]
   a-join-clause)
 
 ;;; TODO -- this probably ought to live in [[metabase.lib.query]]
 (defmethod join-clause-method :mbql/query
-  [_query _stage-number another-query]
+  [another-query]
   (-> {:lib/type :mbql/join
        :stages   (:stages (lib.util/pipeline another-query))}
       lib.options/ensure-uuid))
 
 ;;; TODO -- this probably ought to live in [[metabase.lib.stage]]
 (defmethod join-clause-method :mbql.stage/mbql
-  [_query _stage-number mbql-stage]
+  [mbql-stage]
   (-> {:lib/type :mbql/join
        :stages   [mbql-stage]}
       lib.options/ensure-uuid))
 
-(defmethod join-clause-method :dispatch-type/fn
-  [query stage-number f]
-  (join-clause-method query
-                      stage-number
-                      (or (f query stage-number)
-                          (throw (ex-info "Error creating join clause: (f query stage-number) returned nil"
-                                          {:query        query
-                                           :stage-number stage-number
-                                           :f            f})))))
-
-;; TODO -- too complicated, why don't we just ask people to use a separate [[with-join-conditions]] instead of having
-;; all these arities.
-(mu/defn join-clause :- [:or PartialJoin fn?]
+(mu/defn with-join-conditions :- PartialJoin
+  "Update the `:conditions` (filters) for a Join clause."
+  {:style/indent [:form]}
+  [a-join     :- PartialJoin
+   conditions :- [:maybe [:sequential [:or ::lib.schema.expression/boolean ::lib.schema.common/external-op]]]]
+  (u/assoc-dissoc a-join :conditions (not-empty (mapv lib.common/->op-arg conditions))))
+
+(mu/defn join-clause :- PartialJoin
   "Create an MBQL join map from something that can conceptually be joined against. A `Table`? An MBQL or native query? A
   Saved Question? You should be able to join anything, and this should return a sensible MBQL join map."
   ([joinable]
-   (fn [query stage-number]
-     (join-clause query stage-number joinable)))
+   (join-clause-method joinable))
 
   ([joinable conditions]
-   (fn [query stage-number]
-     (join-clause query stage-number joinable conditions)))
-
-  ([query stage-number joinable]
-   (join-clause-method query stage-number joinable))
+   (with-join-conditions (join-clause-method joinable) conditions)))
 
-  ([query stage-number joinable conditions]
-   (cond-> (join-clause query stage-number joinable)
-     conditions (assoc :conditions (mapv #(lib.common/->op-arg query stage-number %) conditions)))))
-
-(mu/defn with-join-fields :- [:or PartialJoin fn?]
+(mu/defn with-join-fields :- PartialJoin
   "Update a join (or a function that will return a join) to include `:fields`, either `:all`, `:none`, or a sequence of
   references."
-  [joinable :- [:or PartialJoin fn?]
-   fields]
-  (if (fn? joinable)
-    (fn [query stage-number]
-      (let [resolved (joinable query stage-number)
-            ;; if joinable is unresolved, then make sure all of the Fields get resolved too if needed... but what if
-            ;; we have a resolved join + unresolved Fields? We don't handle that FIXME
-            fields   (if (keyword? fields)
-                       fields
-                       (mapv #(lib.common/->op-arg query stage-number %) fields))]
-        (with-join-fields resolved fields)))
-    (u/assoc-dissoc joinable :fields fields)))
+  [joinable :- PartialJoin
+   fields   :- [:maybe [:or [:enum :all :none] [:sequential some?]]]]
+  (u/assoc-dissoc joinable :fields (if (keyword? fields)
+                                     fields
+                                     (not-empty (mapv lib.ref/ref fields)))))
 
 (defn- select-home-column
   [home-cols cond-fields]
@@ -394,11 +368,8 @@
 
   ([query        :- ::lib.schema/query
     stage-number :- :int
-    a-join       :- [:or PartialJoin fn?]]
-   (let [a-join (if (fn? a-join)
-                  (a-join query stage-number)
-                  a-join)
-         a-join (if (contains? a-join :alias)
+    a-join       :- PartialJoin]
+   (let [a-join (if (contains? a-join :alias)
                   ;; if the join clause comes with an alias, keep it and assume that the
                   ;; condition fields have the right join-aliases too
                   a-join
@@ -441,14 +412,11 @@
   [a-join :- ::lib.schema.join/join]
   (get a-join :strategy :left-join))
 
-(mu/defn with-join-strategy :- [:or PartialJoin fn?]
+(mu/defn with-join-strategy :- PartialJoin
   "Return a copy of `a-join` with its `:strategy` set to `strategy`."
-  [a-join   :- [:or PartialJoin fn?]
+  [a-join   :- PartialJoin
    strategy :- ::lib.schema.join/strategy]
-  (if (fn? a-join)
-    (fn [query stage-metadata]
-      (with-join-strategy (a-join query stage-metadata) strategy))
-    (assoc a-join :strategy strategy)))
+  (assoc a-join :strategy strategy))
 
 (mu/defn available-join-strategies :- [:sequential ::lib.schema.join/strategy]
   "Get available join strategies for the current Database (based on the Database's
@@ -616,17 +584,4 @@
    (when-let [pk-col (pk-column query stage-number joinable)]
      (when-let [fk-col (fk-column-for query stage-number pk-col)]
        (lib.common/->op-arg
-        query
-        stage-number
         (lib.filter/filter-clause (equals-join-condition-operator-definition) fk-col pk-col))))))
-
-(mu/defn with-join-conditions :- [:or PartialJoin fn?]
-  "Update the `:conditions` (filters) for a Join clause."
-  {:style/indent [:form]}
-  [a-join     :- [:or PartialJoin fn?]
-   conditions :- [:maybe [:sequential [:or ::lib.schema.expression/boolean fn?]]]]
-  (if (fn? a-join)
-    (fn [query stage-number]
-      (with-join-conditions (a-join query stage-number)
-                            (mapv (partial lib.common/->op-arg query stage-number) conditions)))
-    (u/assoc-dissoc a-join :conditions conditions)))

src/metabase/lib/js.cljs

@@ -131,13 +131,16 @@
 
 (defn ^:export order-by-clause
   "Create an order-by clause independently of a query, e.g. for `replace` or whatever."
-  [a-query stage-number x direction]
-  (lib.core/order-by-clause a-query stage-number (lib.core/normalize (js->clj x :keywordize-keys true)) direction))
+  ([orderable]
+   (order-by-clause orderable :asc))
+
+  ([orderable direction]
+   (lib.core/order-by-clause (lib.core/normalize (js->clj orderable :keywordize-keys true)) (keyword direction))))
 
 (defn ^:export order-by
   "Add an `order-by` clause to `a-query`. Returns updated query."
-  [a-query stage-number x direction]
-  (lib.core/order-by a-query stage-number x (keyword direction)))
+  [a-query stage-number orderable direction]
+  (lib.core/order-by a-query stage-number orderable (keyword direction)))
 
 (defn ^:export order-bys
   "Get the order-by clauses (as an array of opaque objects) in `a-query` at a given `stage-number`.
@@ -482,8 +485,8 @@
   "Create a join clause (an `:mbql/join` map) against something `joinable` (Table metadata, a Saved Question, another
   query, etc.) with `conditions`, which should be an array of filter clauses. You can then manipulate this join clause
   with stuff like [[with-join-fields]], or add it to a query with [[join]]."
-  [a-query stage-number joinable conditions]
-  (lib.core/join-clause a-query stage-number joinable conditions))
+  [joinable conditions]
+  (lib.core/join-clause joinable conditions))
 
 (defn ^:export join
   "Add a join clause (as created by [[join-clause]]) to a stage of a query."

src/metabase/lib/metadata/calculation.cljc

@@ -180,10 +180,6 @@
   [_query _stage-number expr]
   (lib.schema.expresssion/type-of expr))
 
-(defmethod type-of-method :dispatch-type/fn
-  [query stage-number f]
-  (type-of query stage-number (f query stage-number)))
-
 ;;; for MBQL clauses whose type is the same as the type of the first arg. Also used
 ;;; for [[metabase.lib.schema.expression/type-of]].
 (defmethod type-of-method :lib.type-of/type-is-type-of-first-arg

src/metabase/lib/order_by.cljc

@@ -41,23 +41,18 @@
   (assoc (lib.metadata.calculation/display-info query stage-number expr)
          :direction tag))
 
-(defmulti ^:private ->order-by-clause
-  {:arglists '([query stage-number x])}
-  (fn [_query _stage-number x]
-    (lib.dispatch/dispatch-value x))
+(defmulti ^:private order-by-clause-method
+  {:arglists '([orderable])}
+  lib.dispatch/dispatch-value
   :hierarchy lib.hierarchy/hierarchy)
 
-(defmethod ->order-by-clause ::order-by-clause
-  [_query _stage-number clause]
+(defmethod order-by-clause-method ::order-by-clause
+  [clause]
   (lib.options/ensure-uuid clause))
 
-(defmethod ->order-by-clause :dispatch-type/fn
-  [query stage-number f]
-  (->order-by-clause query stage-number (f query stage-number)))
-
 ;;; by default, try to convert `x` to a ref and then order by `:asc`
-(defmethod ->order-by-clause :default
-  [_query _stage-number x]
+(defmethod order-by-clause-method :default
+  [x]
   (when (nil? x)
     (throw (ex-info (i18n/tru "Can''t order by nil") {})))
   (lib.options/ensure-uuid [:asc (lib.ref/ref x)]))
@@ -70,22 +65,12 @@
 
 (mu/defn order-by-clause
   "Create an order-by clause independently of a query, e.g. for `replace` or whatever."
-  ([x]
-   (fn [query stage-number]
-     (order-by-clause query stage-number x nil)))
-  ([x
-    direction :- [:maybe [:enum :asc :desc]]]
-   (fn [query stage-number]
-     (order-by-clause query stage-number x direction)))
-  ([query :- ::lib.schema/query
-    stage-number :- [:maybe :int]
-    x]
-   (order-by-clause query stage-number x nil))
-  ([query :- ::lib.schema/query
-    stage-number :- [:maybe :int]
-    x
+  ([orderable]
+   (order-by-clause orderable :asc))
+
+  ([orderable :- some?
     direction :- [:maybe [:enum :asc :desc]]]
-   (-> (->order-by-clause query (or stage-number -1) x)
+   (-> (order-by-clause-method orderable)
        (with-direction (or direction :asc)))))
 
 (mu/defn order-by
@@ -93,19 +78,19 @@
   Field, or `:field` clause, or expression of some sort, etc.
 
   You can teach Metabase lib how to generate order by clauses for different things by implementing the
-  underlying [[->order-by-clause]] multimethod."
-  ([query x]
-   (order-by query -1 x nil))
+  underlying [[order-by-clause-method]] multimethod."
+  ([query orderable]
+   (order-by query -1 orderable nil))
 
-  ([query x direction]
-   (order-by query -1 x direction))
+  ([query orderable direction]
+   (order-by query -1 orderable direction))
 
   ([query
     stage-number :- [:maybe :int]
-    x            :- some?
+    orderable    :- some?
     direction    :- [:maybe [:enum :asc :desc]]]
    (let [stage-number (or stage-number -1)
-         new-order-by (cond-> (->order-by-clause query stage-number x)
+         new-order-by (cond-> (order-by-clause-method orderable)
                         direction (with-direction direction))]
      (lib.util/update-query-stage query stage-number update :order-by (fn [order-bys]
                                                                         (conj (vec order-bys) new-order-by))))))

src/metabase/lib/remove_replace.cljc

@@ -122,7 +122,7 @@
 
 (defn- remove-replace* [query stage-number target-clause remove-or-replace replacement]
   (binding [mu/*enforce* false]
-    (let [target-clause (lib.common/->op-arg query stage-number target-clause)
+    (let [target-clause (lib.common/->op-arg target-clause)
           stage (lib.util/query-stage query stage-number)
           location (m/find-first
                      (fn [possible-location]
@@ -133,7 +133,7 @@
                      (stage-paths query stage-number))
           replace? (= :replace remove-or-replace)
           replacement-clause (when replace?
-                               (lib.common/->op-arg query stage-number replacement))
+                               (lib.common/->op-arg replacement))
           remove-replace-fn (if replace?
                               #(lib.util/replace-clause %1 %2 %3 replacement-clause)
                               lib.util/remove-clause)

src/metabase/lib/schema/aggregation.cljc

@@ -6,35 +6,40 @@
    [metabase.shared.util.i18n :as i18n]
    [metabase.util.malli.registry :as mr]))
 
-;; count has an optional expression arg
+;; count has an optional expression arg. This is the number of non-NULL values -- corresponds to count(<expr>) in SQL
 (mbql-clause/define-catn-mbql-clause :count :- :type/Integer
-  [:expression [:? [:schema [:ref ::expression/number]]]])
+  [:expression [:? [:schema [:ref ::expression/expression]]]])
 
 ;; cum-count has an optional expression arg
 (mbql-clause/define-catn-mbql-clause :cum-count :- :type/Integer
-  [:expression [:? [:schema [:ref ::expression/number]]]])
+  [:expression [:? [:schema [:ref ::expression/expression]]]])
 
 (mbql-clause/define-tuple-mbql-clause :avg :- :type/Float
   [:schema [:ref ::expression/number]])
 
+;;; number of distinct values of something.
 (mbql-clause/define-tuple-mbql-clause :distinct :- :type/Integer
   [:schema [:ref ::expression/expression]])
 
 (mbql-clause/define-tuple-mbql-clause :count-where :- :type/Integer
   [:schema [:ref ::expression/boolean]])
 
+;;; min and max should work on anything orderable, including numbers, temporal values, and even text values.
 (mbql-clause/define-tuple-mbql-clause :max
-  [:schema [:ref ::expression/number]])
+  [:schema [:ref ::expression/orderable]])
 
 (lib.hierarchy/derive :max :lib.type-of/type-is-type-of-first-arg)
 
+;;; apparently median and percentile only work for numeric args in Postgres, as opposed to anything orderable. Not
+;;; sure this makes sense conceptually, but since there probably isn't as much of a use case we can keep that
+;;; restriction in MBQL for now.
 (mbql-clause/define-tuple-mbql-clause :median
   [:schema [:ref ::expression/number]])
 
 (lib.hierarchy/derive :median :lib.type-of/type-is-type-of-first-arg)
 
 (mbql-clause/define-tuple-mbql-clause :min
-  [:schema [:ref ::expression/number]])
+  [:schema [:ref ::expression/orderable]])
 
 (lib.hierarchy/derive :min :lib.type-of/type-is-type-of-first-arg)
 

src/metabase/lib/schema/expression.cljc

@@ -57,7 +57,7 @@
 
 (defmethod type-of-method :default
   [expr]
-  (throw (ex-info (i18n/tru "Don''t know how to determine the type of {0}" (pr-str expr))
+  (throw (ex-info (i18n/tru "{0}: Don''t know how to determine the type of {1}" `type-of (pr-str expr))
                   {:expr expr})))
 
 ;;; for MBQL clauses whose type is the same as the type of the first arg. Also used

src/metabase/lib/table.cljc

@@ -70,10 +70,8 @@
                        :lib/desired-column-alias (unique-name-fn (or (:name col) ""))))))))
 
 (defmethod lib.join/join-clause-method :metadata/table
-  [query stage-number {::keys [join-alias join-fields], :as table-metadata}]
-  (cond-> (lib.join/join-clause query
-                                stage-number
-                                {:lib/type     :mbql.stage/mbql
+  [{::keys [join-alias join-fields], :as table-metadata}]
+  (cond-> (lib.join/join-clause {:lib/type     :mbql.stage/mbql
                                  :lib/options  {:lib/uuid (str (random-uuid))}
                                  :source-table (:id table-metadata)})
     join-alias  (lib.join/with-join-alias join-alias)