diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn
index 526d273b2f6db00c904839ab7967963938bef1be..a5df1d9182d589d2b84aa5bc2e6309d983eb9e9f 100644
--- a/.clj-kondo/config.edn
+++ b/.clj-kondo/config.edn
@@ -567,6 +567,7 @@
metabase.test.data/$ids hooks.metabase.test.data/$ids
metabase.test.data/dataset hooks.metabase.test.data/dataset
metabase.test.data/mbql-query hooks.metabase.test.data/mbql-query
+ metabase.test.data/mbql-query-no-test hooks.metabase.test.data/mbql-query
metabase.test.data/run-mbql-query hooks.metabase.test.data/mbql-query
metabase.test.util.async/with-open-channels hooks.common/let-with-optional-value-for-last-binding
metabase.test.util.log/with-log-level hooks.common/with-ignored-first-arg
@@ -580,6 +581,7 @@
metabase.test/dataset hooks.metabase.test.data/dataset
metabase.test/discard-setting-changes hooks.common/with-ignored-first-arg
metabase.test/mbql-query hooks.metabase.test.data/mbql-query
+ metabase.test/mbql-query-no-test hooks.metabase.test.data/mbql-query
metabase.test/query hooks.metabase.test.data/mbql-query
metabase.test/test-drivers hooks.common/do*
metabase.test/run-mbql-query hooks.metabase.test.data/mbql-query
diff --git a/src/metabase/lib/convert.cljc b/src/metabase/lib/convert.cljc
index 064f867450b15ae49380556a1823481d2db2f8b1..d92cfa8c7bb2d8e6cdc5679a2f26b6bf215f98b9 100644
--- a/src/metabase/lib/convert.cljc
+++ b/src/metabase/lib/convert.cljc
@@ -1,6 +1,7 @@
(ns metabase.lib.convert
(:require
[clojure.set :as set]
+ [medley.core :as m]
[metabase.lib.dispatch :as lib.dispatch]
[metabase.lib.options :as lib.options]
[metabase.lib.util :as lib.util]))
@@ -25,6 +26,9 @@
[query]
query)
+(def ^:private stage-keys
+ [:aggregation :breakout :expressions :fields :filter :order-by :joins])
+
(defmethod ->pMBQL :mbql.stage/mbql
[stage]
(reduce
@@ -33,7 +37,7 @@
stage
(update stage k ->pMBQL)))
stage
- [:aggregation :breakout :expressions :fields :filter :order-by :joins]))
+ stage-keys))
(defmethod ->pMBQL :mbql/join
[join]
@@ -83,9 +87,21 @@
{:arglists '([x])}
lib.dispatch/dispatch-value)
+(defn- lib-key? [x]
+ (and (qualified-keyword? x)
+ (= (namespace x) "lib")))
+
(defn- disqualify [x]
- #?(:cljs (when (number? x) (js-debugger)))
- (select-keys x (remove qualified-keyword? (keys x))))
+ (->> x
+ keys
+ (remove lib-key?)
+ (select-keys x)))
+
+(defn- aggregation->legacy-MBQL [[tag options & args]]
+ (let [inner (into [tag] (map ->legacy-MBQL args))]
+ (if-let [aggregation-opts (not-empty (disqualify options))]
+ [:aggregation-options inner aggregation-opts]
+ inner)))
(defn- clause-with-options->legacy-MBQL [[k options & args]]
(if (map? options)
@@ -96,22 +112,47 @@
(defmethod ->legacy-MBQL :default
[x]
- (if (and (vector? x)
- (keyword? (first x)))
- (clause-with-options->legacy-MBQL x)
- (do
- #?(:cljs (when-not (or (nil? x) (string? x) (number? x) (boolean? x) (keyword? x))
- (js/console.log "undefined ->legacy-MBQL for" (lib.dispatch/dispatch-value x) x)
- (throw (ex-info "undefined ->legacy-MBQL" {:dispatch-value (lib.dispatch/dispatch-value x)
- :value x}))))
- x)))
-
-(defn- chain-stages [x]
- (let [stages (map ->legacy-MBQL (:stages x))]
- (reduce (fn [inner stage]
- (assoc stage :source-query inner))
- (first stages)
- (rest stages))))
+ (cond
+ (and (vector? x)
+ (keyword? (first x))) (clause-with-options->legacy-MBQL x)
+ (map? x) (-> x
+ disqualify
+ (update-vals ->legacy-MBQL))
+ :else x))
+
+(doseq [clause [;; Aggregations
+ :count :avg :count-where :distinct
+ :max :median :min :percentile
+ :share :stddev :sum :sum-where
+
+ ;; Expressions
+ :+ :- :* :/
+ :case :coalesce
+ :abs :log :exp :sqrt :ceil :floor :round :power :interval
+ :relative-datetime :time :absolute-datetime :now :convert-timezone
+ :get-week :get-year :get-month :get-day :get-hour
+ :get-minute :get-second :get-quarter
+ :datetime-add :datetime-subtract
+ :concat :substring :replace :regexextract :length
+ :trim :ltrim :rtrim :upper :lower]]
+ (defmethod ->legacy-MBQL clause [input]
+ (aggregation->legacy-MBQL input)))
+
+(defn- chain-stages [{:keys [stages]}]
+ ;; :source-metadata aka :lib/stage-metadata is handled differently in the two formats.
+ ;; In legacy, an inner query might have both :source-query, and :source-metadata giving the metadata for that nested
+ ;; :source-query.
+ ;; In pMBQL, the :lib/stage-metadata is attached to the same stage it applies to.
+ ;; So when chaining pMBQL stages back into legacy form, if stage n has :lib/stage-metadata, stage n+1 needs
+ ;; :source-metadata attached.
+ (first (reduce (fn [[inner stage-metadata] stage]
+ [(cond-> (->legacy-MBQL stage)
+ inner (assoc :source-query inner)
+ stage-metadata (assoc :source-metadata (mapv ->legacy-MBQL (:columns stage-metadata))))
+ ;; Get the :lib/stage-metadata off the original pMBQL stage, not the converted one.
+ (:lib/stage-metadata stage)])
+ nil
+ stages)))
(defmethod ->legacy-MBQL :dispatch-type/map [m]
(-> m
@@ -139,37 +180,25 @@
(chain-stages base))))
(defmethod ->legacy-MBQL :mbql.stage/mbql [stage]
- (-> stage
- disqualify
- (update-vals ->legacy-MBQL)))
+ (reduce #(m/update-existing %1 %2 ->legacy-MBQL)
+ (-> stage
+ disqualify
+ (m/update-existing :aggregation #(mapv aggregation->legacy-MBQL %)))
+ (remove #{:aggregation} stage-keys)))
(defmethod ->legacy-MBQL :mbql.stage/native [stage]
(-> stage
disqualify
- (set/rename-keys {:native :query})
(update-vals ->legacy-MBQL)))
(defmethod ->legacy-MBQL :mbql/query [query]
(let [base (disqualify query)
inner-query (chain-stages base)
- query-type (if (-> query :stages first :lib/type (= :mbql.stage/native))
+ query-type (if (-> query :stages last :lib/type (= :mbql.stage/native))
:native
- :query)
- result
- (merge (-> base
- (dissoc :stages)
- (update-vals ->legacy-MBQL))
- {:type query-type
- query-type inner-query})]
- #?(:cljs (js/console.log "->legacy-MBQL on query" query result))
- result))
-
-;;; placeholder, feel free to delete @braden.
-(defmethod ->legacy-MBQL :count
- [[_tag opts field]]
- (let [clause (if field
- [:count (->legacy-MBQL field)]
- [:count])]
- (if-let [aggregation-options-opts (not-empty (select-keys opts [:name :display-name]))]
- [:aggregation-options clause aggregation-options-opts]
- clause)))
+ :query)]
+ (merge (-> base
+ (dissoc :stages)
+ (update-vals ->legacy-MBQL))
+ {:type query-type
+ query-type inner-query})))
diff --git a/src/metabase/lib/js.cljs b/src/metabase/lib/js.cljs
index 6eea37e4fc2d92401c2e2e69de0e2adae658569f..3598fd6689f47d4e6ccd6f5c915ed224664e4626 100644
--- a/src/metabase/lib/js.cljs
+++ b/src/metabase/lib/js.cljs
@@ -85,12 +85,10 @@
As an example of such a value, `(get-in card [:template-tags \"some-tag\" :widget-type])` can be `:date/all-options`."
[x]
(cond
- (keyword? x) (if-let [ns-part (namespace x)]
- (str ns-part "/" (name x))
- (name x))
- (map? x) (update-vals x fix-namespaced-values)
- (sequential? x) (map fix-namespaced-values x)
- :else x))
+ (qualified-keyword? x) (str (namespace x) "/" (name x))
+ (map? x) (update-vals x fix-namespaced-values)
+ (sequential? x) (map fix-namespaced-values x)
+ :else x))
(defn ^:export legacy-query
"Coerce a CLJS pMBQL query back to (1) a legacy query (2) in vanilla JS."
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index 248818f1db9c67e8586355729c186ceed34a4cbf..a75e36252c953e0c1e40d0c83076c022977f78e9 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -2347,9 +2347,10 @@
;; card with agggregation and binning columns
[Card [{mbql-card-id :id}
(merge (mt/card-with-source-metadata-for-query
- (mt/mbql-query venues {:limit 5
- :aggregation [:count]
- :breakout [[:field %latitude {:binning {:strategy :num-bins :num-bins 10}}]]}))
+ (mt/mbql-query-no-test venues
+ {:limit 5
+ :aggregation [:count]
+ :breakout [[:field %latitude {:binning {:strategy :num-bins :num-bins 10}}]]}))
{:name "MBQL question"
:database_id (mt/id)
:table_id (mt/id :venues)})]
diff --git a/test/metabase/lib/convert_test.cljc b/test/metabase/lib/convert_test.cljc
index 281e9d4e1b211697cadbe20329c662d1cec3fcac..1671e91b7ed3b64ba15a9b671e4ae50280017ac4 100644
--- a/test/metabase/lib/convert_test.cljc
+++ b/test/metabase/lib/convert_test.cljc
@@ -107,3 +107,38 @@
:aggregation [[:aggregation-options
[:sum [:field 1 nil]]
{:display-name "Revenue"}]]}}))))
+
+(deftest ^:parallel round-trip-test
+ ;; Miscellaneous queries that have caused test failures in the past, captured here for quick feedback.
+ (are [query] (= query (-> query lib.convert/->pMBQL lib.convert/->legacy-MBQL))
+ ;; :aggregation-options on a non-aggregate expression with an inner aggregate.
+ {:database 194
+ :query {:aggregation [[:aggregation-options
+ [:- [:sum [:field 1677 nil]] 41]
+ {:name "Sum-41"}]]
+ :breakout [[:field 1677 nil]]
+ :source-table 517}
+ :type :query}
+
+ ;; :aggregation-options nested, not at the top level under :aggregation
+ {:database 194
+ :query {:aggregation [[:- [:aggregation-options
+ [:sum [:field 1677 nil]]
+ {:name "Sum-41"}] 41]]
+ :breakout [[:field 1677 nil]]
+ :source-table 517}
+ :type :query}
+
+ {:database 67
+ :query {:aggregation [[:aggregation-options
+ [:avg
+ [:field
+ 809
+ {:metabase.query-processor.util.add-alias-info/source-alias "RATING"
+ :metabase.query-processor.util.add-alias-info/source-table 224}]]
+ {:name "avg"
+ :metabase.query-processor.util.add-alias-info/desired-alias "avg"
+ :metabase.query-processor.util.add-alias-info/position 1
+ :metabase.query-processor.util.add-alias-info/source-alias "avg"}]]
+ :source-table 224}
+ :type :query}))
diff --git a/test/metabase/query_processor_test/expressions_test.clj b/test/metabase/query_processor_test/expressions_test.clj
index 0071a90db61ee14769a1c15f2c0dede1c5e8f65a..5ff4f4c2901e05382be91fd260cb7fd4b8f9635b 100644
--- a/test/metabase/query_processor_test/expressions_test.clj
+++ b/test/metabase/query_processor_test/expressions_test.clj
@@ -408,7 +408,7 @@
(mt/test-drivers (mt/normal-drivers-with-feature :expressions)
(testing "Can we use expression with same column name as table (#14267)"
(mt/dataset sample-dataset
- (let [query (mt/mbql-query products
+ (let [query (mt/mbql-query-no-test products
{:expressions {:CATEGORY [:concat $category "2"]}
:breakout [:expression :CATEGORY]
:aggregation [:count]
diff --git a/test/metabase/test.clj b/test/metabase/test.clj
index 778778679f3ba68b3f386c7293162aeadbf4cf68..395fa102fcd39c6c390b839687adba31809bac54 100644
--- a/test/metabase/test.clj
+++ b/test/metabase/test.clj
@@ -107,6 +107,7 @@
format-name
id
mbql-query
+ mbql-query-no-test
native-query
query
run-mbql-query
diff --git a/test/metabase/test/data.clj b/test/metabase/test/data.clj
index b78e6f306ba366bb10e756d5d6601bece6d20f1c..294f54381d0361202a020acd909f0d2e1972ab42 100644
--- a/test/metabase/test/data.clj
+++ b/test/metabase/test/data.clj
@@ -40,6 +40,8 @@
[metabase.db :as mdb]
[metabase.db.schema-migrations-test.impl :as schema-migrations-test.impl]
[metabase.driver.ddl.interface :as ddl.i]
+ [metabase.lib.convert :as lib.convert]
+ [metabase.mbql.normalize :refer [normalize]]
[metabase.models.permissions-group :as perms-group]
[metabase.query-processor :as qp]
[metabase.test.data.impl :as data.impl]
@@ -118,6 +120,22 @@
([table-name & body]
(mbql-query-impl/parse-tokens table-name `(do ~@body))))
+(defmacro mbql-query-no-test
+ "Macro for easily building MBQL queries for test purposes.
+
+ See [[mbql-query]] for the interface."
+ {:style/indent 1}
+ ([table-name]
+ `(mbql-query-no-test ~table-name {}))
+
+ ([table-name inner-query]
+ {:pre [(map? inner-query)]}
+ (as-> inner-query <>
+ (mbql-query-impl/parse-tokens table-name <>)
+ (mbql-query-impl/maybe-add-source-table <> table-name)
+ (mbql-query-impl/wrap-inner-query <>)
+ (vary-meta <> assoc :type :mbql))))
+
(defmacro mbql-query
"Macro for easily building MBQL queries for test purposes.
@@ -141,11 +159,10 @@
([table-name inner-query]
{:pre [(map? inner-query)]}
- (as-> inner-query <>
- (mbql-query-impl/parse-tokens table-name <>)
- (mbql-query-impl/maybe-add-source-table <> table-name)
- (mbql-query-impl/wrap-inner-query <>)
- (vary-meta <> assoc :type :mbql))))
+ `(let [query# (mbql-query-no-test ~table-name ~inner-query)]
+ (t/is (= (normalize query#) (-> query# normalize lib.convert/->pMBQL lib.convert/->legacy-MBQL normalize))
+ "Legacy MBQL queries should round trip to pMBQL and back")
+ query#)))
(defmacro query
"Like `mbql-query`, but operates on an entire 'outer' query rather than the 'inner' MBQL query. Like `mbql-query`,
@@ -184,7 +201,7 @@
"Like `mbql-query`, but runs the query as well."
{:style/indent 1}
[table-name & [query]]
- `(run-mbql-query* (mbql-query ~table-name ~(or query {}))))
+ `(run-mbql-query* (mbql-query-no-test ~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 current