Fix expressions with duplicate column names (#21208)

src/metabase/query_processor/util/add_alias_info.clj

@@ -196,21 +196,32 @@
 
 (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))]
+  (let [normalized    (normalize-fn field-clause)
+        all-exports   (exports source-query)
+        field-exports (filter (partial mbql.u/is-clause? :field)
+                              all-exports)]
     ;; first look for an EXACT match in the `exports`
     (or (some (fn [a-clause]
                 (when (= (normalize-fn a-clause) normalized)
                   a-clause))
-              exports)
+              field-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)))))
+                field-exports))
+        ;; look for a matching expression clause with the same name if still no match
+        (when-let [field-name (let [[_ id-or-name] field-clause]
+                                (when (string? id-or-name)
+                                  id-or-name))]
+          (when-let [expression-exports (not-empty (filter (partial mbql.u/is-clause? :expression) all-exports))]
+            (some
+             (fn [[_ expression-name :as expression-clause]]
+               (when (= expression-name field-name)
+                 expression-clause))
+             expression-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

test/metabase/query_processor/util/add_alias_info_test.clj

@@ -12,7 +12,7 @@
 
 (comment h2/keep-me)
 
-(deftest normalize-clause-test
+(deftest ^:parallel normalize-clause-test
   (is (= [:expression "wow"]
          (#'add/normalize-clause [:expression "wow"])
          (#'add/normalize-clause [:expression "wow" nil])
@@ -394,12 +394,8 @@
                                               "double_price"
                                               {:base-type          :type/Integer
                                                ::add/source-table  ::add/source
-                                               ;; TODO -- these don't agree with the source query (maybe they should
-                                               ;; both be prefixed by another `COOL.` I think) although I'm not sure it
-                                               ;; makes sense to try to assume this stuff either. Arguably the field
-                                               ;; clause should be `[:field "COOL.double_price" ...]` or something
-                                               ::add/source-alias  "double_price"
-                                               ::add/desired-alias "COOL.double_price"
+                                               ::add/source-alias  "COOL.double_price"
+                                               ::add/desired-alias "COOL.COOL.double_price"
                                                ::add/position      0}]]
                             {:aggregation [[:aggregation-options [:count] {:name               "COOL.count"
                                                                            ::add/position      1
@@ -606,3 +602,38 @@
                                                                    :temporal-unit      :month}]]}
                                    :alias        "Q2"}]})
                 [:field (mt/id :products :created_at) {:join-alias "Q2"}])))))))
+
+(deftest ^:parallel expression-from-source-query-alias-test
+  (testing "Make sure we use the exported alias from the source query for expressions (#21131)"
+    (let [source-query {:source-table 3
+                        :expressions  {"PRICE" [:+
+                                                [:field 2 {::add/source-table  3
+                                                           ::add/source-alias  "price"
+                                                           ::add/desired-alias "price"
+                                                           ::add/position      1}]
+                                                2]}
+                        :fields       [[:field 2 {::add/source-table  3
+                                                  ::add/source-alias  "price"
+                                                  ::add/desired-alias "price"
+                                                  ::add/position      1}]
+                                       [:expression "PRICE" {::add/desired-alias "PRICE_2"
+                                                             ::add/position      2}]]}]
+      (testing `add/exports
+        (is (= #{[:field 2 {::add/source-table  3
+                            ::add/source-alias  "price"
+                            ::add/desired-alias "price"
+                            ::add/position      1}]
+                 [:expression "PRICE" {::add/desired-alias "PRICE_2"
+                                       ::add/position      2}]}
+               (#'add/exports source-query))))
+      (testing `add/matching-field-in-source-query
+        (is (= [:expression "PRICE" {::add/desired-alias "PRICE_2"
+                                     ::add/position      2}]
+               (#'add/matching-field-in-source-query
+                {:source-query source-query}
+                [:field "PRICE" {:base-type :type/Float}]))))
+      (testing `add/field-alias-in-source-query
+        (is (= "PRICE_2"
+               (#'add/field-alias-in-source-query
+                {:source-query source-query}
+                [:field "PRICE" {:base-type :type/Float}])))))))

test/metabase/query_processor_test/expressions_test.clj

@@ -482,3 +482,20 @@
             (is (= [["Doohickey" 42 2 "Doohickey" 42 2]]
                    (mt/formatted-rows [str int int str int int]
                      (qp/process-query query))))))))))
+
+(deftest nested-expressions-with-existing-names-test
+  (testing "Expressions with the same name as existing columns should work correctly in nested queries (#21131)"
+    (mt/test-drivers (mt/normal-drivers-with-feature :nested-queries :expressions)
+      (mt/dataset sample-dataset
+        (doseq [expression-name ["PRICE" "price"]]
+          (testing (format "Expression name = %s" (pr-str expression-name))
+            (let [query (mt/mbql-query products
+                          {:source-query {:source-table $$products
+                                          :expressions  {expression-name [:+ $price 2]}
+                                          :fields       [$id $price [:expression expression-name]]
+                                          :order-by     [[:asc $id]]
+                                          :limit        2}})]
+              (mt/with-native-query-testing-context query
+                (is (= [[1 29.46 31.46] [2 70.08 72.08]]
+                       (mt/formatted-rows [int 2.0 2.0]
+                         (qp/process-query query))))))))))))