From d96c318dbc779f950bfc73af84403ab3b2f44475 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Fri, 2 Jun 2023 12:07:33 -0700
Subject: [PATCH] Use MLv2 for metadata calculation in QP (part 1); make column
name calculation consistent with QP (#29583)
* Simplified impl
* Add #31266 test
* Yay
* Very nice
* Test fixes
* Fix Kondo warnings
---
.../row_level_restrictions_test.clj | 8 +-
.../metabase/driver/druid/query_processor.clj | 8 +-
.../metabase/driver/mongo/query_processor.clj | 14 +-
.../mongo/test/metabase/test/data/mongo.clj | 11 +
shared/src/metabase/mbql/normalize.cljc | 6 +
shared/test/metabase/mbql/normalize_test.cljc | 12 +
src/metabase/driver/sql/query_processor.clj | 31 +-
src/metabase/lib/aggregation.cljc | 53 +-
src/metabase/lib/core.cljc | 3 +-
src/metabase/lib/equality.cljc | 2 +
src/metabase/lib/expression.cljc | 16 +-
src/metabase/lib/field.cljc | 1 -
.../lib/metadata/cached_provider.cljc | 2 +
src/metabase/lib/metadata/calculation.cljc | 6 +-
.../lib/metadata/composed_provider.cljc | 25 +
src/metabase/lib/metadata/protocols.cljc | 8 +-
.../query_processor/middleware/annotate.clj | 222 ++------
.../middleware/pre_alias_aggregations.clj | 10 +-
.../middleware/resolve_joins.clj | 29 +-
src/metabase/query_processor/store.clj | 42 +-
.../store/metadata_provider.clj | 39 ++
.../query_processor/util/nest_query.clj | 9 +-
test/metabase/api/card_test.clj | 2 +-
test/metabase/api/embed_test.clj | 14 +-
.../driver/sql/query_processor_test.clj | 105 ++--
test/metabase/lib/aggregation_test.cljc | 48 +-
test/metabase/lib/expression_test.cljc | 2 +-
test/metabase/lib/field_test.cljc | 23 +-
test/metabase/lib/join_test.cljc | 3 +-
.../lib/metadata/composed_provider_test.cljc | 28 ++
test/metabase/lib/order_by_test.cljc | 4 +-
test/metabase/lib/remove_replace_test.cljc | 4 +-
test/metabase/lib/stage_test.cljc | 12 +-
test/metabase/lib/test_util.cljc | 43 +-
.../middleware/add_source_metadata_test.clj | 174 ++++---
.../middleware/annotate_test.clj | 475 +++++++++---------
.../pre_alias_aggregations_test.clj | 95 ++--
test/metabase/query_processor/pivot_test.clj | 8 +-
.../util/add_alias_info_test.clj | 38 +-
.../query_processor/util/nest_query_test.clj | 22 +-
.../query_processor_test/aggregation_test.clj | 154 +++---
.../query_processor_test/filter_test.clj | 6 +-
.../nested_queries_test.clj | 42 +-
.../server/middleware/session_test.clj | 14 +-
test/metabase/test/data.clj | 2 +-
test/metabase/test/data/h2.clj | 10 +-
test/metabase/test/data/interface.clj | 15 +-
test/metabase/test/data/sql.clj | 3 +
48 files changed, 1039 insertions(+), 864 deletions(-)
create mode 100644 src/metabase/lib/metadata/composed_provider.cljc
create mode 100644 src/metabase/query_processor/store/metadata_provider.clj
create mode 100644 test/metabase/lib/metadata/composed_provider_test.cljc
diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
index cb24be83d08..469aa6c0e8b 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
@@ -215,7 +215,7 @@
:condition [:= $venue_id &v.venues.id]}]
:aggregation [[:count]]}
- ::row-level-restrictions/original-metadata [{:base_type :type/BigInteger
+ ::row-level-restrictions/original-metadata [{:base_type :type/Integer
:semantic_type :type/Quantity
:name "count"
:display_name "Count"
@@ -229,8 +229,10 @@
:joins [{:source-table $$venues
:alias "v"
:strategy :left-join
- :condition [:= $venue_id &v.venues.id]}]}))))))
+ :condition [:= $venue_id &v.venues.id]}]}))))))))
+(deftest middleware-native-quest-test
+ (testing "Make sure the middleware does the correct transformation given the GTAPs we have"
(testing "Should substitute appropriate value in native query"
(met/with-gtaps {:gtaps {:venues (venues-category-native-gtap-def)}
:attributes {"cat" 50}}
@@ -243,7 +245,7 @@
"ORDER BY \"PUBLIC\".\"VENUES\".\"ID\" ASC")
:params []}}
- ::row-level-restrictions/original-metadata [{:base_type :type/BigInteger
+ ::row-level-restrictions/original-metadata [{:base_type :type/Integer
:semantic_type :type/Quantity
:name "count"
:display_name "Count"
diff --git a/modules/drivers/druid/src/metabase/driver/druid/query_processor.clj b/modules/drivers/druid/src/metabase/driver/druid/query_processor.clj
index 989e02e21b8..d53acd48edc 100644
--- a/modules/drivers/druid/src/metabase/driver/druid/query_processor.clj
+++ b/modules/drivers/druid/src/metabase/driver/druid/query_processor.clj
@@ -669,8 +669,8 @@
{:aggregations [(ag:doubleMax ag-field (or output-name :max))]}])))
(s/defn ^:private handle-aggregation
- [query-type, ag-clause :- mbql.s/Aggregation, druid-query]
- (let [output-name (annotate/aggregation-name ag-clause)
+ [query-type ag-clause :- mbql.s/Aggregation druid-query]
+ (let [output-name (annotate/aggregation-name *query* ag-clause)
[ag-type ag-field & args] (mbql.u/match-one ag-clause
[:aggregation-options ag & _] #_:clj-kondo/ignore (recur ag)
_ &match)]
@@ -727,11 +727,11 @@
;; If it's a named expression, we want to preserve the included name, so recurse, but merge in the name
[:aggregation-options ag _]
(merge (expression-post-aggregation (second expression))
- {:name (annotate/aggregation-name expression)})
+ {:name (annotate/aggregation-name *query* expression)})
_
{:type :arithmetic
- :name (annotate/aggregation-name expression)
+ :name (annotate/aggregation-name *query* expression)
:fn operator
:fields (vec (for [arg args]
(mbql.u/match-one arg
diff --git a/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj b/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj
index 30aab14b58b..a7acd4a2cab 100644
--- a/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj
+++ b/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj
@@ -222,7 +222,7 @@
;;
(defmethod ->lvalue :aggregation
[[_ index]]
- (annotate/aggregation-name (mbql.u/aggregation-at-index *query* index *nesting-level*)))
+ (annotate/aggregation-name (:query *query*) (mbql.u/aggregation-at-index *query* index *nesting-level*)))
(defmethod ->lvalue :field
[[_ id-or-name _props :as field]]
@@ -976,7 +976,7 @@
(for [field-or-expr breakout-fields]
[(field-alias field-or-expr) (format "$_id.%s" (field-alias field-or-expr))])
(for [ag aggregations
- :let [ag-name (annotate/aggregation-name ag)]]
+ :let [ag-name (annotate/aggregation-name (:query *query*) ag)]]
[ag-name (if (mbql.u/is-clause? :distinct (unwrap-named-ag ag))
{$size (str \$ ag-name)}
true)])))
@@ -1007,7 +1007,7 @@
pred)]
{:group {(subs count-where-expr 1) (aggregation->rvalue [:count-where pred])
(subs count-expr 1) (aggregation->rvalue [:count])}
- :post [{(annotate/aggregation-name ag) {$divide [count-where-expr count-expr]}}]}))
+ :post [{(annotate/aggregation-name (:query *query*) ag) {$divide [count-where-expr count-expr]}}]}))
;; MongoDB doesn't have a variance operator, but you calculate it by taking the square of the standard deviation.
;; However, `$pow` is not allowed in the `$group` stage. So calculate standard deviation in the
@@ -1016,11 +1016,11 @@
(let [[_ expr] (unwrap-named-ag ag)
stddev-expr (name (gensym "$stddev-"))]
{:group {(subs stddev-expr 1) (aggregation->rvalue [:stddev expr])}
- :post [{(annotate/aggregation-name ag) {:$pow [stddev-expr 2]}}]}))
+ :post [{(annotate/aggregation-name (:query *query*) ag) {:$pow [stddev-expr 2]}}]}))
(defmethod expand-aggregation :default
[ag]
- {:group {(annotate/aggregation-name ag) (aggregation->rvalue ag)}})
+ {:group {(annotate/aggregation-name (:query *query*) ag) (aggregation->rvalue ag)}})
(defn- extract-aggregations
"Extract aggregation expressions embedded in `aggr-expr` using `parent-name`
@@ -1055,7 +1055,7 @@
(aggregation-op op)
(let [aliases-taken (set (vals aggregations-seen))
- aggr-name (annotate/aggregation-name aggr-expr)
+ aggr-name (annotate/aggregation-name (:query *query*) aggr-expr)
desired-alias (str parent-name "~" aggr-name)
;; find a free alias by appending increasing integers
;; to the desired alias
@@ -1098,7 +1098,7 @@
fields generated by the groups. Each map in the `:post` vector may (and
usually does) refer to the fields introduced by the preceding maps."
[aggr-expr]
- (let [aggr-name (annotate/aggregation-name aggr-expr)
+ (let [aggr-name (annotate/aggregation-name (:query *query*) aggr-expr)
[aggr-expr' aggregations-seen] (simplify-extracted-aggregations
aggr-name
(extract-aggregations aggr-expr aggr-name))
diff --git a/modules/drivers/mongo/test/metabase/test/data/mongo.clj b/modules/drivers/mongo/test/metabase/test/data/mongo.clj
index c362a93ed69..59d1b6c0517 100644
--- a/modules/drivers/mongo/test/metabase/test/data/mongo.clj
+++ b/modules/drivers/mongo/test/metabase/test/data/mongo.clj
@@ -124,3 +124,14 @@
{:$sort {"_id" 1}}
{:$project {"_id" false, "count" true}}])
:collection (name table-name)})
+
+(defmethod tx/aggregate-column-info :mongo
+ ([driver ag-type]
+ (merge ((get-method tx/aggregate-column-info ::tx/test-extensions) driver ag-type)
+ (when (#{:count :cum-count} ag-type)
+ {:base_type :type/Integer})))
+
+ ([driver ag-type column-metadata]
+ (merge ((get-method tx/aggregate-column-info ::tx/test-extensions) driver ag-type column-metadata)
+ (when (= ag-type :cum-count)
+ {:base_type :type/Integer}))))
diff --git a/shared/src/metabase/mbql/normalize.cljc b/shared/src/metabase/mbql/normalize.cljc
index 9aabf5b8189..b14f57a9a44 100644
--- a/shared/src/metabase/mbql/normalize.cljc
+++ b/shared/src/metabase/mbql/normalize.cljc
@@ -453,6 +453,12 @@
;; afterwards
[:field id-or-name (not-empty opts)]))
+(defmethod canonicalize-mbql-clause :aggregation
+ [[_tag index opts]]
+ (if (empty? opts)
+ [:aggregation index]
+ [:aggregation index opts]))
+
;;; legacy Field clauses
(defmethod canonicalize-mbql-clause :field-id
diff --git a/shared/test/metabase/mbql/normalize_test.cljc b/shared/test/metabase/mbql/normalize_test.cljc
index 378e0b85184..884560edbc5 100644
--- a/shared/test/metabase/mbql/normalize_test.cljc
+++ b/shared/test/metabase/mbql/normalize_test.cljc
@@ -1447,3 +1447,15 @@
[:field "date_seen" {:base-type :type/Date}]
"2021-05-01T12:30:00"]}}
(mbql.normalize/normalize query))))))
+
+(t/deftest ^:parallel normalize-aggregation-ref-test
+ (t/are [clause] (= {:database 1
+ :type :query
+ :query {:order-by [[:asc [:aggregation 0]]]}}
+ (metabase.mbql.normalize/normalize
+ {:database 1
+ :type :query
+ :query {:order-by [[:asc clause]]}}))
+ [:aggregation 0 nil]
+ [:aggregation 0 {}]
+ [:aggregation 0]))
diff --git a/src/metabase/driver/sql/query_processor.clj b/src/metabase/driver/sql/query_processor.clj
index 81957c8929c..4ab0ae757ed 100644
--- a/src/metabase/driver/sql/query_processor.clj
+++ b/src/metabase/driver/sql/query_processor.clj
@@ -1028,12 +1028,13 @@
;;; -------------------------------------------------- aggregation ---------------------------------------------------
(defmethod apply-top-level-clause [:sql :aggregation]
- [driver _ honeysql-form {aggregations :aggregation}]
+ [driver _top-level-clause honeysql-form {aggregations :aggregation, :as inner-query}]
(let [honeysql-ags (vec (for [ag aggregations
:let [ag-expr (->honeysql driver ag)
+ ag-name (annotate/aggregation-name inner-query ag)
ag-alias (->honeysql driver (hx/identifier
:field-alias
- (driver/escape-alias driver (annotate/aggregation-name ag))))]]
+ (driver/escape-alias driver ag-name)))]]
(case (long hx/*honey-sql-version*)
1 [ag-expr ag-alias]
2 [ag-expr [ag-alias]])))]
@@ -1371,13 +1372,19 @@
(defn- apply-top-level-clauses
"`apply-top-level-clause` for all of the top-level clauses in `inner-query`, progressively building a HoneySQL form.
Clauses are applied according to the order in `top-level-clause-application-order`."
- [driver honeysql-form inner-query]
- (->> (reduce
- (fn [honeysql-form k]
- (apply-top-level-clause driver k honeysql-form inner-query))
- honeysql-form
- (query->keys-in-application-order inner-query))
- (add-default-select driver)))
+ ([driver honeysql-form inner-query]
+ (apply-top-level-clauses driver honeysql-form inner-query identity))
+
+ ([driver honeysql-form inner-query xform]
+ (transduce
+ xform
+ (fn
+ ([honeysql-form]
+ (add-default-select driver honeysql-form))
+ ([honeysql-form k]
+ (apply-top-level-clause driver k honeysql-form inner-query)))
+ honeysql-form
+ (query->keys-in-application-order inner-query))))
(declare apply-clauses)
@@ -1403,7 +1410,7 @@
2 [table-alias]))]]))
(defn- apply-clauses
- "Like `apply-top-level-clauses`, but handles `source-query` as well, which needs to be handled in a special way
+ "Like [[apply-top-level-clauses]], but handles `source-query` as well, which needs to be handled in a special way
because it is aliased."
[driver honeysql-form {:keys [source-query], :as inner-query}]
(binding [*inner-query* inner-query]
@@ -1411,7 +1418,9 @@
(apply-top-level-clauses
driver
(apply-source-query driver honeysql-form inner-query)
- (dissoc inner-query :source-query))
+ inner-query
+ ;; don't try to do anything with the source query recursively.
+ (remove (partial = :source-query)))
(apply-top-level-clauses driver honeysql-form inner-query))))
(defmulti preprocess
diff --git a/src/metabase/lib/aggregation.cljc b/src/metabase/lib/aggregation.cljc
index 5e09a68844d..07b74dc4d2c 100644
--- a/src/metabase/lib/aggregation.cljc
+++ b/src/metabase/lib/aggregation.cljc
@@ -1,7 +1,6 @@
(ns metabase.lib.aggregation
(:refer-clojure :exclude [count distinct max min var])
(:require
- [clojure.math :as math]
[medley.core :as m]
[metabase.lib.common :as lib.common]
[metabase.lib.equality :as lib.equality]
@@ -86,14 +85,16 @@
:count (i18n/tru "Count")
:cum-count (i18n/tru "Cumulative count"))))
-(defmethod lib.metadata.calculation/column-name-method :count
- [query stage-number [tag _opts x]]
- (let [prefix (case tag
- :count "count"
- :cum-count "cum_count")]
- (if x
- (str prefix \_ (lib.metadata.calculation/column-name query stage-number x))
- prefix)))
+(defmethod lib.metadata.calculation/column-name-method ::count-aggregation
+ [_query _stage-number [tag :as _clause]]
+ (case tag
+ :count "count"
+ :cum-count "cum_count"))
+
+(defmethod lib.metadata.calculation/metadata-method ::count-aggregation
+ [query stage-number clause]
+ (assoc ((get-method lib.metadata.calculation/metadata-method ::aggregation) query stage-number clause)
+ :semantic-type :type/Quantity))
(defmethod lib.metadata.calculation/display-name-method :case
[_query _stage-number _case _style]
@@ -119,20 +120,18 @@
(lib.hierarchy/derive tag ::unary-aggregation))
(defmethod lib.metadata.calculation/column-name-method ::unary-aggregation
- [query stage-number [tag _opts arg]]
- (let [arg (lib.metadata.calculation/column-name-method query stage-number arg)]
- (str
- (case tag
- :avg "avg_"
- :cum-sum "cum_sum_"
- :distinct "distinct_"
- :max "max_"
- :median "median_"
- :min "min_"
- :stddev "std_dev_"
- :sum "sum_"
- :var "var_")
- arg)))
+ [_query _stage-number [tag _opts _arg]]
+ (case tag
+ :avg "avg"
+ :cum-sum "sum"
+ :distinct "count"
+ :max "max"
+ :median "median"
+ :min "min"
+ :stddev "stddev"
+ :sum "sum"
+ :var "var"))
+
(defmethod lib.metadata.calculation/display-name-method ::unary-aggregation
[query stage-number [tag _opts arg] style]
@@ -153,12 +152,8 @@
(i18n/tru "{0}th percentile of {1}" p (lib.metadata.calculation/display-name query stage-number x style)))
(defmethod lib.metadata.calculation/column-name-method :percentile
- [query stage-number [_percentile _opts x p]]
- ;; if `p` is between `0` and `1` then just use the first two digits for the name, e.g. `p95_whatever`
- (let [p (if (< 0 p 1)
- (int (math/round (* p 100.0)))
- p)]
- (lib.util/format "p%s_%s" p (lib.metadata.calculation/column-name query stage-number x))))
+ [_query _stage-number _clause]
+ "percentile")
(lib.hierarchy/derive :percentile ::aggregation)
diff --git a/src/metabase/lib/core.cljc b/src/metabase/lib/core.cljc
index 89faa7f9aab..2e45cb45f63 100644
--- a/src/metabase/lib/core.cljc
+++ b/src/metabase/lib/core.cljc
@@ -91,7 +91,8 @@
field
query-for-table-id
query-for-table-name
- table]
+ table
+ ref-lookup]
[lib.expression
expression
expressions
diff --git a/src/metabase/lib/equality.cljc b/src/metabase/lib/equality.cljc
index ea1576d7275..e431da8475a 100644
--- a/src/metabase/lib/equality.cljc
+++ b/src/metabase/lib/equality.cljc
@@ -87,6 +87,8 @@
(sequential? x) ((get-method = :dispatch-type/sequential) x y)
:else (clojure.core/= x y)))
+;;; TODO I think to field refs with different `:base-type`s but with other info the same should probably be considered
+;;; the same, right? Like if we improve type calculation it shouldn't break existing queries
(defn ref=
"Are two refs `x` and `y` equal?"
[x y]
diff --git a/src/metabase/lib/expression.cljc b/src/metabase/lib/expression.cljc
index 13e369dc947..9f04ef125da 100644
--- a/src/metabase/lib/expression.cljc
+++ b/src/metabase/lib/expression.cljc
@@ -110,21 +110,9 @@
[query stage-number [tag _opts & args] _style]
(infix-display-name query stage-number (get infix-operator-display-name tag) args))
-(defn- infix-column-name
- [query stage-number operator-str args]
- (str/join (str \_ operator-str \_)
- (map (partial lib.metadata.calculation/column-name query stage-number)
- args)))
-
-(def ^:private infix-operator-column-name
- {:+ "plus"
- :- "minus"
- :/ "divided_by"
- :* "times"})
-
(defmethod lib.metadata.calculation/column-name-method ::infix-operator
- [query stage-number [tag _opts & args]]
- (infix-column-name query stage-number (get infix-operator-column-name tag) args))
+ [_query _stage-number _expr]
+ "expression")
;;; `:+`, `:-`, and `:*` all have the same logic; also used for [[metabase.lib.schema.expression/type-of]].
;;;
diff --git a/src/metabase/lib/field.cljc b/src/metabase/lib/field.cljc
index a9e26602a10..08598b9f331 100644
--- a/src/metabase/lib/field.cljc
+++ b/src/metabase/lib/field.cljc
@@ -192,7 +192,6 @@
join-display-name (when (= style :long)
(or
(when fk-field-id
-
;; Implicitly joined column pickers don't use the target table's name, they use the FK field's name with
;; "ID" dropped instead.
;; This is very intentional: one table might have several FKs to one foreign table, each with different
diff --git a/src/metabase/lib/metadata/cached_provider.cljc b/src/metabase/lib/metadata/cached_provider.cljc
index b540370c161..e5f8ef39f6c 100644
--- a/src/metabase/lib/metadata/cached_provider.cljc
+++ b/src/metabase/lib/metadata/cached_provider.cljc
@@ -49,6 +49,8 @@
(for [id ids]
(get-in-cache cache [metadata-type id]))))
+;;; wraps another metadata provider and caches results. Implements
+;;; the [[lib.metadata.protocols/CachedMetadataProvider]] protocol which allows warming the cache before use.
(deftype CachedProxyMetadataProvider [cache metadata-provider]
lib.metadata.protocols/MetadataProvider
(database [_this] (get-in-cache-or-fetch cache [:metadata/database] #(lib.metadata.protocols/database metadata-provider)))
diff --git a/src/metabase/lib/metadata/calculation.cljc b/src/metabase/lib/metadata/calculation.cljc
index 373776cef5a..45740d614bf 100644
--- a/src/metabase/lib/metadata/calculation.cljc
+++ b/src/metabase/lib/metadata/calculation.cljc
@@ -27,6 +27,10 @@
name generation. For a joined column, this might look like \"Venues → Price\"."
[:enum :default :long])
+(def ^:dynamic *display-name-style*
+ "Display name style to use when not explicitly passed in to [[display-name]]."
+ :default)
+
(defmulti display-name-method
"Calculate a nice human-friendly display name for something."
{:arglists '([query stage-number x display-name-style])}
@@ -48,7 +52,7 @@
(display-name query -1 x))
([query stage-number x]
- (display-name query stage-number x :default))
+ (display-name query stage-number x *display-name-style*))
([query :- ::lib.schema/query
stage-number :- :int
diff --git a/src/metabase/lib/metadata/composed_provider.cljc b/src/metabase/lib/metadata/composed_provider.cljc
new file mode 100644
index 00000000000..63881417053
--- /dev/null
+++ b/src/metabase/lib/metadata/composed_provider.cljc
@@ -0,0 +1,25 @@
+(ns metabase.lib.metadata.composed-provider
+ (:require
+ [clojure.core.protocols]
+ [clojure.datafy :as datafy]
+ [medley.core :as m]
+ [metabase.lib.metadata.protocols :as metadata.protocols]))
+
+(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] (m/distinct-by :id (mapcat metadata.protocols/tables metadata-providers)))
+ (fields [_this table-id] (m/distinct-by :id (mapcat #(metadata.protocols/fields % table-id) metadata-providers)))
+
+ clojure.core.protocols/Datafiable
+ (datafy [_this]
+ (cons `composed-metadata-provider (map datafy/datafy metadata-providers)))))
diff --git a/src/metabase/lib/metadata/protocols.cljc b/src/metabase/lib/metadata/protocols.cljc
index 6b8045930e9..9df210d30a0 100644
--- a/src/metabase/lib/metadata/protocols.cljc
+++ b/src/metabase/lib/metadata/protocols.cljc
@@ -69,9 +69,11 @@
(satisfies? MetadataProvider x))
(#?(:clj p/defprotocol+ :cljs defprotocol) CachedMetadataProvider
- "Optional. A protocol for a MetadataProvider that some sort of internal cache. This is mostly useful for MetadataProviders that
- can hit some sort of relatively expensive external service,
- e.g. [[metabase.lib.metadata.jvm/application-database-metadata-provider]].
+ "Optional. A protocol for a MetadataProvider that some sort of internal cache. This is mostly useful for
+ MetadataProviders that can hit some sort of relatively expensive external service,
+ e.g. [[metabase.lib.metadata.jvm/application-database-metadata-provider]]. The main purpose of this is to allow
+ pre-warming the cache with stuff that was already fetched elsewhere.
+ See [[metabase.models.metric/warmed-metadata-provider]] for example.
See [[cached-metadata-provider]] below to wrap for a way to wrap an existing MetadataProvider to add caching on top
of it."
diff --git a/src/metabase/query_processor/middleware/annotate.clj b/src/metabase/query_processor/middleware/annotate.clj
index dc847f145e7..393d69bb688 100644
--- a/src/metabase/query_processor/middleware/annotate.clj
+++ b/src/metabase/query_processor/middleware/annotate.clj
@@ -4,21 +4,26 @@
[clojure.set :as set]
[clojure.string :as str]
[medley.core :as m]
- [metabase.driver :as driver]
[metabase.driver.common :as driver.common]
- [metabase.mbql.predicates :as mbql.preds]
+ [metabase.lib.convert :as lib.convert]
+ [metabase.lib.core :as lib]
+ [metabase.lib.metadata.calculation :as lib.metadata.calculation]
+ [metabase.lib.schema.common :as lib.schema.common]
+ [metabase.mbql.normalize :as mbql.normalize]
[metabase.mbql.schema :as mbql.s]
[metabase.mbql.util :as mbql.u]
[metabase.mbql.util.match :as mbql.match]
[metabase.models.humanization :as humanization]
[metabase.query-processor.error-type :as qp.error-type]
- [metabase.query-processor.middleware.escape-join-aliases :as escape-join-aliases]
+ [metabase.query-processor.middleware.escape-join-aliases
+ :as escape-join-aliases]
[metabase.query-processor.reducible :as qp.reducible]
[metabase.query-processor.store :as qp.store]
[metabase.query-processor.util :as qp.util]
[metabase.sync.analyze.fingerprint.fingerprinters :as fingerprinters]
[metabase.util :as u]
[metabase.util.i18n :refer [deferred-tru tru]]
+ [metabase.util.malli :as mu]
[metabase.util.schema :as su]
[schema.core :as s]))
@@ -307,176 +312,53 @@
(throw (ex-info (tru "Don''t know how to get information about Field: {0}" &match)
{:field &match}))))
-
-;;; ---------------------------------------------- Aggregate Field Info ----------------------------------------------
-
-(defn- expression-arg-display-name
- "Generate an appropriate name for an `arg` in an expression aggregation."
- [recursive-name-fn arg]
- (mbql.u/match-one arg
- ;; if the arg itself is a nested expression, recursively find a name for it, and wrap in parens
- [(_ :guard #{:+ :- :/ :*}) & _]
- (str "(" (recursive-name-fn &match) ")")
-
- ;; if the arg is another aggregation, recurse to get its name. (Only aggregations, nested expressions, or numbers
- ;; are allowed as args to expression aggregations; thus anything that's an MBQL clause, but not a nested
- ;; expression, is a ag clause.)
- [(_ :guard keyword?) & _]
- (recursive-name-fn &match)
-
- ;; otherwise for things like numbers just use that directly
- _ &match))
-
-(s/defn aggregation-name :- su/NonBlankString
+(defn- mlv2-query [inner-query]
+ (qp.store/cached [:mlv2-query (hash inner-query)]
+ (try
+ (lib/query
+ (qp.store/metadata-provider)
+ (lib.convert/->pMBQL (lib.convert/legacy-query-from-inner-query
+ (:id (qp.store/database))
+ (mbql.normalize/normalize-fragment [:query] inner-query))))
+ (catch Throwable e
+ (throw (ex-info (tru "Error converting query to pMBQL: {0}" (ex-message e))
+ {:inner-query inner-query, :type qp.error-type/qp}
+ e))))))
+
+(s/defn ^:private col-info-for-aggregation-clause
+ "Return appropriate column metadata for an `:aggregation` clause."
+ ;; `clause` is normally an aggregation clause but this function can call itself recursively; see comments by the
+ ;; `match` pattern for field clauses below
+ [inner-query :- su/Map clause]
+ (let [mlv2-clause (lib.convert/->pMBQL clause)]
+ ;; for some mystery reason it seems like the annotate code uses `:long` style display names when something appears
+ ;; inside an aggregation clause, e.g.
+ ;;
+ ;; Distinct values of Category → Name
+ ;;
+ ;; but `:default` style names when they appear on their own or in breakouts, e.g.
+ ;;
+ ;; Name
+ ;;
+ ;; why is this the case? Who knows! But that's the old pre-MLv2 behavior. I think we should try to fix it, but it's
+ ;; probably going to involve updating a ton of tests that encode the old behavior.
+ (binding [lib.metadata.calculation/*display-name-style* :long]
+ (-> (lib.metadata.calculation/metadata (mlv2-query inner-query) -1 mlv2-clause)
+ (update-keys u/->snake_case_en)
+ (dissoc :lib/type)))))
+
+(mu/defn aggregation-name :- ::lib.schema.common/non-blank-string
"Return an appropriate aggregation name/alias *used inside a query* for an `:aggregation` subclause (an aggregation
or expression). Takes an options map as schema won't support passing keypairs directly as a varargs.
These names are also used directly in queries, e.g. in the equivalent of a SQL `AS` clause."
- [ag-clause :- mbql.s/Aggregation]
- (when-not driver/*driver*
- (throw (Exception. (tru "*driver* is unbound."))))
- (mbql.u/match-one ag-clause
- [:aggregation-options _ (options :guard :name)]
- (:name options)
-
- [:aggregation-options ag _]
- #_:clj-kondo/ignore
- (recur ag)
-
- ;; For unnamed expressions, just compute a name like "sum + count"
- ;; Top level expressions need a name without a leading __ as those are automatically removed from the results
- [(operator :guard #{:+ :- :/ :*}) & args]
- "expression"
-
- ;; for historic reasons a distinct count clause is still named "count". Don't change this or you might break FE
- ;; stuff that keys off of aggregation names.
- ;;
- ;; `cum-count` and `cum-sum` get the same names as the non-cumulative versions, due to limitations (they are
- ;; written out of the query before we ever see them). For other code that makes use of this function give them
- ;; the correct names to expect.
- [(_ :guard #{:distinct :cum-count}) _]
- "count"
-
- [:cum-sum _]
- "sum"
-
- ;; for any other aggregation just use the name of the clause e.g. `sum`.
- [clause-name & _]
- (name clause-name)))
-
-(declare aggregation-display-name)
-
-(s/defn ^:private aggregation-arg-display-name :- su/NonBlankString
- "Name to use for an aggregation clause argument such as a Field when constructing the complete aggregation name."
- [inner-query, ag-arg :- Object]
- (or (when (mbql.preds/Field? ag-arg)
- (when-let [info (col-info-for-field-clause inner-query ag-arg)]
- (some info [:display_name :name])))
- (aggregation-display-name inner-query ag-arg)))
-
-(s/defn aggregation-display-name :- su/NonBlankString
- "Return an appropriate user-facing display name for an aggregation clause."
- [inner-query ag-clause]
- (mbql.u/match-one ag-clause
- [:aggregation-options _ (options :guard :display-name)]
- (:display-name options)
-
- [:aggregation-options ag _]
- #_:clj-kondo/ignore
- (recur ag)
-
- [(operator :guard #{:+ :- :/ :*}) & args]
- (str/join (format " %s " (name operator))
- (for [arg args]
- (expression-arg-display-name (partial aggregation-arg-display-name inner-query) arg)))
-
- [:count] (tru "Count")
- [:case] (tru "Case")
- [:distinct arg] (tru "Distinct values of {0}" (aggregation-arg-display-name inner-query arg))
- [:count arg] (tru "Count of {0}" (aggregation-arg-display-name inner-query arg))
- [:avg arg] (tru "Average of {0}" (aggregation-arg-display-name inner-query arg))
- ;; cum-count and cum-sum get names for count and sum, respectively (see explanation in `aggregation-name`)
- [:cum-count arg] (tru "Count of {0}" (aggregation-arg-display-name inner-query arg))
- [:cum-sum arg] (tru "Sum of {0}" (aggregation-arg-display-name inner-query arg))
- [:stddev arg] (tru "SD of {0}" (aggregation-arg-display-name inner-query arg))
- [:sum arg] (tru "Sum of {0}" (aggregation-arg-display-name inner-query arg))
- [:min arg] (tru "Min of {0}" (aggregation-arg-display-name inner-query arg))
- [:max arg] (tru "Max of {0}" (aggregation-arg-display-name inner-query arg))
- [:var arg] (tru "Variance of {0}" (aggregation-arg-display-name inner-query arg))
- [:median arg] (tru "Median of {0}" (aggregation-arg-display-name inner-query arg))
- [:percentile arg p] (tru "{0}th percentile of {1}" p (aggregation-arg-display-name inner-query arg))
-
- ;; until we have a way to generate good names for filters we'll just have to say 'matching condition' for now
- [:sum-where arg _] (tru "Sum of {0} matching condition" (aggregation-arg-display-name inner-query arg))
- [:share _] (tru "Share of rows matching condition")
- [:count-where _] (tru "Count of rows matching condition")
-
- (_ :guard mbql.preds/Field?)
- (:display_name (col-info-for-field-clause inner-query ag-clause))
-
- _
- (aggregation-name ag-clause)))
-
-(defn- ag->name-info [inner-query ag]
- {:name (aggregation-name ag)
- :display_name (aggregation-display-name inner-query ag)})
-
-(s/defn col-info-for-aggregation-clause
- "Return appropriate column metadata for an `:aggregation` clause."
- ;; `clause` is normally an aggregation clause but this function can call itself recursively; see comments by the
- ;; `match` pattern for field clauses below
- [inner-query :- su/Map, clause]
- (mbql.u/match-one clause
- ;; ok, if this is a aggregation w/ options recurse so we can get information about the ag it wraps
- [:aggregation-options ag _]
- (merge
- (col-info-for-aggregation-clause inner-query ag)
- (ag->name-info inner-query &match))
-
- ;; Always treat count or distinct count as an integer even if the DB in question returns it as something
- ;; wacky like a BigDecimal or Float
- [(_ :guard #{:count :distinct}) & args]
- (merge
- (col-info-for-aggregation-clause inner-query args)
- {:base_type :type/BigInteger
- :semantic_type :type/Quantity}
- (ag->name-info inner-query &match))
-
- [:count-where _]
- (merge
- {:base_type :type/Integer
- :semantic_type :type/Quantity}
- (ag->name-info inner-query &match))
-
- [:share _]
- (merge
- {:base_type :type/Float
- :semantic_type :type/Share}
- (ag->name-info inner-query &match))
-
- ;; get info from a Field if we can (theses Fields are matched when ag clauses recursively call
- ;; `col-info-for-ag-clause`, and this info is added into the results)
- (_ :guard mbql.preds/Field?)
- (select-keys (col-info-for-field-clause inner-query &match) [:base_type :semantic_type :settings])
- #{:expression :+ :- :/ :*}
- (merge
- (infer-expression-type &match)
- (when (mbql.preds/Aggregation? &match)
- (ag->name-info inner-query &match)))
-
- ;; the type returned by a case statement depends on what its expressions are; we'll just return the type info for
- ;; the first expression for the time being. I guess it's possible the expression might return a string for one
- ;; case and a number for another, but I think in post cases it should be the same type for every clause.
- [:case & _]
- (merge
- (infer-expression-type &match)
- (ag->name-info inner-query &match))
-
- ;; get name/display-name of this ag
- [(_ :guard keyword?) arg & _]
- (merge
- (col-info-for-aggregation-clause inner-query arg)
- (ag->name-info inner-query &match))))
+ [inner-query :- [:and
+ [:map]
+ [:fn
+ {:error/message "legacy inner-query with :source-table or :source-query"}
+ (some-fn :source-table :source-query)]]
+ ag-clause]
+ (lib.metadata.calculation/column-name (mlv2-query inner-query) (lib.convert/->pMBQL ag-clause)))
;;; ----------------------------------------- Putting it all together (MBQL) -----------------------------------------
@@ -521,8 +403,6 @@
(cols-for-ags-and-breakouts inner-query)
(cols-for-fields inner-query)))
-
-
(s/defn ^:private merge-source-metadata-col :- (s/maybe su/Map)
[source-metadata-col :- (s/maybe su/Map) col :- (s/maybe su/Map)]
(merge
diff --git a/src/metabase/query_processor/middleware/pre_alias_aggregations.clj b/src/metabase/query_processor/middleware/pre_alias_aggregations.clj
index de48ce8639f..9b9844c9eab 100644
--- a/src/metabase/query_processor/middleware/pre_alias_aggregations.clj
+++ b/src/metabase/query_processor/middleware/pre_alias_aggregations.clj
@@ -4,24 +4,24 @@
[metabase.mbql.util :as mbql.u]
[metabase.query-processor.middleware.annotate :as annotate]))
-(defn- ag-name [ag-clause]
- (driver/escape-alias driver/*driver* (annotate/aggregation-name ag-clause)))
+(defn- ag-name [inner-query ag-clause]
+ (driver/escape-alias driver/*driver* (annotate/aggregation-name inner-query ag-clause)))
-(defn- pre-alias-and-uniquify [aggregations]
+(defn- pre-alias-and-uniquify [inner-query aggregations]
(mapv
(fn [original-ag updated-ag]
(if (= original-ag updated-ag)
original-ag
(with-meta updated-ag {:auto-generated? true})))
aggregations
- (mbql.u/pre-alias-and-uniquify-aggregations ag-name aggregations)))
+ (mbql.u/pre-alias-and-uniquify-aggregations (partial ag-name inner-query) aggregations)))
(defn pre-alias-aggregations-in-inner-query
"Make sure all aggregations have aliases, and all aliases are unique, in an 'inner' MBQL query."
[{:keys [aggregation source-query joins], :as inner-query}]
(cond-> inner-query
(seq aggregation)
- (update :aggregation pre-alias-and-uniquify)
+ (update :aggregation (partial pre-alias-and-uniquify inner-query))
source-query
(update :source-query pre-alias-aggregations-in-inner-query)
diff --git a/src/metabase/query_processor/middleware/resolve_joins.clj b/src/metabase/query_processor/middleware/resolve_joins.clj
index f30467812f4..415fdd56457 100644
--- a/src/metabase/query_processor/middleware/resolve_joins.clj
+++ b/src/metabase/query_processor/middleware/resolve_joins.clj
@@ -9,6 +9,7 @@
[metabase.query-processor.middleware.add-implicit-clauses
:as qp.add-implicit-clauses]
[metabase.query-processor.store :as qp.store]
+ [metabase.query-processor.util.add-alias-info :as add]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
[metabase.util.schema :as su]
@@ -117,6 +118,28 @@
[{breakouts :breakout, aggregations :aggregation}]
(every? empty? [aggregations breakouts]))
+(defn- append-join-fields [fields join-fields]
+ (into []
+ (comp cat
+ (m/distinct-by (fn [clause]
+ (-> clause
+ ;; remove namespaced options and other things that are definitely irrelevant
+ add/normalize-clause
+ ;; we shouldn't consider different type info to mean two Fields are different even if
+ ;; everything else is the same. So give everything `:base-type` of `:type/*` (it will
+ ;; complain if we remove `:base-type` entirely from fields with a string name)
+ (mbql.u/update-field-options (fn [opts]
+ (-> opts
+ (assoc :base-type :type/*)
+ (dissoc :effective-type))))))))
+ [fields join-fields]))
+
+(defn append-join-fields-to-fields
+ "Add the fields from join `:fields`, if any, to the parent-level `:fields`."
+ [inner-query join-fields]
+ (cond-> inner-query
+ (seq join-fields) (update :fields append-join-fields join-fields)))
+
(s/defn ^:private merge-joins-fields :- UnresolvedMBQLQuery
"Append the `:fields` from `:joins` into their parent level as appropriate so joined columns appear in the final
query results, and remove the `:fields` entry for all joins.
@@ -132,11 +155,7 @@
(cond-> join
(keyword? fields) (dissoc :fields)))
joins)))]
- (cond-> inner-query
- (seq join-fields) (update :fields (fn [fields]
- (into []
- (comp cat (m/distinct-by mbql.u/remove-namespaced-options))
- [fields join-fields]))))))
+ (append-join-fields-to-fields inner-query join-fields)))
(s/defn ^:private resolve-joins-in-mbql-query :- ResolvedMBQLQuery
[query :- mbql.s/MBQLQuery]
diff --git a/src/metabase/query_processor/store.clj b/src/metabase/query_processor/store.clj
index 7a37c66e614..a7d6d80fd95 100644
--- a/src/metabase/query_processor/store.clj
+++ b/src/metabase/query_processor/store.clj
@@ -13,6 +13,10 @@
but fetching all Fields in a single pass and storing them for reuse is dramatically more efficient than fetching
those Fields potentially dozens of times in a single query execution."
(:require
+ [metabase.lib.metadata.composed-provider
+ :as lib.metadata.composed-provider]
+ [metabase.lib.metadata.jvm :as lib.metadata.jvm]
+ [metabase.lib.metadata.protocols :as lib.metadata.protocols]
[metabase.models.database :refer [Database]]
[metabase.models.field :refer [Field]]
[metabase.models.interface :as mi]
@@ -20,13 +24,19 @@
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
[metabase.util.schema :as su]
+ [pretty.core :as pretty]
[schema.core :as s]
[toucan2.core :as t2]))
+(set! *warn-on-reflection* true)
+
;;; ---------------------------------------------- Setting up the Store ----------------------------------------------
(def ^:private uninitialized-store
- (delay (throw (Exception. (tru "Error: Query Processor store is not initialized.")))))
+ (reify
+ clojure.lang.IDeref
+ (deref [_this]
+ (throw (ex-info (tru "Error: Query Processor store is not initialized.") {})))))
(def ^:private ^:dynamic *store*
"Dynamic var used as the QP store for a given query execution."
@@ -316,3 +326,33 @@
;; for the unique key use a gensym prefixed by the namespace to make for easier store debugging if needed
(let [ks (into [(list 'quote (gensym (str (name (ns-name *ns*)) "/misc-cache-")))] (u/one-or-many k-or-ks))]
`(cached-fn ~ks (fn [] ~@body))))
+
+(defn- base-metadata-provider []
+ (reify
+ lib.metadata.protocols/MetadataProvider
+ (database [_this]
+ (some-> (database) (update-keys u/->kebab-case-en) (assoc :lib/type :metadata/database)))
+
+ (table [_this table-id]
+ (some-> (table table-id) (update-keys u/->kebab-case-en) (assoc :lib/type :metadata/table)))
+
+ (field [_this field-id]
+ (some-> (field field-id) (update-keys u/->kebab-case-en) (assoc :lib/type :metadata/field)))
+
+ (card [_this _card-id] nil)
+ (metric [_this _metric-id] nil)
+ (segment [_this _segment-id] nil)
+ (tables [_metadata-provider] nil)
+ (fields [_metadata-provider _table-id] nil)
+
+ pretty/PrettyPrintable
+ (pretty [_this]
+ `metadata-provider)))
+
+(defn metadata-provider
+ "Create a new MLv2 metadata provider that uses the QP store."
+ []
+ (cached ::metadata-provider
+ (lib.metadata.composed-provider/composed-metadata-provider
+ (base-metadata-provider)
+ (lib.metadata.jvm/application-database-metadata-provider (:id (database))))))
diff --git a/src/metabase/query_processor/store/metadata_provider.clj b/src/metabase/query_processor/store/metadata_provider.clj
new file mode 100644
index 00000000000..ebbb135a021
--- /dev/null
+++ b/src/metabase/query_processor/store/metadata_provider.clj
@@ -0,0 +1,39 @@
+(ns metabase.query-processor.store.metadata-provider
+ (:require
+ [metabase.lib.metadata.composed-provider
+ :as lib.metadata.composed-provider]
+ [metabase.lib.metadata.jvm :as lib.metadata.jvm]
+ [metabase.lib.metadata.protocols :as lib.metadata.protocols]
+ [metabase.query-processor.store :as qp.store]
+ [metabase.util :as u]
+ [pretty.core :as pretty]))
+
+(defn- base-metadata-provider []
+ (reify
+ lib.metadata.protocols/MetadataProvider
+ (database [_this]
+ (some-> (qp.store/database) (update-keys u/->kebab-case-en) (assoc :lib/type :metadata/database)))
+
+ (table [_this table-id]
+ (some-> (qp.store/table table-id) (update-keys u/->kebab-case-en) (assoc :lib/type :metadata/table)))
+
+ (field [_this field-id]
+ (some-> (qp.store/field field-id) (update-keys u/->kebab-case-en) (assoc :lib/type :metadata/field)))
+
+ (card [_this _card-id] nil)
+ (metric [_this _metric-id] nil)
+ (segment [_this _segment-id] nil)
+ (tables [_metadata-provider] nil)
+ (fields [_metadata-provider _table-id] nil)
+
+ pretty/PrettyPrintable
+ (pretty [_this]
+ `metadata-provider)))
+
+(defn metadata-provider
+ "Create a new MLv2 metadata provider that uses the QP store."
+ []
+ (qp.store/cached ::metadata-provider
+ (lib.metadata.composed-provider/composed-metadata-provider
+ (base-metadata-provider)
+ (lib.metadata.jvm/application-database-metadata-provider (:id (qp.store/database))))))
diff --git a/src/metabase/query_processor/util/nest_query.clj b/src/metabase/query_processor/util/nest_query.clj
index b5402fcb411..4ec6caa9ca6 100644
--- a/src/metabase/query_processor/util/nest_query.clj
+++ b/src/metabase/query_processor/util/nest_query.clj
@@ -12,6 +12,8 @@
[metabase.mbql.util :as mbql.u]
[metabase.plugins.classloader :as classloader]
[metabase.query-processor.middleware.annotate :as annotate]
+ [metabase.query-processor.middleware.resolve-joins
+ :as qp.middleware.resolve-joins]
[metabase.query-processor.store :as qp.store]
[metabase.query-processor.util.add-alias-info :as add]
[metabase.util :as u]))
@@ -27,11 +29,6 @@
[:field _ (_ :guard :join-alias)]
&match)))
-(defn- add-joined-fields-to-fields [joined-fields source]
- (cond-> source
- (seq joined-fields) (update :fields (fn [fields]
- (m/distinct-by add/normalize-clause (concat fields joined-fields))))))
-
(defn- keep-source+alias-props [field]
(update field 2 select-keys [::add/source-alias ::add/source-table :join-alias]))
@@ -70,7 +67,7 @@
(add/add-alias-info source)
(:query source)
(dissoc source :limit)
- (add-joined-fields-to-fields (joined-fields inner-query) source)
+ (qp.middleware.resolve-joins/append-join-fields-to-fields source (joined-fields inner-query))
(remove-unused-fields inner-query source)
(cond-> source
keep-filter? (assoc :filter filter-clause)))]
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index 84a7b38e7af..1bc7d825c78 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -849,7 +849,7 @@
(assoc (card-with-name-and-query card-name)
:collection_id (u/the-id collection)))
(testing "check the correct metadata was fetched and was saved in the DB"
- (is (= [{:base_type :type/BigInteger
+ (is (= [{:base_type :type/Integer
:display_name "Count"
:name "count"
:semantic_type :type/Quantity
diff --git a/test/metabase/api/embed_test.clj b/test/metabase/api/embed_test.clj
index b48afa78c9f..2a815a943b3 100644
--- a/test/metabase/api/embed_test.clj
+++ b/test/metabase/api/embed_test.clj
@@ -105,13 +105,13 @@
(defn ^:deprecated test-query-results
"DEPRECATED -- you should use `schema=` instead"
([actual]
- (is (= {:data {:cols [(mt/obj->json->obj (qp.test/aggregate-col :count))]
- :rows [[100]]
- :insights nil
- :results_timezone "UTC"}
- :json_query {}
- :status "completed"}
- actual)))
+ (is (=? {:data {:cols [(mt/obj->json->obj (qp.test/aggregate-col :count))]
+ :rows [[100]]
+ :insights nil
+ :results_timezone "UTC"}
+ :json_query {}
+ :status "completed"}
+ actual)))
([results-format actual]
(case results-format
diff --git a/test/metabase/driver/sql/query_processor_test.clj b/test/metabase/driver/sql/query_processor_test.clj
index c05545316be..f8c442a3ffc 100644
--- a/test/metabase/driver/sql/query_processor_test.clj
+++ b/test/metabase/driver/sql/query_processor_test.clj
@@ -503,44 +503,47 @@
mbql->native
sql.qp-test-util/sql->sql-map))))))
+(def ^:private reference-aggregation-expressions-in-joins-test-expected-sql
+ '{:select [source.ID AS ID
+ source.NAME AS NAME
+ source.CATEGORY_ID AS CATEGORY_ID
+ source.LATITUDE AS LATITUDE
+ source.LONGITUDE AS LONGITUDE
+ source.PRICE AS PRICE
+ source.RelativePrice AS RelativePrice
+ source.CategoriesStats__CATEGORY_ID AS CategoriesStats__CATEGORY_ID
+ source.CategoriesStats__MaxPrice AS CategoriesStats__MaxPrice
+ source.CategoriesStats__AvgPrice AS CategoriesStats__AvgPrice
+ source.CategoriesStats__MinPrice AS CategoriesStats__MinPrice]
+ :from [{:select [VENUES.ID AS ID
+ VENUES.NAME AS NAME
+ VENUES.CATEGORY_ID AS CATEGORY_ID
+ VENUES.LATITUDE AS LATITUDE
+ VENUES.LONGITUDE AS LONGITUDE
+ VENUES.PRICE AS PRICE
+ CAST (VENUES.PRICE AS float)
+ /
+ CASE WHEN CategoriesStats.AvgPrice = 0 THEN NULL
+ ELSE CategoriesStats.AvgPrice END AS RelativePrice
+ CategoriesStats.CATEGORY_ID AS CategoriesStats__CATEGORY_ID
+ CategoriesStats.MaxPrice AS CategoriesStats__MaxPrice
+ CategoriesStats.AvgPrice AS CategoriesStats__AvgPrice
+ CategoriesStats.MinPrice AS CategoriesStats__MinPrice]
+ :from [VENUES]
+ :left-join [{:select [VENUES.CATEGORY_ID AS CATEGORY_ID
+ MAX (VENUES.PRICE) AS MaxPrice
+ AVG (VENUES.PRICE) AS AvgPrice
+ MIN (VENUES.PRICE) AS MinPrice]
+ :from [VENUES]
+ :group-by [VENUES.CATEGORY_ID]
+ :order-by [VENUES.CATEGORY_ID ASC]} AS CategoriesStats
+ ON VENUES.CATEGORY_ID = CategoriesStats.CATEGORY_ID]}
+ AS source]
+ :limit [3]})
+
(deftest ^:parallel reference-aggregation-expressions-in-joins-test
(testing "See if we can correctly compile a query that references expressions that come from a join"
- (is (= '{:select [source.ID AS ID
- source.NAME AS NAME
- source.CATEGORY_ID AS CATEGORY_ID
- source.LATITUDE AS LATITUDE
- source.LONGITUDE AS LONGITUDE
- source.PRICE AS PRICE
- source.RelativePrice AS RelativePrice
- source.CategoriesStats__CATEGORY_ID AS CategoriesStats__CATEGORY_ID
- source.CategoriesStats__MaxPrice AS CategoriesStats__MaxPrice
- source.CategoriesStats__AvgPrice AS CategoriesStats__AvgPrice
- source.CategoriesStats__MinPrice AS CategoriesStats__MinPrice]
- :from [{:select [VENUES.ID AS ID
- VENUES.NAME AS NAME
- VENUES.CATEGORY_ID AS CATEGORY_ID
- VENUES.LATITUDE AS LATITUDE
- VENUES.LONGITUDE AS LONGITUDE
- VENUES.PRICE AS PRICE
- CAST (VENUES.PRICE AS float)
- /
- CASE WHEN CategoriesStats.AvgPrice = 0 THEN NULL
- ELSE CategoriesStats.AvgPrice END AS RelativePrice
- CategoriesStats.CATEGORY_ID AS CategoriesStats__CATEGORY_ID
- CategoriesStats.MaxPrice AS CategoriesStats__MaxPrice
- CategoriesStats.AvgPrice AS CategoriesStats__AvgPrice
- CategoriesStats.MinPrice AS CategoriesStats__MinPrice]
- :from [VENUES]
- :left-join [{:select [VENUES.CATEGORY_ID AS CATEGORY_ID
- MAX (VENUES.PRICE) AS MaxPrice
- AVG (VENUES.PRICE) AS AvgPrice
- MIN (VENUES.PRICE) AS MinPrice]
- :from [VENUES]
- :group-by [VENUES.CATEGORY_ID]
- :order-by [VENUES.CATEGORY_ID ASC]} AS CategoriesStats
- ON VENUES.CATEGORY_ID = CategoriesStats.CATEGORY_ID]}
- AS source]
- :limit [3]}
+ (is (= reference-aggregation-expressions-in-joins-test-expected-sql
(-> (mt/mbql-query venues
{:fields [$id
$name
@@ -567,6 +570,36 @@
mbql->native
sql.qp-test-util/sql->sql-map)))))
+(deftest ^:parallel wrong-type-info-test
+ (testing (str "Same as the test above, but the :field refs have base types different from what the QP would normally "
+ "calculate; query should still work")
+ (is (= reference-aggregation-expressions-in-joins-test-expected-sql
+ (-> (mt/mbql-query venues
+ {:fields [$id
+ $name
+ $category_id
+ $latitude
+ $longitude
+ $price
+ [:expression "RelativePrice"]
+ &CategoriesStats.category_id
+ &CategoriesStats.*MaxPrice/Number
+ &CategoriesStats.*AvgPrice/Number
+ &CategoriesStats.*MinPrice/Number]
+ :expressions {"RelativePrice" [:/ $price &CategoriesStats.*AvgPrice/Number]}
+ :joins [{:strategy :left-join
+ :condition [:= $category_id &CategoriesStats.venues.category_id]
+ :source-query {:source-table $$venues
+ :aggregation [[:aggregation-options [:max $price] {:name "MaxPrice"}]
+ [:aggregation-options [:avg $price] {:name "AvgPrice"}]
+ [:aggregation-options [:min $price] {:name "MinPrice"}]]
+ :breakout [$category_id]}
+ :alias "CategoriesStats"
+ :fields :all}]
+ :limit 3})
+ mbql->native
+ sql.qp-test-util/sql->sql-map)))))
+
(deftest expressions-and-coercions-test
(testing "Don't cast in both inner select and outer select when expression (#12430)"
(mt/with-temp-vals-in-db Field (mt/id :venues :price) {:coercion_strategy :Coercion/UNIXSeconds->DateTime
diff --git a/test/metabase/lib/aggregation_test.cljc b/test/metabase/lib/aggregation_test.cljc
index 37b443f6a01..824119bb86a 100644
--- a/test/metabase/lib/aggregation_test.cljc
+++ b/test/metabase/lib/aggregation_test.cljc
@@ -2,6 +2,7 @@
(:require
#?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))
[clojure.test :refer [are deftest is testing]]
+ [medley.core :as m]
[metabase.lib.convert :as lib.convert]
[metabase.lib.core :as lib]
[metabase.lib.metadata :as lib.metadata]
@@ -53,19 +54,19 @@
{:column-name "count", :display-name "Count"}
[:distinct {} (lib.tu/field-clause :venues :id)]
- {:column-name "distinct_ID", :display-name "Distinct values of ID"}
+ {:column-name "count", :display-name "Distinct values of ID"}
[:sum {} (lib.tu/field-clause :venues :id)]
- {:column-name "sum_ID", :display-name "Sum of ID"}
+ {:column-name "sum", :display-name "Sum of ID"}
[:+ {} [:count {}] 1]
- {:column-name "count_plus_1", :display-name "Count + 1"}
+ {:column-name "expression", :display-name "Count + 1"}
[:+
{}
[:min {} (lib.tu/field-clause :venues :id)]
[:* {} 2 [:avg {} (lib.tu/field-clause :venues :price)]]]
- {:column-name "min_ID_plus_2_times_avg_PRICE"
+ {:column-name "expression"
:display-name "Min of ID + (2 × Average of Price)"}
[:+
@@ -77,7 +78,7 @@
[:avg {} (lib.tu/field-clause :venues :price)]
3
[:- {} [:max {} (lib.tu/field-clause :venues :category-id)] 4]]]
- {:column-name "min_ID_plus_2_times_avg_PRICE_times_3_times_max_CATEGORY_ID_minus_4"
+ {:column-name "expression"
:display-name "Min of ID + (2 × Average of Price × 3 × (Max of Category ID - 4))"}
;; user-specified names
@@ -97,11 +98,11 @@
{:display-name "User-specified Name"}
[:min {} (lib.tu/field-clause :venues :id)]
[:* {} 2 [:avg {} (lib.tu/field-clause :venues :price)]]]
- {:column-name "min_ID_plus_2_times_avg_PRICE"
+ {:column-name "expression"
:display-name "User-specified Name"}
[:percentile {} (lib.tu/field-clause :venues :id) 0.95]
- {:column-name "p95_ID", :display-name "0.95th percentile of ID"}))
+ {:column-name "percentile", :display-name "0.95th percentile of ID"}))
;;; the following tests use raw legacy MBQL because they're direct ports of JavaScript tests from MLv1 and I wanted to
;;; make sure that given an existing query, the expected description was generated correctly.
@@ -144,13 +145,13 @@
;; :count, no field
[:/ {} [:count {}] 2]
{:base-type :type/Float
- :name "count_divided_by_2"
+ :name "expression"
:display-name "Count ÷ 2"}
;; :sum
[:sum {} [:+ {} (lib.tu/field-clause :venues :price) 1]]
{:base-type :type/Integer
- :name "sum_PRICE_plus_1"
+ :name "sum"
:display-name "Sum of Price + 1"}
;; options map
@@ -164,7 +165,7 @@
(deftest ^:parallel col-info-named-aggregation-test
(testing "col info for an `expression` aggregation w/ a named expression should work as expected"
(is (=? {:base-type :type/Integer
- :name "sum_double-price"
+ :name "sum"
:display-name "Sum of double-price"}
(col-info-for-aggregation-clause
(lib.tu/venues-query-with-last-stage
@@ -240,7 +241,7 @@
(lib/aggregate (lib/sum [:expression {:lib/uuid (str (random-uuid))} "double-price"])))]
(is (=? [{:lib/type :metadata/field
:base-type :type/Integer
- :name "sum_double-price"
+ :name "sum"
:display-name "Sum of double-price"}]
(lib/aggregations-metadata query)))
(is (= :type/Integer
@@ -410,7 +411,7 @@
agg-query))
(is (=? [{:lib/type :metadata/field
:effective-type :type/Integer
- :name "sum_double-price"
+ :name "sum"
:display-name "Sum of double-price"
:lib/source :source/aggregations}
{:lib/type :metadata/field
@@ -421,7 +422,7 @@
{:settings {:is_priceless true}
:lib/type :metadata/field
:effective-type :type/Integer
- :name "sum_PRICE"
+ :name "sum"
:display-name "Sum of Price"
:lib/source :source/aggregations}]
(lib/aggregations-metadata agg-query)))))))
@@ -528,7 +529,7 @@
(is (=? {:settings {:is_priceless true}
:lib/type :metadata/field
:effective-type :type/Integer
- :name "sum_PRICE"
+ :name "sum"
:display-name "Sum of Price"
:lib/source :source/aggregations}
(lib.metadata.calculation/metadata query (first (lib/aggregations-metadata query -1))))))))
@@ -634,12 +635,12 @@
:fingerprint {:global {:distinct-count 28, :nil% 0.0}}}
{:lib/type :metadata/field
:base-type :type/Integer
- :name "sum_case"
+ :name "sum"
:display-name "Sum of Case"
:lib/source :source/aggregations
:lib/source-uuid string?
- :lib/source-column-alias "sum_case"
- :lib/desired-column-alias "sum_case"}]
+ :lib/source-column-alias "sum"
+ :lib/desired-column-alias "sum"}]
(lib.metadata.calculation/metadata query)))))
(deftest ^:parallel count-display-name-test
@@ -668,3 +669,16 @@
(map (partial lib/display-name query)
(lib.metadata.calculation/metadata query)))
"display name"))))))))
+
+(deftest ^:parallel aggregation-name-from-previous-stage-test
+ (testing "Maintain the column names in refs from the QP/MLv1 (#31266)"
+ (let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :products))
+ (lib/expression "Half Price" (lib// (meta/field-metadata :products :price) 2))
+ (lib/aggregate (lib/avg (lib/ref-lookup :expression "Half Price")))
+ (lib/breakout (lib/with-temporal-bucket (meta/field-metadata :products :created-at) :month))
+ lib/append-stage)
+ ag-op (m/find-first #(= (:short %) :max)
+ (lib/available-aggregation-operators query))
+ expr-metadata (last (lib/aggregation-operator-columns ag-op))]
+ (is (=? [:field {:lib/uuid string?, :base-type :type/Float} "avg"]
+ (lib/ref expr-metadata))))))
diff --git a/test/metabase/lib/expression_test.cljc b/test/metabase/lib/expression_test.cljc
index 850bf851703..606aabb66cd 100644
--- a/test/metabase/lib/expression_test.cljc
+++ b/test/metabase/lib/expression_test.cljc
@@ -131,7 +131,7 @@
[:expression {:lib/uuid (str (random-uuid))} "double-price"]]]
(is (= "Sum of double-price"
(lib.metadata.calculation/display-name query -1 expr)))
- (is (= "sum_double-price"
+ (is (= "sum"
(lib.metadata.calculation/column-name query -1 expr)))))
(deftest ^:parallel coalesce-names-test
diff --git a/test/metabase/lib/field_test.cljc b/test/metabase/lib/field_test.cljc
index 069baac40f7..9cc55c27795 100644
--- a/test/metabase/lib/field_test.cljc
+++ b/test/metabase/lib/field_test.cljc
@@ -6,6 +6,8 @@
[metabase.lib.core :as lib]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
+ [metabase.lib.metadata.composed-provider
+ :as lib.metadata.composed-provider]
[metabase.lib.schema.expression :as lib.schema.expression]
[metabase.lib.temporal-bucket :as lib.temporal-bucket]
[metabase.lib.test-metadata :as meta]
@@ -157,7 +159,7 @@
time-field (assoc (meta/field-metadata :orders :created-at)
:base-type :type/Time
:effective-type :type/Time)
- metadata-provider (lib.tu/composed-metadata-provider
+ metadata-provider (lib.metadata.composed-provider/composed-metadata-provider
(lib.tu/mock-metadata-provider
{:fields [date-field
time-field]})
@@ -391,7 +393,16 @@
(meta/id :categories :name)]]]}]}
query')))
(is (= "Venues, Sorted by Category → Name ascending"
- (lib/describe-query query')))))))
+ (lib/describe-query query'))))
+ (testing "inside aggregations"
+ (let [query' (lib/aggregate query (lib/distinct categories-name))
+ [aggregation] (lib/aggregations query')]
+ (is (= "Venues, Distinct values of Category → Name"
+ (lib/describe-query query')))
+ (are [style expected] (= expected
+ (lib/display-name query' -1 aggregation style))
+ :long "Distinct values of Category → Name"
+ :default "Distinct values of Name"))))))
(deftest ^:parallel source-card-table-display-info-test
(let [query (assoc lib.tu/venues-query :lib/metadata lib.tu/metadata-provider-with-card)
@@ -413,7 +424,7 @@
:query {:source-table (meta/id :checkins)
:aggregation [[:count]]
:breakout [[:field (meta/id :checkins :user-id) nil]]}}}
- metadata-provider (lib.tu/composed-metadata-provider
+ metadata-provider (lib.metadata.composed-provider/composed-metadata-provider
meta/metadata-provider
(lib.tu/mock-metadata-provider
{:cards [card-1]}))
@@ -453,10 +464,10 @@
:lib/source :source/breakouts
:lib/source-column-alias "LAST_LOGIN"
:lib/desired-column-alias "LAST_LOGIN"}
- {:name "avg_count"
+ {:name "avg"
:lib/source :source/aggregations
- :lib/source-column-alias "avg_count"
- :lib/desired-column-alias "avg_count"}]
+ :lib/source-column-alias "avg"
+ :lib/desired-column-alias "avg"}]
(lib.metadata.calculation/metadata query))))))
(deftest ^:parallel with-fields-test
diff --git a/test/metabase/lib/join_test.cljc b/test/metabase/lib/join_test.cljc
index 76556cf5dee..e5aed011b8a 100644
--- a/test/metabase/lib/join_test.cljc
+++ b/test/metabase/lib/join_test.cljc
@@ -6,6 +6,7 @@
[metabase.lib.join :as lib.join]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
+ [metabase.lib.metadata.composed-provider :as lib.metadata.composed-provider]
[metabase.lib.test-metadata :as meta]
[metabase.lib.test-util :as lib.tu]
#?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))))
@@ -158,7 +159,7 @@
:query {:source-table (meta/id :checkins)
:aggregation [[:count]]
:breakout [[:field (meta/id :checkins :user-id) nil]]}}}
- metadata-provider (lib.tu/composed-metadata-provider
+ metadata-provider (lib.metadata.composed-provider/composed-metadata-provider
meta/metadata-provider
(lib.tu/mock-metadata-provider
{:cards [card-1]}))
diff --git a/test/metabase/lib/metadata/composed_provider_test.cljc b/test/metabase/lib/metadata/composed_provider_test.cljc
new file mode 100644
index 00000000000..a54b3735109
--- /dev/null
+++ b/test/metabase/lib/metadata/composed_provider_test.cljc
@@ -0,0 +1,28 @@
+(ns metabase.lib.metadata.composed-provider-test
+ (:require
+ [clojure.test :refer [deftest is testing]]
+ [metabase.lib.metadata :as lib.metadata]
+ [metabase.lib.metadata.composed-provider
+ :as lib.metadata.composed-provider]
+ [metabase.lib.test-metadata :as meta]
+ [metabase.lib.test-util :as lib.tu]
+ #?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))))
+
+#?(:cljs
+ (comment metabase.test-runner.assert-exprs.approximately-equal/keep-me))
+
+(deftest ^:parallel composed-metadata-provider-test
+ (testing "Return things preferentially from earlier metadata providers"
+ (let [time-field (assoc (meta/field-metadata :people :birth-date)
+ :base-type :type/Time
+ :effective-type :type/Time)
+ metadata-provider (lib.metadata.composed-provider/composed-metadata-provider
+ (lib.tu/mock-metadata-provider
+ {:fields [time-field]})
+ meta/metadata-provider)]
+ (is (=? {:name "BIRTH_DATE"
+ :base-type :type/Time
+ :effective-type :type/Time}
+ (lib.metadata/field
+ metadata-provider
+ (meta/id :people :birth-date)))))))
diff --git a/test/metabase/lib/order_by_test.cljc b/test/metabase/lib/order_by_test.cljc
index 005e3d28417..0caf3bcc031 100644
--- a/test/metabase/lib/order_by_test.cljc
+++ b/test/metabase/lib/order_by_test.cljc
@@ -141,12 +141,12 @@
:base-type :type/Integer}
{:lib/type :metadata/field
:base-type :type/Integer
- :name "sum_PRICE"
+ :name "sum"
:display-name "Sum of Price"
:lib/source :source/aggregations}
{:lib/type :metadata/field
:base-type :type/Float
- :name "avg_PRICE_plus_1"
+ :name "avg"
:display-name "Average of Price + 1"
:lib/source :source/aggregations}]
(lib/orderable-columns query)))))))
diff --git a/test/metabase/lib/remove_replace_test.cljc b/test/metabase/lib/remove_replace_test.cljc
index ae426057aeb..018f5ac03eb 100644
--- a/test/metabase/lib/remove_replace_test.cljc
+++ b/test/metabase/lib/remove_replace_test.cljc
@@ -190,7 +190,7 @@
(-> query
(lib/order-by (lib.dev/ref-lookup :aggregation 0))
(lib/append-stage)
- (lib/filter (lib/= [:field {:lib/uuid (str (random-uuid)) :base-type :type/Integer} "sum_ID"] 1))
+ (lib/filter (lib/= [:field {:lib/uuid (str (random-uuid)) :base-type :type/Integer} "sum"] 1))
(lib/remove-clause 0 (first aggregations))))))))
(deftest ^:parallel remove-clause-expression-test
@@ -360,7 +360,7 @@
(lib/expression "expr" (lib.dev/ref-lookup :aggregation 0))
(lib/append-stage)
;; TODO Should be able to create a ref with lib/field [#29763]
- (lib/filter (lib/= [:field {:lib/uuid (str (random-uuid)) :base-type :type/Integer} "sum_ID"] 1))
+ (lib/filter (lib/= [:field {:lib/uuid (str (random-uuid)) :base-type :type/Integer} "sum"] 1))
(lib/replace-clause 0 (first aggregations) (lib/sum (lib/field "VENUES" "PRICE")))))))))
(deftest ^:parallel replace-clause-expression-test
diff --git a/test/metabase/lib/stage_test.cljc b/test/metabase/lib/stage_test.cljc
index bb19c5846be..09e4007ca49 100644
--- a/test/metabase/lib/stage_test.cljc
+++ b/test/metabase/lib/stage_test.cljc
@@ -41,15 +41,15 @@
0.8
[:avg {} (lib.tu/field-clause :venues :price)]]]})]
(is (=? [{:base-type :type/Float
- :name "0_8_times_avg_PRICE"
+ :name "expression"
:display-name "0.8 × Average of Price"
- :lib/source-column-alias "0_8_times_avg_PRICE"
- :lib/desired-column-alias "0_8_times_avg_PRICE"}
+ :lib/source-column-alias "expression"
+ :lib/desired-column-alias "expression"}
{:base-type :type/Float
- :name "0_8_times_avg_PRICE"
+ :name "expression"
:display-name "0.8 × Average of Price"
- :lib/source-column-alias "0_8_times_avg_PRICE"
- :lib/desired-column-alias "0_8_times_avg_PRICE_2"}]
+ :lib/source-column-alias "expression"
+ :lib/desired-column-alias "expression_2"}]
(lib.metadata.calculation/metadata query -1 query)))))))
(deftest ^:parallel stage-display-name-card-source-query
diff --git a/test/metabase/lib/test_util.cljc b/test/metabase/lib/test_util.cljc
index a87bab7327d..bc5bdb2b025 100644
--- a/test/metabase/lib/test_util.cljc
+++ b/test/metabase/lib/test_util.cljc
@@ -2,12 +2,12 @@
"Misc test utils for Metabase lib."
(:require
[clojure.core.protocols]
- [clojure.datafy :as datafy]
- [clojure.test :refer [deftest is testing]]
+ [clojure.test :refer [is]]
[malli.core :as mc]
[medley.core :as m]
[metabase.lib.core :as lib]
- [metabase.lib.metadata :as lib.metadata]
+ [metabase.lib.metadata.composed-provider
+ :as lib.metadata.composed-provider]
[metabase.lib.metadata.protocols :as metadata.protocols]
[metabase.lib.schema :as lib.schema]
[metabase.lib.test-metadata :as meta]
@@ -71,44 +71,11 @@
(datafy [_this]
(list `mock-metadata-provider m))))
-(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] (m/distinct-by :id (mapcat metadata.protocols/tables metadata-providers)))
- (fields [_this table-id] (m/distinct-by :id (mapcat #(metadata.protocols/fields % table-id) metadata-providers)))
- clojure.core.protocols/Datafiable
- (datafy [_this]
- (cons `composed-metadata-provider (map datafy/datafy metadata-providers)))))
-
-(deftest ^:parallel composed-metadata-provider-test
- (testing "Return things preferentially from earlier metadata providers"
- (let [time-field (assoc (meta/field-metadata :people :birth-date)
- :base-type :type/Time
- :effective-type :type/Time)
- metadata-provider (composed-metadata-provider
- (mock-metadata-provider
- {:fields [time-field]})
- meta/metadata-provider)]
- (is (=? {:name "BIRTH_DATE"
- :base-type :type/Time
- :effective-type :type/Time}
- (lib.metadata/field
- metadata-provider
- (meta/id :people :birth-date)))))))
(def metadata-provider-with-card
"[[meta/metadata-provider]], but with a Card with ID 1."
- (composed-metadata-provider
+ (lib.metadata.composed-provider/composed-metadata-provider
meta/metadata-provider
(mock-metadata-provider
{:cards [{:name "My Card"
@@ -131,7 +98,7 @@
(def metadata-provider-with-card-with-result-metadata
"[[meta/metadata-provider]], but with a Card with results metadata as ID 1."
- (composed-metadata-provider
+ (lib.metadata.composed-provider/composed-metadata-provider
meta/metadata-provider
(mock-metadata-provider
{:cards [{:name "My Card"
diff --git a/test/metabase/query_processor/middleware/add_source_metadata_test.clj b/test/metabase/query_processor/middleware/add_source_metadata_test.clj
index 89c247720bd..8d9a72f767b 100644
--- a/test/metabase/query_processor/middleware/add_source_metadata_test.clj
+++ b/test/metabase/query_processor/middleware/add_source_metadata_test.clj
@@ -32,7 +32,7 @@
(mt/run-mbql-query venues {:fields (for [id field-ids] [:field id nil])
:limit 1})))))
-(deftest basic-test
+(deftest ^:parallel basic-test
(testing "Can we automatically add source metadata to the parent level of a query? If the source query has `:fields`"
(is (= (mt/mbql-query venues
{:source-query {:source-table $$venues
@@ -41,8 +41,9 @@
(add-source-metadata
(mt/mbql-query venues
{:source-query {:source-table $$venues
- :fields [$id $name]}})))))
+ :fields [$id $name]}}))))))
+(deftest ^:parallel basic-parent-level-test
(testing (str "Can we automatically add source metadata to the parent level of a query? If the source query does not "
"have `:fields`")
(is (= (mt/mbql-query venues
@@ -50,8 +51,9 @@
:source-metadata (venues-source-metadata)})
(add-source-metadata
(mt/mbql-query venues
- {:source-query {:source-table $$venues}})))))
+ {:source-query {:source-table $$venues}}))))))
+(deftest ^:parallel basic-summary-columns-test
(testing "Can we add source metadata for a source query that has breakouts/aggregations?"
(is (= (mt/mbql-query venues
{:source-query {:source-table $$venues
@@ -61,15 +63,16 @@
(venues-source-metadata :price)
[{:name "count"
:display_name "Count"
- :base_type :type/BigInteger
+ :base_type :type/Integer
:semantic_type :type/Quantity
:field_ref [:aggregation 0]}])})
(add-source-metadata
(mt/mbql-query venues
{:source-query {:source-table $$venues
:aggregation [[:count]]
- :breakout [$price]}})))))
+ :breakout [$price]}}))))))
+(deftest ^:parallel basic-aggregation-with-field-test
(testing "Can we add source metadata for a source query that has an aggregation for a specific Field?"
(is (= (mt/mbql-query venues
{:source-query {:source-table $$venues
@@ -79,8 +82,7 @@
(venues-source-metadata :price)
[{:name "avg"
:display_name "Average of ID"
- :base_type :type/BigInteger
- :semantic_type :type/PK
+ :base_type :type/Float
:settings nil
:field_ref [:aggregation 0]}])})
(add-source-metadata
@@ -92,7 +94,7 @@
(defn- source-metadata [query]
(get-in query [:query :source-metadata] query))
-(deftest named-aggregations-test
+(deftest ^:parallel named-aggregations-test
(testing "adding source metadata for source queries with named aggregations"
(testing "w/ `:name` and `:display-name`"
(is (= (mt/mbql-query venues
@@ -105,8 +107,7 @@
(venues-source-metadata :price)
[{:name "some_generated_name"
:display_name "My Cool Ag"
- :base_type :type/BigInteger
- :semantic_type :type/PK
+ :base_type :type/Float
:settings nil
:field_ref [:aggregation 0]}])})
(add-source-metadata
@@ -115,35 +116,35 @@
:aggregation [[:aggregation-options
[:avg $id]
{:name "some_generated_name", :display-name "My Cool Ag"}]]
- :breakout [$price]}})))))
+ :breakout [$price]}})))))))
- (testing "w/ `:name` only"
- (is (= [{:name "some_generated_name"
- :display_name "Average of ID"
- :base_type :type/BigInteger
- :semantic_type :type/PK
- :settings nil
- :field_ref [:aggregation 0]}]
- (source-metadata
- (add-source-metadata
- (mt/mbql-query venues
- {:source-query {:source-table $$venues
- :aggregation [[:aggregation-options [:avg $id] {:name "some_generated_name"}]]}}))))))
+(deftest ^:parallel named-aggregations-name-only-test
+ (testing "w/ `:name` only"
+ (is (= [{:name "some_generated_name"
+ :display_name "Average of ID"
+ :base_type :type/Float
+ :settings nil
+ :field_ref [:aggregation 0]}]
+ (source-metadata
+ (add-source-metadata
+ (mt/mbql-query venues
+ {:source-query {:source-table $$venues
+ :aggregation [[:aggregation-options [:avg $id] {:name "some_generated_name"}]]}})))))))
- (testing "w/ `:display-name` only"
- (is (= [{:name "avg"
- :display_name "My Cool Ag"
- :base_type :type/BigInteger
- :semantic_type :type/PK
- :settings nil
- :field_ref [:aggregation 0]}]
- (source-metadata
- (add-source-metadata
- (mt/mbql-query venues
- {:source-query {:source-table $$venues
- :aggregation [[:aggregation-options [:avg $id] {:display-name "My Cool Ag"}]]}}))))))))
+(deftest ^:parallel named-aggregations-display-name-only-test
+ (testing "w/ `:display-name` only"
+ (is (= [{:name "avg"
+ :display_name "My Cool Ag"
+ :base_type :type/Float
+ :settings nil
+ :field_ref [:aggregation 0]}]
+ (source-metadata
+ (add-source-metadata
+ (mt/mbql-query venues
+ {:source-query {:source-table $$venues
+ :aggregation [[:aggregation-options [:avg $id] {:display-name "My Cool Ag"}]]}})))))))
-(deftest nested-sources-test
+(deftest ^:parallel nested-sources-test
(testing (str "Can we automatically add source metadata to the parent level of a query? If the source query has a "
"source query with source metadata")
(is (= (mt/mbql-query venues
@@ -155,8 +156,9 @@
(mt/mbql-query venues
{:source-query {:source-query {:source-table $$venues
:fields [$id $name]}
- :source-metadata (venues-source-metadata :id :name)}})))))
+ :source-metadata (venues-source-metadata :id :name)}}))))))
+(deftest ^:parallel nested-sources-3-levels-with-source-metadata-test
(testing "Can we automatically add source metadata if a source-query nested 3 levels has `:source-metadata`?"
(is (= (mt/mbql-query venues
{:source-query {:source-query {:source-query {:source-table $$venues
@@ -170,8 +172,9 @@
{:source-query
{:source-query {:source-table $$venues
:fields [$id $name]}
- :source-metadata (venues-source-metadata :id :name)}}})))))
+ :source-metadata (venues-source-metadata :id :name)}}}))))))
+(deftest ^:parallel nested-sources-3-levels-with-no-source-metadata-test
(testing "Ok, how about a source query nested 3 levels with no `source-metadata`?"
(is (= (mt/mbql-query venues
{:source-query {:source-query {:source-query {:source-table $$venues}
@@ -180,44 +183,49 @@
:source-metadata (venues-source-metadata)})
(add-source-metadata
(mt/mbql-query venues
- {:source-query {:source-query {:source-query {:source-table $$venues}}}}))))
+ {:source-query {:source-query {:source-query {:source-table $$venues}}}}))))))
- (testing "with `fields`"
- (is (= (mt/mbql-query venues
- {:source-query {:source-query {:source-query {:source-table $$venues
- :fields [$id $name]}
- :source-metadata (venues-source-metadata :id :name)}
- :source-metadata (venues-source-metadata :id :name)}
- :source-metadata (venues-source-metadata :id :name)})
- (add-source-metadata
- (mt/mbql-query venues
- {:source-query {:source-query {:source-query {:source-table $$venues
- :fields [$id $name]}}}})))))
- (testing "with breakouts/aggregations"
- ;; field ref for the count aggregation differs slightly depending on what level of the query we're at; at the
- ;; most-deeply-nested level we can use the `[:aggregation 0]` ref to refer to it; at higher levels we have to
- ;; refer to it with a field literal
- (is (= (letfn [(metadata-with-count-field-ref [field-ref]
- (concat
- (venues-source-metadata :price)
- (let [[count-col] (results-metadata (mt/run-mbql-query venues {:aggregation [[:count]]}))]
- [(-> count-col
- (dissoc :effective_type)
- (assoc :field_ref field-ref))])))]
- (mt/mbql-query venues
- {:source-query {:source-query {:source-query {:source-table $$venues
- :aggregation [[:count]]
- :breakout [$price]}
- :source-metadata (metadata-with-count-field-ref [:aggregation 0])}
- :source-metadata (metadata-with-count-field-ref *count/BigInteger)}
- :source-metadata (metadata-with-count-field-ref *count/BigInteger)}))
- (add-source-metadata
- (mt/mbql-query venues
- {:source-query {:source-query {:source-query {:source-table $$venues
- :aggregation [[:count]]
- :breakout [$price]}}}}))))))
+(deftest ^:parallel nested-sources-3-levels-with-fields-test
+ (testing "nested 3 levels with `fields`"
+ (is (= (mt/mbql-query venues
+ {:source-query {:source-query {:source-query {:source-table $$venues
+ :fields [$id $name]}
+ :source-metadata (venues-source-metadata :id :name)}
+ :source-metadata (venues-source-metadata :id :name)}
+ :source-metadata (venues-source-metadata :id :name)})
+ (add-source-metadata
+ (mt/mbql-query venues
+ {:source-query {:source-query {:source-query {:source-table $$venues
+ :fields [$id $name]}}}}))))))
+
+(deftest ^:parallel nested-sources-3-levels-with-summary-columns-test
+ (testing "nested 3 levels with breakouts/aggregations"
+ ;; field ref for the count aggregation differs slightly depending on what level of the query we're at; at the
+ ;; most-deeply-nested level we can use the `[:aggregation 0]` ref to refer to it; at higher levels we have to
+ ;; refer to it with a field literal
+ (is (= (letfn [(metadata-with-count-field-ref [field-ref]
+ (concat
+ (venues-source-metadata :price)
+ (let [[count-col] (results-metadata (mt/run-mbql-query venues {:aggregation [[:count]]}))]
+ [(-> count-col
+ (dissoc :effective_type)
+ (assoc :field_ref field-ref
+ :base_type :type/Integer))])))]
+ (mt/mbql-query venues
+ {:source-query {:source-query {:source-query {:source-table $$venues
+ :aggregation [[:count]]
+ :breakout [$price]}
+ :source-metadata (metadata-with-count-field-ref [:aggregation 0])}
+ :source-metadata (metadata-with-count-field-ref *count/Integer)}
+ :source-metadata (metadata-with-count-field-ref *count/Integer)}))
+ (add-source-metadata
+ (mt/mbql-query venues
+ {:source-query {:source-query {:source-query {:source-table $$venues
+ :aggregation [[:count]]
+ :breakout [$price]}}}}))))))
+(deftest ^:parallel nested-sources-with-source-native-query-test
(testing "can we add `source-metadata` to the parent level if the source query has a native source query, but itself has `source-metadata`?"
(is (= (mt/mbql-query venues
{:source-query {:source-query {:native "SELECT \"ID\", \"NAME\" FROM \"VENUES\";"}
@@ -228,7 +236,7 @@
{:source-query {:source-query {:native "SELECT \"ID\", \"NAME\" FROM \"VENUES\";"}
:source-metadata (venues-source-metadata :id :name)}}))))))
-(deftest joins-test
+(deftest ^:parallel joins-test
(testing "should work inside JOINS as well"
(is (= (mt/mbql-query venues
{:source-table $$venues
@@ -249,17 +257,21 @@
:aggregation [[:count]]
:breakout [[:field %latitude {:binning {:strategy :default}}]]}
:source-metadata (concat
- (let [[lat-col] (venues-source-metadata :latitude)]
+ (let [[lat-col] (venues-source-metadata :latitude)
+ [count-col] (results-metadata (mt/run-mbql-query venues {:aggregation [[:count]]}))]
[(assoc lat-col :field_ref [:field
(mt/id :venues :latitude)
- {:binning {:strategy :bin-width
+ {:binning {:strategy :bin-width
:min-value 10.0
:max-value 45.0
:num-bins 7
- :bin-width 5.0}}])])
- ;; computed column doesn't have an effective type in middleware before query
- (map #(dissoc % :effective_type)
- (results-metadata (mt/run-mbql-query venues {:aggregation [[:count]]}))))})
+ :bin-width 5.0}}])
+ ;; computed column doesn't have an effective type in middleware before query
+ (-> count-col
+ (dissoc :effective_type)
+ ;; the type that comes back from H2 is :type/BigInteger but the type that comes
+ ;; back from calculating it with MLv2 is just plain :type/Integer
+ (assoc :base_type :type/Integer))]))})
(add-source-metadata
(mt/mbql-query venues
{:source-query
@@ -267,7 +279,7 @@
:aggregation [[:count]]
:breakout [[:field %latitude {:binning {:strategy :default}}]]}})))))))
-(deftest deduplicate-column-names-test
+(deftest ^:parallel deduplicate-column-names-test
(testing "Metadata that gets added to source queries should have deduplicated column names"
(let [query (add-source-metadata
(mt/mbql-query checkins
diff --git a/test/metabase/query_processor/middleware/annotate_test.clj b/test/metabase/query_processor/middleware/annotate_test.clj
index 9a1b4b0358d..a15db537ba5 100644
--- a/test/metabase/query_processor/middleware/annotate_test.clj
+++ b/test/metabase/query_processor/middleware/annotate_test.clj
@@ -69,61 +69,81 @@
(is (= [(merge (info-for-field :venues :price)
{:source :fields
:field_ref $price})]
- (doall
- (annotate/column-info
- {:type :query, :query {:fields [$price]}}
- {:columns [:price]}))))))))
+ (annotate/column-info
+ {:type :query, :query {:fields [$price]}}
+ {:columns [:price]})))))))
-(deftest ^:parallel col-info-for-fks-and-joins-test
+(deftest ^:parallel col-info-for-implicit-joins-test
(mt/with-everything-store
(mt/$ids venues
(testing (str "when a `:field` with `:source-field` (implicit join) is used, we should add in `:fk_field_id` "
"info about the source Field")
(is (= [(merge (info-for-field :categories :name)
- {:fk_field_id %category_id
- :source :fields
- :field_ref $category_id->categories.name})]
- (doall
+ {:fk_field_id %category_id
+ :source :fields
+ :field_ref $category_id->categories.name
+ ;; for whatever reason this is what the `annotate` middleware traditionally returns here, for
+ ;; some reason we use the `:long` style inside aggregations and the `:default` style elsewhere,
+ ;; who knows why. See notes
+ ;; on [[metabase.query-processor.middleware.annotate/col-info-for-aggregation-clause]]
+ :display_name "Name"})]
+ (annotate/column-info
+ {:type :query, :query {:fields [$category_id->categories.name]}}
+ {:columns [:name]})))))))
+
+(deftest ^:parallel col-info-for-implicit-joins-aggregation-test
+ (mt/with-everything-store
+ (mt/$ids venues
+ (testing (str "when a `:field` with `:source-field` (implicit join) is used, we should add in `:fk_field_id` "
+ "info about the source Field")
+ (is (=? [{:source :aggregation
+ :field_ref [:aggregation 0]
+ :display_name "Distinct values of Category → Name"}]
(annotate/column-info
- {:type :query, :query {:fields [$category_id->categories.name]}}
- {:columns [:name]})))))
-
- (testing "joins"
- (testing (str "we should get `:fk_field_id` and information where possible when using joins; "
- "display_name should include the display name of the FK field (for IMPLICIT JOINS)")
- (is (= [(merge (info-for-field :categories :name)
- {:display_name "Category → Name"
- :source :fields
- :field_ref $category_id->categories.name
- :fk_field_id %category_id
- :source_alias "CATEGORIES__via__CATEGORY_ID"})]
- (doall
- (annotate/column-info
- {:type :query
- :query {:fields [&CATEGORIES__via__CATEGORY_ID.categories.name]
- :joins [{:alias "CATEGORIES__via__CATEGORY_ID"
- :source-table $$venues
- :condition [:= $category_id &CATEGORIES__via__CATEGORY_ID.categories.id]
- :strategy :left-join
- :fk-field-id %category_id}]}}
- {:columns [:name]})))))
-
- (testing (str "for EXPLICIT JOINS (which do not include an `:fk-field-id` in the Join info) the returned "
- "`:field_ref` should be have only `:join-alias`, and no `:source-field`")
- (is (= [(merge (info-for-field :categories :name)
- {:display_name "Categories → Name"
- :source :fields
- :field_ref &Categories.categories.name
- :source_alias "Categories"})]
- (doall
- (annotate/column-info
- {:type :query
- :query {:fields [&Categories.categories.name]
- :joins [{:alias "Categories"
- :source-table $$venues
- :condition [:= $category_id &Categories.categories.id]
- :strategy :left-join}]}}
- {:columns [:name]})))))))))
+ {:type :query
+ :query {:source-table $$venues
+ :aggregation [[:distinct $category_id->categories.name]]}}
+ {:columns [:name]})))))))
+
+(deftest ^:parallel col-info-for-explicit-joins-with-fk-field-id-test
+ (mt/with-everything-store
+ (mt/$ids venues
+ (testing (str "we should get `:fk_field_id` and information where possible when using joins; "
+ "display_name should include the display name of the FK field (for IMPLICIT JOINS)")
+ (is (= [(merge (info-for-field :categories :name)
+ {:display_name "Category → Name"
+ :source :fields
+ :field_ref $category_id->categories.name
+ :fk_field_id %category_id
+ :source_alias "CATEGORIES__via__CATEGORY_ID"})]
+ (annotate/column-info
+ {:type :query
+ :query {:fields [&CATEGORIES__via__CATEGORY_ID.categories.name]
+ :joins [{:alias "CATEGORIES__via__CATEGORY_ID"
+ :source-table $$venues
+ :condition [:= $category_id &CATEGORIES__via__CATEGORY_ID.categories.id]
+ :strategy :left-join
+ :fk-field-id %category_id}]}}
+ {:columns [:name]})))))))
+
+(deftest ^:parallel col-info-for-explicit-joins-without-fk-field-id-test
+ (mt/with-everything-store
+ (mt/$ids venues
+ (testing (str "for EXPLICIT JOINS (which do not include an `:fk-field-id` in the Join info) the returned "
+ "`:field_ref` should be have only `:join-alias`, and no `:source-field`")
+ (is (= [(merge (info-for-field :categories :name)
+ {:display_name "Categories → Name"
+ :source :fields
+ :field_ref &Categories.categories.name
+ :source_alias "Categories"})]
+ (annotate/column-info
+ {:type :query
+ :query {:fields [&Categories.categories.name]
+ :joins [{:alias "Categories"
+ :source-table $$venues
+ :condition [:= $category_id &Categories.categories.id]
+ :strategy :left-join}]}}
+ {:columns [:name]})))))))
(deftest ^:parallel col-info-for-field-with-temporal-unit-test
(mt/with-everything-store
@@ -136,8 +156,11 @@
(doall
(annotate/column-info
{:type :query, :query {:fields (mt/$ids venues [!month.price])}}
- {:columns [:price]})))))
+ {:columns [:price]}))))))))
+(deftest ^:parallel col-info-for-field-literal-with-temporal-unit-test
+ (mt/with-everything-store
+ (mt/$ids venues
(testing "datetime unit should work on field literals too"
(is (= [{:name "price"
:base_type :type/Number
@@ -148,8 +171,10 @@
(doall
(annotate/column-info
{:type :query, :query {:fields [[:field "price" {:base-type :type/Number, :temporal-unit :month}]]}}
- {:columns [:price]}))))))
+ {:columns [:price]}))))))))
+(deftest ^:parallel col-info-for-field-with-temporal-unit-from-nested-query-test
+ (mt/with-everything-store
(testing "should add the correct info if the Field originally comes from a nested query"
(mt/$ids checkins
(is (= [{:name "DATE", :unit :month, :field_ref [:field %date {:temporal-unit :default}]}
@@ -230,8 +255,9 @@
:display_name "Child"
:fingerprint nil
:base_type :type/Text}
- (into {} (#'annotate/col-info-for-field-clause {} [:field (u/the-id child) nil])))))))
+ (into {} (#'annotate/col-info-for-field-clause {} [:field (u/the-id child) nil]))))))))
+(deftest col-info-combine-grandparent-field-names-test
(testing "nested-nested fields should include grandparent name (etc)"
(t2.with-temp/with-temp [Field grandparent {:name "grandparent", :table_id (mt/id :venues)}
Field parent {:name "parent", :table_id (mt/id :venues), :parent_id (u/the-id grandparent)}
@@ -322,13 +348,12 @@
;; test that added information about aggregations looks the way we'd expect
(defn- aggregation-names
([ag-clause]
- (aggregation-names {} ag-clause))
+ (aggregation-names {:source-table (mt/id :venues)} ag-clause))
([inner-query ag-clause]
(binding [driver/*driver* :h2]
(mt/with-everything-store
- {:name (annotate/aggregation-name ag-clause)
- :display_name (annotate/aggregation-display-name inner-query ag-clause)}))))
+ (select-keys (#'annotate/col-info-for-aggregation-clause inner-query ag-clause) [:name :display_name])))))
(deftest ^:parallel aggregation-names-test
(testing "basic aggregations"
@@ -350,14 +375,14 @@
(aggregation-names [:+ [:count] 1]))))
(testing "expression with nested expressions"
- (is (= {:name "expression", :display_name "Min of ID + (2 * Average of Price)"}
+ (is (= {:name "expression", :display_name "Min of ID + (2 × Average of Price)"}
(aggregation-names
[:+
[:min [:field (mt/id :venues :id) nil]]
[:* 2 [:avg [:field (mt/id :venues :price) nil]]]]))))
(testing "very complicated expression"
- (is (= {:name "expression", :display_name "Min of ID + (2 * Average of Price * 3 * (Max of Category ID - 4))"}
+ (is (= {:name "expression", :display_name "Min of ID + (2 × Average of Price × 3 × (Max of Category ID - 4))"}
(aggregation-names
[:+
[:min [:field (mt/id :venues :id) nil]]
@@ -376,7 +401,7 @@
{:name "generated_name", :display-name "User-specified Name"}]))))
(testing "`:name` only"
- (is (= {:name "generated_name", :display_name "Min of ID + (2 * Average of Price)"}
+ (is (= {:name "generated_name", :display_name "Min of ID + (2 × Average of Price)"}
(aggregation-names
[:aggregation-options
[:+ [:min [:field (mt/id :venues :id) nil]] [:* 2 [:avg [:field (mt/id :venues :price) nil]]]]
@@ -391,7 +416,7 @@
(defn- col-info-for-aggregation-clause
([clause]
- (col-info-for-aggregation-clause {} clause))
+ (col-info-for-aggregation-clause {:source-table (mt/id :venues)} clause))
([inner-query clause]
(binding [driver/*driver* :h2]
@@ -401,60 +426,60 @@
(mt/with-everything-store
(testing "basic aggregation clauses"
(testing "`:count` (no field)"
- (is (= {:base_type :type/Float, :name "expression", :display_name "Count / 2"}
- (col-info-for-aggregation-clause [:/ [:count] 2]))))
+ (is (=? {:base_type :type/Float, :name "expression", :display_name "Count ÷ 2"}
+ (col-info-for-aggregation-clause [:/ [:count] 2]))))
(testing "`:sum`"
- (is (= {:base_type :type/Float, :name "sum", :display_name "Sum of Price + 1"}
- (mt/$ids venues
- (col-info-for-aggregation-clause [:sum [:+ $price 1]]))))))
+ (is (=? {:base_type :type/Integer, :name "sum", :display_name "Sum of Price + 1"}
+ (mt/$ids venues
+ (col-info-for-aggregation-clause [:sum [:+ $price 1]]))))))
(testing "`:aggregation-options`"
(testing "`:name` and `:display-name`"
- (is (= {:base_type :type/Integer
- :semantic_type :type/Category
- :settings nil
- :name "sum_2"
- :display_name "My custom name"}
- (mt/$ids venues
- (col-info-for-aggregation-clause
- [:aggregation-options [:sum $price] {:name "sum_2", :display-name "My custom name"}])))))
+ (is (=? {:base_type :type/Integer
+ :settings nil
+ :name "sum_2"
+ :display_name "My custom name"}
+ (mt/$ids venues
+ (col-info-for-aggregation-clause
+ [:aggregation-options [:sum $price] {:name "sum_2", :display-name "My custom name"}])))))
(testing "`:name` only"
- (is (= {:base_type :type/Integer
- :semantic_type :type/Category
- :settings nil
- :name "sum_2"
- :display_name "Sum of Price"}
- (mt/$ids venues
- (col-info-for-aggregation-clause [:aggregation-options [:sum $price] {:name "sum_2"}])))))
+ (is (=? {:base_type :type/Integer
+ :settings nil
+ :name "sum_2"
+ :display_name "Sum of Price"}
+ (mt/$ids venues
+ (col-info-for-aggregation-clause [:aggregation-options [:sum $price] {:name "sum_2"}])))))
(testing "`:display-name` only"
- (is (= {:base_type :type/Integer
- :semantic_type :type/Category
- :settings nil
- :name "sum"
- :display_name "My Custom Name"}
- (mt/$ids venues
- (col-info-for-aggregation-clause
- [:aggregation-options [:sum $price] {:display-name "My Custom Name"}]))))))
+ (is (=? {:base_type :type/Integer
+ :settings nil
+ :name "sum"
+ :display_name "My Custom Name"}
+ (mt/$ids venues
+ (col-info-for-aggregation-clause
+ [:aggregation-options [:sum $price] {:display-name "My Custom Name"}]))))))
(testing (str "if a driver is kind enough to supply us with some information about the `:cols` that come back, we "
"should include that information in the results. Their information should be preferred over ours")
- (is (= {:cols [{:name "metric"
- :display_name "Total Events"
- :base_type :type/Text
- :effective_type :type/Text
- :source :aggregation
- :field_ref [:aggregation 0]}]}
- (add-column-info
- (mt/mbql-query venues {:aggregation [[:metric "ga:totalEvents"]]})
- {:cols [{:name "totalEvents", :display_name "Total Events", :base_type :type/Text}]}))))
+ (is (=? {:cols [{:name "metric"
+ :display_name "Total Events"
+ :base_type :type/Text
+ :effective_type :type/Text
+ :source :aggregation
+ :field_ref [:aggregation 0]}]}
+ (add-column-info
+ (mt/mbql-query venues {:aggregation [[:metric "ga:totalEvents"]]})
+ {:cols [{:name "totalEvents", :display_name "Total Events", :base_type :type/Text}]}))))
(testing "col info for an `expression` aggregation w/ a named expression should work as expected"
- (is (= {:base_type :type/Float, :name "sum", :display_name "Sum of double-price"}
- (mt/$ids venues
- (col-info-for-aggregation-clause {:expressions {"double-price" [:* $price 2]}} [:sum [:expression "double-price"]])))))))
+ (is (=? {:base_type :type/Integer, :name "sum", :display_name "Sum of double-price"}
+ (mt/$ids venues
+ (col-info-for-aggregation-clause
+ {:source-table (mt/id :venues)
+ :expressions {"double-price" [:* $price 2]}}
+ [:sum [:expression "double-price"]])))))))
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -550,71 +575,71 @@
(deftest ^:parallel test-string-extracts
(is (= {:base_type :type/Text}
- (infered-col-type [:trim "foo"])))
+ (infered-col-type [:trim "foo"])))
(is (= {:base_type :type/Text}
- (infered-col-type [:ltrim "foo"])))
+ (infered-col-type [:ltrim "foo"])))
(is (= {:base_type :type/Text}
- (infered-col-type [:rtrim "foo"])))
+ (infered-col-type [:rtrim "foo"])))
(is (= {:base_type :type/BigInteger}
- (infered-col-type [:length "foo"])))
+ (infered-col-type [:length "foo"])))
(is (= {:base_type :type/Text}
- (infered-col-type [:upper "foo"])))
+ (infered-col-type [:upper "foo"])))
(is (= {:base_type :type/Text}
- (infered-col-type [:lower "foo"])))
+ (infered-col-type [:lower "foo"])))
(is (= {:base_type :type/Text}
- (infered-col-type [:substring "foo" 2])))
+ (infered-col-type [:substring "foo" 2])))
(is (= {:base_type :type/Text}
- (infered-col-type [:replace "foo" "f" "b"])))
+ (infered-col-type [:replace "foo" "f" "b"])))
(is (= {:base_type :type/Text}
- (infered-col-type [:regex-match-first "foo" "f"])))
+ (infered-col-type [:regex-match-first "foo" "f"])))
(is (= {:base_type :type/Text}
- (infered-col-type [:concat "foo" "bar"])))
+ (infered-col-type [:concat "foo" "bar"])))
(is (= {:base_type :type/Text}
- (infered-col-type [:coalesce "foo" "bar"])))
+ (infered-col-type [:coalesce "foo" "bar"])))
(is (= {:base_type :type/Text
:semantic_type :type/Name}
- (infered-col-type [:coalesce [:field (mt/id :venues :name) nil] "bar"]))))
+ (infered-col-type [:coalesce [:field (mt/id :venues :name) nil] "bar"]))))
(deftest ^:parallel unique-name-key-test
(testing "Make sure `:cols` always come back with a unique `:name` key (#8759)"
- (is (= {:cols
- [{:base_type :type/Number
- :effective_type :type/Number
- :semantic_type :type/Quantity
- :name "count"
- :display_name "count"
- :source :aggregation
- :field_ref [:aggregation 0]}
- {:source :aggregation
- :name "sum"
- :display_name "sum"
- :base_type :type/Number
- :effective_type :type/Number
- :field_ref [:aggregation 1]}
- {:base_type :type/Number
- :effective_type :type/Number
- :semantic_type :type/Quantity
- :name "count_2"
- :display_name "count"
- :source :aggregation
- :field_ref [:aggregation 2]}
- {:base_type :type/Number
- :effective_type :type/Number
- :semantic_type :type/Quantity
- :name "count_3"
- :display_name "count_2"
- :source :aggregation
- :field_ref [:aggregation 3]}]}
- (add-column-info
- (mt/mbql-query venues
- {:aggregation [[:count]
- [:sum]
- [:count]
- [:aggregation-options [:count] {:display-name "count_2"}]]})
- {:cols [{:name "count", :display_name "count", :base_type :type/Number}
- {:name "sum", :display_name "sum", :base_type :type/Number}
- {:name "count", :display_name "count", :base_type :type/Number}
- {:name "count_2", :display_name "count_2", :base_type :type/Number}]})))))
+ (is (=? {:cols
+ [{:base_type :type/Number
+ :effective_type :type/Number
+ :semantic_type :type/Quantity
+ :name "count"
+ :display_name "count"
+ :source :aggregation
+ :field_ref [:aggregation 0]}
+ {:source :aggregation
+ :name "sum"
+ :display_name "sum"
+ :base_type :type/Number
+ :effective_type :type/Number
+ :field_ref [:aggregation 1]}
+ {:base_type :type/Number
+ :effective_type :type/Number
+ :semantic_type :type/Quantity
+ :name "count_2"
+ :display_name "count"
+ :source :aggregation
+ :field_ref [:aggregation 2]}
+ {:base_type :type/Number
+ :effective_type :type/Number
+ :semantic_type :type/Quantity
+ :name "count_3"
+ :display_name "count_2"
+ :source :aggregation
+ :field_ref [:aggregation 3]}]}
+ (add-column-info
+ (mt/mbql-query venues
+ {:aggregation [[:count]
+ [:sum]
+ [:count]
+ [:aggregation-options [:count] {:display-name "count_2"}]]})
+ {:cols [{:name "count", :display_name "count", :base_type :type/Number}
+ {:name "sum", :display_name "sum", :base_type :type/Number}
+ {:name "count", :display_name "count", :base_type :type/Number}
+ {:name "count_2", :display_name "count_2", :base_type :type/Number}]})))))
(deftest ^:parallel expressions-keys-test
(testing "make sure expressions come back with the right set of keys, including `:expression_name` (#8854)"
@@ -636,20 +661,20 @@
(deftest ^:parallel deduplicate-expression-names-test
(testing "make sure multiple expressions come back with deduplicated names"
(testing "expressions in aggregations"
- (is (= [{:base_type :type/Float, :name "expression", :display_name "0.9 * Average of Price", :source :aggregation, :field_ref [:aggregation 0]}
- {:base_type :type/Float, :name "expression_2", :display_name "0.8 * Average of Price", :source :aggregation, :field_ref [:aggregation 1]}]
- (:cols (add-column-info
- (mt/mbql-query venues
- {:aggregation [[:* 0.9 [:avg $price]] [:* 0.8 [:avg $price]]]
- :limit 10})
- {})))))
+ (is (=? [{:base_type :type/Float, :name "expression", :display_name "0.9 × Average of Price", :source :aggregation, :field_ref [:aggregation 0]}
+ {:base_type :type/Float, :name "expression_2", :display_name "0.8 × Average of Price", :source :aggregation, :field_ref [:aggregation 1]}]
+ (:cols (add-column-info
+ (mt/mbql-query venues
+ {:aggregation [[:* 0.9 [:avg $price]] [:* 0.8 [:avg $price]]]
+ :limit 10})
+ {})))))
(testing "named :expressions"
- (is (= [{:name "prev_month", :display_name "prev_month", :base_type :type/DateTime, :expression_name "prev_month", :source :fields, :field_ref [:expression "prev_month"]}]
- (:cols (add-column-info
- (mt/mbql-query users
- {:expressions {:prev_month [:+ $last_login [:interval -1 :month]]}
- :fields [[:expression "prev_month"]], :limit 10})
- {})))))))
+ (is (=? [{:name "prev_month", :display_name "prev_month", :base_type :type/DateTime, :expression_name "prev_month", :source :fields, :field_ref [:expression "prev_month"]}]
+ (:cols (add-column-info
+ (mt/mbql-query users
+ {:expressions {:prev_month [:+ $last_login [:interval -1 :month]]}
+ :fields [[:expression "prev_month"]], :limit 10})
+ {})))))))
(deftest mbql-cols-nested-queries-test
(testing "Should be able to infer MBQL columns with nested queries"
@@ -677,39 +702,38 @@
(testing "Aggregated question with source is an aggregated models should infer display_name correctly (#23248)"
(mt/dataset sample-dataset
- (mt/with-temp* [Card [{card-id :id}
- {:dataset true
- :dataset_query
- (mt/$ids :products
- {:type :query
- :database (mt/id)
- :query {:source-table $$products
- :aggregation
- [[:aggregation-options
- [:sum $price]
- {:name "sum"}]
- [:aggregation-options
- [:max $rating]
- {:name "max"}]]
- :breakout $category
- :order-by [[:asc $category]]}})}]]
- (let [query (qp/preprocess
+ (t2.with-temp/with-temp [Card {card-id :id} {:dataset true
+ :dataset_query
+ (mt/$ids :products
+ {:type :query
+ :database (mt/id)
+ :query {:source-table $$products
+ :aggregation
+ [[:aggregation-options
+ [:sum $price]
+ {:name "sum"}]
+ [:aggregation-options
+ [:max $rating]
+ {:name "max"}]]
+ :breakout $category
+ :order-by [[:asc $category]]}})}]
+ (let [query (qp/preprocess
(mt/mbql-query nil
- {:source-table (str "card__" card-id)
- :aggregation [[:aggregation-options
- [:sum
- [:field
- "sum"
- {:base-type :type/Float}]]
- {:name "sum"}]
- [:aggregation-options
- [:count]
- {:name "count"}]]
- :limit 1}))]
- (is (= ["Sum of Sum of Price" "Count"]
- (->> (add-column-info query {})
- :cols
- (map :display_name)))))))))
+ {:source-table (str "card__" card-id)
+ :aggregation [[:aggregation-options
+ [:sum
+ [:field
+ "sum"
+ {:base-type :type/Float}]]
+ {:name "sum"}]
+ [:aggregation-options
+ [:count]
+ {:name "count"}]]
+ :limit 1}))]
+ (is (= ["Sum of Sum of Price" "Count"]
+ (->> (add-column-info query {})
+ :cols
+ (map :display_name)))))))))
(deftest ^:parallel inception-test
(testing "Should return correct metadata for an 'inception-style' nesting of source > source > source with a join (#14745)"
@@ -741,42 +765,43 @@
:field_ref &Products.ean})
(ean-metadata (add-column-info nested-query {}))))))))))))))
-;; metabase#14787
(deftest col-info-for-fields-from-card-test
- (mt/dataset sample-dataset
- (let [card-1-query (mt/mbql-query orders
- {:joins [{:fields :all
- :source-table $$products
- :condition [:= $product_id &Products.products.id]
- :alias "Products"}]})]
- (mt/with-temp* [Card [{card-1-id :id} {:dataset_query card-1-query}]
- Card [{card-2-id :id} {:dataset_query (mt/mbql-query people)}]]
- (testing "when a nested query is from a saved question, there should be no `:join-alias` on the left side"
- (mt/$ids nil
- (let [base-query (qp/preprocess
- (mt/mbql-query nil
- {:source-table (str "card__" card-1-id)
- :joins [{:fields :all
- :source-table (str "card__" card-2-id)
- :condition [:= $orders.user_id &Products.products.id]
- :alias "Q"}]
- :limit 1}))
- fields #{%orders.discount %products.title %people.source}]
- (is (= [{:display_name "Discount" :field_ref [:field %orders.discount nil]}
- {:display_name "Products → Title" :field_ref [:field %products.title nil]}
- {:display_name "Q → Source" :field_ref [:field %people.source {:join-alias "Q"}]}]
- (->> (:cols (add-column-info base-query {}))
- (filter #(fields (:id %)))
- (map #(select-keys % [:display_name :field_ref])))))))))))
-
- (testing "Has the correct display names for joined fields from cards"
+ (testing "#14787"
+ (mt/dataset sample-dataset
+ (let [card-1-query (mt/mbql-query orders
+ {:joins [{:fields :all
+ :source-table $$products
+ :condition [:= $product_id &Products.products.id]
+ :alias "Products"}]})]
+ (t2.with-temp/with-temp [Card {card-1-id :id} {:dataset_query card-1-query}
+ Card {card-2-id :id} {:dataset_query (mt/mbql-query people)}]
+ (testing "when a nested query is from a saved question, there should be no `:join-alias` on the left side"
+ (mt/$ids nil
+ (let [base-query (qp/preprocess
+ (mt/mbql-query nil
+ {:source-table (str "card__" card-1-id)
+ :joins [{:fields :all
+ :source-table (str "card__" card-2-id)
+ :condition [:= $orders.user_id &Products.products.id]
+ :alias "Q"}]
+ :limit 1}))
+ fields #{%orders.discount %products.title %people.source}]
+ (is (= [{:display_name "Discount" :field_ref [:field %orders.discount nil]}
+ {:display_name "Products → Title" :field_ref [:field %products.title nil]}
+ {:display_name "Q → Source" :field_ref [:field %people.source {:join-alias "Q"}]}]
+ (->> (:cols (add-column-info base-query {}))
+ (filter #(fields (:id %)))
+ (map #(select-keys % [:display_name :field_ref])))))))))))))
+
+(deftest col-info-for-joined-fields-from-card-test
+ (testing "Has the correct display names for joined fields from cards (#14787)"
(letfn [(native [query] {:type :native
:native {:query query :template-tags {}}
:database (mt/id)})]
- (mt/with-temp* [Card [{card1-id :id} {:dataset_query
- (native "select 'foo' as A_COLUMN")}]
- Card [{card2-id :id} {:dataset_query
- (native "select 'foo' as B_COLUMN")}]]
+ (t2.with-temp/with-temp [Card {card1-id :id} {:dataset_query
+ (native "select 'foo' as A_COLUMN")}
+ Card {card2-id :id} {:dataset_query
+ (native "select 'foo' as B_COLUMN")}]
(doseq [card-id [card1-id card2-id]]
;; populate metadata
(mt/user-http-request :rasta :post 202 (format "card/%d/query" card-id)))
diff --git a/test/metabase/query_processor/middleware/pre_alias_aggregations_test.clj b/test/metabase/query_processor/middleware/pre_alias_aggregations_test.clj
index e3b3b552a13..fef9d4daa15 100644
--- a/test/metabase/query_processor/middleware/pre_alias_aggregations_test.clj
+++ b/test/metabase/query_processor/middleware/pre_alias_aggregations_test.clj
@@ -7,13 +7,15 @@
[metabase.driver :as driver]
[metabase.query-processor.middleware.pre-alias-aggregations
:as qp.pre-alias-aggregations]
+ [metabase.query-processor.store :as qp.store]
[metabase.test :as mt]))
(defn- pre-alias [query]
- (driver/with-driver (or driver/*driver* :h2)
- (qp.pre-alias-aggregations/pre-alias-aggregations query)))
+ (mt/with-everything-store
+ (driver/with-driver (or driver/*driver* :h2)
+ (qp.pre-alias-aggregations/pre-alias-aggregations query))))
-(deftest pre-alias-aggregations-test
+(deftest ^:parallel pre-alias-aggregations-test
(is (= (mt/mbql-query checkins
{:source-table $$checkins
:aggregation [[:aggregation-options [:sum $user_id] {:name "sum"}]
@@ -23,7 +25,7 @@
{:source-table $$checkins
:aggregation [[:sum $user_id] [:sum $venue_id]]})))))
-(deftest named-aggregations-test
+(deftest ^:parallel named-aggregations-test
(testing "if one or more aggregations are already named, do things still work correctly?"
(is (= (mt/mbql-query checkins
{:source-table $$checkins
@@ -35,20 +37,20 @@
:aggregation [[:sum $user_id]
[:aggregation-options [:sum $venue_id] {:name "sum"}]]}))))))
-(deftest source-queries-test
- (testing "do aggregations inside source queries get pre-aliased?")
- (is (= (mt/mbql-query checkins
- {:source-query {:source-table $$checkins
- :aggregation [[:aggregation-options [:sum $user_id] {:name "sum"}]
- [:aggregation-options [:sum $venue_id] {:name "sum_2"}]]}
- :aggregation [[:aggregation-options [:count] {:name "count"}]]})
- (pre-alias
- (mt/mbql-query checkins
- {:source-query {:source-table $$checkins
- :aggregation [[:sum $user_id] [:sum $venue_id]]}
- :aggregation [[:count]]})))))
+(deftest ^:parallel source-queries-test
+ (testing "do aggregations inside source queries get pre-aliased?"
+ (is (= (mt/mbql-query checkins
+ {:source-query {:source-table $$checkins
+ :aggregation [[:aggregation-options [:sum $user_id] {:name "sum"}]
+ [:aggregation-options [:sum $venue_id] {:name "sum_2"}]]}
+ :aggregation [[:aggregation-options [:count] {:name "count"}]]})
+ (pre-alias
+ (mt/mbql-query checkins
+ {:source-query {:source-table $$checkins
+ :aggregation [[:sum $user_id] [:sum $venue_id]]}
+ :aggregation [[:count]]}))))))
-(deftest source-queries-inside-joins-test
+(deftest ^:parallel source-queries-inside-joins-test
(testing "do aggregatons inside of source queries inside joins get pre-aliased?"
(is (= (mt/mbql-query checkins
{:source-table $$venues
@@ -69,7 +71,7 @@
:alias "checkins"
:condition [:= &checkins.venue_id $venues.id]}]}))))))
-(deftest expressions-test
+(deftest ^:parallel expressions-test
(testing "does pre-aliasing work the way we'd expect with expressions?"
(is (= {:database 1
:type :query
@@ -79,30 +81,32 @@
{:database 1
:type :query
:query {:source-table 1
- :aggregation [[:+ 20 [:sum [:field 2 nil]]]]}})))
+ :aggregation [[:+ 20 [:sum [:field 2 nil]]]]}})))))
- (is (= {:database 1
- :type :query
- :query {:source-table 1
- :aggregation [[:aggregation-options
- [:+ 20 [:sum [:field 2 nil]]]
- {:name "expression"}]
- [:aggregation-options
- [:- 20 [:sum [:field 2 nil]]]
- {:name "expression_2"}]]}}
- (pre-alias
- {:database 1
- :type :query
- :query {:source-table 1
- :aggregation [[:+ 20 [:sum [:field 2 nil]]]
- [:- 20 [:sum [:field 2 nil]]]]}})))))
+(deftest ^:parallel expressions-test-2
+ (is (= {:database 1
+ :type :query
+ :query {:source-table 1
+ :aggregation [[:aggregation-options
+ [:+ 20 [:sum [:field 2 nil]]]
+ {:name "expression"}]
+ [:aggregation-options
+ [:- 20 [:sum [:field 2 nil]]]
+ {:name "expression_2"}]]}}
+ (pre-alias
+ {:database 1
+ :type :query
+ :query {:source-table 1
+ :aggregation [[:+ 20 [:sum [:field 2 nil]]]
+ [:- 20 [:sum [:field 2 nil]]]]}}))))
-(driver/register! ::test-driver, :abstract? true, :parent :sql)
+(driver/register! ::test-driver, :parent :sql)
-(defmethod driver/escape-alias ::test-driver [_ custom-field-name]
+(defmethod driver/escape-alias ::test-driver
+ [_driver custom-field-name]
(str \_ custom-field-name))
-(deftest use-escape-alias-test
+(deftest ^:parallel use-escape-alias-test
(testing (str "we should use [[driver/escape-alias]] on the generated aggregation names in case the "
"drivers need to tweak the default names we generate."))
(is (= {:database 1
@@ -110,10 +114,13 @@
:query {:source-table 1
:aggregation [[:aggregation-options [:+ 20 [:sum [:field 2 nil]]] {:name "_expression"}]
[:aggregation-options [:count] {:name "_count"}]]}}
- (driver/with-driver ::test-driver
- (pre-alias
- {:database 1
- :type :query
- :query {:source-table 1
- :aggregation [[:+ 20 [:sum [:field 2 nil]]]
- [:count]]}})))))
+ (let [db (mt/db)]
+ (driver/with-driver ::test-driver
+ (qp.store/with-store
+ (qp.store/store-database! db)
+ (qp.pre-alias-aggregations/pre-alias-aggregations
+ {:database 1
+ :type :query
+ :query {:source-table 1
+ :aggregation [[:+ 20 [:sum [:field 2 nil]]]
+ [:count]]}})))))))
diff --git a/test/metabase/query_processor/pivot_test.clj b/test/metabase/query_processor/pivot_test.clj
index 22fc61bdb45..993d704e907 100644
--- a/test/metabase/query_processor/pivot_test.clj
+++ b/test/metabase/query_processor/pivot_test.clj
@@ -17,7 +17,7 @@
(set! *warn-on-reflection* true)
-(deftest group-bitmask-test
+(deftest ^:parallel group-bitmask-test
(doseq [[indices expected] {[0] 6
[0 1] 4
[0 1 2] 0
@@ -25,7 +25,7 @@
(is (= expected
(#'qp.pivot/group-bitmask 3 indices)))))
-(deftest powerset-test
+(deftest ^:parallel powerset-test
(is (= [[]]
(#'qp.pivot/powerset [])))
(is (= [[0] []]
@@ -35,7 +35,7 @@
(is (= [[0 1 2] [1 2] [0 2] [2] [0 1] [1] [0] []]
(#'qp.pivot/powerset [0 1 2]))))
-(deftest breakout-combinations-test
+(deftest ^:parallel breakout-combinations-test
(testing "Should return the combos that Paul specified in (#14329)"
(is (= [[0 1 2]
[0 1]
@@ -70,7 +70,7 @@
[]]
(#'qp.pivot/breakout-combinations 3 [] []))))))
-(deftest validate-pivot-rows-cols-test
+(deftest ^:parallel validate-pivot-rows-cols-test
(testing "Should throw an Exception if you pass in invalid pivot-rows"
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
diff --git a/test/metabase/query_processor/util/add_alias_info_test.clj b/test/metabase/query_processor/util/add_alias_info_test.clj
index 28238422004..741bd3c50c2 100644
--- a/test/metabase/query_processor/util/add_alias_info_test.clj
+++ b/test/metabase/query_processor/util/add_alias_info_test.clj
@@ -39,7 +39,7 @@
(driver/with-driver (or driver/*driver* :h2)
(-> query qp/preprocess add/add-alias-info remove-source-metadata (dissoc :middleware)))))
-(deftest join-in-source-query-test
+(deftest ^:parallel join-in-source-query-test
(is (query= (mt/mbql-query venues
{:source-query {:source-table $$venues
:joins [{:strategy :left-join
@@ -83,7 +83,7 @@
:breakout [&Cat.categories.name]
:limit 1})))))
-(deftest multiple-joins-test
+(deftest ^:parallel multiple-joins-test
(mt/dataset sample-dataset
(is (query= (mt/mbql-query orders
{:source-query {:source-table $$orders
@@ -159,7 +159,7 @@
:alias "P2"}]}}]
:limit 1}))))))
-(deftest uniquify-aliases-test
+(deftest ^:parallel uniquify-aliases-test
(mt/dataset sample-dataset
(is (query= (mt/mbql-query products
{:source-table $$products
@@ -184,7 +184,7 @@
[:expression "CATEGORY"]]
:limit 1}))))))
-(deftest not-null-test
+(deftest ^:parallel not-null-test
(is (query= (mt/mbql-query checkins
{:aggregation [[:aggregation-options
[:count]
@@ -208,7 +208,7 @@
{:aggregation [[:count]]
:filter [:not-null $date]})))))
-(deftest duplicate-aggregations-test
+(deftest ^:parallel duplicate-aggregations-test
(is (query= (mt/mbql-query venues
{:source-query {:source-table $$venues
:aggregation [[:aggregation-options
@@ -229,17 +229,17 @@
::add/source-alias "count_3"
::add/desired-alias "count_3"
::add/position 2}]]}
- :fields [[:field "count" {:base-type :type/BigInteger
+ :fields [[:field "count" {:base-type :type/Integer
::add/source-table ::add/source
::add/source-alias "count"
::add/desired-alias "count"
::add/position 0}]
- [:field "count_2" {:base-type :type/BigInteger
+ [:field "count_2" {:base-type :type/Integer
::add/source-table ::add/source
::add/source-alias "count_2"
::add/desired-alias "count_2"
::add/position 1}]
- [:field "count_3" {:base-type :type/BigInteger
+ [:field "count_3" {:base-type :type/Integer
::add/source-table ::add/source
::add/source-alias "count_3"
::add/desired-alias "count_3"
@@ -257,7 +257,7 @@
[:count]]}
:limit 1})))))
-(deftest multiple-expressions-test
+(deftest ^:parallel multiple-expressions-test
(mt/with-everything-store
(is (query= (mt/$ids venues
{$price 0
@@ -384,7 +384,7 @@
[_driver field-alias]
(prefix-alias field-alias))
-(deftest custom-escape-alias-test
+(deftest ^:parallel custom-escape-alias-test
(let [db (mt/db)]
(driver/with-driver ::custom-escape
(mt/with-db db
@@ -424,7 +424,7 @@
add-alias-info
:query)))))))
-(deftest custom-escape-alias-filtering-aggregation-test
+(deftest ^:parallel custom-escape-alias-filtering-aggregation-test
(let [db (mt/db)]
(driver/with-driver ::custom-escape
(mt/with-db db
@@ -443,7 +443,7 @@
::add/position 1}
outer-count-opts (-> count-opts
(dissoc :name)
- (assoc :base-type :type/BigInteger
+ (assoc :base-type :type/Integer
::add/source-table ::add/source)
(update ::add/source-alias prefix-alias)
(update ::add/desired-alias prefix-alias))]
@@ -460,7 +460,7 @@
[:field
"strange count"
outer-count-opts]
- [:value 10 {:base_type :type/BigInteger}]]
+ [:value 10 {:base_type :type/Integer}]]
:limit 1}))
(-> (mt/mbql-query venues
{:source-query {:source-table $$venues
@@ -468,7 +468,7 @@
[:count]
{:name "strange count"}]]
:breakout [$price]}
- :filter [:< [:field "strange count" {:base-type :type/BigInteger}] 10]
+ :filter [:< [:field "strange count" {:base-type :type/Integer}] 10]
:limit 1})
add-alias-info
:query)))))))
@@ -480,7 +480,7 @@
(-> ((get-method driver/escape-alias :h2) driver field-alias)
(str/replace #"\s" "_")))
-(deftest use-correct-alias-for-joined-field-test
+(deftest ^:parallel use-correct-alias-for-joined-field-test
(testing "Make sure we call `driver/escape-alias` for the `:source-alias` for Fields coming from joins (#20413)"
(mt/dataset sample-dataset
(let [db (mt/db)]
@@ -570,7 +570,7 @@
add-alias-info
:query)))))))))
-(deftest query->expected-cols-test
+(deftest ^:parallel query->expected-cols-test
(testing "field_refs in expected columns have the original join aliases (#30648)"
(mt/dataset sample-dataset
(binding [driver/*driver* ::custom-escape-spaces-to-underscores]
@@ -646,7 +646,7 @@
add-alias-info
:query)))))))
-(deftest aggregation-reference-test
+(deftest ^:parallel aggregation-reference-test
(testing "Make sure we add info to `:aggregation` reference clauses correctly"
(is (query= (mt/mbql-query checkins
{:aggregation [[:aggregation-options
@@ -663,7 +663,7 @@
{:aggregation [[:sum $user_id]]
:order-by [[:asc [:aggregation 0]]]}))))))
-(deftest uniquify-aggregation-names-text
+(deftest ^:parallel uniquify-aggregation-names-text
(is (query= (mt/mbql-query checkins
{:expressions {"count" [:+ 1 1]}
:breakout [[:expression "count" {::add/desired-alias "count"
@@ -682,7 +682,7 @@
:aggregation [[:count]]
:limit 1})))))
-(deftest fuzzy-field-info-test
+(deftest ^:parallel fuzzy-field-info-test
(testing "[[add/alias-from-join]] should match Fields in the Join source query even if they have temporal units"
(mt/with-driver :h2
(mt/dataset sample-dataset
diff --git a/test/metabase/query_processor/util/nest_query_test.clj b/test/metabase/query_processor/util/nest_query_test.clj
index 676e29b0c16..d27ec5eefa1 100644
--- a/test/metabase/query_processor/util/nest_query_test.clj
+++ b/test/metabase/query_processor/util/nest_query_test.clj
@@ -27,7 +27,7 @@
nest-query/nest-expressions
remove-source-metadata))))
-(deftest nest-expressions-test
+(deftest ^:parallel nest-expressions-test
(driver/with-driver :h2
(mt/with-everything-store
(is (partial= (mt/$ids venues
@@ -68,7 +68,7 @@
add/add-alias-info
nest-expressions))))))
-(deftest nest-expressions-with-existing-non-expression-fields-test
+(deftest ^:parallel nest-expressions-with-existing-non-expression-fields-test
(driver/with-driver :h2
(mt/with-everything-store
(testing "Other `:fields` besides the `:expressions` should be preserved in the top level"
@@ -119,7 +119,7 @@
add/add-alias-info
nest-expressions)))))))
-(deftest multiple-expressions-test
+(deftest ^:parallel multiple-expressions-test
(testing "Make sure the nested version of the query doesn't mix up expressions if we have ones that reference others"
(driver/with-driver :h2
(mt/with-everything-store
@@ -170,7 +170,7 @@
add/add-alias-info
nest-expressions)))))))
-(deftest nest-expressions-ignore-source-queries-test
+(deftest ^:parallel nest-expressions-ignore-source-queries-test
(testing (str "When 'raising' :expression clauses, only raise ones in the current level. Handle duplicate expression "
"names correctly.")
(driver/with-driver :h2
@@ -235,7 +235,9 @@
:desired-alias "PRICE"}]
[:expression "x" #::add{:desired-alias "x"}]]}}}
:limit 1})
- (-> query add/add-alias-info nest-expressions))))))))
+ (-> query add/add-alias-info nest-expressions)))))))))
+
+(deftest nest-expressions-ignore-source-queries-from-joins-test
(testing "Ignores source-query from joins (#20809)"
(let [query {:source-table 2,
:expressions {"CC" [:+ 1 1]},
@@ -305,7 +307,7 @@
:expressions {"CC" [:+ 1 1]}
:limit 2})))))))))
-(deftest nest-expressions-with-joins-test
+(deftest ^:parallel nest-expressions-with-joins-test
(driver/with-driver :h2
(mt/with-everything-store
(testing "If there are any `:joins`, those need to be nested into the `:source-query` as well."
@@ -364,7 +366,7 @@
::add/source-alias "MaxPrice"
::add/desired-alias "CategoriesStats__MaxPrice"
::add/position 8}]
- [:field "AvgPrice" {:base-type :type/Integer
+ [:field "AvgPrice" {:base-type :type/Float
:join-alias "CategoriesStats"
::add/source-table "CategoriesStats"
::add/source-alias "AvgPrice"
@@ -424,7 +426,7 @@
::add/source-alias "MaxPrice"
::add/desired-alias "CategoriesStats__MaxPrice"
::add/position 8}]
- [:field "AvgPrice" {:base-type :type/Integer
+ [:field "AvgPrice" {:base-type :type/Float
:join-alias "CategoriesStats"
::add/source-table "CategoriesStats"
::add/source-alias "AvgPrice"
@@ -552,7 +554,7 @@
add/add-alias-info
nest-expressions))))))))
-(deftest multiple-joins-with-expressions-test
+(deftest ^:parallel multiple-joins-with-expressions-test
(testing "We should be able to compile a complicated query with multiple joins and expressions correctly"
(driver/with-driver :h2
(mt/dataset sample-dataset
@@ -632,7 +634,7 @@
add/add-alias-info
nest-expressions))))))))
-(deftest uniquify-aliases-test
+(deftest ^:parallel uniquify-aliases-test
(driver/with-driver :h2
(mt/dataset sample-dataset
(mt/with-everything-store
diff --git a/test/metabase/query_processor_test/aggregation_test.clj b/test/metabase/query_processor_test/aggregation_test.clj
index 7aceba838a7..9dfe3186403 100644
--- a/test/metabase/query_processor_test/aggregation_test.clj
+++ b/test/metabase/query_processor_test/aggregation_test.clj
@@ -8,7 +8,7 @@
[metabase.test.data :as data]
[metabase.test.util :as tu]))
-(deftest no-aggregation-test
+(deftest ^:parallel no-aggregation-test
(mt/test-drivers (mt/normal-drivers)
(testing "Test that no aggregation just returns rows as-is."
(is (= [[1 "Red Medicine" 4 10.0646 -165.374 3]
@@ -25,33 +25,39 @@
(mt/run-mbql-query venues
{:limit 10, :order-by [[:asc $id]]})))))))
-(deftest basic-aggregations-test
+(deftest ^:parallel count-test
(mt/test-drivers (mt/normal-drivers)
(testing "count aggregation"
(is (= [[100]]
(mt/formatted-rows [int]
(mt/run-mbql-query venues
- {:aggregation [[:count]]})))))
+ {:aggregation [[:count]]})))))))
+(deftest ^:parallel sum-test
+ (mt/test-drivers (mt/normal-drivers)
(testing "sum aggregation"
(is (= [[203]]
(mt/formatted-rows [int]
(mt/run-mbql-query venues
- {:aggregation [[:sum $price]]})))))
+ {:aggregation [[:sum $price]]})))))))
+(deftest ^:parallel avg-test
+ (mt/test-drivers (mt/normal-drivers)
(testing "avg aggregation"
(is (= [[35.5059]]
(mt/formatted-rows [4.0]
(mt/run-mbql-query venues
- {:aggregation [[:avg $latitude]]})))))
+ {:aggregation [[:avg $latitude]]})))))))
+(deftest ^:parallel distinct-count-test
+ (mt/test-drivers (mt/normal-drivers)
(testing "distinct count aggregation"
(is (= [[15]]
(mt/formatted-rows [int]
(mt/run-mbql-query checkins
{:aggregation [[:distinct $user_id]]})))))))
-(deftest standard-deviation-test
+(deftest ^:parallel standard-deviation-test
(mt/test-drivers (mt/normal-drivers-with-feature :standard-deviation-aggregations)
(testing "standard deviation aggregations"
(let [query (mt/mbql-query venues {:aggregation [[:stddev $latitude]]})]
@@ -60,8 +66,9 @@
:rows [[3.4]]}
(qp.test/rows-and-cols
(mt/format-rows-by [1.0]
- (mt/process-query query)))))))))
+ (mt/process-query query))))))))))
+(deftest ^:parallel standard-deviation-unsupported-test
(mt/test-drivers (mt/normal-drivers-without-feature :standard-deviation-aggregations)
(testing "Make sure standard deviations fail for drivers that don't support it"
(is (thrown-with-msg?
@@ -77,28 +84,32 @@
;;; | MIN & MAX |
;;; +----------------------------------------------------------------------------------------------------------------+
-(deftest min-test
+(deftest ^:parallel min-test
(mt/test-drivers (mt/normal-drivers)
(is (= [1]
(mt/first-row
(mt/format-rows-by [int]
(mt/run-mbql-query venues
- {:aggregation [[:min $price]]})))))
+ {:aggregation [[:min $price]]})))))))
+(deftest ^:parallel min-test-2
+ (mt/test-drivers (mt/normal-drivers)
(is (= [[1 34.0071] [2 33.7701] [3 10.0646] [4 33.983]]
(mt/formatted-rows [int 4.0]
(mt/run-mbql-query venues
{:aggregation [[:min $latitude]]
:breakout [$price]}))))))
-(deftest max-test
+(deftest ^:parallel max-test
(mt/test-drivers (mt/normal-drivers)
(is (= [4]
(mt/first-row
(mt/format-rows-by [int]
(mt/run-mbql-query venues
- {:aggregation [[:max $price]]})))))
+ {:aggregation [[:max $price]]})))))))
+(deftest ^:parallel max-test-2
+ (mt/test-drivers (mt/normal-drivers)
(is (= [[1 37.8078] [2 40.7794] [3 40.7262] [4 40.7677]]
(mt/formatted-rows [int 4.0]
(mt/run-mbql-query venues
@@ -110,45 +121,44 @@
;;; | MULTIPLE AGGREGATIONS |
;;; +----------------------------------------------------------------------------------------------------------------+
-(deftest multiple-aggregations-test
+(deftest ^:parallel multiple-aggregations-test
(mt/test-drivers (mt/normal-drivers)
(testing "two aggregations"
(is (= [[100 203]]
(mt/formatted-rows [int int]
(mt/run-mbql-query venues
- {:aggregation [[:count] [:sum $price]]})))))
+ {:aggregation [[:count] [:sum $price]]})))))))
+(deftest ^:parallel multiple-aggregations-test-2
+ (mt/test-drivers (mt/normal-drivers)
(testing "three aggregations"
(is (= [[2 100 203]]
(mt/formatted-rows [int int int]
(mt/run-mbql-query venues
{:aggregation [[:avg $price] [:count] [:sum $price]]})))))))
-(deftest multiple-aggregations-metadata-test
- ;; TODO - this isn't tested against Mongo because those driver doesn't currently work correctly with multiple
- ;; columns with the same name. It seems like it would be pretty easy to take the stuff we have for BigQuery and
- ;; generalize it so we can use it with Mongo
- ;;
- ;; TODO part 2 -- not sure if this is still the case?
- (mt/test-drivers (disj (mt/normal-drivers) :mongo)
+(deftest ^:parallel multiple-aggregations-metadata-test
+ (mt/test-drivers (mt/normal-drivers)
(testing "make sure that multiple aggregations of the same type have the correct metadata (#4003)"
(is (= [(qp.test/aggregate-col :count)
(assoc (qp.test/aggregate-col :count) :name "count_2", :field_ref [:aggregation 1])]
(mt/cols
- (mt/run-mbql-query venues
- {:aggregation [[:count] [:count]]})))))))
+ (mt/run-mbql-query venues
+ {:aggregation [[:count] [:count]]})))))))
;;; ------------------------------------------------- CUMULATIVE SUM -------------------------------------------------
-(deftest cumulate-sum-test
+(deftest ^:parallel cumulative-sum-test
(mt/test-drivers (mt/normal-drivers)
(testing "cum_sum w/o breakout should be treated the same as sum"
(is (= [[120]]
(mt/formatted-rows [int]
(mt/run-mbql-query users
- {:aggregation [[:cum-sum $id]]})))))
+ {:aggregation [[:cum-sum $id]]})))))))
+(deftest ^:parallel cumulative-sum-test-2
+ (mt/test-drivers (mt/normal-drivers)
(testing " Simple cumulative sum where breakout field is same as cum_sum field"
(is (= [[ 1 1]
[ 2 3]
@@ -168,8 +178,10 @@
(mt/formatted-rows [int int]
(mt/run-mbql-query users
{:aggregation [[:cum-sum $id]]
- :breakout [$id]})))))
+ :breakout [$id]})))))))
+(deftest ^:parallel cumulative-sum-test-3
+ (mt/test-drivers (mt/normal-drivers)
(testing " Cumulative sum w/ a different breakout field"
(is (= [["Broen Olujimi" 14]
["Conchúr Tihomir" 21]
@@ -189,8 +201,10 @@
(mt/formatted-rows [str int]
(mt/run-mbql-query users
{:aggregation [[:cum-sum $id]]
- :breakout [$name]})))))
+ :breakout [$name]})))))))
+(deftest ^:parallel cumulative-sum-test-4
+ (mt/test-drivers (mt/normal-drivers)
(testing " Cumulative sum w/ a different breakout field that requires grouping"
(is (= [[1 1211]
[2 4066]
@@ -203,7 +217,7 @@
;;; ------------------------------------------------ CUMULATIVE COUNT ------------------------------------------------
-(deftest cumulative-count-test
+(deftest ^:parallel cumulative-count-test
(mt/test-drivers (mt/normal-drivers)
(testing "cumulative count aggregations"
(testing "w/o breakout should be treated the same as count"
@@ -212,44 +226,48 @@
(qp.test/rows-and-cols
(mt/format-rows-by [int]
(mt/run-mbql-query users
- {:aggregation [[:cum-count $id]]}))))))
-
- (testing "w/ breakout on field with distinct values"
- (is (= {:rows [["Broen Olujimi" 1]
- ["Conchúr Tihomir" 2]
- ["Dwight Gresham" 3]
- ["Felipinho Asklepios" 4]
- ["Frans Hevel" 5]
- ["Kaneonuskatew Eiran" 6]
- ["Kfir Caj" 7]
- ["Nils Gotam" 8]
- ["Plato Yeshua" 9]
- ["Quentin Sören" 10]
- ["Rüstem Hebel" 11]
- ["Shad Ferdynand" 12]
- ["Simcha Yan" 13]
- ["Spiros Teofil" 14]
- ["Szymon Theutrich" 15]]
- :cols [(qp.test/breakout-col :users :name)
- (qp.test/aggregate-col :cum-count :users :id)]}
- (qp.test/rows-and-cols
- (mt/format-rows-by [str int]
- (mt/run-mbql-query users
- {:aggregation [[:cum-count $id]]
- :breakout [$name]}))))))
-
- (testing "w/ breakout on field that requires grouping"
- (is (= {:cols [(qp.test/breakout-col :venues :price)
- (qp.test/aggregate-col :cum-count :venues :id)]
- :rows [[1 22]
- [2 81]
- [3 94]
- [4 100]]}
- (qp.test/rows-and-cols
- (mt/format-rows-by [int int]
- (mt/run-mbql-query venues
- {:aggregation [[:cum-count $id]]
- :breakout [$price]})))))))))
+ {:aggregation [[:cum-count $id]]})))))))))
+
+(deftest ^:parallel cumulative-count-with-breakout-test
+ (mt/test-drivers (mt/normal-drivers)
+ (testing "w/ breakout on field with distinct values"
+ (is (= {:rows [["Broen Olujimi" 1]
+ ["Conchúr Tihomir" 2]
+ ["Dwight Gresham" 3]
+ ["Felipinho Asklepios" 4]
+ ["Frans Hevel" 5]
+ ["Kaneonuskatew Eiran" 6]
+ ["Kfir Caj" 7]
+ ["Nils Gotam" 8]
+ ["Plato Yeshua" 9]
+ ["Quentin Sören" 10]
+ ["Rüstem Hebel" 11]
+ ["Shad Ferdynand" 12]
+ ["Simcha Yan" 13]
+ ["Spiros Teofil" 14]
+ ["Szymon Theutrich" 15]]
+ :cols [(qp.test/breakout-col :users :name)
+ (qp.test/aggregate-col :cum-count :users :id)]}
+ (qp.test/rows-and-cols
+ (mt/format-rows-by [str int]
+ (mt/run-mbql-query users
+ {:aggregation [[:cum-count $id]]
+ :breakout [$name]}))))))))
+
+(deftest ^:parallel cumulative-count-with-breakout-test-2
+ (mt/test-drivers (mt/normal-drivers)
+ (testing "w/ breakout on field that requires grouping"
+ (is (= {:cols [(qp.test/breakout-col :venues :price)
+ (qp.test/aggregate-col :cum-count :venues :id)]
+ :rows [[1 22]
+ [2 81]
+ [3 94]
+ [4 100]]}
+ (qp.test/rows-and-cols
+ (mt/format-rows-by [int int]
+ (mt/run-mbql-query venues
+ {:aggregation [[:cum-count $id]]
+ :breakout [$price]}))))))))
(deftest field-settings-for-aggregate-fields-test
(testing "Does `:settings` show up for aggregate Fields?"
@@ -261,15 +279,15 @@
(or (-> results mt/cols first)
results)))))))
-(deftest duplicate-aggregations-test
+(deftest ^:parallel duplicate-aggregations-test
(mt/test-drivers (mt/normal-drivers)
(testing "Do we properly handle queries that have more than one of the same aggregation? (#5393)"
(is (= [[5050 203]]
(mt/formatted-rows [int int]
(mt/run-mbql-query venues
- {:aggregation [[:sum $id] [:sum $price]]})))))))
+ {:aggregation [[:sum $id] [:sum $price]]})))))))
-(deftest multiple-distinct-aggregations-test
+(deftest ^:parallel multiple-distinct-aggregations-test
(testing "Multiple `:distinct` aggregations should work correctly (#13097)"
(mt/test-drivers (mt/normal-drivers)
(is (= [[100 4]]
diff --git a/test/metabase/query_processor_test/filter_test.clj b/test/metabase/query_processor_test/filter_test.clj
index 9eb1a4fbc15..08fcc24b1c1 100644
--- a/test/metabase/query_processor_test/filter_test.clj
+++ b/test/metabase/query_processor_test/filter_test.clj
@@ -104,9 +104,9 @@
;; test's results
(when (= :snowflake driver/*driver*)
(driver/notify-database-updated driver/*driver* (mt/id)))
- (is (= {:rows [[29]]
- :cols [(qp.test/aggregate-col :count)]}
- (qp.test/rows-and-cols
+ (is (=? {:rows [[29]]
+ :cols [(qp.test/aggregate-col :count)]}
+ (qp.test/rows-and-cols
(mt/format-rows-by [int]
(mt/run-mbql-query checkins
{:aggregation [[:count]]
diff --git a/test/metabase/query_processor_test/nested_queries_test.clj b/test/metabase/query_processor_test/nested_queries_test.clj
index 8a4f9a3520b..faf7b6c109d 100644
--- a/test/metabase/query_processor_test/nested_queries_test.clj
+++ b/test/metabase/query_processor_test/nested_queries_test.clj
@@ -1,10 +1,12 @@
(ns metabase.query-processor-test.nested-queries-test
"Tests for handling queries with nested expressions."
(:require
+ [clojure.string :as str]
[clojure.test :refer :all]
[honey.sql :as sql]
[java-time :as t]
[medley.core :as m]
+ [metabase.db.query :as mdb.query]
[metabase.driver :as driver]
[metabase.driver.sql.query-processor-test-util :as sql.qp-test-util]
[metabase.mbql.schema :as mbql.s]
@@ -401,20 +403,30 @@
(deftest aggregatation-references-test
(testing "make sure that aggregation references match up to aggregations from the same level they're from"
;; e.g. the ORDER BY in the source-query should refer the 'stddev' aggregation, NOT the 'avg' aggregation
- (is (= {:query (str "SELECT AVG(\"source\".\"stddev\") AS \"avg\" FROM ("
- "SELECT \"PUBLIC\".\"VENUES\".\"PRICE\" AS \"PRICE\", STDDEV_POP(\"PUBLIC\".\"VENUES\".\"ID\") AS \"stddev\" "
- "FROM \"PUBLIC\".\"VENUES\" "
- "GROUP BY \"PUBLIC\".\"VENUES\".\"PRICE\" "
- "ORDER BY \"stddev\" DESC, \"PUBLIC\".\"VENUES\".\"PRICE\" ASC"
- ") AS \"source\"")
+ (is (= {:query ["SELECT"
+ " AVG(\"source\".\"stddev\") AS \"avg\""
+ "FROM"
+ " ("
+ " SELECT"
+ " \"PUBLIC\".\"VENUES\".\"PRICE\" AS \"PRICE\","
+ " STDDEV_POP(\"PUBLIC\".\"VENUES\".\"ID\") AS \"stddev\""
+ " FROM"
+ " \"PUBLIC\".\"VENUES\""
+ " GROUP BY"
+ " \"PUBLIC\".\"VENUES\".\"PRICE\""
+ " ORDER BY"
+ " \"stddev\" DESC,"
+ " \"PUBLIC\".\"VENUES\".\"PRICE\" ASC"
+ " ) AS \"source\""]
:params nil}
- (qp/compile
- (mt/mbql-query venues
- {:source-query {:source-table $$venues
- :aggregation [[:stddev $id]]
- :breakout [$price]
- :order-by [[[:aggregation 0] :descending]]}
- :aggregation [[:avg *stddev/Integer]]}))))))
+ (-> (qp/compile
+ (mt/mbql-query venues
+ {:source-query {:source-table $$venues
+ :aggregation [[:stddev $id]]
+ :breakout [$price]
+ :order-by [[[:aggregation 0] :descending]]}
+ :aggregation [[:avg *stddev/Integer]]}))
+ (update :query #(str/split-lines (mdb.query/format-sql % :h2))))))))
(deftest handle-incorrect-field-forms-gracefully-test
(testing "make sure that we handle [:field [:field <name> ...]] forms gracefully, despite that not making any sense"
@@ -549,7 +561,7 @@
;; include the unit; however `:unit` is still `:year` so the frontend can use the correct formatting to
;; display values of the column.
(is (= [(assoc date-col :field_ref [:field (mt/id :checkins :date) {:temporal-unit :default}], :unit :year)
- (assoc count-col :field_ref [:field "count" {:base-type (:base_type count-col)}])]
+ (assoc count-col :field_ref [:field "count" {:base-type :type/Integer}])]
(mt/cols
(qp/process-query (query-with-source-card card)))))))))))
@@ -866,7 +878,7 @@
:base_type :type/Text}
{:name "count"
:display_name "Count"
- :field_ref [:field "count" {:base-type :type/BigInteger}]
+ :field_ref [:field "count" {:base-type :type/Integer}]
:base_type (:base_type (qp.test/aggregate-col :count))}])
(for [col (mt/cols results)]
(select-keys col [:name :display_name :id :field_ref :base_type]))))))))
diff --git a/test/metabase/server/middleware/session_test.clj b/test/metabase/server/middleware/session_test.clj
index a3eceb18b96..d1195ebc0d8 100644
--- a/test/metabase/server/middleware/session_test.clj
+++ b/test/metabase/server/middleware/session_test.clj
@@ -87,7 +87,7 @@
;; if request is an HTTPS request then we should set `:secure true`. There are several different headers we check for
;; this. Make sure they all work.
-(deftest secure-cookie-test
+(deftest ^:parallel secure-cookie-test
(doseq [[headers expected] [[{"x-forwarded-proto" "https"} true]
[{"x-forwarded-proto" "http"} false]
[{"x-forwarded-protocol" "https"} true]
@@ -190,20 +190,20 @@
identity
(fn [e] (throw e))))
-(deftest no-session-id-in-request-test
+(deftest ^:parallel no-session-id-in-request-test
(testing "no session-id in the request"
(is (= nil
(-> (wrapped-handler (ring.mock/request :get "/anyurl"))
:metabase-session-id)))))
-(deftest header-test
+(deftest ^:parallel header-test
(testing "extract session-id from header"
(is (= "foobar"
(:metabase-session-id
(wrapped-handler
(ring.mock/header (ring.mock/request :get "/anyurl") session-header "foobar")))))))
-(deftest cookie-test
+(deftest ^:parallel cookie-test
(testing "extract session-id from cookie"
(is (= "cookie-session"
(:metabase-session-id
@@ -211,7 +211,7 @@
(assoc (ring.mock/request :get "/anyurl")
:cookies {session-cookie {:value "cookie-session"}})))))))
-(deftest both-header-and-cookie-test
+(deftest ^:parallel both-header-and-cookie-test
(testing "if both header and cookie session-ids exist, then we expect the cookie to take precedence"
(is (= "cookie-session"
(:metabase-session-id
@@ -219,7 +219,7 @@
(assoc (ring.mock/header (ring.mock/request :get "/anyurl") session-header "foobar")
:cookies {session-cookie {:value "cookie-session"}})))))))
-(deftest anti-csrf-headers-test
+(deftest ^:parallel anti-csrf-headers-test
(testing "`wrap-session-id` should handle anti-csrf headers they way we'd expect"
(let [request (-> (ring.mock/request :get "/anyurl")
(assoc :cookies {embedded-session-cookie {:value (str test-uuid)}})
@@ -347,7 +347,7 @@
(-> (ring.mock/request :get "/anyurl")
(assoc :metabase-user-id user-id)))
-(deftest add-user-id-key-test
+(deftest ^:parallel add-user-id-key-test
(testing "with valid user-id"
(is (= {:user-id (mt/user->id :rasta)
:user {:id (mt/user->id :rasta)
diff --git a/test/metabase/test/data.clj b/test/metabase/test/data.clj
index ae2d7c80daf..cd79fc18fbe 100644
--- a/test/metabase/test/data.clj
+++ b/test/metabase/test/data.clj
@@ -112,7 +112,7 @@
* Only symbols that end in alphanumeric characters will be parsed, so as to avoid accidentally parsing things that
do not refer to Fields."
- {:style/indent [:defn 1]}
+ {:style/indent :defn}
([form]
`($ids nil ~form))
diff --git a/test/metabase/test/data/h2.clj b/test/metabase/test/data/h2.clj
index 34bc053b3b7..2f66cb801d5 100644
--- a/test/metabase/test/data/h2.clj
+++ b/test/metabase/test/data/h2.clj
@@ -4,6 +4,7 @@
[metabase.db :as mdb]
[metabase.db.spec :as mdb.spec]
[metabase.driver.ddl.interface :as ddl.i]
+ [metabase.driver.h2]
[metabase.driver.sql.util :as sql.u]
[metabase.models.database :refer [Database]]
[metabase.test.data.impl :as data.impl]
@@ -17,6 +18,8 @@
[metabase.util :as u]
[toucan2.core :as t2]))
+(comment metabase.driver.h2/keep-me)
+
(sql-jdbc.tx/add-test-extensions! :h2)
(defonce ^:private h2-test-dbs-created-by-this-instance (atom #{}))
@@ -95,12 +98,15 @@
(defmethod tx/aggregate-column-info :h2
([driver ag-type]
- ((get-method tx/aggregate-column-info ::tx/test-extensions) driver ag-type))
+ (merge
+ ((get-method tx/aggregate-column-info ::tx/test-extensions) driver ag-type)
+ (when (= ag-type :count)
+ {:base_type :type/BigInteger})))
([driver ag-type field]
(merge
((get-method tx/aggregate-column-info ::tx/test-extensions) driver ag-type field)
- (when (= ag-type :sum)
+ (when (#{:sum :cum-count} ag-type)
{:base_type :type/BigInteger}))))
(defmethod execute/execute-sql! :h2
diff --git a/test/metabase/test/data/interface.clj b/test/metabase/test/data/interface.clj
index 5fa109cd5cf..7411f7ccdd0 100644
--- a/test/metabase/test/data/interface.clj
+++ b/test/metabase/test/data/interface.clj
@@ -375,8 +375,7 @@
(defmethod aggregate-column-info ::test-extensions
([_ aggregation-type]
- ;; TODO - Can `:cum-count` be used without args as well ??
- (assert (= aggregation-type :count))
+ (assert (#{:count :cum-count} aggregation-type))
{:base_type :type/BigInteger
:semantic_type :type/Quantity
:name "count"
@@ -386,10 +385,14 @@
([_driver aggregation-type {field-id :id, table-id :table_id}]
{:pre [(some? table-id)]}
- (first (qp/query->expected-cols {:database (t2/select-one-fn :db_id Table :id table-id)
- :type :query
- :query {:source-table table-id
- :aggregation [[aggregation-type [:field-id field-id]]]}}))))
+ (merge
+ (first (qp/query->expected-cols {:database (t2/select-one-fn :db_id Table :id table-id)
+ :type :query
+ :query {:source-table table-id
+ :aggregation [[aggregation-type [:field-id field-id]]]}}))
+ (when (= aggregation-type :cum-count)
+ {:base_type :type/BigInteger
+ :semantic_type :type/Quantity}))))
(defmulti count-with-template-tag-query
diff --git a/test/metabase/test/data/sql.clj b/test/metabase/test/data/sql.clj
index c30e602b73b..514b5700a18 100644
--- a/test/metabase/test/data/sql.clj
+++ b/test/metabase/test/data/sql.clj
@@ -4,12 +4,15 @@
[clojure.string :as str]
[metabase.driver :as driver]
[metabase.driver.ddl.interface :as ddl.i]
+ [metabase.driver.sql]
[metabase.driver.sql.util :as sql.u]
[metabase.query-processor :as qp]
[metabase.test.data :as data]
[metabase.test.data.interface :as tx]
[metabase.util.log :as log]))
+(comment metabase.driver.sql/keep-me)
+
(driver/register! :sql/test-extensions, :abstract? true)
(tx/add-test-extensions! :sql/test-extensions)
--
GitLab