diff --git a/.circleci/config.yml b/.circleci/config.yml
index a631e971e762d535ab66c6b49d5cf5299f970090..8e1fb11a0d403cc5e7854627998b28661e1170f1 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -341,13 +341,15 @@ jobs:
docker:
- image: circleci/clojure:lein-2.8.1-node-browsers
- image: metabase/presto-mb-ci
+ environment:
+ JAVA_TOOL_OPTIONS: "-Xmx2g"
steps:
- attach_workspace:
at: /home/circleci/
- restore_cache:
<<: *restore-be-deps-cache
- run:
- name: Wait for SparkSQL to be ready
+ name: Wait for Presto to be ready
command: >
/home/circleci/metabase/metabase/.circleci/skip-driver-tests.sh sparksql ||
while ! nc -z localhost 8080; do sleep 0.1; done
diff --git a/src/metabase/driver/bigquery.clj b/src/metabase/driver/bigquery.clj
index ba4b160034fd30dbccca1085aa837a779ca8de3c..165b7ce36b4830b530de8365c9c45eac0c80160a 100644
--- a/src/metabase/driver/bigquery.clj
+++ b/src/metabase/driver/bigquery.clj
@@ -246,7 +246,9 @@
:let [parser-fn (type->parser (.getType field))]]
(parser-fn *bigquery-timezone*)))
columns (for [column (table-schema->metabase-field-info schema)]
- (set/rename-keys column {:base-type :base_type}))]
+ (-> column
+ (set/rename-keys {:base-type :base_type})
+ (dissoc :database-type)))]
{:columns (map (comp u/keyword->qualified-name :name) columns)
:cols columns
:rows (for [^TableRow row (.getRows response)]
@@ -364,36 +366,19 @@
(str/replace #"(^\d)" "_$1"))]
(subs replaced-str 0 (min 128 (count replaced-str)))))
-(s/defn ^:private bg-aggregate-name :- su/NonBlankString
+(s/defn ^:private bq-aggregate-name :- su/NonBlankString
+ "Return an approriate name for an `ag-clause`."
[ag-clause :- mbql.s/Aggregation]
(-> ag-clause annotate/aggregation-name format-custom-field-name))
-;; TODO - I think we should move `pre-alias-aggregations` into `mbql.util`
-(defn- unique-name-fn
- "Return a function that used names with and returns a guaranteed unique name every time.
-
- (let [unique-name (#'bigquery/unique-name-fn)]
- [(unique-name \"count\")
- (unique-name \"count\")])
- ;; -> [\"count\", \"count_2\"]"
- []
- (let [aliases (atom {})]
- (fn [original-name]
- (let [total-count (get (swap! aliases update original-name #(if % (inc %) 1))
- original-name)]
- (if (= total-count 1)
- original-name
- (recur (str original-name \_ total-count)))))))
-
-(defn- pre-alias-aggregations
+(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."
- [outer-query]
- (let [unique-name (unique-name-fn)]
- (mbql.u/replace-in outer-query [:query :aggregation]
- [:named ag ag-name] [:named ag (unique-name ag-name)]
- [(_ :guard keyword?) & _] [:named &match (unique-name (bg-aggregate-name &match))])))
+ [{{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 bq-aggregate-name))))
;; These provide implementations of `->honeysql` that prevent HoneySQL from converting forms to prepared statement
;; parameters (`?` symbols)
diff --git a/src/metabase/driver/presto.clj b/src/metabase/driver/presto.clj
index 41766b118c7d1cadf974ae484000292e661177ea..2305f09abd1d71c112d3405e519f1cb37820a58d 100644
--- a/src/metabase/driver/presto.clj
+++ b/src/metabase/driver/presto.clj
@@ -149,14 +149,6 @@
(defn- quote+combine-names [& names]
(str/join \. (map quote-name names)))
-(defn- rename-duplicates [values]
- ;; Appends _2, _3 and so on to duplicated values
- (loop [acc [], [h & tail] values, seen {}]
- (let [value (if (seen h) (str h "_" (inc (seen h))) h)]
- (if tail
- (recur (conj acc value) tail (assoc seen h (inc (get seen h 0))))
- (conj acc value)))))
-
;;; IDriver implementation
(defn- can-connect? [{:keys [catalog] :as details}]
@@ -241,18 +233,28 @@
[_ [_ value]]
(hx/cast :time (time->str value (driver/report-timezone))))
-(defn- execute-query [{database-id :database, :keys [settings], {sql :query, params :params} :native, :as outer-query}]
+(defn- execute-query [{database-id :database
+ :keys [settings]
+ {sql :query, params :params} :native
+ query-type :type
+ :as outer-query}]
(let [sql (str "-- "
(qputil/query->remark outer-query) "\n"
(unprepare/unprepare (cons sql params) :quote-escape "'", :iso-8601-fn :from_iso8601_timestamp))
details (merge (db/select-one-field :details Database :id (u/get-id database-id))
settings)
{:keys [columns rows]} (execute-presto-query! details sql)
- columns (for [[col name] (map vector columns (rename-duplicates (map :name columns)))]
+ columns (for [[col name] (map vector columns (map :name columns))]
{:name name, :base_type (presto-type->base-type (:type col))})]
- {:cols columns
- :columns (map (comp u/keyword->qualified-name :name) columns)
- :rows rows}))
+ (merge
+ {:columns (map (comp u/keyword->qualified-name :name) columns)
+ :rows rows}
+ ;; only include `:cols` info for native queries for the time being, since it changes all the types up for MBQL
+ ;; queries (e.g. `:count` aggregations come back as `:type/BigInteger` instead of `:type/Integer`.) I don't want
+ ;; to deal with fixing a million tests to make it work at this second since it doesn't make a difference from an
+ ;; FE perspective. Perhaps when we get our test story sorted out a bit better we can fix this
+ (when (= query-type :native)
+ {:cols columns}))))
(defn- humanize-connection-error-message [message]
diff --git a/src/metabase/mbql/util.clj b/src/metabase/mbql/util.clj
index 37125e8b53693e55e13a026a8b1d1f7b4f68c054..d6fac37a89167abc013947a23bfb8bac474f7c12 100644
--- a/src/metabase/mbql/util.clj
+++ b/src/metabase/mbql/util.clj
@@ -390,3 +390,57 @@
[field]
(or (isa? (:base_type field) :type/DateTime)
(isa? (:special_type field) :type/DateTime)))
+
+
+;;; --------------------------------- Unique names & transforming ags to have names ----------------------------------
+
+(s/defn uniquify-names :- (s/constrained [s/Str] distinct? "sequence of unique strings")
+ "Make the names in a sequence of string names unique by adding suffixes such as `_2`.
+
+ (uniquify-names [\"count\" \"sum\" \"count\" \"count_2\"])
+ ;; -> [\"count\" \"sum\" \"count_2\" \"count_2_2\"]"
+ [names :- [s/Str]]
+ (let [aliases (atom {})
+ unique-name (fn [original-name]
+ (let [total-count (get (swap! aliases update original-name #(if % (inc %) 1))
+ original-name)]
+ (if (= total-count 1)
+ original-name
+ (recur (str original-name \_ total-count)))))]
+ (map unique-name names)))
+
+(def ^:private NamedAggregationsWithUniqueNames
+ (s/constrained [mbql.s/named] #(distinct? (map last %)) "sequence of named aggregations with unique names"))
+
+(s/defn uniquify-named-aggregations :- NamedAggregationsWithUniqueNames
+ "Make the names of a sequence of named aggregations unique by adding suffixes such as `_2`."
+ [named-aggregations :- [mbql.s/named]]
+ (map (fn [[_ ag] unique-name]
+ [:named ag unique-name])
+ named-aggregations
+ (uniquify-names (map last named-aggregations))))
+
+(s/defn pre-alias-aggregations :- [mbql.s/named]
+ "Wrap every aggregation clause in a `:named` clause, using the name returned by `(aggregation->name-fn ag-clause)`
+ as names for any clauses that are not already wrapped in `:name`.
+
+ (pre-alias-aggregations annotate/aggregation-name [[:count] [:count] [:named [:sum] \"Sum-41\"]])
+ ;; -> [[:named [:count] \"count\"]
+ [:named [:count] \"count\"]
+ [:named [:sum [:field-id 1]] \"Sum-41\"]]
+
+ Most often, `aggregation->name-fn` will be something like `annotate/aggregation-name`, but for purposes of keeping
+ the `metabase.mbql` module seperate from the `metabase.query-processor` code we'll let you pass that in yourself."
+ {:style/indent 1}
+ [aggregation->name-fn :- (s/pred fn?), aggregations :- [mbql.s/Aggregation]]
+ (replace aggregations
+ [:named ag ag-name] [:named ag ag-name]
+ [(_ :guard keyword?) & _] [:named &match (aggregation->name-fn &match)]))
+
+(s/defn pre-alias-and-uniquify-aggregations :- NamedAggregationsWithUniqueNames
+ "Wrap every aggregation clause in a `:named` clause with a unique name. Combines `pre-alias-aggregations` with
+ `uniquify-named-aggregations`."
+ {:style/indent 1}
+ [aggregation->name-fn :- (s/pred fn?), aggregations :- [mbql.s/Aggregation]]
+ (-> (pre-alias-aggregations aggregation->name-fn aggregations)
+ uniquify-named-aggregations))
diff --git a/src/metabase/query_processor/interface.clj b/src/metabase/query_processor/interface.clj
index 190862cb7c1970274840dc4ab51ceb647a456c93..4f0d078e279d641e74853c10d0bbddac6a628b4b 100644
--- a/src/metabase/query_processor/interface.clj
+++ b/src/metabase/query_processor/interface.clj
@@ -1,6 +1,9 @@
(ns metabase.query-processor.interface
"Dynamic variables, constants, and other things used across the query builder namespaces.")
+;; TODO - Not 100% sure we really need this namespace since it's almost completely empty these days. Seems like the
+;; things here could be moved elsewhere
+;; TODO - I think this could go in the `limit` namespace
(def absolute-max-results
"Maximum number of rows the QP should ever return.
@@ -8,12 +11,14 @@
https://support.office.com/en-nz/article/Excel-specifications-and-limits-1672b34d-7043-467e-8e27-269d656771c3"
1048576)
+;; TODO - maybe we should do this more generally with the help of a macro like `do-with-suppressed-output` from the
+;; test utils, perhaps implemented as separate middleware (and using a `:middleware` option). Or perhaps even make QP
+;; log level an option so you could do debug individual queries
(def ^:dynamic ^Boolean *disable-qp-logging*
"Should we disable logging for the QP? (e.g., during sync we probably want to turn it off to keep logs less
cluttered)."
false)
-
(def ^:dynamic *driver*
"The driver that will be used to run the query we are currently parsing.
Always bound when running queries the normal way, e.g. via `metabase.driver/process-query`.
diff --git a/src/metabase/query_processor/middleware/annotate.clj b/src/metabase/query_processor/middleware/annotate.clj
index 5c013ccdfbd9432bbe48da82a3d7b1f66ea09555..724dd03d9012aaca107b017ddd482cea0d5de1a2 100644
--- a/src/metabase/query_processor/middleware/annotate.clj
+++ b/src/metabase/query_processor/middleware/annotate.clj
@@ -17,6 +17,22 @@
[schema :as su]]
[schema.core :as s]))
+(def ^:private Col
+ "Schema for a valid map of column info as found in the `:cols` key of the results after this namespace has ran."
+ ;; name and display name can be blank because some wacko DBMSes like SQL Server return blank column names for
+ ;; unaliased aggregations like COUNT(*) (this only applies to native queries, since we determine our own names for
+ ;; MBQL.)
+ {:name s/Str
+ :display_name s/Str
+ ;; type of the Field. For Native queries we look at the values in the first 100 rows to make an educated guess
+ :base_type su/FieldType
+ (s/optional-key :special_type) (s/maybe su/FieldType)
+ ;; where this column came from in the original query.
+ :source (s/enum :aggregation :fields :breakout :native)
+ ;; various other stuff from the original Field can and should be included such as `:settings`
+ s/Any s/Any})
+
+
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Adding :cols info for native queries |
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -29,10 +45,12 @@
(vec (for [i (range (count columns))
:let [col (nth columns i)]]
{:name (name col)
- :display_name (humanization/name->human-readable-name (name col))
+ :display_name (or (humanization/name->human-readable-name (u/keyword->qualified-name col))
+ (u/keyword->qualified-name col))
:base_type (or (driver/values->base-type (for [row rows]
(nth row i)))
- :type/*)})))
+ :type/*)
+ :source :native})))
(defn- add-native-column-info
[{:keys [columns], :as results}]
@@ -235,20 +253,43 @@
(assoc results :cols cols)))
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | Deduplicating names |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(def ^:private ColsWithUniqueNames
+ (s/constrained [Col] #(distinct? (map :name %)) ":cols with unique names"))
+
+(s/defn ^:private deduplicate-cols-names :- ColsWithUniqueNames
+ [cols :- [Col]]
+ (map (fn [col unique-name]
+ (assoc col :name unique-name))
+ cols
+ (mbql.u/uniquify-names (map :name cols))))
+
+
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | GENERAL MIDDLEWARE |
;;; +----------------------------------------------------------------------------------------------------------------+
-(defn- add-column-info* [{query-type :type, :as query} {cols-returned-by-driver :cols, :as results}]
- (cond-> (if-not (= query-type :query)
- (add-native-column-info results)
- (add-mbql-column-info query results))
- ;; If the driver returned a `:cols` map with its results, which is completely optional, merge our `:cols` derived
- ;; from logic above with theirs. We'll prefer the values in theirs to ours. This is important for wacky drivers
- ;; like GA that use things like native metrics, which we have no information about.
- ;;
- ;; It's the responsibility of the driver to make sure the `:cols` are returned in the correct number and order.
- (seq cols-returned-by-driver) (update :cols #(map merge % cols-returned-by-driver))))
+(s/defn ^:private add-column-info* :- {:cols ColsWithUniqueNames, s/Keyword s/Any}
+ [{query-type :type, :as query} {cols-returned-by-driver :cols, :as results}]
+ (->
+ ;; add `:cols` info to the query, using the appropriate function based on query type
+ (if-not (= query-type :query)
+ (add-native-column-info results)
+ (add-mbql-column-info query results))
+ ;; If the driver returned a `:cols` map with its results, which is completely optional, merge our `:cols` derived
+ ;; from logic above with theirs. We'll prefer the values in theirs to ours. This is important for wacky drivers
+ ;; like GA that use things like native metrics, which we have no information about.
+ ;;
+ ;; It's the responsibility of the driver to make sure the `:cols` are returned in the correct number and order.
+ (update :cols (if (seq cols-returned-by-driver)
+ #(map merge % cols-returned-by-driver)
+ identity))
+ ;; Finally, make sure the `:name` of each map in `:cols` is unique, since the FE uses it as a key for stuff like
+ ;; column settings
+ (update :cols deduplicate-cols-names)))
(defn add-column-info
"Middleware for adding type information about the columns in the query results (the `:cols` key)."
diff --git a/src/metabase/query_processor/middleware/dev.clj b/src/metabase/query_processor/middleware/dev.clj
index b23da1d4f3f78dbf22d8ef351da6b5e235ec4f8a..0a1a244a22fb49cc969711a2bc714a2e7e0ef625 100644
--- a/src/metabase/query_processor/middleware/dev.clj
+++ b/src/metabase/query_processor/middleware/dev.clj
@@ -1,6 +1,6 @@
(ns metabase.query-processor.middleware.dev
- "Middleware that's only active in dev and test scenarios. These middleware functions do additional checks
- of query processor behavior that are undesirable in normal production use."
+ "Middleware that's only active in dev and test scenarios. These middleware functions do additional checks of query
+ processor behavior that are undesirable in normal production use."
(:require [metabase.config :as config]
[schema.core :as s]))
@@ -11,19 +11,21 @@
"Schema for the expected format of results returned by a query processor."
{:columns [(s/cond-pre s/Keyword s/Str)]
;; This is optional because QPs don't neccesarily have to add it themselves; annotate will take care of that
+ ;; If QPs do add it, those will be merged in with what annotate adds
+ ;;
+ ;; A more complete schema is used to check this in `annotate`
(s/optional-key :cols) [{s/Keyword s/Any}]
:rows s/Any
s/Keyword s/Any})
(def ^{:arglists '([results])} validate-results
- "Validate that the RESULTS of executing a query match the `QPResultsFormat` schema.
- Throws an `Exception` if they are not; returns RESULTS as-is if they are."
+ "Validate that the RESULTS of executing a query match the `QPResultsFormat` schema. Throws an `Exception` if they are
+ not; returns RESULTS as-is if they are."
(s/validator QPResultsFormat))
(def ^{:arglists '([qp])} check-results-format
- "Make sure the results of a QP execution are in the expected format.
- This takes place *after* the 'annotation' stage of post-processing.
- This check is skipped in prod to avoid wasting CPU cycles."
+ "Make sure the results of a QP execution are in the expected format. This takes place *after* the 'annotation' stage
+ of post-processing. This check is skipped in prod to avoid wasting CPU cycles."
(if config/is-prod?
identity
(fn [qp]
diff --git a/src/metabase/util/date.clj b/src/metabase/util/date.clj
index 4c8cda71f8b3346c8a70d39114fd368b58c952b6..b4503600bd2e163daea3ce153b5fec0d0986aa6c 100644
--- a/src/metabase/util/date.clj
+++ b/src/metabase/util/date.clj
@@ -61,13 +61,16 @@
(when (and (not report-timezone)
jvm-data-tz-conflict?)
(log/warn (str (trs "Possible timezone conflict found on database {0}." (:name db))
+ " "
(trs "JVM timezone is {0} and detected database timezone is {1}."
(.getID jvm-timezone) (.getID data-timezone))
+ " "
(trs "Configure a report timezone to ensure proper date and time conversions."))))
;; This database doesn't support a report timezone, check the JVM and data timezones, if they don't match,
;; warn the user
(when jvm-data-tz-conflict?
(log/warn (str (trs "Possible timezone conflict found on database {0}." (:name db))
+ " "
(trs "JVM timezone is {0} and detected database timezone is {1}."
(.getID jvm-timezone) (.getID data-timezone)))))))))
diff --git a/test/metabase/driver/bigquery_test.clj b/test/metabase/driver/bigquery_test.clj
index 742e8ec6ca9c61302d97859da9ee3e03da1dd364..1b711da80f5c73a7bed974cebe41821c34790491 100644
--- a/test/metabase/driver/bigquery_test.clj
+++ b/test/metabase/driver/bigquery_test.clj
@@ -51,9 +51,9 @@
;; ordering shouldn't apply (Issue #2821)
(expect-with-engine :bigquery
{:columns ["venue_id" "user_id" "checkins_id"],
- :cols [{:name "venue_id", :display_name "Venue ID", :base_type :type/Integer}
- {:name "user_id", :display_name "User ID", :base_type :type/Integer}
- {:name "checkins_id", :display_name "Checkins ID", :base_type :type/Integer}]}
+ :cols [{:name "venue_id", :display_name "Venue ID", :source :native, :base_type :type/Integer}
+ {:name "user_id", :display_name "User ID", :source :native, :base_type :type/Integer}
+ {:name "checkins_id", :display_name "Checkins ID", :source :native, :base_type :type/Integer}]}
(select-keys (:data (qp/process-query
{:native {:query (str "SELECT `test_data.checkins`.`venue_id` AS `venue_id`, "
@@ -77,23 +77,6 @@
["field-id" (data/id :checkins :venue_id)]]]
"User ID Plus Venue ID"]]}})))
-;; can we generate unique names?
-(expect
- ["count" "sum" "count_2" "count_3"]
- (let [unique-name (#'bigquery/unique-name-fn)]
- [(unique-name "count")
- (unique-name "sum")
- (unique-name "count")
- (unique-name "count")]))
-
-;; what if we try to trick it by using a name it would have generated?
-(expect
- ["count" "count_2" "count_2_2"]
- (let [unique-name (#'bigquery/unique-name-fn)]
- [(unique-name "count")
- (unique-name "count")
- (unique-name "count_2")]))
-
;; ok, make sure we actually wrap all of our ag clauses in `:named` clauses with unique names
(defn- aggregation-names [query]
(mbql.u/match (-> query :query :aggregation)
@@ -133,6 +116,13 @@
[: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
+ {}
+ (binding [qpi/*driver* (driver/engine->driver :bigquery)]
+ (#'bigquery/pre-alias-aggregations {})))
+
+
(expect-with-engine :bigquery
{:rows [[7929 7929]], :columns ["sum" "sum_2"]}
(qptest/rows+column-names
diff --git a/test/metabase/driver/druid_test.clj b/test/metabase/driver/druid_test.clj
index 5d49a47afaa6cc8ec4ab8ee5d1e91a38cc7e1f05..e3caff49df87a10e4362efb0b8dedabf4c90c395 100644
--- a/test/metabase/driver/druid_test.clj
+++ b/test/metabase/driver/druid_test.clj
@@ -98,12 +98,12 @@
:rows [["2013-01-03T08:00:00.000Z" "931" "Simcha Yan" "1" "Kinaree Thai Bistro" 1]
["2013-01-10T08:00:00.000Z" "285" "Kfir Caj" "2" "Ruen Pair Thai Restaurant" 1]]
:cols (mapv #(merge col-defaults %)
- [{:name "timestamp", :display_name "Timestamp"}
- {:name "id", :display_name "ID"}
- {:name "user_name", :display_name "User Name"}
- {:name "venue_price", :display_name "Venue Price"}
- {:name "venue_name", :display_name "Venue Name"}
- {:name "count", :display_name "Count", :base_type :type/Integer}])
+ [{:name "timestamp", :source :native, :display_name "Timestamp"}
+ {:name "id", :source :native, :display_name "ID"}
+ {:name "user_name", :source :native, :display_name "User Name"}
+ {:name "venue_price", :source :native, :display_name "Venue Price"}
+ {:name "venue_name", :source :native, :display_name "Venue Name"}
+ {:name "count", :source :native, :display_name "Count", :base_type :type/Integer}])
:native_form {:query native-query-1}}}
(-> (process-native-query native-query-1)
(m/dissoc-in [:data :insights])))
diff --git a/test/metabase/driver/generic_sql/native_test.clj b/test/metabase/driver/generic_sql/native_test.clj
index 3367bdacf6f235aeca3f8bb9e59704438c453fb6..eaca29833c69eb973d7f3fa25296f5d42872cf0a 100644
--- a/test/metabase/driver/generic_sql/native_test.clj
+++ b/test/metabase/driver/generic_sql/native_test.clj
@@ -13,7 +13,7 @@
:data {:rows [[100]
[99]]
:columns ["ID"]
- :cols [{:name "ID", :display_name "ID", :base_type :type/Integer}]
+ :cols [{:name "ID", :display_name "ID", :base_type :type/Integer, :source :native}]
:native_form {:query "SELECT ID FROM VENUES ORDER BY ID DESC LIMIT 2", :params []}}}
(-> (qp/process-query {:native {:query "SELECT ID FROM VENUES ORDER BY ID DESC LIMIT 2"}
:type :native
@@ -28,9 +28,9 @@
:data {:rows [[100 "Mohawk Bend" 46]
[99 "Golden Road Brewing" 10]]
:columns ["ID" "NAME" "CATEGORY_ID"]
- :cols [{:name "ID", :display_name "ID", :base_type :type/Integer}
- {:name "NAME", :display_name "Name", :base_type :type/Text}
- {:name "CATEGORY_ID", :display_name "Category ID", :base_type :type/Integer}]
+ :cols [{:name "ID", :display_name "ID", :source :native, :base_type :type/Integer}
+ {:name "NAME", :display_name "Name", :source :native, :base_type :type/Text}
+ {:name "CATEGORY_ID", :display_name "Category ID", :source :native, :base_type :type/Integer}]
:native_form {:query "SELECT ID, NAME, CATEGORY_ID FROM VENUES ORDER BY ID DESC LIMIT 2", :params []}}}
(-> (qp/process-query {:native {:query "SELECT ID, NAME, CATEGORY_ID FROM VENUES ORDER BY ID DESC LIMIT 2"}
:type :native
diff --git a/test/metabase/driver/googleanalytics_test.clj b/test/metabase/driver/googleanalytics_test.clj
index 2f8e1ce80ebccbd1205af38e3872d40b3b89171b..c89cc2bc754ad8093c814f34ff656ab5b461ad8b 100644
--- a/test/metabase/driver/googleanalytics_test.clj
+++ b/test/metabase/driver/googleanalytics_test.clj
@@ -1,6 +1,7 @@
(ns metabase.driver.googleanalytics-test
"Tests for the Google Analytics driver and query processor."
(:require [expectations :refer [expect]]
+ [medley.core :as m]
[metabase
[query-processor :as qp]
[util :as u]]
@@ -241,12 +242,15 @@
(do-with-some-fields
(fn [objects]
(let [results {:columns [:ga:eventLabel :ga:totalEvents]
+ :cols [{}, {:base_type :type/Text}]
:rows [["Toucan Sighting" 1000]]}
qp (#'metabase.query-processor/qp-pipeline (constantly results))
query (query-with-some-fields objects)]
(-> (tu/doall-recursive (qp query))
(update-in [:data :cols] #(for [col %]
- (dissoc col :table_id :id)))))))))
+ (dissoc col :table_id :id)))
+ (m/dissoc-in [:data :results_metadata])
+ (m/dissoc-in [:data :insights])))))))
;;; ------------------------------------------------ Saving GA Cards -------------------------------------------------
diff --git a/test/metabase/driver/mongo_test.clj b/test/metabase/driver/mongo_test.clj
index 6847f916a6b992d72a6092579be2cd2c81ba5674..d508242aca625b0e2e69dda39e6905d6e01e2e7e 100644
--- a/test/metabase/driver/mongo_test.clj
+++ b/test/metabase/driver/mongo_test.clj
@@ -77,7 +77,7 @@
:row_count 1
:data {:rows [[1]]
:columns ["count"]
- :cols [{:name "count", :display_name "Count", :base_type :type/Integer}]
+ :cols [{:name "count", :display_name "Count", :base_type :type/Integer, :source :native}]
:native_form {:collection "venues"
:query native-query}}}
(-> (qp/process-query {:native {:query native-query
diff --git a/test/metabase/driver/presto_test.clj b/test/metabase/driver/presto_test.clj
index e9d586a1f9a2fc74416e44cea31585b80a7fb890..0d8ad69132faa575675527efcce7249219d58b8d 100644
--- a/test/metabase/driver/presto_test.clj
+++ b/test/metabase/driver/presto_test.clj
@@ -71,10 +71,6 @@
"\"weird . \"\"schema\".\"weird.table\"\" name\""
(#'presto/quote+combine-names "weird . \"schema" "weird.table\" name"))
-(expect
- ["name" "count" "count_2" "sum", "sum_2", "sum_3"]
- (#'presto/rename-duplicates ["name" "count" "count" "sum" "sum" "sum"]))
-
;; DESCRIBE-DATABASE
(datasets/expect-with-engine :presto
{:tables #{{:name "categories" :schema "default"}
diff --git a/test/metabase/mbql/util_test.clj b/test/metabase/mbql/util_test.clj
index 23a6899ab2f071ba8b1f8b095091c1e64b598595..8000e0c6ef6b9df836aaa78803f504e014914afe 100644
--- a/test/metabase/mbql/util_test.clj
+++ b/test/metabase/mbql/util_test.clj
@@ -507,3 +507,106 @@
(expect
[:min [:field-id 1]]
(mbql.u/aggregation-at-index query-with-some-nesting 1 1))
+
+
+;;; --------------------------------- Unique names & transforming ags to have names ----------------------------------
+
+;; can we generate unique names?
+(expect
+ ["count" "sum" "count_2" "count_3"]
+ (mbql.u/uniquify-names ["count" "sum" "count" "count"]))
+
+(expect
+ [[:named [:count] "count"]
+ [:named [:sum [:field-id 1]] "sum"]
+ [:named [:count] "count_2"]
+ [:named [:count] "count_3"]]
+ (mbql.u/uniquify-named-aggregations [[:named [:count] "count"]
+ [:named [:sum [:field-id 1]] "sum"]
+ [:named [:count] "count"]
+ [:named [:count] "count"]]))
+
+;; what if we try to trick it by using a name it would have generated?
+(expect
+ ["count" "count_2" "count_2_2"]
+ (mbql.u/uniquify-names ["count" "count" "count_2"]))
+
+(expect
+ [[:named [:count] "count"]
+ [:named [:count] "count_2"]
+ [:named [:count] "count_2_2"]]
+ (mbql.u/uniquify-named-aggregations [[:named [:count] "count"]
+ [:named [:count] "count"]
+ [:named [:count] "count_2"]]))
+
+;; for wacky DBMSes like SQLServer that return blank column names sometimes let's make sure we handle those without
+;; exploding
+(expect
+ ["" "_2"]
+ (mbql.u/uniquify-names ["" ""]))
+
+;; can we wrap all of our aggregation clauses in `:named` clauses?
+(defn- simple-ag->name [[ag-name]]
+ (name ag-name))
+
+(expect
+ [[:named [:sum [:field-id 1]] "sum"]
+ [:named [:count [:field-id 1]] "count"]
+ [:named [:sum [:field-id 1]] "sum"]
+ [:named [:avg [:field-id 1]] "avg"]
+ [:named [:sum [:field-id 1]] "sum"]
+ [:named [:min [:field-id 1]] "min"]]
+ (mbql.u/pre-alias-aggregations simple-ag->name
+ [[:sum [:field-id 1]]
+ [:count [:field-id 1]]
+ [:sum [:field-id 1]]
+ [:avg [:field-id 1]]
+ [:sum [:field-id 1]]
+ [:min [:field-id 1]]]))
+
+;; we shouldn't change the name of ones that are already named
+(expect
+ [[:named [:sum [:field-id 1]] "sum"]
+ [:named [:count [:field-id 1]] "count"]
+ [:named [:sum [:field-id 1]] "sum"]
+ [:named [:avg [:field-id 1]] "avg"]
+ [:named [:sum [:field-id 1]] "sum_2"]
+ [:named [:min [:field-id 1]] "min"]]
+ (mbql.u/pre-alias-aggregations simple-ag->name
+ [[:sum [:field-id 1]]
+ [:count [:field-id 1]]
+ [:sum [:field-id 1]]
+ [:avg [:field-id 1]]
+ [:named [:sum [:field-id 1]] "sum_2"]
+ [:min [:field-id 1]]]))
+
+;; ok, can we do the same thing as the tests above but make those names *unique* at the same time?
+(expect
+ [[:named [:sum [:field-id 1]] "sum"]
+ [:named [:count [:field-id 1]] "count"]
+ [:named [:sum [:field-id 1]] "sum_2"]
+ [:named [:avg [:field-id 1]] "avg"]
+ [:named [:sum [:field-id 1]] "sum_3"]
+ [:named [:min [:field-id 1]] "min"]]
+ (mbql.u/pre-alias-and-uniquify-aggregations simple-ag->name
+ [[:sum [:field-id 1]]
+ [:count [:field-id 1]]
+ [:sum [:field-id 1]]
+ [:avg [:field-id 1]]
+ [:sum [:field-id 1]]
+ [:min [:field-id 1]]]))
+
+(expect
+ [[:named [:sum [:field-id 1]] "sum"]
+ [:named [:count [:field-id 1]] "count"]
+ [:named [:sum [:field-id 1]] "sum_2"]
+ [:named [:avg [:field-id 1]] "avg"]
+ [:named [:sum [:field-id 1]] "sum_2_2"]
+ [:named [:min [:field-id 1]] "min"]]
+ (mbql.u/pre-alias-and-uniquify-aggregations simple-ag->name
+ [[:sum [:field-id 1]]
+ [:count [:field-id 1]]
+ [:sum [:field-id 1]]
+ [:avg [:field-id 1]]
+ [:named [:sum [:field-id 1]] "sum_2"]
+ [:min [:field-id 1]]]))
diff --git a/test/metabase/query_processor/middleware/annotate_test.clj b/test/metabase/query_processor/middleware/annotate_test.clj
index fdee84e262de60cebd83d657f532f5d7beeaed48..c8828a5fe315c19fe6ad6012bb81d51bf1111d9d 100644
--- a/test/metabase/query_processor/middleware/annotate_test.clj
+++ b/test/metabase/query_processor/middleware/annotate_test.clj
@@ -15,8 +15,8 @@
;; make sure that `add-native-column-info` can still infer types even if the initial value(s) are `nil` (#4256)
(expect
- [{:name "a", :display_name "A", :base_type :type/Integer}
- {:name "b", :display_name "B", :base_type :type/Integer}]
+ [{:name "a", :display_name "A", :base_type :type/Integer, :source :native}
+ {:name "b", :display_name "B", :base_type :type/Integer, :source :native}]
(:cols (#'annotate/add-native-column-info {:columns [:a :b], :rows [[1 nil]
[2 nil]
[3 nil]
@@ -26,7 +26,7 @@
;; make sure that `add-native-column-info` defaults `base_type` to `type/*` if there are no non-nil
;; values when we peek.
(expect
- [{:name "a", :display_name "A", :base_type :type/*}]
+ [{:name "a", :display_name "A", :base_type :type/*, :source :native}]
(:cols (#'annotate/add-native-column-info {:columns [:a], :rows [[nil]]})))
@@ -197,3 +197,45 @@
:type :query
:query {:source-table (data/id :venues)
:aggregation [[:metric "ga:totalEvents"]]}})))
+
+;; Make sure columns always come back with a unique `:name` key (#8759)
+(expect
+ {:cols
+ [{:base_type :type/Number
+ :special_type :type/Number
+ :name "count"
+ :display_name "count"
+ :source :aggregation}
+ {:source :aggregation
+ :name "sum"
+ :display_name "sum"
+ :base_type :type/Number}
+ {:base_type :type/Number
+ :special_type :type/Number
+ :name "count_2"
+ :display_name "count"
+ :source :aggregation}
+ {:base_type :type/Number
+ :special_type :type/Number
+ :name "count_2_2"
+ :display_name "count_2"
+ :source :aggregation}]
+ :columns ["count" "sum" "count" "count_2"]}
+ (binding [qp.i/*driver* (H2Driver.)]
+ ((annotate/add-column-info (constantly {:cols [{:name "count"
+ :display_name "count"
+ :base_type :type/Number}
+ {:name "sum"
+ :display_name "sum"
+ :base_type :type/Number}
+ {:name "count"
+ :display_name "count"
+ :base_type :type/Number}
+ {:name "count_2"
+ :display_name "count_2"
+ :base_type :type/Number}]
+ :columns ["count" "sum" "count" "count_2"]}))
+ {:database (data/id)
+ :type :query
+ :query {:source-table (data/id :venues)
+ :aggregation [[:count] [:sum] [:count] [:named [:count] "count_2"]]}})))
diff --git a/test/metabase/query_processor_test/aggregation_test.clj b/test/metabase/query_processor_test/aggregation_test.clj
index 37341bf3beaf7f88d18f9a99d3bbd107cebff48b..45729eecb8b6ea97523274fd28096a9f7d6d403f 100644
--- a/test/metabase/query_processor_test/aggregation_test.clj
+++ b/test/metabase/query_processor_test/aggregation_test.clj
@@ -164,7 +164,7 @@
;; so we can use it with Mongo
(datasets/expect-with-engines (disj non-timeseries-engines :mongo)
[(aggregate-col :count)
- (aggregate-col :count)]
+ (assoc (aggregate-col :count) :name "count_2")]
(-> (data/run-mbql-query venues
{:aggregation [[:count] [:count]]})
:data :cols))
diff --git a/test/metabase/query_processor_test/remapping_test.clj b/test/metabase/query_processor_test/remapping_test.clj
index 6f5f4a78a99c7fdb5f3bb31c464431c18795346b..ec2810fd4cfa11d31a496abe6235bbb9294dde46 100644
--- a/test/metabase/query_processor_test/remapping_test.clj
+++ b/test/metabase/query_processor_test/remapping_test.clj
@@ -69,7 +69,7 @@
(assoc (categories-col :name)
:fk_field_id (data/id :venues :category_id)
:display_name "Foo"
- :name (data/format-name "name")
+ :name (data/format-name "name_2")
:remapped_from (data/format-name "category_id"))]
:native_form true}
(data/with-data
@@ -97,7 +97,7 @@
(assoc (categories-col :name)
:fk_field_id (data/id :venues :category_id)
:display_name "Foo"
- :name (data/format-name "name")
+ :name (data/format-name "name_2")
:remapped_from (data/format-name "category_id"))]
:native_form true}
(data/with-data
diff --git a/test/metabase/test/data.clj b/test/metabase/test/data.clj
index 7d0a53bd0e843a3591b73b3dc3a98587165ce708..52c120caffe81e9852694400ad6f76eed9fadaf0 100644
--- a/test/metabase/test/data.clj
+++ b/test/metabase/test/data.clj
@@ -190,7 +190,7 @@
(defn id
"Get the ID of the current database or one of its `Tables` or `Fields`.
- Relies on the dynamic variable `*get-db`, which can be rebound with `with-db`."
+ Relies on the dynamic variable `*get-db*`, which can be rebound with `with-db`."
([]
{:post [(integer? %)]}
(:id (db)))