Skip to content
Snippets Groups Projects
Unverified Commit 33ac1862 authored by Cam Saul's avatar Cam Saul Committed by GitHub
Browse files

`add-alias-info`: recognize `:field` clauses in join source queries to be the... 

`add-alias-info`: recognize `:field` clauses in join source queries to be the same if it has a `:temporal-unit` (#19789)

* Backport Debug QP improvements from #19754

* Enable test

* "Fuzzy" matching when looking for Fields in join source queries

* Add/update tests

* Test fix :wrench:

* Remove trailing whitespace

* Fix a few docstrings
parent 97771d56
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 224 additions and 120 deletions
dev/src/dev/debug_qp.clj
+ 113
− 68
View file @ 33ac1862
......@@ -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)
......
frontend/test/metabase/scenarios/question/reproductions/18512-cannot-join-two-saved-questions-with-same-implicit-explicit-grouped-field.cy.spec.js
+ 1
− 1
View file @ 33ac1862
......@@ -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");
......
shared/src/metabase/mbql/schema.cljc
+ 0
− 9
View file @ 33ac1862
......@@ -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})
......
src/metabase/driver.clj
+ 3
− 3
View file @ 33ac1862
......@@ -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))))
......
src/metabase/driver/sql/query_processor/deprecated.clj
+ 4
− 5
View file @ 33ac1862
......@@ -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)
......
src/metabase/query_processor/util/add_alias_info.clj
+ 35
− 20
View file @ 33ac1862
......@@ -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]
......
test/metabase/query_processor/util/add_alias_info_test.clj
+ 34
− 14
View file @ 33ac1862
......@@ -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"}])))))))
test/metabase/query_processor_test/explicit_joins_test.clj
+ 34
− 0
View file @ 33ac1862
......@@ -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)
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment