Skip to content

Commit

Permalink
[QP] Fix confusion of expressions with the same name as columns (#39255)
Browse files Browse the repository at this point in the history 
When determining column aliases in `add-alias-info`, reuse an existing
desired column alias if one is present.

Fixes #39059. Might fix #25931.
  • Loading branch information
@bshepherdson
bshepherdson committed Mar 4, 2024
1 parent 4e82659 commit a60cd5e
Show file tree
Hide file tree
Showing 8 changed files with 89 additions and 26 deletions.
2 changes: 1 addition & 1 deletion modules/drivers/sqlite/test/metabase/driver/sqlite_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
28 changes: 15 additions & 13 deletions src/metabase/lib/expression.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
[metabase.lib.util :as lib.util]
[metabase.shared.util.i18n :as i18n]
[metabase.types :as types]
[metabase.util :as u]
[metabase.util.malli :as mu]))

(mu/defn column-metadata->expression-ref :- :mbql.clause/expression
Expand Down Expand Up @@ -186,11 +185,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]
Expand All @@ -209,14 +208,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])
Expand Down
20 changes: 17 additions & 3 deletions src/metabase/query_processor/util/add_alias_info.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))))
Expand Down Expand Up @@ -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]]
Expand All @@ -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)})
Expand Down Expand Up @@ -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))

Expand Down
10 changes: 7 additions & 3 deletions src/metabase/query_processor/util/nest_query.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,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
Expand Down
2 changes: 1 addition & 1 deletion test/metabase/driver/sql/query_processor_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -719,7 +719,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]
Expand Down
5 changes: 4 additions & 1 deletion test/metabase/lib/expression_test.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,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))
Expand Down
3 changes: 2 additions & 1 deletion test/metabase/query_processor/util/add_alias_info_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -692,7 +692,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
Expand Down
45 changes: 42 additions & 3 deletions test/metabase/query_processor/util/nest_query_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -664,15 +664,15 @@
: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"
::add/position 1}]]
: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
Expand All @@ -684,4 +684,43 @@
qp/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 comments on commit a60cd5e

Please sign in to comment.