diff --git a/src/metabase/lib/aggregation.cljc b/src/metabase/lib/aggregation.cljc
index f2307a3187f165bc892b7c3040d8f973baf3825f..3aa20c3eeadbd484ec2d3f328d9766a400117741 100644
--- a/src/metabase/lib/aggregation.cljc
+++ b/src/metabase/lib/aggregation.cljc
@@ -2,14 +2,14 @@
(:refer-clojure :exclude [count distinct max min])
(:require
[metabase.lib.common :as lib.common]
+ [metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.schema :as lib.schema]
[metabase.lib.schema.aggregation :as lib.schema.aggregation]
[metabase.lib.schema.common :as lib.schema.common]
[metabase.lib.util :as lib.util]
[metabase.shared.util.i18n :as i18n]
- [metabase.util.malli :as mu])
- #?(:cljs (:require-macros [metabase.lib.aggregation])))
+ [metabase.util.malli :as mu]))
(mu/defn resolve-aggregation :- ::lib.schema.aggregation/aggregation
"Resolve an aggregation with a specific `index`."
@@ -206,3 +206,17 @@
update :aggregation
(fn [aggregations]
(conj (vec aggregations) (lib.common/->op-arg query stage-number an-aggregate-clause)))))))
+
+(mu/defn aggregations :- [:maybe [:sequential lib.metadata/ColumnMetadata]]
+ "Get metadata about the aggregations in a given stage of a query."
+ [query :- ::lib.schema/query
+ stage-number :- :int]
+ (when-let [aggregation-exprs (not-empty (:aggregation (lib.util/query-stage query stage-number)))]
+ (map-indexed (fn [i aggregation]
+ (let [metadata (lib.metadata.calculation/metadata query stage-number aggregation)
+ ag-ref [:aggregation
+ {:lib/uuid (str (random-uuid))
+ :base-type (:base_type metadata)}
+ i]]
+ (assoc metadata :field_ref ag-ref, :source :aggregation)))
+ aggregation-exprs)))
diff --git a/src/metabase/lib/breakout.cljc b/src/metabase/lib/breakout.cljc
index 23d49c4cd4aa8a6b34933c86ae769c09d3647826..edcdc2801542fecbf62a4adf486846217a7ba10e 100644
--- a/src/metabase/lib/breakout.cljc
+++ b/src/metabase/lib/breakout.cljc
@@ -1,6 +1,7 @@
(ns metabase.lib.breakout
(:require
[clojure.string :as str]
+ [metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.schema :as lib.schema]
[metabase.lib.schema.expression :as lib.schema.expression]
@@ -29,3 +30,12 @@
expr)]
(lib.util/update-query-stage query stage-number update :breakout (fn [breakouts]
(conj (vec breakouts) expr))))))
+
+(mu/defn breakouts :- [:maybe [:sequential lib.metadata/ColumnMetadata]]
+ "Get metadata about the breakouts in a given stage of a `query`."
+ [query :- ::lib.schema/query
+ stage-number :- :int]
+ (when-let [breakout-exprs (not-empty (:breakout (lib.util/query-stage query stage-number)))]
+ (mapv (fn [field-ref]
+ (assoc (lib.metadata.calculation/metadata query stage-number field-ref) :source :breakout))
+ breakout-exprs)))
diff --git a/src/metabase/lib/core.cljc b/src/metabase/lib/core.cljc
index f6ecb6e39f467367ea41d22718ab7019a1ec4138..e202e9b0e9905891750d1d836139913b16d76e15 100644
--- a/src/metabase/lib/core.cljc
+++ b/src/metabase/lib/core.cljc
@@ -99,8 +99,6 @@
rtrim
upper
lower]
- [lib.field
- with-join-alias]
[lib.filter
filter
add-filter
@@ -138,7 +136,9 @@
[lib.join
join
join-clause
- joins]
+ joins
+ with-join-alias
+ with-join-fields]
[lib.limit
current-limit
limit]
@@ -150,7 +150,8 @@
[lib.order-by
order-by
order-by-clause
- order-bys]
+ order-bys
+ orderable-columns]
[lib.query
native-query
query
diff --git a/src/metabase/lib/expression.cljc b/src/metabase/lib/expression.cljc
index d517a1db38e809216c30a4dbed700663c2f8ccce..556a2b5a10013222419549844aeeca6e5b10dd48 100644
--- a/src/metabase/lib/expression.cljc
+++ b/src/metabase/lib/expression.cljc
@@ -1,8 +1,11 @@
(ns metabase.lib.expression
- (:refer-clojure :exclude [+ - * / case coalesce abs time concat replace])
+ (:refer-clojure
+ :exclude
+ [+ - * / case coalesce abs time concat replace])
(:require
[clojure.string :as str]
[metabase.lib.common :as lib.common]
+ [metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.schema :as lib.schema]
[metabase.lib.schema.common :as lib.schema.common]
@@ -197,3 +200,14 @@
(lib.common/defop rtrim [s])
(lib.common/defop upper [s])
(lib.common/defop lower [s])
+
+(mu/defn expressions :- [:sequential lib.metadata/ColumnMetadata]
+ "Get metadata about the expressions in a given stage of a `query`."
+ [query :- ::lib.schema/query
+ stage-number :- :int]
+ (for [[expression-name expression-definition] (:expressions (lib.util/query-stage query stage-number))]
+ (let [metadata (lib.metadata.calculation/metadata query stage-number expression-definition)]
+ (merge
+ metadata
+ {:field_ref [:expression {:lib/uuid (str (random-uuid)), :base-type (:base_type metadata)} expression-name]
+ :source :expressions}))))
diff --git a/src/metabase/lib/field.cljc b/src/metabase/lib/field.cljc
index 471b4e4fd7787aafd85fe7209e733afd0aa68f2d..24e3c91ef2fd72ca4d2ad5ec00debad7e4a432db 100644
--- a/src/metabase/lib/field.cljc
+++ b/src/metabase/lib/field.cljc
@@ -148,10 +148,6 @@
([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)))
+(defmethod lib.join/with-join-alias-method :field
+ [field-ref join-alias]
+ (lib.options/update-options field-ref assoc :join-alias join-alias))
diff --git a/src/metabase/lib/join.cljc b/src/metabase/lib/join.cljc
index b0cd714795dc6e20f84ad50f93440da684a36303..2d1476a61a9c63dc3c22b15b7fd303a0f935b98f 100644
--- a/src/metabase/lib/join.cljc
+++ b/src/metabase/lib/join.cljc
@@ -141,6 +141,37 @@
(cond-> (join-clause query stage-number x)
condition (assoc :condition (join-condition query stage-number condition)))))
+(defmulti with-join-alias-method
+ "Implementation for [[with-join-alias]]."
+ {:arglists '([x join-alias])}
+ (fn [x _join-alias]
+ (lib.dispatch/dispatch-value x)))
+
+(mu/defn with-join-alias
+ "Add a specific `join-alias` to something `x`, either a `:field` or join map. Does not recursively update other
+ references (yet; we can add this in the future)."
+ [x join-alias :- ::lib.schema.common/non-blank-string]
+ (with-join-alias-method x join-alias))
+
+(defmethod with-join-alias-method :dispatch-type/fn
+ [f join-alias]
+ (fn [query stage-number]
+ (let [x (f query stage-number)]
+ (with-join-alias-method x join-alias))))
+
+(defmethod with-join-alias-method :mbql/join
+ [join join-alias]
+ (assoc join :alias join-alias))
+
+(mu/defn with-join-fields
+ "Update a join (or a function that will return a join) to include `:fields`, either `:all`, `:none`, or a sequence of
+ references."
+ [x fields :- ::lib.schema.join/fields]
+ (if (fn? x)
+ (fn [query stage-number]
+ (with-join-fields (x query stage-number) fields))
+ (assoc x :fields fields)))
+
(mu/defn join :- ::lib.schema/query
"Create a join map as if by [[join-clause]] and add it to a `query`.
diff --git a/src/metabase/lib/metadata.cljc b/src/metabase/lib/metadata.cljc
index 482bfe12eb34070130e6c7ea62716aad35be00f6..958689e89630df5b4b9956623c5b468159f80554 100644
--- a/src/metabase/lib/metadata.cljc
+++ b/src/metabase/lib/metadata.cljc
@@ -42,7 +42,7 @@
that they are largely compatible. So they're the same for now. We can revisit this in the future if we actually want
to differentiate between the two versions."
[:map
- [:lib/type [:= :metadata/field]]
+ [:lib/type [:= :metadata/field]] ; TODO -- should this be changed to `:metadata/column`?
[:id {:optional true} ::lib.schema.id/field]
[:name ::lib.schema.common/non-blank-string]])
diff --git a/src/metabase/lib/order_by.cljc b/src/metabase/lib/order_by.cljc
index 6063942568cbd1352187009fdcb22deeb8329437..c4f7814054b067359d6024556afb89cd280d69e9 100644
--- a/src/metabase/lib/order_by.cljc
+++ b/src/metabase/lib/order_by.cljc
@@ -1,11 +1,16 @@
(ns metabase.lib.order-by
(:require
+ [metabase.lib.aggregation :as lib.aggregation]
+ [metabase.lib.breakout :as lib.breakout]
[metabase.lib.dispatch :as lib.dispatch]
[metabase.lib.field :as lib.field]
+ [metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.options :as lib.options]
[metabase.lib.schema :as lib.schema]
+ [metabase.lib.schema.expression :as lib.schema.expression]
[metabase.lib.schema.order-by :as lib.schema.order-by]
+ [metabase.lib.stage :as lib.stage]
[metabase.lib.util :as lib.util]
[metabase.shared.util.i18n :as i18n]
[metabase.util.malli :as mu]))
@@ -87,5 +92,42 @@
([query :- ::lib.schema/query]
(order-bys query -1))
([query :- ::lib.schema/query
- stage-number :- [:int]]
+ stage-number :- :int]
(not-empty (get (lib.util/query-stage query stage-number) :order-by))))
+
+(defn- orderable-column? [{base-type :base_type, :as _column-metadata}]
+ (some (fn [orderable-base-type]
+ (isa? base-type orderable-base-type))
+ lib.schema.expression/orderable-types))
+
+(mu/defn orderable-columns :- [:sequential lib.metadata/ColumnMetadata]
+ "Get column metadata for all the columns you can order by in a given `stage-number` of a `query`. Rules are as
+ follows:
+
+ 1. If the stage has aggregations or breakouts, you can only order by those columns. E.g.
+
+ SELECT id, count(*) AS count FROM core_user GROUP BY id ORDER BY count ASC;
+
+ You can't ORDER BY something not in the SELECT, e.g. ORDER BY user.first_name would not make sense here.
+
+ 2. If the stage has no aggregations or breakouts, you can order by any visible Field:
+
+ a. You can filter by any custom `:expressions` in this stage of the query
+
+ b. You can filter by any Field 'exported' by the previous stage of the query, if there is one; otherwise you can
+ filter by any Fields from the current `:source-table`.
+
+ c. You can filter by any Fields exported by any explicit joins
+
+ d. You can filter by and Fields in Tables that are implicitly joinable."
+ ([query :- ::lib.schema/query]
+ (orderable-columns query -1))
+
+ ([query :- ::lib.schema/query
+ stage-number :- :int]
+ (let [breakouts (not-empty (lib.breakout/breakouts query stage-number))
+ aggregations (not-empty (lib.aggregation/aggregations query stage-number))]
+ (filter orderable-column?
+ (if (or breakouts aggregations)
+ (concat breakouts aggregations)
+ (lib.stage/visible-columns query stage-number))))))
diff --git a/src/metabase/lib/schema/expression.cljc b/src/metabase/lib/schema/expression.cljc
index 008238342afeda15acec07e02b087db94ca4bc95..428d7798348efef0f4c44899dc4cf9fe2546365a 100644
--- a/src/metabase/lib/schema/expression.cljc
+++ b/src/metabase/lib/schema/expression.cljc
@@ -126,8 +126,12 @@
(mr/def ::temporal
(expression-schema :type/Temporal "expression returning a date, time, or date time"))
+(def orderable-types
+ "Set of base types that are orderable."
+ #{:type/Text :type/Number :type/Temporal})
+
(mr/def ::orderable
- (expression-schema #{:type/Text :type/Number :type/Temporal}
+ (expression-schema orderable-types
"an expression that can be compared with :> or :<"))
(mr/def ::equality-comparable
diff --git a/src/metabase/lib/schema/join.cljc b/src/metabase/lib/schema/join.cljc
index 596eff3156aa9161e3de3c321d66232e5c9e4573..8b9a7e1d8620c5b3ce2d94f88eac30bac7532081 100644
--- a/src/metabase/lib/schema/join.cljc
+++ b/src/metabase/lib/schema/join.cljc
@@ -24,7 +24,7 @@
(mr/def ::fields
[:or
[:enum :all :none]
- ;;; TODO -- Pretty sure fields are supposed to be unique, even excluding `:lib/uuid`
+ ;; TODO -- Pretty sure fields are supposed to be unique, even excluding `:lib/uuid`
[:sequential {:min 1} [:ref ::ref/ref]]])
;;; The name used to alias the joined table or query. This is usually generated automatically and generally looks
diff --git a/src/metabase/lib/stage.cljc b/src/metabase/lib/stage.cljc
index 69734957490139a9d21d2c9140ce086a98f1cedb..24395ec728b6a8ffe3953a2ac7beb772b87ec979 100644
--- a/src/metabase/lib/stage.cljc
+++ b/src/metabase/lib/stage.cljc
@@ -2,6 +2,10 @@
"Method implementations for a stage of a query."
(:require
[clojure.string :as str]
+ [medley.core :as m]
+ [metabase.lib.aggregation :as lib.aggregation]
+ [metabase.lib.breakout :as lib.breakout]
+ [metabase.lib.expression :as lib.expression]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.schema :as lib.schema]
@@ -16,24 +20,6 @@
(declare stage-metadata)
-(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 [field-ref]
- (assoc (lib.metadata.calculation/metadata query stage-number field-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]
- (let [metadata (lib.metadata.calculation/metadata query stage-number aggregation)
- ag-ref [:aggregation {:lib/uuid (str (random-uuid))} i]]
- (assoc metadata :field_ref ag-ref)))
- aggregations)))
-
(mu/defn ^:private fields-columns :- [:maybe [:sequential lib.metadata/ColumnMetadata]]
[query :- ::lib.schema/query
stage-number :- :int]
@@ -58,6 +44,14 @@
[(or position 0) (u/lower-case-en (or field-name ""))])
field-metadatas))
+(defn- ensure-field-refs [field-metadatas]
+ (for [field-metadata field-metadatas]
+ (cond-> field-metadata
+ (not (:field_ref field-metadata))
+ (assoc :field_ref [:field
+ {:lib/uuid (str (random-uuid)), :base-type (:base_type field-metadata)}
+ ((some-fn :id :name) field-metadata)]))))
+
(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]]."
@@ -66,7 +60,8 @@
(when-let [field-metadatas (lib.metadata/fields query table-id)]
(->> field-metadatas
remove-hidden-default-fields
- sort-default-fields)))
+ sort-default-fields
+ ensure-field-refs)))
(mu/defn ^:private default-join-alias :- ::lib.schema.common/non-blank-string
"Generate an alias for a join that doesn't already have one."
@@ -94,7 +89,7 @@
(not (:alias join)) (assoc :alias (unique-name-generator (default-join-alias query stage-number join)))))
joins)))
-(mu/defn ^:private default-columns-added-by-join :- [:sequential lib.metadata/ColumnMetadata]
+(mu/defn ^:private default-columns-added-by-join :- [:maybe [:sequential lib.metadata/ColumnMetadata]]
[query :- ::lib.schema/query
stage-number :- :int
join :- ::lib.schema.join/join]
@@ -109,7 +104,7 @@
(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]
+(mu/defn ^:private default-columns :- [:sequential lib.metadata/ColumnMetadata]
"Calculate the columns to return if `:aggregations`/`:breakout`/`:fields` are unspecified.
Formula for the so-called 'default' columns is
@@ -118,7 +113,9 @@
1b. Default 'visible' Fields for our `:source-table`, OR
- 1c. `:lib/stage-metadata` if this is a `:mbql.stage/native` stage
+ 1c. Metadata associated with a Saved Question, if `:source-table` is a `card__<id>` string, OR
+
+ 1d. `:lib/stage-metadata` if this is a `:mbql.stage/native` stage
PLUS
@@ -132,11 +129,22 @@
(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)))))
+ (or
+ ;; 1b: default visible Fields for the source Table
+ (when (integer? source-table)
+ (source-table-default-fields query source-table))
+ ;; 1c. Metadata associated with a Saved Question, if `:source-table` is a `card__<id>` string, OR
+ (when (string? source-table)
+ (when-let [[_match card-id-str] (re-find #"^card__(\d+)$" source-table)]
+ (when-let [card-id (parse-long card-id-str)]
+ (when-let [result-metadata (:result_metadata (lib.metadata/card query card-id))]
+ (not-empty (for [col (:columns result-metadata)]
+ (assoc col
+ :field_ref [:field
+ {:lib/uuid (str (random-uuid)), :base-type (:base_type col)}
+ (:name col)])))))))
+ ;; 1d: `: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)))
@@ -170,8 +178,8 @@
(or
(not-empty (into []
cat
- [(breakout-columns query stage-number)
- (aggregation-columns query stage-number)
+ [(lib.breakout/breakouts query stage-number)
+ (lib.aggregation/aggregations query stage-number)
(fields-columns query stage-number)]))
(default-columns query stage-number)))))
@@ -199,3 +207,45 @@
descriptions (for [k parts]
(lib.metadata.calculation/describe-top-level-key query stage-number k))]
(str/join ", " (remove str/blank? descriptions))))
+
+(defn- implicitly-joinable-columns
+ "Columns that are implicitly joinable from some other columns in `column-metadatas`. To be joinable, the column has to
+ have appropriate FK metadata, i.e. have an `:fk_target_field_id` pointing to another Field. (I think we only include
+ this information for Databases that support FKs and joins, so I don't think we need to do an additional DB feature
+ check here.)
+
+ This does not include columns from any Tables that are already explicitly joined, and does not include multiple
+ versions of a column when there are multiple pathways to it (i.e. if there is more than one FK to a Table). This
+ behavior matches how things currently work in MLv1, at least for order by; we can adjust as needed in the future if
+ it turns out we do need that stuff.
+
+ Does not include columns that would be implicitly joinable via multiple hops."
+ [query column-metadatas]
+ (let [existing-table-ids (into #{} (map :table_id) column-metadatas)]
+ (into []
+ (comp (filter :fk_target_field_id)
+ (m/distinct-by :fk_target_field_id)
+ (map (fn [{source-field-id :id, target-field-id :fk_target_field_id}]
+ (-> (lib.metadata/field query target-field-id)
+ (assoc :source-field-id source-field-id))))
+ (remove #(contains? existing-table-ids (:table_id %)))
+ (m/distinct-by :table_id)
+ (mapcat (fn [{table-id :table_id, :keys [source-field-id]}]
+ (for [field (source-table-default-fields query table-id)]
+ (assoc field :field_ref [:field
+ {:lib/uuid (str (random-uuid))
+ :base-type (:base_type field)
+ :source-field source-field-id}
+ (:id field)])))))
+ column-metadatas)))
+
+(mu/defn visible-columns :- [:sequential lib.metadata/ColumnMetadata]
+ "Columns that are visible inside a given stage of a query. Ignores `:fields`, `:breakout`, and `:aggregation`.
+ Includes columns that are implicitly joinable from other Tables."
+ [query stage-number]
+ (let [query (lib.util/update-query-stage query stage-number dissoc :fields :breakout :aggregation)
+ columns (default-columns query stage-number)]
+ (concat
+ (lib.expression/expressions query stage-number)
+ columns
+ (implicitly-joinable-columns query columns))))
diff --git a/src/metabase/lib/util.cljc b/src/metabase/lib/util.cljc
index e2b49c9446ac36ef053f98609752492f28abcb0f..90ac25a96413db9d4b86666e86c09014e6ab42b5 100644
--- a/src/metabase/lib/util.cljc
+++ b/src/metabase/lib/util.cljc
@@ -140,7 +140,7 @@
stage-number'))
(defn previous-stage-number
- "The index of the previous stage, if there is one."
+ "The index of the previous stage, if there is one. `nil` if there is no previous stage."
[{:keys [stages], :as _query} stage-number]
(let [stage-number (if (neg? stage-number)
(+ (count stages) stage-number)
diff --git a/test/metabase/lib/order_by_test.cljc b/test/metabase/lib/order_by_test.cljc
index eabffcb2fb1759f21250d17f9aa6d21dc392efcd..5326919b1f96151bdd9847df2153d68b4f76b9e7 100644
--- a/test/metabase/lib/order_by_test.cljc
+++ b/test/metabase/lib/order_by_test.cljc
@@ -1,12 +1,16 @@
(ns metabase.lib.order-by-test
(:require
[clojure.test :refer [deftest is testing]]
+ [medley.core :as m]
[metabase.lib.convert :as lib.convert]
[metabase.lib.core :as lib]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.query :as lib.query]
[metabase.lib.test-metadata :as meta]
+ [metabase.lib.test-util :as lib.tu]
+ [metabase.lib.util :as lib.util]
+ [metabase.util :as u]
#?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))))
#?(:cljs (comment metabase.test-runner.assert-exprs.approximately-equal/keep-me))
@@ -135,3 +139,265 @@
;; });
)
+
+(deftest ^:parallel orderable-columns-breakouts-test
+ (testing "If query has aggregations and/or breakouts, you can only order by those."
+ (let [query (-> (lib/query-for-table-name meta/metadata-provider "VENUES")
+ (lib/aggregate (lib/sum (lib/field "VENUES" "PRICE")))
+ (lib/aggregate (lib/avg (lib/+ (lib/field "VENUES" "PRICE") 1)))
+ (lib/breakout (lib/field "VENUES" "CATEGORY_ID")))]
+ (testing (lib.util/format "Query =\n%s" (u/pprint-to-str query))
+ (is (=? [{:database_type "INTEGER"
+ :semantic_type :type/FK
+ :lib/type :metadata/field
+ :table_id (meta/id :venues)
+ :name "CATEGORY_ID"
+ :has_field_values :none
+ :source :breakout
+ :fk_target_field_id (meta/id :categories :id)
+ :field_ref [:field
+ {:base-type :type/Integer, :lib/uuid string?}
+ (meta/id :venues :category-id)]
+ :effective_type :type/Integer
+ :id (meta/id :venues :category-id)
+ :display_name "Category ID"
+ :base_type :type/Integer}
+ {:lib/type :metadata/field
+ :base_type :type/Integer
+ :name "sum_price"
+ :display_name "Sum of Price"
+ :field_ref [:aggregation {:lib/uuid string?, :base-type :type/Integer} 0]
+ :source :aggregation}
+ {:lib/type :metadata/field
+ :base_type :type/Float
+ :name "avg_price_plus_1"
+ :display_name "Average of Price + 1"
+ :field_ref [:aggregation {:lib/uuid string?, :base-type :type/Float} 1]
+ :source :aggregation}]
+ (lib/orderable-columns query)))))))
+
+(deftest ^:parallel orderable-columns-breakouts-with-expression-test
+ (testing "If query has aggregations and/or breakouts, you can only order by those (with an expression)"
+ (let [query (-> (lib/query-for-table-name meta/metadata-provider "VENUES")
+ (lib/expression "Category ID + 1" (lib/+ (lib/field "VENUES" "CATEGORY_ID") 1))
+ (lib/breakout [:expression {:lib/uuid (str (random-uuid))} "Category ID + 1"]))]
+ (testing (lib.util/format "Query =\n%s" (u/pprint-to-str query))
+ (is (=? [{:lib/type :metadata/field
+ :field_ref [:expression {:lib/uuid string?} "Category ID + 1"]
+ :name "Category ID + 1"
+ :display_name "Category ID + 1"
+ :base_type :type/Integer
+ :source :breakout}]
+ (lib/orderable-columns query)))))))
+
+(deftest ^:parallel orderable-columns-test
+ (let [query (lib/query-for-table-name meta/metadata-provider "VENUES")]
+ (testing (lib.util/format "Query =\n%s" (u/pprint-to-str query))
+ (is (=? [{:lib/type :metadata/field
+ :name "ID"
+ :display_name "ID"
+ :id (meta/id :venues :id)
+ :table_id (meta/id :venues)
+ :base_type :type/BigInteger
+ :field_ref [:field {:lib/uuid string?, :base-type :type/BigInteger} (meta/id :venues :id)]}
+ {:lib/type :metadata/field
+ :name "NAME"
+ :display_name "Name"
+ :id (meta/id :venues :name)
+ :table_id (meta/id :venues)
+ :base_type :type/Text
+ :field_ref [:field {:lib/uuid string?, :base-type :type/Text} (meta/id :venues :name)]}
+ {:lib/type :metadata/field
+ :name "CATEGORY_ID"
+ :display_name "Category ID"
+ :id (meta/id :venues :category-id)
+ :table_id (meta/id :venues)
+ :field_ref [:field {:lib/uuid string?, :base-type :type/Integer} (meta/id :venues :category-id)]}
+ {:lib/type :metadata/field
+ :name "LATITUDE"
+ :display_name "Latitude"
+ :id (meta/id :venues :latitude)
+ :table_id (meta/id :venues)
+ :base_type :type/Float
+ :field_ref [:field {:lib/uuid string?, :base-type :type/Float} (meta/id :venues :latitude)]}
+ {:lib/type :metadata/field
+ :name "LONGITUDE"
+ :display_name "Longitude"
+ :id (meta/id :venues :longitude)
+ :table_id (meta/id :venues)
+ :base_type :type/Float
+ :field_ref [:field {:lib/uuid string?, :base-type :type/Float} (meta/id :venues :longitude)]}
+ {:lib/type :metadata/field
+ :name "PRICE"
+ :display_name "Price"
+ :id (meta/id :venues :price)
+ :table_id (meta/id :venues)
+ :base_type :type/Integer
+ :field_ref [:field {:lib/uuid string?, :base-type :type/Integer} (meta/id :venues :price)]}
+ {:lib/type :metadata/field
+ :name "ID"
+ :display_name "ID"
+ :id (meta/id :categories :id)
+ :table_id (meta/id :categories)
+ :base_type :type/BigInteger
+ :field_ref [:field
+ {:lib/uuid string?, :base-type :type/BigInteger, :source-field (meta/id :venues :category-id)}
+ (meta/id :categories :id)]}
+ {:lib/type :metadata/field
+ :name "NAME"
+ :display_name "Name"
+ :id (meta/id :categories :name)
+ :table_id (meta/id :categories)
+ :base_type :type/Text
+ :field_ref [:field
+ {:lib/uuid string?, :base-type :type/Text, :source-field (meta/id :venues :category-id)}
+ (meta/id :categories :name)]}]
+ (lib/orderable-columns query))))))
+
+(deftest ^:parallel orderable-expressions-test
+ (testing "orderable-columns should include expressions"
+ (let [query (-> (lib/query-for-table-name meta/metadata-provider "VENUES")
+ (lib/expression "Category ID + 1" (lib/+ (lib/field "VENUES" "CATEGORY_ID") 1)))]
+ (testing (lib.util/format "Query =\n%s" (u/pprint-to-str query))
+ (is (=? [{:lib/type :metadata/field
+ :base_type :type/Integer
+ :name "category_id_plus_1"
+ :display_name "Category ID + 1"
+ :field_ref [:expression
+ {:lib/uuid string?, :base-type :type/Integer}
+ "Category ID + 1"]
+ :source :expressions}
+ {:id (meta/id :venues :id), :name "ID"}
+ {:id (meta/id :venues :name), :name "NAME"}
+ {:id (meta/id :venues :category-id), :name "CATEGORY_ID"}
+ {:id (meta/id :venues :latitude), :name "LATITUDE"}
+ {:id (meta/id :venues :longitude), :name "LONGITUDE"}
+ {:id (meta/id :venues :price), :name "PRICE"}
+ {:id (meta/id :categories :id), :name "ID"}
+ {:id (meta/id :categories :name), :name "NAME"}]
+ (lib/orderable-columns query)))))))
+
+(defn- FIXME-is-empty
+ "FIXME: Currently no way to add an `:is-empty` clause inline."
+ [expr]
+ (fn [query stage-number]
+ (lib/is-empty query stage-number expr)))
+
+(deftest ^:parallel orderable-expressions-exclude-boolean-expressions-test
+ (testing "orderable-columns should filter out boolean expressions."
+ (let [query (-> (lib/query-for-table-name meta/metadata-provider "VENUES")
+ (lib/expression "Name is empty?" (FIXME-is-empty (lib/field "VENUES" "NAME"))))]
+ (testing (lib.util/format "Query =\n%s" (u/pprint-to-str query))
+ (is (=? [{:id (meta/id :venues :id), :name "ID"}
+ {:id (meta/id :venues :name), :name "NAME"}
+ {:id (meta/id :venues :category-id), :name "CATEGORY_ID"}
+ {:id (meta/id :venues :latitude), :name "LATITUDE"}
+ {:id (meta/id :venues :longitude), :name "LONGITUDE"}
+ {:id (meta/id :venues :price), :name "PRICE"}
+ {:id (meta/id :categories :id), :name "ID"}
+ {:id (meta/id :categories :name), :name "NAME"}]
+ (lib/orderable-columns query)))))))
+
+(defn- FIXME-=
+ "FIXME: [[metabase.lib.core]] doesn't have a test-friendly version of `:=` yet."
+ [a b]
+ (fn [query stage-number]
+ (let [a (if (fn? a)
+ (a query stage-number)
+ a)
+ b (if (fn? b)
+ (b query stage-number)
+ b)]
+ ;; This is another FIXME!
+ #_{:clj-kondo/ignore [:invalid-arity]}
+ (lib/= query stage-number a b))))
+
+(deftest ^:parallel orderable-explicit-joins-test
+ (testing "orderable-columns should include columns from explicit joins"
+ (let [query (-> (lib/query-for-table-name meta/metadata-provider "VENUES")
+ (lib/join (-> (lib/join-clause
+ (meta/table-metadata :categories)
+ (FIXME-=
+ (lib/field "VENUES" "CATEGORY_ID")
+ (lib/with-join-alias (lib/field "CATEGORIES" "ID") "Cat")))
+ (lib/with-join-alias "Cat")
+ (lib/with-join-fields :all))))]
+ (testing (lib.util/format "Query =\n%s" (u/pprint-to-str query))
+ (is (=? [{:id (meta/id :venues :id), :name "ID"}
+ {:id (meta/id :venues :name), :name "NAME"}
+ {:id (meta/id :venues :category-id), :name "CATEGORY_ID"}
+ {:id (meta/id :venues :latitude), :name "LATITUDE"}
+ {:id (meta/id :venues :longitude), :name "LONGITUDE"}
+ {:id (meta/id :venues :price), :name "PRICE"}
+ {:lib/type :metadata/field
+ :name "ID"
+ :display_name "Categories → ID" ; should we be using the explicit alias we gave this join?
+ :source_alias "Cat"
+ :id (meta/id :categories :id)
+ :table_id (meta/id :categories)
+ :base_type :type/BigInteger
+ :field_ref [:field
+ {:lib/uuid string?, :base-type :type/BigInteger, :join-alias "Cat"}
+ (meta/id :categories :id)]}
+ {:lib/type :metadata/field
+ :name "NAME"
+ :display_name "Categories → Name"
+ :source_alias "Cat"
+ :id (meta/id :categories :name)
+ :table_id (meta/id :categories)
+ :base_type :type/Text
+ :field_ref [:field
+ {:lib/uuid string?, :base-type :type/Text, :join-alias "Cat"}
+ (meta/id :categories :name)]}]
+ (lib/orderable-columns query)))))))
+
+(deftest ^:parallel orderable-columns-source-metadata-test
+ (testing "orderable-columns should use metadata for source query."
+ (let [query (lib.tu/query-with-card-source-table)]
+ (testing (lib.util/format "Query =\n%s" (u/pprint-to-str query))
+ (is (=? [{:name "ID"
+ :base_type :type/BigInteger
+ :field_ref [:field {:lib/uuid string?, :base-type :type/BigInteger} "ID"]}
+ {:name "NAME"
+ :base_type :type/Text
+ :field_ref [:field {:lib/uuid string?, :base-type :type/Text} "NAME"]}
+ {:name "CATEGORY_ID"
+ :base_type :type/Integer
+ :field_ref [:field {:lib/uuid string?, :base-type :type/Integer} "CATEGORY_ID"]}
+ {:name "LATITUDE"
+ :base_type :type/Float
+ :field_ref [:field {:lib/uuid string?, :base-type :type/Float} "LATITUDE"]}
+ {:name "LONGITUDE"
+ :base_type :type/Float
+ :field_ref [:field {:lib/uuid string?, :base-type :type/Float} "LONGITUDE"]}
+ {:name "PRICE"
+ :base_type :type/Integer
+ :field_ref [:field {:lib/uuid string?, :base-type :type/Integer} "PRICE"]}]
+ (lib/orderable-columns query)))))))
+
+(deftest ^:parallel orderable-columns-e2e-test
+ (testing "Use the metadata returned by `orderable-columns` to add a new order-by to a query."
+ (let [query (lib/query-for-table-name meta/metadata-provider "VENUES")]
+ (is (=? {:lib/type :mbql/query
+ :database (meta/id)
+ :stages [{:lib/type :mbql.stage/mbql
+ :source-table (meta/id :venues)
+ :lib/options {:lib/uuid string?}}]}
+ query))
+ (testing (lib.util/format "Query =\n%s" (u/pprint-to-str query))
+ (let [orderable-columns (lib/orderable-columns query)
+ col (m/find-first #(= (:id %) (meta/id :venues :name)) orderable-columns)
+ query' (lib/order-by query col)]
+ (is (=? {:lib/type :mbql/query
+ :database (meta/id)
+ :stages [{:lib/type :mbql.stage/mbql
+ :source-table (meta/id :venues)
+ :lib/options {:lib/uuid string?}
+ :order-by [[:asc
+ {:lib/uuid string?}
+ [:field {:lib/uuid string?, :base-type :type/Text} (meta/id :venues :name)]]]}]}
+ query'))
+ (is (=? [[:asc
+ {:lib/uuid string?}
+ [:field {:lib/uuid string?, :base-type :type/Text} (meta/id :venues :name)]]]
+ (lib/order-bys query'))))))))
diff --git a/test/metabase/lib/test_metadata.cljc b/test/metabase/lib/test_metadata.cljc
index e4ea5ec929ee5992f528b5102bfbc86c8bece7d3..9885e057d3234d33fd20ff56113f7610915dfac9 100644
--- a/test/metabase/lib/test_metadata.cljc
+++ b/test/metabase/lib/test_metadata.cljc
@@ -808,7 +808,7 @@
:semantic_type :type/PK
:fingerprint nil}
{:lib/type :metadata/field
- :display_name "NAME"
+ :display_name "NAME" ; TODO -- these display names are icky
:field_ref [:field "NAME" {:base-type :type/Text}]
:name "NAME"
:base_type :type/Text
diff --git a/test/metabase/lib/test_util.cljc b/test/metabase/lib/test_util.cljc
index 16af43435348805a927d2cb7be6bfd745ba0475a..876c165dd2db89febd1101e147d7c88ed27af081 100644
--- a/test/metabase/lib/test_util.cljc
+++ b/test/metabase/lib/test_util.cljc
@@ -64,3 +64,37 @@
(fields [_this table-id] (for [field fields
:when (= (:table_id field) table-id)]
(assoc field :lib/type :metadata/field)))))
+
+(defn composed-metadata-provider
+ "A metadata provider composed of several different `metadata-providers`. Methods try each constituent provider in
+ turn from left to right until one returns a truthy result."
+ [& metadata-providers]
+ (reify metadata.protocols/MetadataProvider
+ (database [_this] (some metadata.protocols/database metadata-providers))
+ (table [_this table-id] (some #(metadata.protocols/table % table-id) metadata-providers))
+ (field [_this field-id] (some #(metadata.protocols/field % field-id) metadata-providers))
+ (card [_this card-id] (some #(metadata.protocols/card % card-id) metadata-providers))
+ (metric [_this metric-id] (some #(metadata.protocols/metric % metric-id) metadata-providers))
+ (segment [_this segment-id] (some #(metadata.protocols/segment % segment-id) metadata-providers))
+ (tables [_this] (some metadata.protocols/tables metadata-providers))
+ (fields [_this table-id] (some #(metadata.protocols/fields % table-id) metadata-providers))))
+
+(def metadata-provider-with-card
+ "[[meta/metadata-provider]], but with a Card with ID 1."
+ (composed-metadata-provider
+ meta/metadata-provider
+ (mock-metadata-provider
+ {:cards [(assoc meta/saved-question
+ :name "My Card"
+ :id 1)]})))
+
+(defn query-with-card-source-table
+ "A query with a `card__<id>` source Table, and a metadata provider that has that Card."
+ []
+ {:lib/type :mbql/query
+ :lib/metadata metadata-provider-with-card
+ :type :pipeline
+ :database (meta/id)
+ :stages [{:lib/type :mbql.stage/mbql
+ :lib/options {:lib/uuid (str (random-uuid))}
+ :source-table "card__1"}]})