diff --git a/dev/src/dev/debug_qp.clj b/dev/src/dev/debug_qp.clj
index 6e446eb790ded31f59d1bfb09afaac7fa7e20af9..5294c796fa13db1723af2e3e6e0dc81428e5d153 100644
--- a/dev/src/dev/debug_qp.clj
+++ b/dev/src/dev/debug_qp.clj
@@ -14,6 +14,85 @@
[metabase.util :as u]
[toucan.db :as db]))
+;;;; [[->sorted-mbql-query-map]]
+
+(def ^:private mbql-clause->sort-order
+ (into {}
+ (map-indexed (fn [i k]
+ [k i]))
+ [;; top-level keys
+ :database
+ :type
+ :query
+ :native
+ ;; inner-query and join keys
+ :source-table
+ :source-query
+ :source-metadata
+ :alias
+ :joins
+ :expressions
+ :breakout
+ :aggregation
+ :condition
+ :fields
+ :strategy
+ :filter
+ :order-by
+ :page
+ :limit]))
+
+(defn- sorted-mbql-query-map []
+ ;; stuff in [[mbql-clause->sort-order]] should always get sorted according to that order. Everything else should go at
+ ;; the end, with non-namespaced stuff first and namespaced stuff last; otherwise sort alphabetically
+ (sorted-map-by (fn [x y]
+ (let [order (fn [k]
+ (or (mbql-clause->sort-order k)
+ (when (and (keyword? k) (namespace k))
+ Integer/MAX_VALUE)
+ (dec Integer/MAX_VALUE)))
+ x-order (order x)
+ y-order (order y)]
+ (if (= x-order y-order)
+ (compare (str x) (str y))
+ (compare x-order y-order))))))
+
+(def ^:dynamic *shorten-namespaced-keywords?*
+ "Whether to shorten something like `:metabase.query-processor.util.add-alias-info/source-table` to
+ `::add/source-table` if an alias exists for the keyword namespace in the current namespace ([[*ns*]])."
+ true)
+
+(defn- alias-for-namespace-in-*ns* [ns-symb]
+ (let [a-namespace (find-ns (symbol ns-symb))]
+ (some
+ (fn [[ns-alias aliased-namespace]]
+ (when (= aliased-namespace a-namespace)
+ ns-alias))
+ (ns-aliases *ns*))))
+
+(defn ->sorted-mbql-query-map
+ "Convert MBQL `query` to a special map type that keeps the keys sorted in the 'preferred' order (e.g. order roughly
+ matches that of SQL, i.e. things like source query and joins come before order by or limit), which is easier to look
+ at (maybe)."
+ [query]
+ (walk/postwalk
+ (fn [form]
+ (cond
+ (map? form)
+ (into (sorted-mbql-query-map) form)
+
+ (and *shorten-namespaced-keywords?*
+ (keyword? form)
+ (namespace form))
+ (if-let [ns-alias (alias-for-namespace-in-*ns* (symbol (namespace form)))]
+ (symbol (format "::%s/%s" ns-alias (name form)))
+ form)
+
+ :else
+ form))
+ query))
+
+
;;;; [[add-names]]
(defn- field-and-table-name [field-id]
@@ -29,35 +108,36 @@
"Walk a MBQL snippet `x` and add comment forms with the names of the Fields referenced to any `:field` clauses nil
encountered. Helpful for debugging!"
[x]
- (walk/postwalk
- (fn add-names* [form]
- (letfn [(add-name-to-field-id [id]
- (when id
- (let [[field-name table-name] (field-and-table-name id)]
- (symbol (format "#_\"%s.%s\"" field-name table-name)))))
- (field-id->name-form [field-id]
- (list 'do (add-name-to-field-id field-id) field-id))]
- (mbql.u/replace form
- [:field (id :guard integer?) opts]
- [:field id (add-name-to-field-id id) (cond-> opts
- (integer? (:source-field opts))
- (update :source-field field-id->name-form))]
-
- (m :guard (every-pred map? (comp integer? :source-table)))
- (add-names* (update m :source-table add-table-id-name))
-
- (m :guard (every-pred map? (comp integer? :metabase.query-processor.util.add-alias-info/source-table)))
- (add-names* (update m :metabase.query-processor.util.add-alias-info/source-table add-table-id-name))
-
- (m :guard (every-pred map? (comp integer? :fk-field-id)))
- (-> m
- (update :fk-field-id field-id->name-form)
- add-names*)
-
- ;; don't recursively replace the `do` lists above, other we'll get vectors.
- (_ :guard (every-pred list? #(= (first %) 'do)))
- &match)))
- x))
+ (-> (walk/postwalk
+ (fn add-names* [form]
+ (letfn [(add-name-to-field-id [id]
+ (when id
+ (let [[field-name table-name] (field-and-table-name id)]
+ (symbol (format "#_\"%s.%s\"" field-name table-name)))))
+ (field-id->name-form [field-id]
+ (list 'do (add-name-to-field-id field-id) field-id))]
+ (mbql.u/replace form
+ [:field (id :guard integer?) opts]
+ [:field id (add-name-to-field-id id) (cond-> opts
+ (integer? (:source-field opts))
+ (update :source-field field-id->name-form))]
+
+ (m :guard (every-pred map? (comp integer? :source-table)))
+ (add-names* (update m :source-table add-table-id-name))
+
+ (m :guard (every-pred map? (comp integer? :metabase.query-processor.util.add-alias-info/source-table)))
+ (add-names* (update m :metabase.query-processor.util.add-alias-info/source-table add-table-id-name))
+
+ (m :guard (every-pred map? (comp integer? :fk-field-id)))
+ (-> m
+ (update :fk-field-id field-id->name-form)
+ add-names*)
+
+ ;; don't recursively replace the `do` lists above, other we'll get vectors.
+ (_ :guard (every-pred list? #(= (first %) 'do)))
+ &match)))
+ x)
+ ->sorted-mbql-query-map))
;;;; [[process-query-debug]]
@@ -387,43 +467,6 @@
(defn- no-$ [x]
(mbql.u/replace x [::$ & args] (into [::no-$] args)))
-(def ^:private mbql-clause->sort-order
- (into {}
- (map-indexed (fn [i k]
- [k i]))
- [;; top-level keys
- :database
- :type
- :query
- :native
- ;; inner-query keys
- :source-table
- :source-query
- :source-metadata
- :joins
- :expressions
- :breakout
- :aggregation
- :fields
- :filter
- :order-by
- :page
- :limit
- ;; join keys
- :alias
- :condition
- :strategy]))
-
-(defn- sorted-mbql-query-map []
- (sorted-map-by (fn [x y]
- (let [x-order (mbql-clause->sort-order x)
- y-order (mbql-clause->sort-order y)]
- (cond
- (and x-order y-order) (compare x-order y-order)
- x-order -1
- y-order 1
- :else (compare (pr-str x) (pr-str y)))))))
-
(defn- symbolize [form]
(mbql.u/replace form
[::-> x y]
@@ -444,7 +487,9 @@
(defn- query-table-name [{:keys [source-table source-query]}]
(cond
source-table
- (str/lower-case (db/select-one-field :name Table :id source-table))
+ (do
+ (assert (integer? source-table))
+ (str/lower-case (db/select-one-field :name Table :id source-table)))
source-query
(recur source-query)))
@@ -454,7 +499,7 @@
(to-mbql-shorthand query (query-table-name (:query query))))
([query table-name]
- (let [symbolized (-> query (expand table-name) symbolize)
+ (let [symbolized (-> query (expand table-name) symbolize ->sorted-mbql-query-map)
table-symb (some-> table-name symbol)]
(if (:query symbolized)
(list 'mt/mbql-query table-symb (-> (:query symbolized)
diff --git a/frontend/test/metabase/scenarios/question/reproductions/18512-cannot-join-two-saved-questions-with-same-implicit-explicit-grouped-field.cy.spec.js b/frontend/test/metabase/scenarios/question/reproductions/18512-cannot-join-two-saved-questions-with-same-implicit-explicit-grouped-field.cy.spec.js
index 1e8149a7ef14b6d013889a40254f3b9e4fb315dc..3159a2290da937ad5fa0cd667d49dc380fd1d1ab 100644
--- a/frontend/test/metabase/scenarios/question/reproductions/18512-cannot-join-two-saved-questions-with-same-implicit-explicit-grouped-field.cy.spec.js
+++ b/frontend/test/metabase/scenarios/question/reproductions/18512-cannot-join-two-saved-questions-with-same-implicit-explicit-grouped-field.cy.spec.js
@@ -6,7 +6,7 @@ const { PRODUCTS, PRODUCTS_ID, REVIEWS, REVIEWS_ID } = SAMPLE_DATABASE;
const question1 = getQuestionDetails("18512#1", "Doohickey");
const question2 = getQuestionDetails("18512#2", "Gizmo");
-describe.skip("issue 18512", () => {
+describe("issue 18512", () => {
beforeEach(() => {
cy.intercept("POST", "/api/dataset").as("dataset");
diff --git a/shared/src/metabase/mbql/schema.cljc b/shared/src/metabase/mbql/schema.cljc
index 08e5417e4846d6f5a2a1b3807cf3e797f51ccc34..2e274d7f3eaa052c4a377c5d32ad9c635a7c8514 100644
--- a/shared/src/metabase/mbql/schema.cljc
+++ b/shared/src/metabase/mbql/schema.cljc
@@ -75,15 +75,6 @@
(apply s/enum datetime-bucketing-units)
"datetime-bucketing-unit"))
-;; TODO -- rename to `TemporalUnit`
-(def ^{:deprecated "0.39.0"} DatetimeFieldUnit
- "Schema for all valid datetime bucketing units. DEPRECATED -- use `DateUnit`, `TimeUnit`, or
- `DateTimeUnit` instead."
- (s/named
- (apply s/enum #{:default :minute :minute-of-hour :hour :hour-of-day :day :day-of-week :day-of-month :day-of-year
- :week :week-of-year :month :month-of-year :quarter :quarter-of-year :year})
- "datetime-unit"))
-
(def ^:private RelativeDatetimeUnit
(s/named
(apply s/enum #{:default :minute :hour :day :week :month :quarter :year})
diff --git a/src/metabase/driver.clj b/src/metabase/driver.clj
index 626a2a74d3fe7587f2fccdd352e580e7e00f21c6..194574ee280793d158141927557c3d27bc514832 100644
--- a/src/metabase/driver.clj
+++ b/src/metabase/driver.clj
@@ -401,14 +401,14 @@
;; Does the driver support percentile calculations (including median)
:percentile-aggregations})
-(defmulti ^:deprecated supports?
+(defmulti supports?
"Does this driver support a certain `feature`? (A feature is a keyword, and can be any of the ones listed above in
[[driver-features]].)
(supports? :postgres :set-timezone) ; -> true
deprecated — [[database-supports?]] is intended to replace this method. However, it driver authors should continue _implementing_ `supports?` for the time being until we get a chance to migrate all our usages."
- {:arglists '([driver feature])}
+ {:arglists '([driver feature]), :deprecated "0.41.0"}
(fn [driver feature]
(when-not (driver-features feature)
(throw (Exception. (tru "Invalid driver feature: {0}" feature))))
@@ -437,7 +437,7 @@
whether a feature is supported for this particular database.
(database-supports? :mongo :set-timezone mongo-db) ; -> true"
- {:arglists '([driver feature database])}
+ {:arglists '([driver feature database]), :added "0.41.0"}
(fn [driver feature database]
(when-not (driver-features feature)
(throw (Exception. (tru "Invalid driver feature: {0}" feature))))
diff --git a/src/metabase/driver/sql/query_processor/deprecated.clj b/src/metabase/driver/sql/query_processor/deprecated.clj
index 21f7b3e03422bc82d5f1a9db6c8520e0d9b5ab62..bcc809bc2984f5937bca61f774ea16189fe92be9 100644
--- a/src/metabase/driver/sql/query_processor/deprecated.clj
+++ b/src/metabase/driver/sql/query_processor/deprecated.clj
@@ -41,14 +41,13 @@
already using it. This multimethod will be removed in a future release.
Drivers that need to access this information can look at the
- `::metabase.query-processor.util.add-alias-info/desired-alias` information in the
+ `:metabase.query-processor.util.add-alias-info/desired-alias` information in the
`:field`/`:expression`/`:aggregation` options map. See [[metabase.query-processor.util.add-alias]] for more
information.
- Drivers that need to customize the aliases used can override
- the [[metabase.query-processor.util.add-alias-info/escape-alias]] multimethod, or change the values of
- `::metabase.query-processor.util.add-alias-info/desired-alias` or
- `::metabase.query-processor.util.add-alias-info/source-alias` in the appropriate [[->honeysql]] methods."
+ Drivers that need to customize the aliases used can override the [[metabase.driver/escape-alias]] multimethod, or
+ change the values of `:metabase.query-processor.util.add-alias-info/desired-alias` or
+ `:metabase.query-processor.util.add-alias-info/source-alias` in the appropriate [[->honeysql]] methods."
{:arglists '([driver field]), :deprecated "0.41.0"}
driver/dispatch-on-initialized-driver
:hierarchy #'driver/hierarchy)
diff --git a/src/metabase/query_processor/util/add_alias_info.clj b/src/metabase/query_processor/util/add_alias_info.clj
index 7e9c2bfdfa8727475acb7d3e45fc3276d8488569..7cec4f4bae71ef186a80cc6d8ddc57981951039d 100644
--- a/src/metabase/query_processor/util/add_alias_info.clj
+++ b/src/metabase/query_processor/util/add_alias_info.clj
@@ -50,7 +50,7 @@
[metabase.mbql.util :as mbql.u]
[metabase.query-processor.error-type :as qp.error-type]
[metabase.query-processor.store :as qp.store]
- [metabase.util.i18n :refer [tru]]))
+ [metabase.util.i18n :refer [trs tru]]))
(defn prefix-field-alias
"Generate a field alias by applying `prefix` to `field-alias`. This is used for automatically-generated aliases for
@@ -100,7 +100,7 @@
[:field id-or-name opts]
;; this doesn't use [[mbql.u/update-field-options]] because this gets called a lot and the overhead actually adds up
;; a bit
- [:field id-or-name (remove-namespaced-options (dissoc opts :source-fields))]
+ [:field id-or-name (remove-namespaced-options (dissoc opts :source-field))]
;; for `:expression` and `:aggregation` references, remove the options map if they are empty.
[:expression expression-name opts]
@@ -168,13 +168,18 @@
(defn- field-source-table-alias
"Determine the appropriate `::source-table` alias for a `field-clause`."
{:arglists '([inner-query field-clause])}
- [{:keys [source-table], :as inner-query} [_ _id-or-name {:keys [join-alias]}, :as field-clause]]
+ [{:keys [source-table source-query], :as inner-query} [_ _id-or-name {:keys [join-alias]}, :as field-clause]]
(let [table-id (field-table-id field-clause)
join-is-this-level? (field-is-from-join-in-this-level? inner-query field-clause)]
(cond
join-is-this-level? join-alias
(and table-id (= table-id source-table)) table-id
- :else ::source)))
+ source-query ::source
+ :else
+ (throw (ex-info (trs "Cannot determine the source table or query for Field clause {0}" (pr-str field-clause))
+ {:type qp.error-type/invalid-query
+ :clause field-clause
+ :query inner-query})))))
(defn- exports [query]
(into #{} (mbql.u/match (dissoc query :source-query :source-metadata :joins)
@@ -186,19 +191,34 @@
join))
joins))
+(defn- matching-field-in-source-query* [source-query field-clause & {:keys [normalize-fn]
+ :or {normalize-fn normalize-clause}}]
+ (let [normalized (normalize-fn field-clause)
+ exports (filter (partial mbql.u/is-clause? :field)
+ (exports source-query))]
+ ;; first look for an EXACT match in the `exports`
+ (or (some (fn [a-clause]
+ (when (= (normalize-fn a-clause) normalized)
+ a-clause))
+ exports)
+ ;; if there is no EXACT match, attempt a 'fuzzy' match by disregarding the `:temporal-unit` and `:binning`
+ (let [fuzzify (fn [clause] (mbql.u/update-field-options clause dissoc :temporal-unit :binning))
+ fuzzy-normalized (fuzzify normalized)]
+ (some (fn [a-clause]
+ (when (= (fuzzify (normalize-fn a-clause)) fuzzy-normalized)
+ a-clause))
+ exports)))))
+
(defn- matching-field-in-join-at-this-level
"If `field-clause` is the result of a join *at this level* with a `:source-query`, return the 'source' `:field` clause
from that source query."
[inner-query [_ _ {:keys [join-alias]} :as field-clause]]
(when join-alias
(when-let [matching-join-source-query (:source-query (join-with-alias inner-query join-alias))]
- (let [normalized (mbql.u/update-field-options (normalize-clause field-clause) dissoc :join-alias)]
- (some (fn [a-clause]
- (when (and (mbql.u/is-clause? :field a-clause)
- (= (mbql.u/update-field-options (normalize-clause a-clause) dissoc :join-alias)
- normalized))
- a-clause))
- (exports matching-join-source-query))))))
+ (matching-field-in-source-query*
+ matching-join-source-query
+ field-clause
+ :normalize-fn #(mbql.u/update-field-options (normalize-clause %) dissoc :join-alias)))))
(defn- field-alias-in-join-at-this-level
"If `field-clause` is the result of a join at this level, return the `::desired-alias` from that join (where the Field is
@@ -208,15 +228,10 @@
desired-alias))
(defn- matching-field-in-source-query
- [{:keys [source-query], :as inner-query} [_ _ {:keys [join-alias]}, :as field-clause]]
- (when (= (field-source-table-alias inner-query field-clause) ::source)
- (let [normalized (normalize-clause field-clause)]
- (some (fn [a-clause]
- (when (and (mbql.u/is-clause? :field a-clause)
- (= (normalize-clause a-clause)
- normalized))
- a-clause))
- (exports source-query)))))
+ [{:keys [source-query], :as inner-query} field-clause]
+ (when (and source-query
+ (= (field-source-table-alias inner-query field-clause) ::source))
+ (matching-field-in-source-query* source-query field-clause)))
(defn- field-alias-in-source-query
[inner-query field-clause]
diff --git a/test/metabase/query_processor/util/add_alias_info_test.clj b/test/metabase/query_processor/util/add_alias_info_test.clj
index cbeb204b949a191222b81eb8b80862d9b7dd54bd..afa9fd1d34ace5e8e276d4d54727e3cba50d7b3c 100644
--- a/test/metabase/query_processor/util/add_alias_info_test.clj
+++ b/test/metabase/query_processor/util/add_alias_info_test.clj
@@ -328,37 +328,35 @@
:alias "Q2"
:strategy :left-join
:condition [:=
- [:field %products.category {::add/source-table ::add/source
- ::add/source-alias "CATEGORY"}]
[:field %products.category {:join-alias "Q2"
::add/source-table "Q2"
::add/source-alias "P2__CATEGORY"
::add/desired-alias "Q2__P2__CATEGORY"
- ::add/position 1}]]}]
- :fields [[:field "count" {:base-type :type/BigInteger
- ::add/source-table ::add/source
- ::add/source-alias "count"
- ::add/desired-alias "count"
- ::add/position 0}]
- [:field %products.category {:join-alias "Q2"
+ ::add/position 0}]
+ [:value 1 {:base_type :type/Text
+ :coercion_strategy nil
+ :database_type "VARCHAR"
+ :effective_type :type/Text
+ :name "CATEGORY"
+ :semantic_type :type/Category}]]}]
+ :fields [[:field %products.category {:join-alias "Q2"
::add/source-table "Q2"
::add/source-alias "P2__CATEGORY"
::add/desired-alias "Q2__P2__CATEGORY"
- ::add/position 1}]
+ ::add/position 0}]
[:field "avg" {:base-type :type/Integer
:join-alias "Q2"
::add/source-table "Q2"
::add/source-alias "avg"
::add/desired-alias "Q2__avg"
- ::add/position 2}]]
+ ::add/position 1}]]
:limit 2})
(add-alias-info
(mt/mbql-query orders
- {:fields [[:field "count" {:base-type :type/BigInteger}]
- &Q2.products.category
+ {:fields [&Q2.products.category
[:field "avg" {:base-type :type/Integer, :join-alias "Q2"}]]
:joins [{:strategy :left-join
- :condition [:= $products.category &Q2.products.category]
+ :condition [:= &Q2.products.category 1]
:alias "Q2"
:source-query {:source-table $$reviews
:aggregation [[:aggregation-options [:avg $reviews.rating] {:name "avg"}]]
@@ -488,3 +486,25 @@
:breakout [[:expression "count"]]
:aggregation [[:count]]
:limit 1})))))
+
+(deftest fuzzy-field-info-test
+ (testing "[[add/alias-from-join]] should match Fields in the Join source query even if they have temporal units"
+ (mt/dataset sample-dataset
+ (mt/with-everything-store
+ (is (= {:field-name "CREATED_AT"
+ :join-is-this-level? "Q2"
+ :alias-from-join "Products__CREATED_AT"
+ :alias-from-source-query nil}
+ (#'add/expensive-field-info
+ (mt/$ids nil
+ {:source-table $$reviews
+ :joins [{:source-query {:source-table $$reviews
+ :breakout [[:field %products.created_at
+ {::add/desired-alias "Products__CREATED_AT"
+ ::add/position 0
+ ::add/source-alias "CREATED_AT"
+ ::add/source-table "Products"
+ :join-alias "Products"
+ :temporal-unit :month}]]}
+ :alias "Q2"}]})
+ [:field (mt/id :products :created_at) {:join-alias "Q2"}])))))))
diff --git a/test/metabase/query_processor_test/explicit_joins_test.clj b/test/metabase/query_processor_test/explicit_joins_test.clj
index 97cd7e590646c76601c1634838a663592301a2ee..aa9f2859e61cf3e8860178f668ce76c2ef0b9757 100644
--- a/test/metabase/query_processor_test/explicit_joins_test.clj
+++ b/test/metabase/query_processor_test/explicit_joins_test.clj
@@ -632,6 +632,40 @@
(mt/formatted-rows [int int]
(qp/process-query query)))))))))))
+(deftest joining-nested-queries-with-same-aggregation-test
+ (mt/test-drivers (mt/normal-drivers-with-feature :nested-queries :left-join)
+ (testing (str "Should be able to join two nested queries with the same aggregation on a Field in their respective "
+ "source queries (#18512)")
+ (mt/dataset sample-dataset
+ (let [query (mt/mbql-query reviews
+ {:source-query {:source-table $$reviews
+ :joins [{:source-table $$products
+ :alias "Products"
+ :condition [:= $product_id &Products.products.id]
+ :fields :all}]
+ :breakout [!month.&Products.products.created_at]
+ :aggregation [[:distinct &Products.products.id]]
+ :filter [:= &Products.products.category "Doohickey"]}
+ :joins [{:source-query {:source-table $$reviews
+ :joins [{:source-table $$products
+ :alias "Products"
+ :condition [:= $product_id &Products.products.id]
+ :fields :all}]
+ :breakout [!month.&Products.products.created_at]
+ :aggregation [[:distinct &Products.products.id]]
+ :filter [:= &Products.products.category "Gizmo"]}
+ :alias "Q2"
+ :condition [:= !month.products.created_at !month.&Q2.products.created_at]
+ :fields :all}]
+ :order-by [[:asc !month.&Products.products.created_at]]
+ :limit 3})]
+ (mt/with-native-query-testing-context query
+ (is (= [["2016-05-01T00:00:00Z" 3 nil nil]
+ ["2016-06-01T00:00:00Z" 2 "2016-06-01T00:00:00Z" 1]
+ ["2016-08-01T00:00:00Z" 2 nil nil]]
+ (mt/formatted-rows [str int str int]
+ (qp/process-query query))))))))))
+
(deftest join-against-same-table-as-source-query-source-table-test
(testing "Joining against the same table as the source table of the source query should work (#18502)"
(mt/test-drivers (mt/normal-drivers-with-feature :nested-queries :left-join)