diff --git a/modules/drivers/bigquery/src/metabase/driver/bigquery.clj b/modules/drivers/bigquery/src/metabase/driver/bigquery.clj
index 0759532eb80c97b9195fef06b3b16bbd36690537..914173636f0ce5a10de37c576ef923de284bb366 100644
--- a/modules/drivers/bigquery/src/metabase/driver/bigquery.clj
+++ b/modules/drivers/bigquery/src/metabase/driver/bigquery.clj
@@ -17,14 +17,11 @@
[google :as google]]
[metabase.driver.sql.query-processor :as sql.qp]
[metabase.driver.sql.util.unprepare :as unprepare]
- [metabase.mbql
- [schema :as mbql.s]
- [util :as mbql.u]]
+ [metabase.mbql.util :as mbql.u]
[metabase.models.table :as table]
[metabase.query-processor
[store :as qp.store]
[util :as qputil]]
- [metabase.query-processor.middleware.annotate :as annotate]
[metabase.util
[date :as du]
[honeysql-extensions :as hx]
@@ -349,21 +346,6 @@
(str/replace #"(^\d)" "_$1"))]
(subs replaced-str 0 (min 128 (count replaced-str)))))
-(s/defn ^:private bq-aggregate-name :- su/NonBlankString
- "Return an approriate name for an `ag-clause`."
- [driver, ag-clause :- mbql.s/Aggregation]
- (->> ag-clause annotate/aggregation-name (driver/format-custom-field-name driver)))
-
-(s/defn ^:private pre-alias-aggregations
- "Expressions are not allowed in the order by clauses of a BQ query. To sort by a custom expression, that custom
- expression must be aliased from the order by. This code will find the aggregations and give them a name if they
- don't already have one. This name can then be used in the order by if one is present."
- [driver {{aggregations :aggregation} :query, :as outer-query}]
- (if-not (seq aggregations)
- outer-query
- (update-in outer-query [:query :aggregation] (partial mbql.u/pre-alias-and-uniquify-aggregations
- (partial bq-aggregate-name driver)))))
-
;; These provide implementations of `->honeysql` that prevent HoneySQL from converting forms to prepared statement
;; parameters (`?` symbols)
(defmethod sql.qp/->honeysql [:bigquery String]
@@ -433,11 +415,10 @@
{source-table-id :source-table, source-query :source-query} :query
:as outer-query}]
(let [dataset-id (-> (qp.store/database) :details :dataset-id)
- aliased-query (pre-alias-aggregations driver outer-query)
{table-name :name} (some-> source-table-id qp.store/table)]
(assert (seq dataset-id))
- (binding [sql.qp/*query* (assoc aliased-query :dataset-id dataset-id)]
- {:query (->> aliased-query
+ (binding [sql.qp/*query* (assoc outer-query :dataset-id dataset-id)]
+ {:query (->> outer-query
(sql.qp/build-honeysql-form :bigquery)
honeysql-form->sql)
:table-name (or table-name
diff --git a/modules/drivers/bigquery/test/metabase/driver/bigquery_test.clj b/modules/drivers/bigquery/test/metabase/driver/bigquery_test.clj
index e1af0169d3b2c2d4662ec552771b145f8c9bef5d..272e5dbbf058a29ea62c282c77d8fe9b29b67543 100644
--- a/modules/drivers/bigquery/test/metabase/driver/bigquery_test.clj
+++ b/modules/drivers/bigquery/test/metabase/driver/bigquery_test.clj
@@ -1,6 +1,5 @@
(ns metabase.driver.bigquery-test
(:require [clj-time.core :as time]
- [expectations :refer [expect]]
[honeysql.core :as hsql]
[metabase
[driver :as driver]
@@ -95,47 +94,9 @@
(mbql.u/match (-> query :query :aggregation)
[:named _ ag-name] ag-name))
-(defn- pre-alias-aggregations [outer-query]
- (binding [driver/*driver* :bigquery]
- (aggregation-names (#'bigquery/pre-alias-aggregations :bigquery outer-query))))
-
-(defn- query-with-aggregations
- [aggregations]
- {:database (data/id)
- :type :query
- :query {:source-table (data/id :venues)
- :aggregation aggregations}})
-
-;; make sure BigQuery can handle two aggregations with the same name (#4089)
-(expect
- ["sum" "count" "sum_2" "avg" "sum_3" "min"]
- (pre-alias-aggregations
- (query-with-aggregations
- [[:sum [:field-id (data/id :venues :id)]]
- [:count [:field-id (data/id :venues :id)]]
- [:sum [:field-id (data/id :venues :id)]]
- [:avg [:field-id (data/id :venues :id)]]
- [:sum [:field-id (data/id :venues :id)]]
- [:min [:field-id (data/id :venues :id)]]])))
-
-(expect
- ["sum" "count" "sum_2" "avg" "sum_2_2" "min"]
- (pre-alias-aggregations
- (query-with-aggregations
- [[:sum [:field-id (data/id :venues :id)]]
- [:count [:field-id (data/id :venues :id)]]
- [:sum [:field-id (data/id :venues :id)]]
- [:avg [:field-id (data/id :venues :id)]]
- [:named [:sum [:field-id (data/id :venues :id)]] "sum_2"]
- [:min [:field-id (data/id :venues :id)]]])))
-
-;; if query has no aggregations then pre-alias-aggregations should do nothing
-(expect
- {}
- (driver/with-driver :bigquery
- (#'bigquery/pre-alias-aggregations :bigquery {})))
-
-
+;; make sure queries with two or more of the same aggregation type still work. Aggregations used to be deduplicated
+;; here in the BigQuery driver; now they are deduplicated as part of the main QP middleware, but no reason not to keep
+;; a few of these tests just to be safe
(datasets/expect-with-driver :bigquery
{:rows [[7929 7929]], :columns ["sum" "sum_2"]}
(qp.test/rows+column-names
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 0a4b33c9e71a79571511d6e95fbc4fa831126330..b05fecd24d25b4b63aab74ac2f32d62188503264 100644
--- a/modules/drivers/druid/src/metabase/driver/druid/query_processor.clj
+++ b/modules/drivers/druid/src/metabase/driver/druid/query_processor.clj
@@ -73,21 +73,26 @@
[this]
this)
-(defmethod ->rvalue :aggregation
- [[_ index]]
- (let [[ag-type :as ag] (nth (:aggregation *query*) index)]
- (cond
- (= [:count] ag)
- :count
+(defn- ag-clause->rvalue [[ag-type :as ag]]
+ (cond
+ (= [:count] ag)
+ :count
- (= ag-type :distinct)
- :distinct___count
+ (= ag-type :distinct)
+ :distinct___count
- ag-type
- ag-type
+ (= ag-type :named)
+ (recur (second ag))
- :else
- (throw (Exception. "Unknown aggregation type!")))))
+ ag-type
+ ag-type
+
+ :else
+ (throw (Exception. "Unknown aggregation type!"))))
+
+(defmethod ->rvalue :aggregation
+ [[_ index]]
+ (ag-clause->rvalue (nth (:aggregation *query*) index)))
(defmethod ->rvalue :field-id
[[_ field-id]]
@@ -581,8 +586,8 @@
[query-type ag-clause updated-query]
(let [output-name (annotate/aggregation-name ag-clause)
[ag-type ag-field & args] (mbql.u/match-one ag-clause
- [:named ag _] (recur ag)
- [_ _ & _] &match)]
+ [:named ag & _] (recur ag)
+ [_ _ & _] &match)]
(if-not (isa? query-type ::ag-query)
updated-query
(let [[projections ag-clauses] (create-aggregation-clause output-name ag-type ag-field args)]
@@ -593,7 +598,7 @@
(defn- add-expression-aggregation-output-names
[[operator & args :as expression]]
(if (mbql.u/is-clause? :named expression)
- [:named (add-expression-aggregation-output-names (second expression)) (last expression)]
+ (update (vec expression) 1 add-expression-aggregation-output-names)
(into [operator]
(for [arg args]
(cond
@@ -661,21 +666,20 @@
(defn- handle-aggregations
[query-type {aggregations :aggregation} updated-query]
- (let [aggregations (mbql.u/pre-alias-and-uniquify-aggregations annotate/aggregation-name aggregations)]
- (loop [[ag & more] aggregations, query updated-query]
- (cond
- (and (mbql.u/is-clause? :named ag)
- (mbql.u/is-clause? #{:+ :- :/ :*} (second ag)))
- (handle-expression-aggregation query-type ag query)
+ (loop [[ag & more] aggregations, query updated-query]
+ (cond
+ (and (mbql.u/is-clause? :named ag)
+ (mbql.u/is-clause? #{:+ :- :/ :*} (second ag)))
+ (handle-expression-aggregation query-type ag query)
- (mbql.u/is-clause? #{:+ :- :/ :*} ag)
- (handle-expression-aggregation query-type ag query)
+ (mbql.u/is-clause? #{:+ :- :/ :*} ag)
+ (handle-expression-aggregation query-type ag query)
- (not ag)
- query
+ (not ag)
+ query
- :else
- (recur more (handle-aggregation query-type ag query))))))
+ :else
+ (recur more (handle-aggregation query-type ag query)))))
;;; ------------------------------------------------ handle-breakout -------------------------------------------------
@@ -859,11 +863,21 @@
(defmethod handle-order-by ::topN
- [_ {[[ag-type]] :aggregation, [breakout-field] :breakout, [[direction field]] :order-by} updated-query]
+ [_ {[ag] :aggregation, [breakout-field] :breakout, [[direction field]] :order-by} updated-query]
(let [field (->rvalue field)
breakout-field (->rvalue breakout-field)
sort-by-breakout? (= field breakout-field)
- ag-field (if (= ag-type :distinct) :distinct___count ag-type)]
+ ag-field (mbql.u/match-one ag
+ :distinct
+ :distinct___count
+
+ [:named wrapped-ag & _]
+ (recur wrapped-ag)
+
+ [(ag-type :guard keyword?) & _]
+ ag-type)]
+ (when-not sort-by-breakout?
+ (assert ag-field))
(assoc-in updated-query [:query :metric] (match [sort-by-breakout? direction]
[true :asc] {:type :alphaNumeric}
[true :desc] {:type :inverted, :metric {:type :alphaNumeric}}
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 3e78b846ed85ee58e408712cc7f7faba8b2a8c67..82e4271ebab71bc8cce26bc322826ad50fd5e174 100644
--- a/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj
+++ b/modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj
@@ -567,18 +567,16 @@
(s/defn ^:private generate-aggregation-pipeline :- {:projections Projections, :query Pipeline}
"Generate the aggregation pipeline. Returns a sequence of maps representing each stage."
[inner-query :- mbql.s/MBQLQuery]
- (let [inner-query (update inner-query :aggregation (partial mbql.u/pre-alias-and-uniquify-aggregations
- annotate/aggregation-name))]
- (reduce (fn [pipeline-ctx f]
- (f inner-query pipeline-ctx))
- {:projections [], :query []}
- [add-initial-projection
- handle-filter
- handle-breakout+aggregation
- handle-order-by
- handle-fields
- handle-limit
- handle-page])))
+ (reduce (fn [pipeline-ctx f]
+ (f inner-query pipeline-ctx))
+ {:projections [], :query []}
+ [add-initial-projection
+ handle-filter
+ handle-breakout+aggregation
+ handle-order-by
+ handle-fields
+ handle-limit
+ handle-page]))
(s/defn ^:private create-unescaping-rename-map :- {s/Keyword s/Keyword}
[original-keys :- Projections]
diff --git a/modules/drivers/oracle/test/metabase/test/data/oracle.clj b/modules/drivers/oracle/test/metabase/test/data/oracle.clj
index a05b9721195c4e5c9761c2479aab526da27c73ea..8254984b0d85379b356acf26153381ba867ec33e 100644
--- a/modules/drivers/oracle/test/metabase/test/data/oracle.clj
+++ b/modules/drivers/oracle/test/metabase/test/data/oracle.clj
@@ -69,12 +69,6 @@
session-schema
(tx/db-qualified-table-name database-name table-name)))
-(defmethod tx/expected-base-type->actual :oracle [_ base-type]
- ;; Oracle doesn't have INTEGERs
- (if (isa? base-type :type/Integer)
- :type/Decimal
- base-type))
-
(defmethod sql.tx/create-db-sql :oracle [& _] nil)
(defmethod sql.tx/drop-db-if-exists-sql :oracle [& _] nil)
diff --git a/modules/drivers/snowflake/test/metabase/test/data/snowflake.clj b/modules/drivers/snowflake/test/metabase/test/data/snowflake.clj
index e9164c459c6e9fb677ffb210d67c7895d5ff6de8..50e2edee9c93723b5daacde2068b2488ca9ea119 100644
--- a/modules/drivers/snowflake/test/metabase/test/data/snowflake.clj
+++ b/modules/drivers/snowflake/test/metabase/test/data/snowflake.clj
@@ -54,11 +54,6 @@
(let [db (sql.tx/qualify-and-quote driver database-name)]
(format "DROP DATABASE IF EXISTS %s; CREATE DATABASE %s;" db db)))
-(defmethod tx/expected-base-type->actual :snowflake [_ base-type]
- (if (isa? base-type :type/Integer)
- :type/Number
- base-type))
-
(defn- no-db-connection-spec
"Connection spec for connecting to our Snowflake instance without specifying a DB."
[]
diff --git a/project.clj b/project.clj
index 51a3b3078681fad3883ed3c6ef7121baa3cdafcc..b2ffe7e60f23fef4f23620e6862c7fac94a7a884 100644
--- a/project.clj
+++ b/project.clj
@@ -99,7 +99,7 @@
com.sun.jdmk/jmxtools
com.sun.jmx/jmxri]]
[medley "1.2.0"] ; lightweight lib of useful functions
- [metabase/mbql "1.0.3"] ; MBQL language schema & util fns
+ [metabase/mbql "1.2.0"] ; MBQL language schema & util fns
[metabase/throttle "1.0.1"] ; Tools for throttling access to API endpoints and other code pathways
[javax.xml.bind/jaxb-api "2.4.0-b180830.0359"] ; add the `javax.xml.bind` classes which we're still using but were removed in Java 11
[net.sf.cssbox/cssbox "4.12" :exclusions [org.slf4j/slf4j-api]] ; HTML / CSS rendering
diff --git a/src/metabase/driver/sql/query_processor.clj b/src/metabase/driver/sql/query_processor.clj
index ebf1b872ad0d706b745f5a5b6bfb53aed01deed6..a199357e753d96d7eba4362f4712cc7571cd1020 100644
--- a/src/metabase/driver/sql/query_processor.clj
+++ b/src/metabase/driver/sql/query_processor.clj
@@ -272,7 +272,7 @@
(defmethod ->honeysql [:sql :aggregation]
[driver [_ index]]
(mbql.u/match-one (mbql.u/aggregation-at-index *query* index *nested-query-level*)
- [:named _ ag-name]
+ [:named _ ag-name & _]
(->honeysql driver (hx/identifier :field-alias ag-name))
;; For some arcane reason we name the results of a distinct aggregation "count", everything else is named the
diff --git a/src/metabase/plugins/classloader.clj b/src/metabase/plugins/classloader.clj
index e89fda818fcba7c766c6024ec64755ce04890f01..f59e35260618e9b292efb01615174f4344c811d9 100644
--- a/src/metabase/plugins/classloader.clj
+++ b/src/metabase/plugins/classloader.clj
@@ -119,9 +119,10 @@
ultimately have access to that URL."
(^DynamicClassLoader []
(the-top-level-classloader (the-classloader)))
+
(^DynamicClassLoader [^DynamicClassLoader classloader]
(some #(when (instance? DynamicClassLoader %) %)
- (classloader-hierarchy (.getContextClassLoader (Thread/currentThread))))))
+ (classloader-hierarchy classloader))))
(defonce ^:private already-added (atom #{}))
diff --git a/src/metabase/query_processor.clj b/src/metabase/query_processor.clj
index f910d999c38fbc5e02ed029c94c8e03aa494c695..9e3b282ec55f58568108304f7b2fe455cef39692 100644
--- a/src/metabase/query_processor.clj
+++ b/src/metabase/query_processor.clj
@@ -38,6 +38,7 @@
[normalize-query :as normalize]
[parameters :as parameters]
[permissions :as perms]
+ [pre-alias-aggregations :as pre-alias-ags]
[process-userland-query :as process-userland-query]
[reconcile-breakout-and-order-by-bucketing :as reconcile-bucketing]
[resolve-database :as resolve-database]
@@ -107,6 +108,7 @@
#'wrap-value-literals/wrap-value-literals
#'annotate/add-column-info
#'perms/check-query-permissions
+ #'pre-alias-ags/pre-alias-aggregations
#'cumulative-ags/handle-cumulative-aggregations
;; ▲▲▲ NO FK->s POINT ▲▲▲ Everything after this point will not see `:fk->` clauses, only `:joined-field`
#'resolve-joins/resolve-joins
diff --git a/src/metabase/query_processor/middleware/add_source_metadata.clj b/src/metabase/query_processor/middleware/add_source_metadata.clj
index 65ef49bb03718a5f14cf4acf516876ea91e4dc59..60fd485d3f4c47eaf5b6616f9818d3baf166ac88 100644
--- a/src/metabase/query_processor/middleware/add_source_metadata.clj
+++ b/src/metabase/query_processor/middleware/add_source_metadata.clj
@@ -9,7 +9,8 @@
[store :as qp.store]]
[metabase.query-processor.middleware
[add-implicit-clauses :as add-implicit-clauses]
- [annotate :as annotate]]
+ [annotate :as annotate]
+ [pre-alias-aggregations :as pre-alias-ags]]
[metabase.util.i18n :refer [trs]]
[schema.core :as s]))
@@ -21,7 +22,7 @@
(let [field-ids (mbql.u/match (concat breakouts aggregations fields) [:field-id id] id)]
(qp.store/fetch-and-store-fields! field-ids))
(for [col (annotate/cols-for-mbql-query source-query)]
- (select-keys col [:name :id :table_id :display_name :base_type :special_type :unit :fingerprint])))
+ (select-keys col [:name :id :table_id :display_name :base_type :special_type :unit :fingerprint :settings])))
(defn- has-same-fields-as-nested-source?
"Whether this source query itself has a nested source query, and will have the exact same fields in the results as its
@@ -78,11 +79,22 @@
{:source-query source-query}))
nil)))
+(defn- partially-preprocess-source-query
+ "Unfortunately there are a few middleware transformations that would normally come later but need to be done here for
+ nested source queries before proceeding. This middleware applies those transformations.
+
+ TODO - in a nicer world there would be no need to do such things. It defeats the purpose of having distinct
+ middleware stages."
+ [source-query]
+ (-> source-query
+ pre-alias-ags/pre-alias-aggregations-in-inner-query
+ add-implicit-clauses/add-implicit-mbql-clauses))
+
(s/defn ^:private add-source-metadata :- {:source-metadata [mbql.s/SourceQueryMetadata], s/Keyword s/Any}
[{{native-source-query? :native, :as source-query} :source-query, :as inner-query}]
(let [source-query (if native-source-query?
source-query
- (add-implicit-clauses/add-implicit-mbql-clauses source-query))
+ (partially-preprocess-source-query source-query))
metadata (source-query->metadata source-query)]
(assoc inner-query :source-metadata metadata)))
diff --git a/src/metabase/query_processor/middleware/annotate.clj b/src/metabase/query_processor/middleware/annotate.clj
index a803cefa873c230e9883114aacba65a40bae9a84..161f2c033ebba73e1c8dd7a12ae2917c45888df9 100644
--- a/src/metabase/query_processor/middleware/annotate.clj
+++ b/src/metabase/query_processor/middleware/annotate.clj
@@ -107,7 +107,7 @@
(format "%s → %s" join-display-name field-display-name)))
(s/defn ^:private col-info-for-field-clause :- su/Map
- [inner-query :- su/Map, clause :- mbql.s/Field]
+ [{:keys [source-metadata], :as inner-query} :- su/Map, clause :- mbql.s/Field]
;; for various things that can wrap Field clauses recurse on the wrapped Field but include a little bit of info
;; about the clause doing the wrapping
(mbql.u/match-one clause
@@ -134,10 +134,13 @@
[:fk-> _ field]
(recur field)
+ ;; for field literals, look for matching `source-metadata`, and use that if we can find it; otherwise generate
+ ;; basic info based on the content of the field literal
[:field-literal field-name field-type]
- {:name field-name
- :base_type field-type
- :display_name (humanization/name->human-readable-name field-name)}
+ (or (some #(when (= (:name %) field-name) %) source-metadata)
+ {:name field-name
+ :base_type field-type
+ :display_name (humanization/name->human-readable-name field-name)})
[:expression expression-name]
{:name expression-name
@@ -198,7 +201,7 @@
(mbql.u/match-one ag-clause
;; if a custom name was provided use it. Some drivers have limits on column names, so call
;; `format-custom-field-name` so they can modify it as needed.
- [:named _ ag-name]
+ [:named _ ag-name & _]
(driver/format-custom-field-name driver/*driver* ag-name)
;; For unnamed expressions, just compute a name like "sum + count"
@@ -240,8 +243,13 @@
"Return an appropriate user-facing display name for an aggregation clause."
[ag-clause]
(mbql.u/match-one ag-clause
- ;; if a custom name was provided use it
- [:named _ ag-name]
+ ;; if the `named` clause has options, and *explicity* specifies {:use-as-display-name? false}, generate a name based
+ ;; on the wrapped clause instead
+ [:named wrapped-clause _ (_ :guard #(false? (:use-as-display-name? %)))]
+ (aggregation-display-name wrapped-clause)
+
+ ;; otherwise for a `named` aggregation use the supplied name as the display name
+ [:named _ ag-name & _]
ag-name
[(operator :guard #{:+ :- :/ :*}) & args]
@@ -284,7 +292,7 @@
[inner-query :- su/Map, clause]
(mbql.u/match-one clause
;; ok, if this is a named aggregation recurse so we can get information about the ag we are naming
- [:named ag _]
+ [:named ag & _]
(merge
(col-info-for-aggregation-clause inner-query ag)
(ag->name-info &match))
@@ -394,9 +402,9 @@
(maybe-merge-source-metadata source-metadata (column-info {:type :native} results))
(mbql-cols source-query results)))
-(defn ^:private mbql-cols
+(s/defn ^:private mbql-cols
"Return the `:cols` result metadata for an 'inner' MBQL query based on the fields/breakouts/aggregations in the query."
- [{:keys [source-metadata source-query fields], :as inner-query} results]
+ [{:keys [source-metadata source-query fields], :as inner-query} :- su/Map, results]
(let [cols (cols-for-mbql-query inner-query)]
(cond
(and (empty? cols) source-query)
@@ -466,14 +474,11 @@
;;; | result-rows-maps-to-vectors middleware |
;;; +----------------------------------------------------------------------------------------------------------------+
-(defn- uniquify-aggregations [inner-query]
- (update inner-query :aggregation (partial mbql.u/pre-alias-and-uniquify-aggregations aggregation-name)))
-
(s/defn ^:private expected-column-sort-order :- [s/Keyword]
"Determine query result row column names (as keywords) sorted in the appropriate order based on a `query`."
- [{query-type :type, inner-query :query, :as query} {[first-row] :rows, :as results}]
+ [{query-type :type, inner-query :query, :as query} :- su/Map, {[first-row] :rows, :as results}]
(if (= query-type :query)
- (map (comp keyword :name) (mbql-cols (uniquify-aggregations inner-query) results))
+ (map (comp keyword :name) (mbql-cols inner-query results))
(map keyword (keys first-row))))
(defn- result-rows-maps->vectors* [query {[first-row :as rows] :rows, columns :columns, :as results}]
@@ -512,11 +517,6 @@
that drivers that use functionality provided by this middleware return deduplicated column names, e.g. `:sum` and
`:sum_2` for queries with multiple `:sum` aggregations.
- Call `mbql.u/pre-alias-and-uniquify-aggregations` on your query before processing it tp add appropriate aliases to
- aggregations. Currently this assumes you are passing `annotate/aggregation-name` as the function to generate
- aggregation names; if your driver is doing something drastically different, you may need to tweak the keys in the
- result row maps so they match up with the keys generated by that function.
-
* For *nested* Fields, this namespace assumes result row maps will come back flattened, and Field name keys will
come back qualified by names of their ancestors, e.g. `parent.child`, `grandparent.parent.child`, etc. This is done
to remove any ambiguity between nested columns with the same name (e.g. a document with both `user.id` and
diff --git a/src/metabase/query_processor/middleware/pre_alias_aggregations.clj b/src/metabase/query_processor/middleware/pre_alias_aggregations.clj
new file mode 100644
index 0000000000000000000000000000000000000000..aebf60f76206be06d8cd89b2ab918c3e58c23eae
--- /dev/null
+++ b/src/metabase/query_processor/middleware/pre_alias_aggregations.clj
@@ -0,0 +1,39 @@
+(ns metabase.query-processor.middleware.pre-alias-aggregations
+ (:require [metabase.driver :as driver]
+ [metabase.mbql.util :as mbql.u]
+ [metabase.query-processor.middleware.annotate :as annotate]))
+
+(defn- ag-name [ag-clause]
+ (driver/format-custom-field-name driver/*driver* (annotate/aggregation-name ag-clause)))
+
+(defn- pre-alias-and-uniquify [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)))
+
+(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)
+
+ source-query
+ (update :source-query pre-alias-aggregations-in-inner-query)
+
+ joins
+ (update :joins (partial mapv pre-alias-aggregations-in-inner-query))))
+
+(defn- maybe-pre-alias-aggregations [{query-type :type, :as query}]
+ (if-not (= query-type :query)
+ query
+ (update query :query pre-alias-aggregations-in-inner-query)))
+
+(defn pre-alias-aggregations
+ "Middleware that generates aliases for all aggregations anywhere in a query, and makes sure they're unique."
+ [qp]
+ (comp qp maybe-pre-alias-aggregations))
diff --git a/src/metabase/query_processor/middleware/results_metadata.clj b/src/metabase/query_processor/middleware/results_metadata.clj
index ad99f653a12f2325c48ba9ef95df7185cb770144..492dfdb748e7c943f173bf1f0d17bfc25f0b9dba 100644
--- a/src/metabase/query_processor/middleware/results_metadata.clj
+++ b/src/metabase/query_processor/middleware/results_metadata.clj
@@ -52,7 +52,7 @@
json/generate-string))
(defn- metadata-checksum
- "Simple, checksum of the column results METADATA.
+ "Simple, checksum of the column results `metadata`.
Results metadata is returned as part of all query results, with the hope that the frontend will pass it back to
us when a Card is saved or updated. This checksum (also passed) is a simple way for us to check whether the metadata
is valid and hasn't been accidentally tampered with.
@@ -72,7 +72,7 @@
codec/base64-encode)))
(defn valid-checksum?
- "Is the CHECKSUM the right one for this column METADATA?"
+ "Is the `checksum` the right one for this column `metadata`?"
[metadata checksum]
(and metadata
checksum
diff --git a/test/expectation_options.clj b/test/expectation_options.clj
index 0525a642dae642742125979f68a217767675662a..238966175f4f056a70c94a9964fa65f92cd4691d 100644
--- a/test/expectation_options.clj
+++ b/test/expectation_options.clj
@@ -191,7 +191,7 @@
;; Uncomment `check-test-data-unchanged` when you want to debug situations where tests are being bad citizens
;; and leaving the metadata about the test data in a different state than it started out with. This is not
;; enabled by default because it adds ~5ms to each test which adds up when we have ~4000 tests (as of May 2019)
- check-test-data-unchanged
+ #_check-test-data-unchanged
log-slow-tests
enforce-timeout
check-table-cleanup))
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 35a771e8fd9ef9cfd3b090a99a8273f8928f1168..3f44f68956bc3739b7cab27e1efd03d7bbb1b88a 100644
--- a/test/metabase/query_processor/middleware/add_source_metadata_test.clj
+++ b/test/metabase/query_processor/middleware/add_source_metadata_test.clj
@@ -19,7 +19,7 @@
(defn- venues-metadata [field-name]
(select-keys
(Field (data/id :venues field-name))
- [:id :table_id :name :display_name :base_type :special_type :unit :fingerprint]))
+ [:id :table_id :name :display_name :base_type :special_type :unit :fingerprint :settings]))
(defn- venues-source-metadata
([]
@@ -85,7 +85,8 @@
[{:name "avg"
:display_name "average of ID"
:base_type :type/BigInteger
- :special_type :type/PK}])})
+ :special_type :type/PK
+ :settings nil}])})
(add-source-metadata
(data/mbql-query venues
{:source-query {:source-table $$venues
@@ -103,7 +104,8 @@
[{:name "my_cool_aggregation"
:display_name "my_cool_aggregation"
:base_type :type/BigInteger
- :special_type :type/PK}])})
+ :special_type :type/PK
+ :settings nil}])})
(add-source-metadata
(data/mbql-query venues
{:source-query {:source-table $$venues
diff --git a/test/metabase/query_processor/middleware/annotate_test.clj b/test/metabase/query_processor/middleware/annotate_test.clj
index 82cae575a5508229d52842380651ade6f6494b43..9f95ddcac90e83cc1af594409ffc4edb63450a67 100644
--- a/test/metabase/query_processor/middleware/annotate_test.clj
+++ b/test/metabase/query_processor/middleware/annotate_test.clj
@@ -47,7 +47,7 @@
;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | add-mbql-column-info |
+;;; | (MBQL) Col info for Field clauses |
;;; +----------------------------------------------------------------------------------------------------------------+
(defn- info-for-field
@@ -192,6 +192,58 @@
:max-value 100}]]}}
{:columns [:price]})))
+;; For fields with parents we should return them with a combined name including parent's name
+(tt/expect-with-temp [Field [parent {:name "parent", :table_id (data/id :venues)}]
+ Field [child {:name "child", :table_id (data/id :venues), :parent_id (u/get-id parent)}]]
+ {:description nil
+ :table_id (data/id :venues)
+ :special_type nil
+ :name "parent.child"
+ :settings nil
+ :parent_id (u/get-id parent)
+ :id (u/get-id child)
+ :visibility_type :normal
+ :display_name "Child"
+ :fingerprint nil
+ :base_type :type/Text}
+ (qp.test-util/with-everything-store
+ (#'annotate/col-info-for-field-clause {} [:field-id (u/get-id child)])))
+
+;; nested-nested fields should include grandparent name (etc)
+(tt/expect-with-temp [Field [grandparent {:name "grandparent", :table_id (data/id :venues)}]
+ Field [parent {:name "parent", :table_id (data/id :venues), :parent_id (u/get-id grandparent)}]
+ Field [child {:name "child", :table_id (data/id :venues), :parent_id (u/get-id parent)}]]
+ {:description nil
+ :table_id (data/id :venues)
+ :special_type nil
+ :name "grandparent.parent.child"
+ :settings nil
+ :parent_id (u/get-id parent)
+ :id (u/get-id child)
+ :visibility_type :normal
+ :display_name "Child"
+ :fingerprint nil
+ :base_type :type/Text}
+ (qp.test-util/with-everything-store
+ (#'annotate/col-info-for-field-clause {} [:field-id (u/get-id child)])))
+
+;; datetime literals should get the information from the matching `:source-metadata` if it was supplied
+(expect
+ {:name "sum"
+ :display_name "sum of User ID"
+ :base_type :type/Integer
+ :special_type :type/FK}
+ (qp.test-util/with-everything-store
+ (#'annotate/col-info-for-field-clause
+ {:source-metadata [{:name "abc", :display_name "another Field", :base_type :type/Integer, :special_type :type/FK}
+ {:name "sum", :display_name "sum of User ID", :base_type :type/Integer, :special_type :type/FK}]}
+ [:field-literal "sum" :type/Integer])))
+
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | (MBQL) Col info for Aggregation clauses |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
;; test that added information about aggregations looks the way we'd expect
(defn- aggregation-names [ag-clause]
(binding [driver/*driver* :h2]
@@ -269,6 +321,32 @@
(data/$ids venues
(col-info-for-aggregation-clause [:sum [:+ $price 1]]))))
+;; if a `:named` aggregation supplies optional `:use-as-display-name?` `options` we should respect that
+;; `use-as-disply-name?` is `true` by default, e.g. in cases where the user supplies the names themselves
+(expect
+ {:base_type :type/Integer
+ :special_type :type/Category
+ :settings nil
+ :name "sum"
+ :display_name "sum"}
+ (qp.test-util/with-everything-store
+ (data/$ids venues
+ (col-info-for-aggregation-clause [:named [:sum $price] "sum" {:use-as-display-name? true}]))))
+
+;; `use-as-display-name?` will normally be `false` when the `:named` clause is generated automatically, e.g. by the
+;; `pre-alias-aggregations` middleware. In this case we want to use the name internally in the query to prevent
+;; duplicate column names, but do not want to use them as display names. See
+;; https://github.com/metabase/mbql/releases/tag/1.2.0 for detailed explanation.
+(expect
+ {:base_type :type/Integer
+ :special_type :type/Category
+ :settings nil
+ :name "sum"
+ :display_name "sum of Price"}
+ (qp.test-util/with-everything-store
+ (data/$ids venues
+ (col-info-for-aggregation-clause [:named [:sum $price] "sum" {:use-as-display-name? false}]))))
+
;; 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
(expect
@@ -285,7 +363,12 @@
(data/mbql-query venues
{:aggregation [[:metric "ga:totalEvents"]]}))))
-;; Make sure columns always come back with a unique `:name` key (#8759)
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | Other MBQL col info tests |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+;; Make sure `:cols` always come back with a unique `:name` key (#8759)
(expect
{:cols
[{:base_type :type/Number
@@ -400,60 +483,3 @@
((annotate/result-rows-maps->vectors (constantly results))
{:database (data/id)
:type :native})))))
-
-;; Does `result-rows-maps->vectors` handle multiple aggregations of the same type? Should assume column keys are
-;; deduplicated using the MBQL lib logic
-(expect
- {:rows [[2 409 20]
- [3 56 4]]
- :columns ["CATEGORY_ID" "sum" "sum_2"]}
- (qp.test-util/with-everything-store
- (driver/with-driver :h2
- (let [results {:rows [{:CATEGORY_ID 2
- :sum 409
- :sum_2 20}
- {:CATEGORY_ID 3
- :sum 56
- :sum_2 4}]}]
- ((annotate/result-rows-maps->vectors (constantly results))
- (data/mbql-query venues
- {:source-table $$venues
- :aggregation [[:sum $id]
- [:sum $price]]
- :breakout [$category_id]
- :limit 2}))))))
-
-;; For fields with parents we should return them with a combined name including parent's name
-(tt/expect-with-temp [Field [parent {:name "parent", :table_id (data/id :venues)}]
- Field [child {:name "child", :table_id (data/id :venues), :parent_id (u/get-id parent)}]]
- {:description nil
- :table_id (data/id :venues)
- :special_type nil
- :name "parent.child"
- :settings nil
- :parent_id (u/get-id parent)
- :id (u/get-id child)
- :visibility_type :normal
- :display_name "Child"
- :fingerprint nil
- :base_type :type/Text}
- (qp.test-util/with-everything-store
- (#'annotate/col-info-for-field-clause {} [:field-id (u/get-id child)])))
-
-;; nested-nested fields should include grandparent name (etc)
-(tt/expect-with-temp [Field [grandparent {:name "grandparent", :table_id (data/id :venues)}]
- Field [parent {:name "parent", :table_id (data/id :venues), :parent_id (u/get-id grandparent)}]
- Field [child {:name "child", :table_id (data/id :venues), :parent_id (u/get-id parent)}]]
- {:description nil
- :table_id (data/id :venues)
- :special_type nil
- :name "grandparent.parent.child"
- :settings nil
- :parent_id (u/get-id parent)
- :id (u/get-id child)
- :visibility_type :normal
- :display_name "Child"
- :fingerprint nil
- :base_type :type/Text}
- (qp.test-util/with-everything-store
- (#'annotate/col-info-for-field-clause {} [:field-id (u/get-id child)])))
diff --git a/test/metabase/query_processor/middleware/parameters/mbql_test.clj b/test/metabase/query_processor/middleware/parameters/mbql_test.clj
index ff3017ffd6f6b9731d38d9adec5e26046d1fa037..b231ec88a384b9fa8a46baa7cd7a4fb6505dd085 100644
--- a/test/metabase/query_processor/middleware/parameters/mbql_test.clj
+++ b/test/metabase/query_processor/middleware/parameters/mbql_test.clj
@@ -4,8 +4,7 @@
[metabase
[driver :as driver]
[query-processor :as qp]
- [query-processor-test :as qp.test]
- [util :as u]]
+ [query-processor-test :as qp.test]]
[metabase.mbql.normalize :as normalize]
[metabase.query-processor.middleware.parameters.mbql :as mbql-params]
[metabase.test.data :as data]
@@ -305,7 +304,7 @@
[37 "bigmista's barbecue" 5 34.118 -118.26 2]
[38 "Zeke's Smokehouse" 5 34.2053 -118.226 2]
[39 "Baby Blues BBQ" 5 34.0003 -118.465 2]]
- (qp.test/format-rows-by [int str int (partial u/round-to-decimals 4) (partial u/round-to-decimals 4) int]
+ (qp.test/format-rows-by :venues
(qp.test/rows
(qp/process-query
(data/query venues
diff --git a/test/metabase/query_processor/middleware/pre_alias_aggregations_test.clj b/test/metabase/query_processor/middleware/pre_alias_aggregations_test.clj
new file mode 100644
index 0000000000000000000000000000000000000000..8bc40efaa1cae782a659811d660973cacecb3e40
--- /dev/null
+++ b/test/metabase/query_processor/middleware/pre_alias_aggregations_test.clj
@@ -0,0 +1,105 @@
+(ns metabase.query-processor.middleware.pre-alias-aggregations-test
+ "Tests for the `pre-alias-aggregations` middleware. For the most part we don't need to test the actual pre-alias
+ logic, as that comes from the MBQL library and is tested thoroughly there -- we just need to test that it gets
+ applied in the correct places."
+ (:require [clojure.string :as str]
+ [expectations :refer [expect]]
+ [metabase.driver :as driver]
+ [metabase.query-processor.middleware.pre-alias-aggregations :as pre-alias-aggregations]
+ [metabase.test.data :as data]))
+
+(defn- pre-alias [query]
+ (driver/with-driver (or driver/*driver* :h2)
+ ((pre-alias-aggregations/pre-alias-aggregations identity) query)))
+
+;; do aggregations get pre-aliased by this middleware?
+(expect
+ (data/mbql-query checkins
+ {:source-table $$checkins
+ :aggregation [[:named [:sum $user_id] "sum" {:use-as-display-name? false}]
+ [:named [:sum $venue_id] "sum_2" {:use-as-display-name? false}]]})
+ (pre-alias
+ (data/mbql-query checkins
+ {:source-table $$checkins
+ :aggregation [[:sum $user_id] [:sum $venue_id]]})))
+
+;; if one or more aggregations are already named, do things still work correctly?
+(expect
+ (data/mbql-query checkins
+ {:source-table $$checkins
+ :aggregation [[:named [:sum $user_id] "sum" {:use-as-display-name? false}]
+ [:named [:sum $venue_id] "sum_2"]]})
+ (pre-alias
+ (data/mbql-query checkins
+ {:source-table $$checkins
+ :aggregation [[:sum $user_id]
+ [:named [:sum $venue_id] "sum"]]})))
+
+;; do aggregations inside source queries get pre-aliased?
+(expect
+ (data/mbql-query checkins
+ {:source-query {:source-table $$checkins
+ :aggregation [[:named [:sum $user_id] "sum" {:use-as-display-name? false}]
+ [:named [:sum $venue_id] "sum_2" {:use-as-display-name? false}]]}
+ :aggregation [[:named [:count] "count" {:use-as-display-name? false}]]})
+ (pre-alias
+ (data/mbql-query checkins
+ {:source-query {:source-table $$checkins
+ :aggregation [[:sum $user_id] [:sum $venue_id]]}
+ :aggregation [[:count]]})))
+
+;; do aggregatons inside of source queries inside joins get pre-aliased?
+(expect
+ (data/mbql-query checkins
+ {:source-table $$venues
+ :aggregation [[:named [:count] "count" {:use-as-display-name? false}]]
+ :joins [{:source-query {:source-table $$checkins
+ :aggregation [[:named [:sum $user_id] "sum" {:use-as-display-name? false}]
+ [:named [:sum $venue_id] "sum_2" {:use-as-display-name? false}]]
+ :breakout [$venue_id]}
+ :alias "checkins"
+ :condition [:= &checkins.venue_id $venues.id]}]})
+ (pre-alias
+ (data/mbql-query checkins
+ {:source-table $$venues
+ :aggregation [[:count]]
+ :joins [{:source-query {:source-table $$checkins
+ :aggregation [[:sum $user_id] [:sum $venue_id]]
+ :breakout [$venue_id]}
+ :alias "checkins"
+ :condition [:= &checkins.venue_id $venues.id]}]})))
+
+;; does pre-aliasing work the way we'd expect with expressions?
+(expect
+ {:database 1
+ :type :query
+ :query {:source-table 1
+ :aggregation [[:named [:+ 20 [:sum [:field-id 2]]] "20 + sum" {:use-as-display-name? false}]]}}
+ (pre-alias
+ {:database 1
+ :type :query
+ :query {:source-table 1
+ :aggregation [[:+ 20 [:sum [:field-id 2]]]]}}))
+
+;; we should use `driver/format-custom-field-name` on the generated aggregation names in case the drivers need to
+;; tweak the default names we generate
+(driver/register! ::test-driver, :abstract? true, :parent :sql)
+
+;; this implementation prepends an underscore to any name that starts with a number. Some DBs, such as BigQuery, do
+;; not allow aliases that start with an underscore
+(defmethod driver/format-custom-field-name ::test-driver [_ custom-field-name]
+ (str/replace custom-field-name #"(^\d)" "_$1"))
+
+(expect
+ {:database 1
+ :type :query
+ :query {:source-table 1
+ :aggregation [[:named [:+ 20 [:sum [:field-id 2]]] "_20 + sum" {:use-as-display-name? false}]
+ [:named [:count] "count" {:use-as-display-name? false}]]}}
+ (driver/with-driver ::test-driver
+ (pre-alias
+ {:database 1
+ :type :query
+ :query {:source-table 1
+ :aggregation [[:+ 20 [:sum [:field-id 2]]]
+ [:count]]}})))
diff --git a/test/metabase/query_processor_test.clj b/test/metabase/query_processor_test.clj
index 67f15f87a8705af845477a80ebef21e1815b13a9..6ae7ef3b4b020d2bd9269923cbaca5f0001c0bf5 100644
--- a/test/metabase/query_processor_test.clj
+++ b/test/metabase/query_processor_test.clj
@@ -6,9 +6,13 @@
[medley.core :as m]
[metabase
[driver :as driver]
+ [query-processor :as qp]
[util :as u]]
[metabase.driver.util :as driver.u]
- [metabase.models.field :refer [Field]]
+ [metabase.models
+ [field :refer [Field]]
+ [table :refer [Table]]]
+ [metabase.query-processor.middleware.add-implicit-joins :as add-implicit-joins]
[metabase.test.data :as data]
[metabase.test.data
[datasets :as datasets]
@@ -91,7 +95,7 @@
(db/select-one [Field :id :table_id :special_type :base_type :name :display_name :fingerprint]
:id (data/id table-kw field-kw))
{:field_ref [:field-id (data/id table-kw field-kw)]}
- (when (= field-kw :last_login)
+ (when (#{:last_login :date} field-kw)
{:unit :default
:field_ref [:datetime-field [:field-id (data/id table-kw field-kw)] :default]})))
@@ -138,17 +142,70 @@
(tx/aggregate-column-info (tx/driver) ag-type (col table-kw field-kw))))
(defn breakout-col
+ "Return expected `:cols` info for a Field used as a breakout.
+
+ (breakout-col :venues :price)"
([col]
(assoc col :source :breakout))
([table-kw field-kw]
(breakout-col (col table-kw field-kw))))
+(defn field-literal-col
+ "Return expected `:cols` info for a Field that was referred to as a `:field-literal`.
+
+ (field-literal-col :venues :price)
+ (field-literal-col (aggregate-col :count))"
+ {:arglists '([col] [table-kw field-kw])}
+ ([{field-name :name, base-type :base_type, unit :unit, :as col}]
+ (-> col
+ (assoc :field_ref [:field-literal field-name base-type]
+ :source :fields)
+ (dissoc :description :parent_id :visibility_type)))
+
+ ([table-kw field-kw]
+ (field-literal-col (col table-kw field-kw))))
+
+(defn fk-col
+ "Return expected `:cols` info for a Field that came in via an implicit join (i.e, via an `fk->` clause)."
+ [source-table-kw source-field-kw, dest-table-kw dest-field-kw]
+ (let [source-col (col source-table-kw source-field-kw)
+ dest-col (col dest-table-kw dest-field-kw)
+ dest-table-name (db/select-one-field :name Table :id (data/id dest-table-kw))
+ join-alias (#'add-implicit-joins/join-alias dest-table-name (:name source-col))]
+ (-> dest-col
+ (update :display_name (partial format "%s → %s" dest-table-name))
+ (assoc :field_ref [:joined-field join-alias [:field-id (:id dest-col)]]
+ :fk_field_id (:id source-col)))))
+
+(declare cols)
+
+(def ^:private ^{:arglists '([db-id table-id field-id])} native-query-col*
+ (memoize
+ (fn [db-id table-id field-id]
+ (first
+ (cols
+ (qp/process-query
+ {:database db-id
+ :type :native
+ :native (qp/query->native
+ {:database db-id
+ :type :query
+ :query {:source-table table-id
+ :fields [[:field_id field-id]]
+ :limit 1}})}))))))
+
+(defn native-query-col
+ "Return expected `:cols` info for a Field from a native query or native source query."
+ [table-kw field-kw]
+ (native-query-col* (data/id) (data/id table-kw) (data/id table-kw field-kw)))
+
(defn ^:deprecated booleanize-native-form
"Convert `:native_form` attribute to a boolean to make test results comparisons easier. Remove `data.results_metadata`
as well since it just takes a lot of space and the checksum can vary based on whether encryption is enabled.
- DEPRECATED: Just use `qp.test/rows` or `qp.test/row-and-cols` instead."
+ DEPRECATED: Just use `qp.test/rows`, `qp.test/row-and-cols`, or `qp.test/rows+column-names` instead, combined with
+ functions like `col` as needed."
[m]
(-> m
(update-in [:data :native_form] boolean)
@@ -160,8 +217,16 @@
:categories [int identity]
:checkins [int identity int int]
:users [int identity identity]
- :venues [int identity int (partial u/round-to-decimals 4) (partial u/round-to-decimals 4) int]
- (throw (IllegalArgumentException. (format "Sorry, we don't have default format-rows-by fns for Table %s." table-kw)))))
+ :venues [int identity int 4.0 4.0 int]
+ (throw
+ (IllegalArgumentException. (format "Sorry, we don't have default format-rows-by fns for Table %s." table-kw)))))
+
+(defn- format-rows-fn
+ "Handle a value formatting function passed to `format-rows-by`."
+ [x]
+ (if (float? x)
+ (partial u/round-to-decimals (int x))
+ x))
(defn format-rows-by
"Format the values in result `rows` with the fns at the corresponding indecies in `format-fns`. `rows` can be a
@@ -172,7 +237,12 @@
`format-fns` can be a sequence of functions, or may be the name of one of the 'big four' test data Tables to use
their defaults:
- (format-rows-by :venue (data/run-mbql-query :venues))
+ (format-rows-by :venues (data/run-mbql-query :venues))
+
+ Additionally, you may specify an floating-point number in the rounding functions vector as shorthand for formatting
+ with `u/round-to-decimals`:
+
+ (format-rows-by [identity 4.0] ...) ;-> (format-rows-by [identity (partial u/round-to-decimals 4)] ...)
By default, does't call fns on `nil` values; pass a truthy value as optional param `format-nil-values`? to override
this behavior."
@@ -185,9 +255,11 @@
(println "Error running query:" (u/pprint-to-str 'red response))
(throw (ex-info (:error response) response)))
- (let [format-fns (if (keyword? format-fns)
- (default-format-rows-by-fns format-fns)
- format-fns)]
+ (let [format-fns (map
+ format-rows-fn
+ (if (keyword? format-fns)
+ (default-format-rows-by-fns format-fns)
+ format-fns))]
(-> response
((fn format-rows [rows]
(cond
diff --git a/test/metabase/query_processor_test/aggregation_test.clj b/test/metabase/query_processor_test/aggregation_test.clj
index 7a5eeb7c858dff47546b29da8184b90f32a506af..e9ab05d224ddaaa7c36ea28f5a3e4710ee74755c 100644
--- a/test/metabase/query_processor_test/aggregation_test.clj
+++ b/test/metabase/query_processor_test/aggregation_test.clj
@@ -1,10 +1,8 @@
(ns metabase.query-processor-test.aggregation-test
"Tests for MBQL aggregations."
(:require [expectations :refer [expect]]
- [metabase
- [query-processor-test :as qp.test]
- [util :as u]]
[metabase.models.field :refer [Field]]
+ [metabase.query-processor-test :as qp.test]
[metabase.test
[data :as data]
[util :as tu]]
@@ -35,7 +33,7 @@
{:rows [[35.5059]]
:cols [(qp.test/aggregate-col :avg :venues :latitude)]}
(qp.test/rows-and-cols
- (qp.test/format-rows-by [(partial u/round-to-decimals 4)]
+ (qp.test/format-rows-by [4.0]
(data/run-mbql-query venues
{:aggregation [[:avg $latitude]]}))))
@@ -72,7 +70,7 @@
{:cols [(qp.test/aggregate-col :stddev :venues :latitude)]
:rows [[3.4]]}
(qp.test/rows-and-cols
- (qp.test/format-rows-by [(partial u/round-to-decimals 1)]
+ (qp.test/format-rows-by [1.0]
(data/run-mbql-query venues {:aggregation [[:stddev $latitude]]}))))
;; Make sure standard deviation fails for the Mongo driver since its not supported
@@ -104,14 +102,14 @@
(qp.test/expect-with-non-timeseries-dbs
[[1 34.0071] [2 33.7701] [3 10.0646] [4 33.983]]
- (qp.test/formatted-rows [int (partial u/round-to-decimals 4)]
+ (qp.test/formatted-rows [int 4.0]
(data/run-mbql-query venues
{:aggregation [[:min $latitude]]
:breakout [$price]})))
(qp.test/expect-with-non-timeseries-dbs
[[1 37.8078] [2 40.7794] [3 40.7262] [4 40.7677]]
- (qp.test/formatted-rows [int (partial u/round-to-decimals 4)]
+ (qp.test/formatted-rows [int 4.0]
(data/run-mbql-query venues
{:aggregation [[:max $latitude]]
:breakout [$price]})))
diff --git a/test/metabase/query_processor_test/breakout_test.clj b/test/metabase/query_processor_test/breakout_test.clj
index 4ed5b0a9128681338d4e346b0597255c1e2ff1ed..d71c591e58781afeaa14ef6f0379d26e606a00c3 100644
--- a/test/metabase/query_processor_test/breakout_test.clj
+++ b/test/metabase/query_processor_test/breakout_test.clj
@@ -132,21 +132,21 @@
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :binning)
[[10.0 1] [32.0 4] [34.0 57] [36.0 29] [40.0 9]]
- (qp.test/formatted-rows [(partial u/round-to-decimals 1) int]
+ (qp.test/formatted-rows [1.0 int]
(data/run-mbql-query venues
{:aggregation [[:count]]
:breakout [[:binning-strategy $latitude :num-bins 20]]})))
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :binning)
[[0.0 1] [20.0 90] [40.0 9]]
- (qp.test/formatted-rows [(partial u/round-to-decimals 1) int]
+ (qp.test/formatted-rows [1.0 int]
(data/run-mbql-query venues
{:aggregation [[:count]]
:breakout [[:binning-strategy $latitude :num-bins 3]]})))
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :binning)
[[10.0 -170.0 1] [32.0 -120.0 4] [34.0 -120.0 57] [36.0 -125.0 29] [40.0 -75.0 9]]
- (qp.test/formatted-rows [(partial u/round-to-decimals 1) (partial u/round-to-decimals 1) int]
+ (qp.test/formatted-rows [1.0 1.0 int]
(data/run-mbql-query venues
{:aggregation [[:count]]
:breakout [[:binning-strategy $latitude :num-bins 20]
@@ -156,7 +156,7 @@
;; specified
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :binning)
[[10.0 1] [30.0 90] [40.0 9]]
- (qp.test/formatted-rows [(partial u/round-to-decimals 1) int]
+ (qp.test/formatted-rows [1.0 int]
(data/run-mbql-query venues
{:aggregation [[:count]]
:breakout [[:binning-strategy $latitude :default]]})))
@@ -164,7 +164,7 @@
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :binning)
[[10.0 1] [30.0 61] [35.0 29] [40.0 9]]
(tu/with-temporary-setting-values [breakout-bin-width 5.0]
- (qp.test/formatted-rows [(partial u/round-to-decimals 1) int]
+ (qp.test/formatted-rows [1.0 int]
(data/run-mbql-query venues
{:aggregation [[:count]]
:breakout [[:binning-strategy $latitude :default]]}))))
@@ -172,7 +172,7 @@
;; Testing bin-width
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :binning)
[[10.0 1] [33.0 4] [34.0 57] [37.0 29] [40.0 9]]
- (qp.test/formatted-rows [(partial u/round-to-decimals 1) int]
+ (qp.test/formatted-rows [1.0 int]
(data/run-mbql-query venues
{:aggregation [[:count]]
:breakout [[:binning-strategy $latitude :bin-width 1]]})))
@@ -180,7 +180,7 @@
;; Testing bin-width using a float
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :binning)
[[10.0 1] [32.5 61] [37.5 29] [40.0 9]]
- (qp.test/formatted-rows [(partial u/round-to-decimals 1) int]
+ (qp.test/formatted-rows [1.0 int]
(data/run-mbql-query venues
{:aggregation [[:count]]
:breakout [[:binning-strategy $latitude :bin-width 2.5]]})))
@@ -188,7 +188,7 @@
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :binning)
[[33.0 4] [34.0 57]]
(tu/with-temporary-setting-values [breakout-bin-width 1.0]
- (qp.test/formatted-rows [(partial u/round-to-decimals 1) int]
+ (qp.test/formatted-rows [1.0 int]
(data/run-mbql-query venues
{:aggregation [[:count]]
:filter [:and
@@ -252,7 +252,7 @@
(tt/with-temp Card [card (qp.test-util/card-with-source-metadata-for-query
(data/mbql-query nil
{:source-query {:source-table $$venues}}))]
- (qp.test/formatted-rows [(partial u/round-to-decimals 1) int]
+ (qp.test/formatted-rows [1.0 int]
(qp/process-query
(nested-venues-query card)))))
diff --git a/test/metabase/query_processor_test/explicit_joins_test.clj b/test/metabase/query_processor_test/explicit_joins_test.clj
index d9b965433a2b254c85786ca9c610fab14c3f6cb6..56eb50044dbfb1e7c575b6951516ce2cb90861ed 100644
--- a/test/metabase/query_processor_test/explicit_joins_test.clj
+++ b/test/metabase/query_processor_test/explicit_joins_test.clj
@@ -3,8 +3,7 @@
[metabase
[driver :as driver]
[query-processor :as qp]
- [query-processor-test :as qp.test]
- [util :as u]]
+ [query-processor-test :as qp.test]]
[metabase.models.card :refer [Card]]
[metabase.query-processor.test-util :as qp.test-util]
[metabase.test.data :as data]
@@ -293,8 +292,7 @@
:columns
(mapv data/format-name ["id" "name" "category_id" "latitude" "longitude" "price" "id_2" "name_2"])}
(tt/with-temp Card [{card-id :id} (qp.test-util/card-with-source-metadata-for-query (data/mbql-query categories))]
- (qp.test/format-rows-by [int identity int (partial u/round-to-decimals 4) (partial u/round-to-decimals 4) int
- int identity]
+ (qp.test/format-rows-by [int identity int 4.0 4.0 int int identity]
(qp.test/rows+column-names
(data/run-mbql-query venues
{:joins [{:alias "cat"
@@ -416,7 +414,7 @@
:columns (mapv data/format-name
["id" "date" "user_id" "venue_id" "id_2" "name" "category_id" "latitude" "longitude" "price"])}
(qp.test/rows+column-names
- (qp.test/format-rows-by [int str int int int str int (partial u/round-to-decimals 3) (partial u/round-to-decimals 3) int]
+ (qp.test/format-rows-by [int str int int int str int 3.0 3.0 int]
(data/run-mbql-query checkins
{:source-query {:source-table $$checkins
:joins
diff --git a/test/metabase/query_processor_test/expressions_test.clj b/test/metabase/query_processor_test/expressions_test.clj
index bf09b3e839262787fd2f40a54d27a91c0b73b37d..fe989c4d8d29fc16793e0c9faeed50461cbb29c8 100644
--- a/test/metabase/query_processor_test/expressions_test.clj
+++ b/test/metabase/query_processor_test/expressions_test.clj
@@ -2,90 +2,96 @@
"Tests for expressions (calculated columns)."
(:require [metabase
[driver :as driver]
- [query-processor-test :refer :all]
- [util :as u]]
+ [query-processor-test :as qp.test]]
[metabase.test.data :as data]
[metabase.test.data.datasets :as datasets]))
;; Do a basic query including an expression
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expressions)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions)
[[1 "Red Medicine" 4 10.0646 -165.374 3 5.0]
[2 "Stout Burgers & Beers" 11 34.0996 -118.329 2 4.0]
[3 "The Apple Pan" 11 34.0406 -118.428 2 4.0]
[4 "Wurstküche" 29 33.9997 -118.465 2 4.0]
[5 "Brite Spot Family Restaurant" 20 34.0778 -118.261 2 4.0]]
- (format-rows-by [int str int (partial u/round-to-decimals 4) (partial u/round-to-decimals 4) int float]
- (rows (data/run-mbql-query venues
- {:expressions {:my-cool-new-field [:+ $price 2]}
- :limit 5
- :order-by [[:asc $id]]}))))
+ (qp.test/format-rows-by [int str int 4.0 4.0 int float]
+ (qp.test/rows
+ (data/run-mbql-query venues
+ {:expressions {:my-cool-new-field [:+ $price 2]}
+ :limit 5
+ :order-by [[:asc $id]]}))))
;; Make sure FLOATING POINT division is done
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expressions)
- [[1 "Red Medicine" 4 10.0646 -165.374 3 1.5] ; 3 / 2 SHOULD BE 1.5, NOT 1 (!)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions)
+ [[1 "Red Medicine" 4 10.0646 -165.374 3 1.5] ; 3 / 2 SHOULD BE 1.5, NOT 1 (!)
[2 "Stout Burgers & Beers" 11 34.0996 -118.329 2 1.0]
[3 "The Apple Pan" 11 34.0406 -118.428 2 1.0]]
- (format-rows-by [int str int (partial u/round-to-decimals 4) (partial u/round-to-decimals 4) int float]
- (rows (data/run-mbql-query venues
- {:expressions {:my-cool-new-field [:/ $price 2]}
- :limit 3
- :order-by [[:asc $id]]}))))
+ (qp.test/format-rows-by [int str int 4.0 4.0 int float]
+ (qp.test/rows
+ (data/run-mbql-query venues
+ {:expressions {:my-cool-new-field [:/ $price 2]}
+ :limit 3
+ :order-by [[:asc $id]]}))))
;; Can we do NESTED EXPRESSIONS ?
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expressions)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions)
[[1 "Red Medicine" 4 10.0646 -165.374 3 3.0]
[2 "Stout Burgers & Beers" 11 34.0996 -118.329 2 2.0]
[3 "The Apple Pan" 11 34.0406 -118.428 2 2.0]]
- (format-rows-by [int str int (partial u/round-to-decimals 4) (partial u/round-to-decimals 4) int float]
- (rows (data/run-mbql-query venues
- {:expressions {:wow [:- [:* $price 2] [:+ $price 0]]}
- :limit 3
- :order-by [[:asc $id]]}))))
+ (qp.test/format-rows-by [int str int 4.0 4.0 int float]
+ (qp.test/rows
+ (data/run-mbql-query venues
+ {:expressions {:wow [:- [:* $price 2] [:+ $price 0]]}
+ :limit 3
+ :order-by [[:asc $id]]}))))
;; Can we have MULTIPLE EXPRESSIONS?
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expressions)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions)
[[1 "Red Medicine" 4 10.0646 -165.374 3 2.0 4.0]
[2 "Stout Burgers & Beers" 11 34.0996 -118.329 2 1.0 3.0]
[3 "The Apple Pan" 11 34.0406 -118.428 2 1.0 3.0]]
- (format-rows-by [int str int (partial u/round-to-decimals 4) (partial u/round-to-decimals 4) int float float]
- (rows (data/run-mbql-query venues
- {:expressions {:x [:- $price 1]
- :y [:+ $price 1]}
- :limit 3
- :order-by [[:asc $id]]}))))
+ (qp.test/format-rows-by [int str int 4.0 4.0 int float float]
+ (qp.test/rows
+ (data/run-mbql-query venues
+ {:expressions {:x [:- $price 1]
+ :y [:+ $price 1]}
+ :limit 3
+ :order-by [[:asc $id]]}))))
;; Can we refer to expressions inside a FIELDS clause?
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expressions)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions)
[[4] [4] [5]]
- (format-rows-by [int]
- (rows (data/run-mbql-query venues
- {:expressions {:x [:+ $price $id]}
- :fields [[:expression :x]]
- :limit 3
- :order-by [[:asc $id]]}))))
+ (qp.test/format-rows-by [int]
+ (qp.test/rows
+ (data/run-mbql-query venues
+ {:expressions {:x [:+ $price $id]}
+ :fields [[:expression :x]]
+ :limit 3
+ :order-by [[:asc $id]]}))))
;; Can we refer to expressions inside an ORDER BY clause?
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expressions)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions)
[[100 "Mohawk Bend" 46 34.0777 -118.265 2 102.0]
[99 "Golden Road Brewing" 10 34.1505 -118.274 2 101.0]
[98 "Lucky Baldwin's Pub" 7 34.1454 -118.149 2 100.0]]
- (format-rows-by [int str int (partial u/round-to-decimals 4) (partial u/round-to-decimals 4) int float]
- (rows (data/run-mbql-query venues
- {:expressions {:x [:+ $price $id]}
- :limit 3
- :order-by [[:desc [:expression :x]]]}))))
+ (qp.test/format-rows-by [int str int 4.0 4.0 int float]
+ (qp.test/rows
+ (data/run-mbql-query venues
+ {:expressions {:x [:+ $price $id]}
+ :limit 3
+ :order-by [[:desc [:expression :x]]]}))))
;; Can we AGGREGATE + BREAKOUT by an EXPRESSION?
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expressions)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions)
[[2 22] [4 59] [6 13] [8 6]]
- (format-rows-by [int int]
- (rows (data/run-mbql-query venues
- {:expressions {:x [:* $price 2.0]}
- :aggregation [[:count]]
- :breakout [[:expression :x]]}))))
+ (qp.test/format-rows-by [int int]
+ (qp.test/rows
+ (data/run-mbql-query venues
+ {:expressions {:x [:* $price 2.0]}
+ :aggregation [[:count]]
+ :breakout [[:expression :x]]}))))
;; Custom aggregation expressions should include their type
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expressions)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions)
(conj #{{:name "x" :base_type :type/Float}}
{:name (data/format-name "category_id")
:base_type (case driver/*driver*
@@ -106,32 +112,37 @@
;; (at least for the purposes of the tests below)
;;
;; e.g. scarcity = 100.0 / num-birds
+(defn- calculate-bird-scarcity* [formula filter-clause]
+ (qp.test/format-rows-by [2.0]
+ (qp.test/rows
+ (data/dataset daily-bird-counts
+ (data/run-mbql-query bird-count
+ {:expressions {"bird-scarcity" formula}
+ :fields [[:expression "bird-scarcity"]]
+ :filter filter-clause
+ :order-by [[:asc $date]]
+ :limit 10})))))
+
(defmacro ^:private calculate-bird-scarcity [formula & [filter-clause]]
`(data/dataset ~'daily-bird-counts
- (->> (data/run-mbql-query ~'bird-count
- {:expressions {"bird-scarcity" ~formula}
- :fields [[:expression "bird-scarcity"]]
- :filter ~filter-clause
- :order-by [[:asc [:field-id ~'$date]]]
- :limit 10})
- rows
- (format-rows-by [(partial u/round-to-decimals 2)]))))
+ (data/$ids ~'bird-count
+ (calculate-bird-scarcity* ~formula ~filter-clause))))
;; hey... expressions should work if they are just a Field! (Also, this lets us take a peek at the raw values being
;; used to calculate the formulas below, so we can tell at a glance if they're right without referring to the EDN def)
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expressions)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions)
[[nil] [0.0] [0.0] [10.0] [8.0] [5.0] [5.0] [nil] [0.0] [0.0]]
(calculate-bird-scarcity [:field-id $count]))
;; do expressions automatically handle division by zero? Should return `nil` in the results for places where that was
;; attempted
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expressions)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions)
[[nil] [nil] [10.0] [12.5] [20.0] [20.0] [nil] [nil] [9.09] [7.14]]
(calculate-bird-scarcity [:/ 100.0 [:field-id $count]]
[:!= $count nil]))
;; do expressions handle division by `nil`? Should return `nil` in the results for places where that was attempted
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expressions)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions)
[[nil] [10.0] [12.5] [20.0] [20.0] [nil] [9.09] [7.14] [12.5] [7.14]]
(calculate-bird-scarcity [:/ 100.0 [:field-id $count]]
[:or
@@ -139,37 +150,37 @@
[:!= $count 0]]))
;; can we handle BOTH NULLS AND ZEROES AT THE SAME TIME????
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expressions)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions)
[[nil] [nil] [nil] [10.0] [12.5] [20.0] [20.0] [nil] [nil] [nil]]
(calculate-bird-scarcity [:/ 100.0 [:field-id $count]]))
;; ok, what if we use multiple args to divide, and more than one is zero?
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expressions)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions)
[[nil] [nil] [nil] [1.0] [1.56] [4.0] [4.0] [nil] [nil] [nil]]
(calculate-bird-scarcity [:/ 100.0 [:field-id $count] [:field-id $count]]))
;; are nulls/zeroes still handled appropriately when nested inside other expressions?
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expressions)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions)
[[nil] [nil] [nil] [20.0] [25.0] [40.0] [40.0] [nil] [nil] [nil]]
(calculate-bird-scarcity [:* [:/ 100.0 [:field-id $count]] 2]))
;; if a zero is present in the NUMERATOR we should return ZERO and not NULL
;; (`0 / 10 = 0`; `10 / 0 = NULL`, at least as far as MBQL is concerned)
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expressions)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions)
[[nil] [0.0] [0.0] [1.0] [0.8] [0.5] [0.5] [nil] [0.0] [0.0]]
(calculate-bird-scarcity [:/ [:field-id $count] 10]))
;; can addition handle nulls & zeroes?
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expressions)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions)
[[nil] [10.0] [10.0] [20.0] [18.0] [15.0] [15.0] [nil] [10.0] [10.0]]
(calculate-bird-scarcity [:+ [:field-id $count] 10]))
;; can subtraction handle nulls & zeroes?
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expressions)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions)
[[nil] [10.0] [10.0] [0.0] [2.0] [5.0] [5.0] [nil] [10.0] [10.0]]
(calculate-bird-scarcity [:- 10 [:field-id $count]]))
;; can multiplications handle nulls & zeros?
-(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :expressions)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions)
[[nil] [0.0] [0.0] [10.0] [8.0] [5.0] [5.0] [nil] [0.0] [0.0]]
(calculate-bird-scarcity [:* 1 [:field-id $count]]))
diff --git a/test/metabase/query_processor_test/nested_queries_test.clj b/test/metabase/query_processor_test/nested_queries_test.clj
index c2acf3075af4123ac834bf364c8f3f4c2de69d5b..ea1e9c1e62abcefc2b0eb7271f911db48b993cfe 100644
--- a/test/metabase/query_processor_test/nested_queries_test.clj
+++ b/test/metabase/query_processor_test/nested_queries_test.clj
@@ -1,10 +1,8 @@
(ns metabase.query-processor-test.nested-queries-test
"Tests for handling queries with nested expressions."
- (:require [clojure.string :as str]
- [expectations :refer [expect]]
+ (:require [expectations :refer [expect]]
[honeysql.core :as hsql]
[metabase
- [driver :as driver]
[query-processor :as qp]
[query-processor-test :as qp.test]
[util :as u]]
@@ -12,7 +10,6 @@
[metabase.models
[card :as card :refer [Card]]
[collection :as collection :refer [Collection]]
- [field :refer [Field]]
[permissions :as perms]
[permissions-group :as group]
[segment :refer [Segment]]]
@@ -24,20 +21,8 @@
[datasets :as datasets]
[users :refer [user->client]]]
[metabase.util.honeysql-extensions :as hx]
- [toucan.db :as db]
[toucan.util.test :as tt]))
-(defn- rows+cols
- "Return the `:rows` and relevant parts of `:cols` from the `results`.
- (This is used to keep the output of various tests below focused and manageable.)"
- {:style/indent 0}
- [results]
- {:rows (qp.test/rows results)
- :cols (for [col (get-in results [:data :cols])]
- {:name (str/lower-case (:name col))
- :base_type (:base_type col)})})
-
-
;; make sure we can do a basic query with MBQL source-query
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :nested-queries)
{:rows [[1 "Red Medicine" 4 10.0646 -165.374 3]
@@ -45,21 +30,16 @@
[3 "The Apple Pan" 11 34.0406 -118.428 2]
[4 "Wurstküche" 29 33.9997 -118.465 2]
[5 "Brite Spot Family Restaurant" 20 34.0778 -118.261 2]]
- :cols [{:name "id", :base_type (data/id-field-type)}
- {:name "name", :base_type :type/Text}
- {:name "category_id", :base_type (data/expected-base-type->actual :type/Integer)}
- {:name "latitude", :base_type :type/Float}
- {:name "longitude", :base_type :type/Float}
- {:name "price", :base_type (data/expected-base-type->actual :type/Integer)}]}
- (qp.test/format-rows-by [int str int (partial u/round-to-decimals 4) (partial u/round-to-decimals 4) int]
- (rows+cols
- (qp/process-query
- {:database (data/id)
- :type :query
- :query {:source-query {:source-table (data/id :venues)
- :order-by [[:asc (data/id :venues :id)]]
- :limit 10}
- :limit 5}}))))
+ :cols (mapv
+ (partial qp.test/field-literal-col :venues)
+ [:id :name :category_id :latitude :longitude :price])}
+ (qp.test/rows-and-cols
+ (qp.test/format-rows-by :venues
+ (data/run-mbql-query nil
+ {:source-query {:source-table $$venues
+ :order-by [[:asc $venues.id]]
+ :limit 10}
+ :limit 5}))))
;; make sure we can do a basic query with a SQL source-query
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :nested-queries)
@@ -68,66 +48,51 @@
[3 -118.428 11 2 "The Apple Pan" 34.0406]
[4 -118.465 29 2 "Wurstküche" 33.9997]
[5 -118.261 20 2 "Brite Spot Family Restaurant" 34.0778]]
- ;; Oracle doesn't have Integer types, they always come back as DECIMAL
- :cols [{:name "id", :base_type (case driver/*driver* :oracle :type/Decimal :type/Integer)}
- {:name "longitude", :base_type :type/Float}
- {:name "category_id", :base_type (case driver/*driver* :oracle :type/Decimal :type/Integer)}
- {:name "price", :base_type (case driver/*driver* :oracle :type/Decimal :type/Integer)}
- {:name "name", :base_type :type/Text}
- {:name "latitude", :base_type :type/Float}]}
- (qp.test/format-rows-by [int (partial u/round-to-decimals 4) int int str (partial u/round-to-decimals 4)]
- (rows+cols
- (qp/process-query
- {:database (data/id)
- :type :query
- :query {:source-query {:native (:query
- (qp/query->native
- (data/mbql-query venues
- {:fields [[:field-id $id]
- [:field-id $longitude]
- [:field-id $category_id]
- [:field-id $price]
- [:field-id $name]
- [:field-id $latitude]]})))}
- :order-by [[:asc [:field-literal (data/format-name :id) :type/Integer]]]
- :limit 5}}))))
-
-
-(def ^:private breakout-results
+ :cols (mapv (partial qp.test/native-query-col :venues) [:id :longitude :category_id :price :name :latitude])}
+ (qp.test/format-rows-by [int 4.0 int int str 4.0]
+ (let [{source-query :query} (qp/query->native
+ (data/mbql-query venues
+ {:fields [$id $longitude $category_id $price $name $latitude]}))]
+ (qp.test/rows-and-cols
+ (data/run-mbql-query venues
+ {:source-query {:native source-query}
+ :order-by [[:asc *venues.id]]
+ :limit 5})))))
+
+
+(defn- breakout-results [& {:keys [has-source-metadata?], :or {has-source-metadata? true}}]
{:rows [[1 22]
[2 59]
[3 13]
[4 6]]
- :cols [{:name "price", :base_type :type/Integer}
- {:name "count", :base_type :type/Integer}]})
+ :cols [(cond-> (qp.test/breakout-col (qp.test/field-literal-col :venues :price))
+ (not has-source-metadata?)
+ (dissoc :id :special_type :settings :fingerprint :table_id))
+ (qp.test/aggregate-col :count)]})
;; make sure we can do a query with breakout and aggregation using an MBQL source query
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :nested-queries)
- breakout-results
- (rows+cols
- (qp.test/format-rows-by [int int]
- (qp/process-query
- {:database (data/id)
- :type :query
- :query {:source-query {:source-table (data/id :venues)}
- :aggregation [:count]
- :breakout [[:field-literal (keyword (data/format-name :price)) :type/Integer]]}}))))
+ (breakout-results)
+ (qp.test/rows-and-cols
+ (qp.test/format-rows-by [int int]
+ (data/run-mbql-query venues
+ {:source-query {:source-table $$venues}
+ :aggregation [:count]
+ :breakout [*price]}))))
;; Test including a breakout of a nested query column that follows an FK
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :nested-queries :foreign-keys)
{:rows [[1 174] [2 474] [3 78] [4 39]]
- :cols [{:name "price", :base_type (data/expected-base-type->actual :type/Integer)}
- {:name "count", :base_type :type/Integer}]}
- (rows+cols
- (qp.test/format-rows-by [int int]
- (qp/process-query
- {:database (data/id)
- :type :query
- :query {:source-query {:source-table (data/id :checkins)
- :filter [:> (data/id :checkins :date) "2014-01-01"]}
- :aggregation [:count]
- :order-by [[:asc [:fk-> (data/id :checkins :venue_id) (data/id :venues :price)]]]
- :breakout [[:fk-> (data/id :checkins :venue_id) (data/id :venues :price)]]}}))))
+ :cols [(qp.test/breakout-col (qp.test/fk-col :checkins :venue_id :venues :price))
+ (qp.test/aggregate-col :count)]}
+ (qp.test/rows-and-cols
+ (qp.test/format-rows-by [int int]
+ (data/run-mbql-query checkins
+ {:source-query {:source-table $$checkins
+ :filter [:> $date "2014-01-01"]}
+ :aggregation [:count]
+ :order-by [[:asc $venue_id->venues.price]]
+ :breakout [$venue_id->venues.price]}))))
;; Test two breakout columns from the nested query, both following an FK
@@ -137,21 +102,19 @@
[2 33.9997 7]
[3 10.0646 2]
[4 33.983 2]],
- :cols [{:name "price", :base_type (data/expected-base-type->actual :type/Integer)}
- {:name "latitude", :base_type :type/Float}
- {:name "count", :base_type :type/Integer}]}
- (rows+cols
- (qp.test/format-rows-by [int (partial u/round-to-decimals 4) int]
- (qp/process-query
- {:database (data/id)
- :type :query
- :query {:source-query {:source-table (data/id :checkins)
- :filter [:> (data/id :checkins :date) "2014-01-01"]}
- :filter [:< [:fk-> (data/id :checkins :venue_id) (data/id :venues :latitude)] 34]
- :aggregation [:count]
- :order-by [[:asc [:fk-> (data/id :checkins :venue_id) (data/id :venues :price)]]]
- :breakout [[:fk-> (data/id :checkins :venue_id) (data/id :venues :price)]
- [:fk-> (data/id :checkins :venue_id) (data/id :venues :latitude)]]}}))))
+ :cols [(qp.test/breakout-col (qp.test/fk-col :checkins :venue_id :venues :price))
+ (qp.test/breakout-col (qp.test/fk-col :checkins :venue_id :venues :latitude))
+ (qp.test/aggregate-col :count)]}
+ (qp.test/rows-and-cols
+ (qp.test/format-rows-by [int 4.0 int]
+ (data/run-mbql-query checkins
+ {:source-query {:source-table $$checkins
+ :filter [:> $date "2014-01-01"]}
+ :filter [:< $venue_id->venues.latitude 34]
+ :aggregation [:count]
+ :order-by [[:asc $venue_id->venues.price]]
+ :breakout [$venue_id->venues.price
+ $venue_id->venues.latitude]}))))
;; Test two breakout columns from the nested query, one following an FK the other from the source table
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :nested-queries :foreign-keys)
@@ -159,44 +122,41 @@
[1 2 14]
[1 3 13]
[1 4 8]
- [1 5 10]],
- :cols [{:name "price", :base_type (data/expected-base-type->actual :type/Integer)}
- {:name "user_id", :base_type :type/Integer}
- {:name "count", :base_type :type/Integer}]}
- (rows+cols
+ [1 5 10]]
+ :cols [(qp.test/breakout-col (qp.test/fk-col :checkins :venue_id :venues :price))
+ (qp.test/breakout-col (qp.test/field-literal-col :checkins :user_id))
+ (qp.test/aggregate-col :count)]}
+ (qp.test/rows-and-cols
(qp.test/format-rows-by [int int int]
- (qp/process-query
- {:database (data/id)
- :type :query
- :query {:source-query {:source-table (data/id :checkins)
- :filter [:> (data/id :checkins :date) "2014-01-01"]}
- :aggregation [:count]
- :filter [:= [:fk-> (data/id :checkins :venue_id) (data/id :venues :price)] 1]
- :order-by [[:asc [:fk-> (data/id :checkins :venue_id) (data/id :venues :price)]]]
- :breakout [[:fk-> (data/id :checkins :venue_id) (data/id :venues :price)]
- [:field-literal (keyword (data/format-name :user_id)) :type/Integer]]
- :limit 5}}))))
+ (data/run-mbql-query checkins
+ {:source-query {:source-table $$checkins
+ :filter [:> $date "2014-01-01"]}
+ :aggregation [:count]
+ :filter [:= $venue_id->venues.price 1]
+ :order-by [[:asc $venue_id->venues.price]]
+ :breakout [$venue_id->venues.price *user_id]
+ :limit 5}))))
;; make sure we can do a query with breakout and aggregation using a SQL source query
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :nested-queries)
- breakout-results
- (rows+cols
- (qp.test/format-rows-by [int int]
- (qp/process-query
- {:database (data/id)
- :type :query
- :query {:source-query {:native (:query (qp/query->native (data/mbql-query venues)))}
- :aggregation [:count]
- :breakout [[:field-literal (keyword (data/format-name :price)) :type/Integer]]}}))))
+ (breakout-results :has-source-metadata? false)
+ (qp.test/rows-and-cols
+ (qp.test/format-rows-by [int int]
+ (data/run-mbql-query venues
+ {:source-query {:native (:query (qp/query->native (data/mbql-query venues)))}
+ :aggregation [:count]
+ :breakout [*price]}))))
(defn- mbql-card-def
"Basic MBQL Card definition. Pass kv-pair clauses for the inner query."
{:style/indent 0}
- [& {:as clauses}]
- {:dataset_query {:database (data/id)
- :type :query
- :query clauses}})
+ ([m]
+ {:dataset_query {:database (data/id)
+ :type :query
+ :query m}})
+ ([k v & {:as more}]
+ (mbql-card-def (merge {k v} more))))
(defn- venues-mbql-card-def
"A basic Card definition that returns raw data for the venues test table.
@@ -206,80 +166,85 @@
(apply mbql-card-def :source-table (data/id :venues) additional-clauses))
-(defn- query-with-source-card {:style/indent 1} [card & {:as additional-clauses}]
- {:database mbql.s/saved-questions-virtual-database-id
- :type :query
- :query (merge {:source-table (str "card__" (u/get-id card))}
- additional-clauses)})
+(defn- query-with-source-card
+ {:style/indent 1}
+ ([card]
+ {:database mbql.s/saved-questions-virtual-database-id
+ :type :query
+ :query {:source-table (str "card__" (u/get-id card))}})
+
+ ([card m]
+ (update (query-with-source-card card) :query merge m))
+
+ ([card k v & {:as more}]
+ (query-with-source-card card (merge {k v} more))))
;; Make sure we can run queries using source table `card__id` format. This is the format that is actually used by the
;; frontend; it gets translated to the normal `source-query` format by middleware. It's provided as a convenience so
;; only minimal changes need to be made to the frontend.
(expect
- breakout-results
+ (breakout-results)
(tt/with-temp Card [card (venues-mbql-card-def)]
- (rows+cols
- (qp.test/format-rows-by [int int]
- (qp/process-query
- (query-with-source-card card
- :aggregation [:count]
- :breakout [[:field-literal (keyword (data/format-name :price)) :type/Integer]]))))))
+ (qp.test/rows-and-cols
+ (qp.test/format-rows-by [int int]
+ (qp/process-query
+ (query-with-source-card card
+ (data/$ids venues
+ {:aggregation [:count]
+ :breakout [*price]})))))))
;; make sure `card__id`-style queries work with native source queries as well
(expect
- breakout-results
+ (breakout-results :has-source-metadata? false)
(tt/with-temp Card [card {:dataset_query {:database (data/id)
:type :native
:native {:query "SELECT * FROM VENUES"}}}]
- (rows+cols
- (qp.test/format-rows-by [int int]
- (qp/process-query
- (query-with-source-card card
- :aggregation [:count]
- :breakout [[:field-literal (keyword (data/format-name :price)) :type/Integer]]))))))
+ (qp.test/rows-and-cols
+ (qp.test/format-rows-by [int int]
+ (qp/process-query
+ (query-with-source-card card
+ (data/$ids venues
+ {:aggregation [:count]
+ :breakout [*price]})))))))
;; Ensure trailing comments are trimmed and don't cause a wrapping SQL query to fail
(expect
- breakout-results
+ (breakout-results :has-source-metadata? false)
(tt/with-temp Card [card {:dataset_query {:database (data/id)
:type :native
:native {:query "SELECT * FROM VENUES -- small comment here"}}}]
- (rows+cols
- (qp.test/format-rows-by [int int]
- (qp/process-query
- (query-with-source-card card
- :aggregation [:count]
- :breakout [[:field-literal (keyword (data/format-name :price)) :type/Integer]]))))))
+ (qp.test/rows-and-cols
+ (qp.test/format-rows-by [int int]
+ (qp/process-query
+ (query-with-source-card card
+ (data/$ids venues
+ {:aggregation [:count]
+ :breakout [*price]})))))))
;; Ensure trailing comments followed by a newline are trimmed and don't cause a wrapping SQL query to fail
(expect
- breakout-results
+ (breakout-results :has-source-metadata? false)
(tt/with-temp Card [card {:dataset_query {:database (data/id)
:type :native
:native {:query "SELECT * FROM VENUES -- small comment here\n"}}}]
- (rows+cols
- (qp.test/format-rows-by [int int]
- (qp/process-query
- (query-with-source-card card
- :aggregation [:count]
- :breakout [[:field-literal (keyword (data/format-name :price)) :type/Integer]]))))))
+ (qp.test/rows-and-cols
+ (qp.test/format-rows-by [int int]
+ (qp/process-query
+ (query-with-source-card card
+ (data/$ids venues
+ {:aggregation [:count]
+ :breakout [*price]})))))))
;; make sure we can filter by a field literal
(expect
{:rows [[1 "Red Medicine" 4 10.0646 -165.374 3]]
- :cols [{:name "id", :base_type :type/BigInteger}
- {:name "name", :base_type :type/Text}
- {:name "category_id", :base_type :type/Integer}
- {:name "latitude", :base_type :type/Float}
- {:name "longitude", :base_type :type/Float}
- {:name "price", :base_type :type/Integer}]}
- (rows+cols
- (qp/process-query
- {:database (data/id)
- :type :query
- :query {:source-query {:source-table (data/id :venues)}
- :filter [:= [:field-literal (data/format-name :id) :type/Integer] 1]}})))
+ :cols (mapv (partial qp.test/field-literal-col :venues)
+ [:id :name :category_id :latitude :longitude :price])}
+ (qp.test/rows-and-cols
+ (data/run-mbql-query venues
+ {:source-query {:source-table $$venues}
+ :filter [:= *id 1]})))
(defn- honeysql->sql
"Convert `honeysql-form` to the format returned by `query->native`. Writing HoneySQL is a lot easier that writing
@@ -352,11 +317,11 @@
:params nil}
(qp/query->native
(data/mbql-query venues
- {:source-query {:source-table $$venues
- :aggregation [[:stddev $id]]
- :breakout [$price]
- :order-by [[[:aggregation 0] :descending]]}
- :aggregation [[:avg *stddev/Integer]]})))
+ {:source-query {:source-table $$venues
+ :aggregation [[:stddev $id]]
+ :breakout [$price]
+ :order-by [[[:aggregation 0] :descending]]}
+ :aggregation [[:avg *stddev/Integer]]})))
;; make sure that we handle [field-id [field-literal ...]] forms gracefully, despite that not making any sense
(expect
@@ -425,108 +390,57 @@
:type :query
:query {:source-table (str "card__" (u/get-id card))}})))
-(defn results-metadata {:style/indent 0} [results]
- (when (= :failed (:status results))
- (throw (ex-info "No results metadata." results)))
- (for [col (get-in results [:data :cols])]
- (u/select-non-nil-keys col [:base_type :display_name :id :name :special_type :table_id :unit :datetime-unit])))
-
;; make sure a query using a source query comes back with the correct columns metadata
(expect
- [{:base_type :type/BigInteger
- :display_name "ID"
- :id (data/id :venues :id)
- :name "ID"
- :special_type :type/PK
- :table_id (data/id :venues)}
- {:base_type :type/Text
- :display_name "Name"
- :id (data/id :venues :name)
- :name "NAME"
- :special_type :type/Name
- :table_id (data/id :venues)}
- {:base_type :type/Integer
- :display_name "Category ID"
- :id (data/id :venues :category_id)
- :name "CATEGORY_ID"
- :special_type :type/FK
- :table_id (data/id :venues)}
- {:base_type :type/Float
- :display_name "Latitude"
- :id (data/id :venues :latitude)
- :name "LATITUDE"
- :special_type :type/Latitude
- :table_id (data/id :venues)}
- {:base_type :type/Float
- :display_name "Longitude"
- :id (data/id :venues :longitude)
- :name "LONGITUDE"
- :special_type :type/Longitude
- :table_id (data/id :venues)}
- {:base_type :type/Integer
- :display_name "Price"
- :id (data/id :venues :price)
- :name "PRICE"
- :special_type :type/Category
- :table_id (data/id :venues)}]
- (-> (tt/with-temp Card [card (venues-mbql-card-def)]
- (qp/process-query (query-with-source-card card)))
- results-metadata))
+ (map
+ (partial qp.test/field-literal-col :venues)
+ [:id :name :category_id :latitude :longitude :price])
+ (qp.test/cols
+ (tt/with-temp Card [card (venues-mbql-card-def)]
+ (qp/process-query (query-with-source-card card)))))
;; make sure a breakout/aggregate query using a source query comes back with the correct columns metadata
(expect
- [{:base_type :type/Text
- :name "PRICE"
- :display_name "Price"}
- {:base_type :type/Integer
- :display_name "count"
- :name "count"
- :special_type :type/Number}]
- (-> (tt/with-temp Card [card (venues-mbql-card-def)]
- (qp/process-query (query-with-source-card card
- :aggregation [[:count]]
- :breakout [[:field-literal "PRICE" :type/Text]])))
- results-metadata))
+ [(qp.test/breakout-col (qp.test/field-literal-col :venues :price))
+ (qp.test/aggregate-col :count)]
+ (qp.test/cols
+ (tt/with-temp Card [card (venues-mbql-card-def)]
+ (qp/process-query
+ (query-with-source-card card
+ (data/$ids venues
+ {:aggregation [[:count]]
+ :breakout [*price]}))))))
;; make sure nested queries return the right columns metadata for SQL source queries and datetime breakouts
(expect
- [{:base_type :type/DateTime
- :display_name "Date"
- :name "DATE"
- :unit :day}
- {:base_type :type/Integer
- :display_name "count"
- :name "count"
- :special_type :type/Number}]
- (-> (tt/with-temp Card [card {:dataset_query {:database (data/id)
- :type :native
- :native {:query "SELECT * FROM CHECKINS"}}}]
- (qp/process-query (query-with-source-card card
- :aggregation [[:count]]
- :breakout [[:datetime-field [:field-literal "DATE" :type/DateTime] :day]])))
- results-metadata))
+ [(-> (qp.test/breakout-col (qp.test/field-literal-col :checkins :date))
+ (assoc :field_ref [:datetime-field [:field-literal "DATE" :type/Date] :day]
+ :unit :day)
+ ;; because this field literal comes from a native query that does not include `:source-metadata` it won't have
+ ;; the usual extra keys
+ (dissoc :special_type :table_id :id :settings :fingerprint))
+ (qp.test/aggregate-col :count)]
+ (qp.test/cols
+ (tt/with-temp Card [card {:dataset_query {:database (data/id)
+ :type :native
+ :native {:query "SELECT * FROM CHECKINS"}}}]
+ (qp/process-query
+ (query-with-source-card card
+ (data/$ids checkins
+ {:aggregation [[:count]]
+ :breakout [!day.*date]}))))))
;; make sure when doing a nested query we give you metadata that would suggest you should be able to break out a *YEAR*
(expect
- [{:base_type :type/Date
- :display_name "Date"
- :id (data/id :checkins :date)
- :name "DATE"
- :table_id (data/id :checkins)
- :unit :year}
- {:base_type :type/Integer
- :display_name "Count"
- :name "count"
- :special_type :type/Number}]
- (-> (tt/with-temp Card [card (mbql-card-def
- :source-table (data/id :checkins)
- :aggregation [[:count]]
- :breakout [[:datetime-field [:field-id (data/id :checkins :date)] :year]])]
- (qp/process-query (query-with-source-card card)))
- results-metadata))
-
-(defn- field-name [table-kw field-kw]
- (db/select-one-field :name Field :id (data/id table-kw field-kw)))
+ [(assoc (qp.test/field-literal-col :checkins :date) :unit :year)
+ (qp.test/field-literal-col (qp.test/aggregate-col :count))]
+ (qp.test/cols
+ (tt/with-temp Card [card (mbql-card-def
+ (data/$ids checkins
+ {:source-table $$checkins
+ :aggregation [[:count]]
+ :breakout [!year.date]}))]
+ (qp/process-query (query-with-source-card card)))))
(defn- completed-status [{:keys [status], :as results}]
(if (= status :completed)
@@ -536,52 +450,49 @@
;; make sure using a time interval filter works
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :nested-queries)
:completed
- (tt/with-temp Card [card (mbql-card-def
- :source-table (data/id :checkins))]
+ (tt/with-temp Card [card (mbql-card-def (data/$ids {:source-table $$checkins}))]
(-> (query-with-source-card card
- :filter [:time-interval [:field-literal (field-name :checkins :date) :type/DateTime] -30 :day])
+ (data/$ids checkins
+ {:filter [:time-interval *date -30 :day]}))
qp/process-query
completed-status)))
;; make sure that wrapping a field literal in a datetime-field clause works correctly in filters & breakouts
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :nested-queries)
:completed
- (tt/with-temp Card [card (mbql-card-def
- :source-table (data/id :checkins))]
+ (tt/with-temp Card [card (mbql-card-def (data/$ids {:source-table $$checkins}))]
(-> (query-with-source-card card
- :aggregation [[:count]]
- :filter [:= [:datetime-field [:field-literal (field-name :checkins :date) :type/Date] :quarter] "2014-01-01T08:00:00.000Z"]
- :breakout [[:datetime-field [:field-literal (field-name :checkins :date) :type/Date] :month]])
+ (data/$ids :checkins
+ {:aggregation [[:count]]
+ :filter [:= !quarter.*date "2014-01-01T08:00:00.000Z"]
+ :breakout [!month.*date]}))
qp/process-query
completed-status)))
;; make sure timeseries queries generated by "drag-to-filter" work correctly
(expect
:completed
- (tt/with-temp Card [card (mbql-card-def
- :source-table (data/id :checkins))]
- (-> (query-with-source-card card
- :aggregation [[:count]]
- :breakout [[:datetime-field [:field-literal "DATE" :type/Date] :week]]
- :filter [:between
- [:datetime-field [:field-literal "DATE" :type/Date] :month]
- "2014-02-01T00:00:00-08:00"
- "2014-05-01T00:00:00-07:00"])
- qp/process-query
- completed-status)))
+ (tt/with-temp Card [card (mbql-card-def (data/$ids {:source-table $$checkins}))]
+ (completed-status
+ (qp/process-query
+ (query-with-source-card card
+ (data/$ids checkins
+ {:aggregation [[:count]]
+ :breakout [!week.*date]
+ :filter [:between !week.*date "2014-02-01T00:00:00-08:00" "2014-05-01T00:00:00-07:00"]}))))))
;; Make sure that macro expansion works inside of a neested query, when using a compound filter clause (#5974)
(expect
[[22]]
- (tt/with-temp* [Segment [segment {:table_id (data/id :venues)
- :definition {:filter [:= (data/id :venues :price) 1]}}]
+ (tt/with-temp* [Segment [segment (data/$ids {:table_id $$venues
+ :definition {:filter [:= $venues.price 1]}})]
Card [card (mbql-card-def
:source-table (data/id :venues)
:filter [:and [:segment (u/get-id segment)]])]]
- (-> (query-with-source-card card
- :aggregation [:count])
- qp/process-query
- qp.test/rows)))
+ (qp.test/rows
+ (qp/process-query
+ (query-with-source-card card
+ {:aggregation [:count]})))))
;; Make suer you're allowed to save a query that uses a SQL-based source query even if you don't have SQL *write*1337
@@ -699,7 +610,7 @@
[37 "bigmista's barbecue" 5 34.118 -118.26 2]
[38 "Zeke's Smokehouse" 5 34.2053 -118.226 2]
[39 "Baby Blues BBQ" 5 34.0003 -118.465 2]]
- (qp.test/format-rows-by [int str int (partial u/round-to-decimals 4) (partial u/round-to-decimals 4) int]
+ (qp.test/format-rows-by :venues
(qp.test/rows
(qp/process-query
(data/mbql-query venues
@@ -718,9 +629,7 @@
{:source-query {:source-table $$checkins
:order-by [[:asc $id]]}
:fields [$id]
- :filter [:=
- [:field-literal (field-name :checkins :date) "type/DateTime"]
- "2014-03-30"]}))))
+ :filter [:= *date "2014-03-30"]}))))
;; make sure filters in source queries are applied correctly!
(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :nested-queries :foreign-keys)
@@ -735,3 +644,30 @@
:order-by [[:asc $venue_id->venues.name]]
:breakout [$venue_id->venues.name]
:filter [:starts-with $venue_id->venues.name "F"]}))))
+
+;; Do nested queries work with two of the same aggregation? (#9767)
+(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :nested-queries :foreign-keys)
+ {:rows
+ [["2014-02-01T00:00:00.000Z" 302 1804]
+ ["2014-03-01T00:00:00.000Z" 350 2362]]
+ :cols
+ [(assoc (qp.test/field-literal-col :checkins :date)
+ :unit :month)
+ (let [{base-type :base_type, :as literal-col} (qp.test/field-literal-col :checkins :user_id)]
+ (assoc (qp.test/aggregate-col :sum literal-col)
+ :source :fields
+ :field_ref [:field-literal "sum" base-type]))
+ (let [{base-type :base_type, :as literal-col} (qp.test/field-literal-col :checkins :venue_id)]
+ (assoc (qp.test/aggregate-col :sum literal-col)
+ :name "sum_2"
+ :source :fields
+ :field_ref [:field-literal "sum_2" base-type]))]}
+ (qp.test/format-rows-by [identity int int]
+ (qp.test/rows-and-cols
+ (data/run-mbql-query checkins
+ {:source-query
+ {:source-table $$checkins
+ :aggregation [[:sum $user_id] [:sum $venue_id]]
+ :breakout [!month.date]}
+ :filter [:> *sum/Float 300]
+ :limit 2}))))
diff --git a/test/metabase/test/data.clj b/test/metabase/test/data.clj
index 2c40fc0af60a1ba62cbd7aea5606748bf8c7268d..58a7e999ca6f364ebadf801080b001b93ce87d59 100644
--- a/test/metabase/test/data.clj
+++ b/test/metabase/test/data.clj
@@ -167,16 +167,17 @@
(defmacro run-mbql-query
"Like `mbql-query`, but runs the query as well."
{:style/indent 1}
- [table & [query]]
+ [table-name & [query]]
`(qp/process-query
- (mbql-query ~table ~(or query {}))))
+ (mbql-query ~table-name ~(or query {}))))
(defn format-name
"Format a SQL schema, table, or field identifier in the correct way for the current database by calling the driver's
implementation of `format-name`. (Most databases use the default implementation of `identity`; H2 uses
`clojure.string/upper-case`.) This function DOES NOT quote the identifier."
- [nm]
- (tx/format-name (tx/driver) (name nm)))
+ [a-name]
+ {:pre [((some-fn keyword? string? symbol?) a-name)]}
+ (tx/format-name (tx/driver) (name a-name)))
(defn id
"Get the ID of the current database or one of its Tables or Fields. Relies on the dynamic variable `*get-db*`, which
@@ -250,13 +251,6 @@
(defn id-field-type [] (tx/id-field-type (tx/driver)))
-(defn expected-base-type->actual
- "Return actual `base_type` that will be used for the given driver if we asked for BASE-TYPE. Mainly for Oracle
- because it doesn't have `INTEGER` types and uses decimals instead."
- [base-type]
- (tx/expected-base-type->actual (tx/driver) base-type))
-
-
;; The functions below are used so infrequently they hardly belong in this namespace.
(defn dataset-field-values
diff --git a/test/metabase/test/data/interface.clj b/test/metabase/test/data/interface.clj
index 07622ab36b8763035595c2a417b17416b4ffbd03..75e47a89c2adcd233c9539bea4b901f8091b0131 100644
--- a/test/metabase/test/data/interface.clj
+++ b/test/metabase/test/data/interface.clj
@@ -285,18 +285,6 @@
:hierarchy #'driver/hierarchy)
-(defmulti expected-base-type->actual
- "Return the base type type that is actually used to store Fields of `base-type`. The default implementation of this
- method is an identity fn. This is provided so DBs that don't support a given base type used in the test data can
- specifiy what type we should expect in the results instead. For example, Oracle has no `INTEGER` data types, so
- `:type/Integer` test values are instead stored as `NUMBER`, which we map to `:type/Decimal`."
- {:arglists '([driver base-type])}
- dispatch-on-driver-with-test-extensions
- :hierarchy #'driver/hierarchy)
-
-(defmethod expected-base-type->actual ::test-extensions [_ base-type] base-type)
-
-
(defmulti format-name
"Transform a lowercase string Table or Field name in a way appropriate for this dataset (e.g., `h2` would want to
upcase these names; `mongo` would want to use `\"_id\"` in place of `\"id\"`. This method should return a string.
diff --git a/test/metabase/timeseries_query_processor_test.clj b/test/metabase/timeseries_query_processor_test.clj
index 054e32033b19d68da8039728db5bca3f6cdb15d3..035559e275fbd480ed5d3a90dfa76bb54ef0ee58 100644
--- a/test/metabase/timeseries_query_processor_test.clj
+++ b/test/metabase/timeseries_query_processor_test.clj
@@ -1,9 +1,7 @@
(ns metabase.timeseries-query-processor-test
"Query processor tests for DBs that are event-based, like Druid.
There architecture is different enough that we can't test them along with our 'normal' DBs in `query-procesor-test`."
- (:require [metabase
- [query-processor-test :as qp.test :refer [rows]]
- [util :as u]]
+ (:require [metabase.query-processor-test :as qp.test]
[metabase.test.data :as data]
[metabase.timeseries-query-processor-test.util :as tqp.test]))
@@ -137,7 +135,7 @@
:rows [[1.992]]}
(->> (data/run-mbql-query checkins
{:aggregation [[:avg $venue_price]]})
- (qp.test/format-rows-by [(partial u/round-to-decimals 3)])
+ (qp.test/format-rows-by [3.0])
qp.test/rows+column-names))
;;; distinct count
@@ -917,21 +915,23 @@
(tqp.test/expect-with-timeseries-dbs
;; some sort of weird quirk w/ druid where all columns in breakout get converted to strings
[["1" 34.0071] ["2" 33.7701] ["3" 10.0646] ["4" 33.983]]
- (rows (data/run-mbql-query checkins
- {:aggregation [[:min $venue_latitude]]
- :breakout [$venue_price]})))
+ (qp.test/rows
+ (data/run-mbql-query checkins
+ {:aggregation [[:min $venue_latitude]]
+ :breakout [$venue_price]})))
(tqp.test/expect-with-timeseries-dbs
[["1" 37.8078] ["2" 40.7794] ["3" 40.7262] ["4" 40.7677]]
- (rows (data/run-mbql-query checkins
- {:aggregation [[:max $venue_latitude]]
- :breakout [$venue_price]})))
+ (qp.test/rows
+ (data/run-mbql-query checkins
+ {:aggregation [[:max $venue_latitude]]
+ :breakout [$venue_price]})))
;; Do we properly handle queries that have more than one of the same aggregation? (#4166)
(tqp.test/expect-with-timeseries-dbs
[[35643 1992]]
(qp.test/format-rows-by [int int]
- (rows
+ (qp.test/rows
(data/run-mbql-query checkins
{:aggregation [[:sum $venue_latitude] [:sum $venue_price]]}))))
@@ -941,8 +941,8 @@
["Chinese" 3.0]
["Wine Bar" 3.0]
["Japanese" 2.7]]
- (qp.test/format-rows-by [str (partial u/round-to-decimals 1)]
- (rows
+ (qp.test/format-rows-by [str 1.0]
+ (qp.test/rows
(data/run-mbql-query checkins
{:aggregation [[:avg $venue_price]]
:breakout [[:field-id $venue_category_name]]