diff --git a/modules/drivers/redshift/test/metabase/driver/redshift_test.clj b/modules/drivers/redshift/test/metabase/driver/redshift_test.clj
index ccd9a35906f5c6113d273868040c5b8057393be1..7e30896db12e243e24dd852d8e586bdd86b2121c 100644
--- a/modules/drivers/redshift/test/metabase/driver/redshift_test.clj
+++ b/modules/drivers/redshift/test/metabase/driver/redshift_test.clj
@@ -49,7 +49,7 @@
(deftest ^:parallel default-select-test
(is (= ["SELECT \"source\".* FROM (SELECT *) AS \"source\""]
- (->> {:from [[[::sql.qp/sql-source-query "SELECT *"]
+ (->> {:from [[{::sql.qp/sql-source-query ["SELECT *"]}
[(h2x/identifier :table-alias "source")]]]}
(#'sql.qp/add-default-select :redshift)
(sql.qp/format-honeysql :redshift)))))
diff --git a/src/metabase/driver/sql/query_processor.clj b/src/metabase/driver/sql/query_processor.clj
index f16277f4ddf66b6dd7848b44c703c61d29f47d93..6c601150dbe30fe098a67fb18bb453449dbeff13 100644
--- a/src/metabase/driver/sql/query_processor.clj
+++ b/src/metabase/driver/sql/query_processor.clj
@@ -38,6 +38,18 @@
"The INNER query currently being processed, for situations where we need to refer back to it."
nil)
+(defn make-nestable-sql*
+ "See [[make-nestable-sql]] but does not wrap in result in parens."
+ [sql]
+ (-> sql
+ (str/replace #";([\s;]*(--.*\n?)*)*$" "")
+ str/trimr
+ (as-> trimmed
+ ;; Query could potentially end with a comment.
+ (if (re-find #"--.*$" trimmed)
+ (str trimmed "\n")
+ trimmed))))
+
(defn make-nestable-sql
"Do best effort edit to the `sql`, to make it nestable in subselect.
@@ -53,23 +65,13 @@
probably require _parsing_ sql string and not just a regular expression replacement. Link to the discussion:
https://github.com/metabase/metabase/pull/30677
- For the limitations see the [[metabase.driver.sql.query-processor-test/make-nestable-sql-test]]"
- [sql]
- (str "("
- (-> sql
- (str/replace #";([\s;]*(--.*\n?)*)*$" "")
- str/trimr
- (as-> trimmed
- ;; Query could potentially end with a comment.
- (if (re-find #"--.*$" trimmed)
- (str trimmed "\n")
- trimmed)))
- ")"))
-
-(defn- format-sql-source-query [_fn [sql params]]
- (into [(make-nestable-sql sql)] params))
-
-(sql/register-fn! ::sql-source-query #'format-sql-source-query)
+ For the limitations see the [[metabase.driver.sql.query-processor-test/make-nestable-sql-test]]" [sql]
+ (str "(" (make-nestable-sql* sql) ")"))
+
+(defn- format-sql-source-query [_clause [sql params]]
+ (into [(make-nestable-sql* sql)] params))
+
+(sql/register-clause! ::sql-source-query #'format-sql-source-query :select)
(defn sql-source-query
"Wrap clause in `::sql-source-query`. Does additional validation."
@@ -84,7 +86,7 @@
(.getCanonicalName (class params)))
{:type qp.error-type/invalid-query
:query params})))
- [::sql-source-query sql params])
+ {::sql-source-query [sql params]})
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Interface (Multimethods) |
@@ -1409,23 +1411,43 @@
(declare apply-clauses)
(defn- apply-source-query
- "Handle a `:source-query` clause by adding a recursive `SELECT` or native query. At the time of this writing, all
- source queries are aliased as `source`."
- [driver honeysql-form {{:keys [native params],
- persisted :persisted-info/native
- :as source-query} :source-query}]
- (assoc honeysql-form
- :from [[(cond
- persisted
- (sql-source-query persisted nil)
-
- native
- (sql-source-query native params)
-
- :else
- (apply-clauses driver {} source-query))
- (let [table-alias (->honeysql driver (h2x/identifier :table-alias source-query-alias))]
- [table-alias])]]))
+ "Handle a `:source-query` clause by adding a recursive `SELECT` or native query.
+ If the source query has ambiguous column names, use a `WITH` statement to rename the source columns.
+ At the time of this writing, all source queries are aliased as `source`."
+ [driver honeysql-form {{:keys [native params] persisted :persisted-info/native :as source-query} :source-query
+ source-metadata :source-metadata}]
+ (let [table-alias (->honeysql driver (h2x/identifier :table-alias source-query-alias))
+ source-clause (cond
+ persisted
+ (sql-source-query persisted nil)
+
+ native
+ (sql-source-query native params)
+
+ :else
+ (apply-clauses driver {} source-query))
+ ;; TODO: Use MLv2 here to get source and desired-aliases
+ alias-info (mapv (fn [{[_ desired-ref-name] :field_ref source-name :name}]
+ [source-name desired-ref-name])
+ source-metadata)
+ source-aliases (mapv first alias-info)
+ desired-aliases (mapv second alias-info)
+ duplicate-source-aliases? (and (> (count source-aliases) 1)
+ (not (apply distinct? source-aliases)))
+ needs-columns? (and (seq desired-aliases)
+ (> (count desired-aliases) 1)
+ duplicate-source-aliases?
+ (apply distinct? desired-aliases)
+ (every? string? desired-aliases))]
+ (merge
+ honeysql-form
+ (if needs-columns?
+ ;; HoneySQL cannot expand [::h2x/identifier :table "source"] in the with alias.
+ ;; This is ok since we control the alias.
+ {:with [[[source-query-alias {:columns (mapv #(h2x/identifier :field %) desired-aliases)}]
+ source-clause]]
+ :from [[table-alias]]}
+ {:from [[source-clause [table-alias]]]}))))
(defn- apply-clauses
"Like [[apply-top-level-clauses]], but handles `source-query` as well, which needs to be handled in a special way
diff --git a/test/metabase/driver/sql/query_processor_test.clj b/test/metabase/driver/sql/query_processor_test.clj
index 7314e101e04453319a98271948e9216d350d15c2..085c70a7ba3059de4246f6efa3f9524b169d6cf3 100644
--- a/test/metabase/driver/sql/query_processor_test.clj
+++ b/test/metabase/driver/sql/query_processor_test.clj
@@ -38,7 +38,7 @@
(testing "[[sql.qp/sql-source-query]] should throw Exceptions if you pass in invalid nonsense"
(doseq [params [nil [1000]]]
(testing (format "Params = %s" (pr-str params))
- (is (= [::sql.qp/sql-source-query "SELECT *" params]
+ (is (= {::sql.qp/sql-source-query ["SELECT *" params]}
(sql.qp/sql-source-query "SELECT *" params)))))
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
diff --git a/test/metabase/driver/sql/query_processor_test_util.clj b/test/metabase/driver/sql/query_processor_test_util.clj
index cca791ec6c5143ac3bcf57fbf2416b49015a76e6..c3ca041680595ed074444a9ddff93386e50e595c 100644
--- a/test/metabase/driver/sql/query_processor_test_util.clj
+++ b/test/metabase/driver/sql/query_processor_test_util.clj
@@ -5,6 +5,8 @@
[metabase.driver :as driver]
[metabase.driver.util :as driver.u]
[metabase.query-processor.compile :as qp.compile]
+ [metabase.test.data.env :as tx.env]
+ [metabase.test.data.interface :as tx]
[metabase.util :as u]))
(set! *warn-on-reflection* true)
@@ -47,6 +49,7 @@
[FULL JOIN]
[GROUP BY]
[ORDER BY]
+ WITH
SELECT
FROM
LIMIT
@@ -151,3 +154,11 @@
`body`."
[query & body]
`(do-with-native-query-testing-context ~query (fn [] ~@body)))
+
+(defn sql-drivers
+ "All the drivers in the :sql hierarchy."
+ []
+ (set
+ (for [driver (tx.env/test-drivers)
+ :when (isa? driver/hierarchy (driver/the-driver driver) (driver/the-driver :sql))]
+ (tx/the-driver-with-test-extensions driver))))
diff --git a/test/metabase/query_processor_test/native_test.clj b/test/metabase/query_processor_test/native_test.clj
index 252a1f69e1bdd87b8bb4b5d79b6bd179d67101d9..0051478a2d3357090ef65b7eac231512d09b0c1a 100644
--- a/test/metabase/query_processor_test/native_test.clj
+++ b/test/metabase/query_processor_test/native_test.clj
@@ -1,6 +1,7 @@
(ns metabase.query-processor-test.native-test
(:require
[clojure.test :refer :all]
+ [metabase.driver.sql.query-processor-test-util :as sql.qp-test-util]
[metabase.models.card :refer [Card]]
[metabase.query-processor :as qp]
[metabase.query-processor.test-util :as qp.test-util]
@@ -37,6 +38,34 @@
(mt/native-query
{:query "select name from users;"}))))))
+(deftest ^:parallel native-with-duplicate-column-names
+ (testing "Should be able to run native query referring a question referring a question (#25988)"
+ (mt/with-test-drivers (sql.qp-test-util/sql-drivers)
+ (t2.with-temp/with-temp [:model/Card card {:dataset_query {:native {:query "select id, id from orders"}
+ :database (mt/id)
+ :type :native}
+ :result_metadata [{:base_type :type/BigInteger,
+ :display_name "ID",
+ :effective_type :type/BigInteger,
+ :field_ref [:field "ID" {:base-type :type/BigInteger}],
+ :fingerprint nil,
+ :name "ID",
+ :semantic_type :type/PK}
+ {:base_type :type/BigInteger,
+ :display_name "ID",
+ :effective_type :type/BigInteger,
+ :field_ref [:field "ID_2" {:base-type :type/BigInteger}],
+ :fingerprint nil,
+ :name "ID",
+ :semantic_type :type/PK}]}]
+ (is (=? {:columns ["ID" "ID_2"]}
+ (mt/rows+column-names
+ (qp/process-query
+ {:query {:source-table (str "card__" (:id card))
+ :fields [[:field "ID" {:base-type :type/Integer}] [:field "ID_2" {:base-type :type/Integer}]]}
+ :database (mt/id)
+ :type :query}))))))))
+
(deftest ^:parallel native-referring-question-referring-question-test
(testing "Should be able to run native query referring a question referring a question (#25988)"
(mt/with-driver :h2