Skip to content
Snippets Groups Projects
Unverified Commit 05062555 authored by Braden Shepherdson's avatar Braden Shepherdson Committed by GitHub
Browse files

[QP] Fix confusion of expressions with the same name as columns (#39255) 

When determining column aliases in `add-alias-info`, reuse an existing
desired column alias if one is present.

Fixes #39059. Might fix #25931.
parent fb466de7
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 89 additions and 26 deletions
modules/drivers/sqlite/test/metabase/driver/sqlite_test.clj
+ 1
− 1
View file @ 05062555
......@@ -215,7 +215,7 @@
(testing "Make sure duplicate identifiers (even with different cases) get unique aliases"
(mt/test-driver :sqlite
(mt/dataset test-data
(is (= '{:select [source.CATEGORY_2 AS CATEGORY_2
(is (= '{:select [source.CATEGORY_2 AS CATEGORY
COUNT (*) AS count]
:from [{:select [products.category AS category
products.category || ? AS CATEGORY_2]
......
src/metabase/lib/expression.cljc
+ 15
− 13
View file @ 05062555
......@@ -24,7 +24,6 @@
[metabase.mbql.util :as mbql.u]
[metabase.shared.util.i18n :as i18n]
[metabase.types :as types]
[metabase.util :as u]
[metabase.util.malli :as mu]
[metabase.util.malli.registry :as mr]))
......@@ -190,11 +189,11 @@
[query stage-number [_coalesce _opts expr _null-expr]]
(lib.metadata.calculation/column-name query stage-number expr))
(defn- conflicting-name? [query stage-number expression-name]
(let [stage (lib.util/query-stage query stage-number)
cols (lib.metadata.calculation/visible-columns query stage-number stage)
expr-name (u/lower-case-en expression-name)]
(some #(-> % :name u/lower-case-en (= expr-name)) cols)))
#_(defn- conflicting-name? [query stage-number expression-name]
(let [stage (lib.util/query-stage query stage-number)
cols (lib.metadata.calculation/visible-columns query stage-number stage)
expr-name (u/lower-case-en expression-name)]
(some #(-> % :name u/lower-case-en (= expr-name)) cols)))
(defn- add-expression-to-stage
[stage expression]
......@@ -213,14 +212,17 @@
expression-name :- ::lib.schema.common/non-blank-string
expressionable]
(let [stage-number (or stage-number -1)]
(when (conflicting-name? query stage-number expression-name)
(throw (ex-info "Expression name conflicts with a column in the same query stage"
{:expression-name expression-name})))
;; TODO: This logic was removed as part of fixing #39059. We might want to bring it back for collisions with other
;; expressions in the same stage; probably not with tables or earlier stages. De-duplicating names is supported by
;; the QP code, and it should be powered by MLv2 in due course.
#_(when (conflicting-name? query stage-number expression-name)
(throw (ex-info "Expression name conflicts with a column in the same query stage"
{:expression-name expression-name})))
(lib.util/update-query-stage
query stage-number
add-expression-to-stage
(-> (lib.common/->op-arg expressionable)
(lib.util/top-level-expression-clause expression-name))))))
query stage-number
add-expression-to-stage
(-> (lib.common/->op-arg expressionable)
(lib.util/top-level-expression-clause expression-name))))))
(lib.common/defop + [x y & more])
(lib.common/defop - [x y & more])
......
src/metabase/query_processor/util/add_alias_info.clj
+ 17
− 3
View file @ 05062555
......@@ -238,9 +238,14 @@
(when-let [field-name (let [[_ id-or-name] field-clause]
(when (string? id-or-name)
id-or-name))]
(or (m/find-first (fn [[_ expression-name :as _expression-clause]]
(or ;; Expressions by exact name.
(m/find-first (fn [[_ expression-name :as _expression-clause]]
(= expression-name field-name))
(filter (partial mbql.u/is-clause? :expression) all-exports))
;; Expressions whose ::desired-alias matches the name we're searching for.
(m/find-first (fn [[_expression _expression-name {::keys [desired-alias]} :as _expression-clause]]
(= desired-alias field-name))
(filter (partial mbql.u/is-clause? :expression) all-exports))
(m/find-first (fn [[_ _ opts :as _aggregation-options-clause]]
(= (::source-alias opts) field-name))
(filter (partial mbql.u/is-clause? :aggregation-options) all-exports))))
......@@ -316,6 +321,11 @@
(qp.store/->legacy-metadata field)))
(:name field)))
(defn- field-requires-original-field-name
"JSON extraction fields need to be named with their outer `field-name`, not use any existing `::desired-alias`."
[field-clause]
(boolean (some-> field-clause field-instance :nfc-path)))
(defn- field-name
"*Actual* name of a `:field` from the database or source query (for Field literals)."
[_inner-query [_ id-or-name :as field-clause]]
......@@ -330,6 +340,7 @@
it around instead of recalculating it a bunch of times."
[inner-query field-clause]
{:field-name (field-name inner-query field-clause)
:override-alias? (field-requires-original-field-name field-clause)
:join-is-this-level? (field-is-from-join-in-this-level? inner-query field-clause)
:alias-from-join (field-alias-in-join-at-this-level inner-query field-clause)
:alias-from-source-query (field-alias-in-source-query inner-query field-clause)})
......@@ -360,10 +371,13 @@
"Determine the appropriate `::desired-alias` for a `field-clause`."
{:arglists '([inner-query field-clause expensive-field-info])}
[_inner-query
[_ _id-or-name {:keys [join-alias]} :as _field-clause]
{:keys [field-name alias-from-join alias-from-source-query]}]
[_ _id-or-name {:keys [join-alias] ::keys [desired-alias]} :as _field-clause]
{:keys [field-name alias-from-join alias-from-source-query override-alias?]}]
(cond
join-alias (prefix-field-alias join-alias (or alias-from-join field-name))
;; JSON fields and similar have to be aliased by the outer field name.
override-alias? field-name
desired-alias desired-alias
alias-from-source-query alias-from-source-query
:else field-name))
......
src/metabase/query_processor/util/nest_query.clj
+ 7
− 3
View file @ 05062555
......@@ -87,10 +87,14 @@
{base-type :base_type} (some-> expression-definition annotate/infer-expression-type)
{::add/keys [desired-alias]} (mbql.u/match-one source-query
[:expression (_ :guard (partial = expression-name)) source-opts]
source-opts)]
source-opts)
source-alias (or desired-alias expression-name)]
[:field
(or desired-alias expression-name)
(assoc opts :base-type (or base-type :type/*))]))
source-alias
(-> opts
(assoc :base-type (or base-type :type/*)
::add/source-table ::add/source
::add/source-alias source-alias))]))
(defn- rewrite-fields-and-expressions [query]
(mbql.u/replace query
......
test/metabase/driver/sql/query_processor_test.clj
+ 1
− 1
View file @ 05062555
......@@ -720,7 +720,7 @@
(deftest ^:parallel expression-with-duplicate-column-name-test
(testing "Can we use expression with same column name as table (#14267)"
(is (= '{:select [source.CATEGORY_2 AS CATEGORY_2
(is (= '{:select [source.CATEGORY_2 AS CATEGORY
COUNT (*) AS count]
:from [{:select [PRODUCTS.CATEGORY AS CATEGORY
CONCAT (PRODUCTS.CATEGORY ?) AS CATEGORY_2]
......
test/metabase/lib/expression_test.cljc
+ 4
− 1
View file @ 05062555
......@@ -251,7 +251,10 @@
(lib/expression "expr" (lib/absolute-datetime "2020" :month))
lib/expressions
(->> (map (fn [expr] (lib/display-info lib.tu/venues-query expr))))))))
(testing "collisions with other column names are detected and rejected"
;; TODO: This logic was removed as part of fixing #39059. We might want to bring it back for collisions with other
;; expressions in the same stage; probably not with tables or earlier stages. De-duplicating names is supported by the
;; QP code, and it should be powered by MLv2 in due course.
#_(testing "collisions with other column names are detected and rejected"
(let [query (lib/query meta/metadata-provider (meta/table-metadata :categories))
ex (try
(lib/expression query "ID" (meta/field-metadata :categories :name))
......
test/metabase/query_processor/util/add_alias_info_test.clj
+ 2
− 1
View file @ 05062555
......@@ -695,7 +695,8 @@
(is (= {:field-name "CREATED_AT"
:join-is-this-level? "Q2"
:alias-from-join "Products__CREATED_AT"
:alias-from-source-query nil}
:alias-from-source-query nil
:override-alias? false}
(#'add/expensive-field-info
(lib.tu.macros/$ids nil
{:source-table $$reviews
......
test/metabase/query_processor/util/nest_query_test.clj
+ 42
− 3
View file @ 05062555
......@@ -665,7 +665,7 @@
:breakout [[:field "CATEGORY_2" {:base-type :type/Text
::add/source-table ::add/source
::add/source-alias "CATEGORY_2"
::add/desired-alias "CATEGORY_2"
::add/desired-alias "CATEGORY"
::add/position 0}]]
:aggregation [[:aggregation-options [:count] {:name "count"
::add/desired-alias "count"
......@@ -673,7 +673,7 @@
:order-by [[:asc [:field "CATEGORY_2" {:base-type :type/Text
::add/source-table ::add/source
::add/source-alias "CATEGORY_2"
::add/desired-alias "CATEGORY_2"
::add/desired-alias "CATEGORY"
::add/position 0}]]]
:limit 1})
(-> (lib.tu.macros/mbql-query products
......@@ -685,4 +685,43 @@
qp.preprocess/preprocess
add/add-alias-info
:query
nest-query/nest-expressions)))))))
nest-query/nest-expressions)))
(testing "multi-stage query with an expression name that matches a table column (#39059)"
(is (=? (lib.tu.macros/$ids orders
{:source-query {:fields [[:field %id {}]
[:field %subtotal {}]
;; Then exported as DISCOUNT from the middle layer.
[:field "DISCOUNT_2" {:base-type :type/Float
::add/source-alias "DISCOUNT_2"
::add/desired-alias "DISCOUNT"}]]
:source-query {:expressions {"DISCOUNT" [:coalesce [:field %discount {}] 0]}
:fields [[:field %id {::add/desired-alias "ID"}]
[:field %subtotal {::add/desired-alias "SUBTOTAL"}]
[:field %discount {::add/desired-alias "DISCOUNT"}]
;; Exported as DISCOUNT_2 from this inner query.
[:expression "DISCOUNT"
{::add/desired-alias "DISCOUNT_2"}]]
:source-table $$orders}}
:source-query/model? true
:fields [[:field %id {}]
[:field %subtotal {}]
[:field "DISCOUNT" {:base-type :type/Float
::add/source-alias "DISCOUNT"
::add/desired-alias "DISCOUNT"}]]})
(-> (lib.tu.macros/$ids orders
{:type :query
:database (meta/id)
:query {:source-query {:expressions {"DISCOUNT" [:coalesce $discount 0]}
:fields [$id
$subtotal
[:expression "DISCOUNT"]]
:source-table $$orders}
:source-query/model? true
:fields [[:field "ID" {:base-type :type/Integer}]
[:field "SUBTOTAL" {:base-type :type/Float}]
[:field "DISCOUNT" {:base-type :type/Float}]]}})
qp.preprocess/preprocess
add/add-alias-info
:query
nest-query/nest-expressions))))))))
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