Fix metadata calculation for breakouts on joined columns (#29907) (#32570)

modules/drivers/presto-jdbc/test/metabase/test/data/presto_jdbc.clj

@@ -163,7 +163,7 @@
   (testing "Make sure logic to strip out NOT NULL and PRIMARY KEY stuff works as expected"
     (let [db-def    (tx/get-dataset-definition defs/test-data)
           table-def (-> db-def :table-definitions second)]
-      (is (= "CREATE TABLE \"test_data\".\"default\".\"categories\" (\"id\" INTEGER, \"name\" VARCHAR) ;"
+      (is (= "CREATE TABLE \"test_data\".\"default\".\"test_data_categories\" (\"id\" INTEGER, \"name\" VARCHAR) ;"
              (sql.tx/create-table-sql :presto-jdbc db-def table-def))))))
 
 (defmethod ddl.i/format-name :presto-jdbc

src/metabase/lib/join.cljc

@@ -198,8 +198,10 @@
    unique-name-fn :- fn?
    col            :- :map]
   (assoc col
-         :lib/source-column-alias  (:name col)
-         :lib/desired-column-alias (unique-name-fn (joined-field-desired-alias (:alias join) (:name col)))))
+         :lib/source-column-alias  ((some-fn :lib/source-column-alias :name) col)
+         :lib/desired-column-alias (unique-name-fn (joined-field-desired-alias
+                                                    (:alias join)
+                                                    ((some-fn :lib/source-column-alias :name) col)))))
 
 (defmethod lib.metadata.calculation/returned-columns-method :mbql/join
   [query
@@ -287,11 +289,11 @@
   "Create an MBQL join map from something that can conceptually be joined against. A `Table`? An MBQL or native query? A
   Saved Question? You should be able to join anything, and this should return a sensible MBQL join map."
   ([joinable]
-   (join-clause-method joinable))
+   (-> (join-clause-method joinable)
+       (u/assoc-default :fields :all)))
 
   ([joinable conditions]
    (-> (join-clause joinable)
-       (u/assoc-default :fields :all)
        (with-join-conditions conditions))))
 
 (mu/defn with-join-fields :- PartialJoin

src/metabase/lib/stage.cljc

@@ -85,8 +85,8 @@
    (for [breakout (lib.breakout/breakouts-metadata query stage-number)]
      (assoc breakout
             :lib/source               :source/breakouts
-            :lib/source-column-alias  (:name breakout)
-            :lib/desired-column-alias (unique-name-fn (:name breakout))))))
+            :lib/source-column-alias  ((some-fn :lib/source-column-alias :name) breakout)
+            :lib/desired-column-alias (unique-name-fn (lib.field/desired-alias query breakout))))))
 
 (mu/defn ^:private aggregations-columns :- [:maybe lib.metadata.calculation/ColumnsWithUniqueAliases]
   [query          :- ::lib.schema/query
@@ -99,6 +99,8 @@
             :lib/source-column-alias  (:name ag)
             :lib/desired-column-alias (unique-name-fn (:name ag))))))
 
+;;; TODO -- maybe the bulk of this logic should be moved into [[metabase.lib.field]], like we did for breakouts and
+;;; aggregations above.
 (mu/defn ^:private fields-columns :- [:maybe lib.metadata.calculation/ColumnsWithUniqueAliases]
   [query          :- ::lib.schema/query
    stage-number   :- :int

test/metabase/actions/test_util.clj

@@ -2,7 +2,10 @@
   (:require
    [clojure.java.jdbc :as jdbc]
    [clojure.test :refer :all]
+   [metabase.driver :as driver]
+   [metabase.driver.ddl.interface :as ddl.i]
    [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
+   [metabase.driver.sql.query-processor :as sql.qp]
    [metabase.http-client :as client]
    [metabase.models :refer [Action Card Database]]
    [metabase.models.action :as action]
@@ -14,6 +17,7 @@
    [metabase.test.data.users :as test.users]
    [metabase.test.initialize :as initialize]
    [metabase.test.util :as tu]
+   [metabase.util.honey-sql-2 :as h2x]
    [toucan2.core :as t2]
    [toucan2.tools.with-temp :as t2.with-temp]))
 
@@ -111,6 +115,23 @@
   [& body]
   `(do-with-dataset-definition (tx/dataset-definition ~(str (gensym))) (fn [] ~@body)))
 
+(defn- delete-categories-1-query []
+  (sql.qp/format-honeysql
+   2
+   (sql.qp/quote-style driver/*driver*)
+   {:delete-from [(h2x/identifier :table (ddl.i/format-name driver/*driver* "categories"))]
+    :where       [:=
+                  (h2x/identifier :field (ddl.i/format-name driver/*driver* "id"))
+                  [:inline 1]]}))
+
+(deftest ^:parallel delete-categories-1-query-test
+  (are [driver query] (= query
+                         (binding [driver/*driver* driver]
+                           (delete-categories-1-query)))
+    :h2       ["DELETE FROM \"CATEGORIES\" WHERE \"ID\" = 1"]
+    :postgres ["DELETE FROM \"categories\" WHERE \"id\" = 1"]
+    :mysql    ["DELETE FROM `categories` WHERE `id` = 1"]))
+
 (deftest with-actions-test-data-test
   (datasets/test-drivers (qp.test/normal-drivers-with-feature :actions/custom)
     (dotimes [i 2]
@@ -124,7 +145,7 @@
             (testing "delete row"
               (is (= [1]
                      (jdbc/execute! (sql-jdbc.conn/db->pooled-connection-spec (data/id))
-                                    "DELETE FROM CATEGORIES WHERE ID = 1;"))))
+                                    (delete-categories-1-query)))))
             (testing "after"
               (is (= [[74]]
                      (row-count))))))))))

test/metabase/lib/join_test.cljc

@@ -55,6 +55,16 @@
                                              :args     [(lib/ref (meta/field-metadata :venues :category-id))
                                                         (lib/ref (meta/field-metadata :categories :id))]}]))))))
 
+(deftest ^:parallel join-clause-test
+  (testing "Should have :fields :all by default (#32419)"
+    (is (=? {:lib/type    :mbql/join
+             :stages      [{:lib/type     :mbql.stage/mbql
+                            :lib/options  {:lib/uuid string?}
+                            :source-table (meta/id :orders)}]
+             :lib/options {:lib/uuid string?}
+             :fields      :all}
+            (lib/join-clause (meta/table-metadata :orders))))))
+
 (deftest ^:parallel join-saved-question-test
   (is (=? {:lib/type :mbql/query
            :database (meta/id)

test/metabase/lib/stage_test.cljc

@@ -3,6 +3,7 @@
    [clojure.test :refer [deftest is testing]]
    [malli.core :as mc]
    [metabase.lib.core :as lib]
+   [metabase.lib.join :as lib.join]
    [metabase.lib.metadata.calculation :as lib.metadata.calculation]
    [metabase.lib.schema :as lib.schema]
    [metabase.lib.stage :as lib.stage]
@@ -10,8 +11,6 @@
    [metabase.lib.test-util :as lib.tu]
    #?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))))
 
-(comment lib/keep-me)
-
 #?(:cljs
    (comment metabase.test-runner.assert-exprs.approximately-equal/keep-me))
 
@@ -249,3 +248,100 @@
                 :lib/source :source/expressions}]
               (filter #(= (:name %) expr-name)
                       (lib.metadata.calculation/visible-columns query)))))))
+
+(defn- metadata-for-breakouts-from-joins-test-query
+  "A query against `ORDERS` with joins against `PRODUCTS` and `PEOPLE`, and breakouts on columns from both of those
+  joins."
+  []
+  (-> (lib/query meta/metadata-provider (meta/table-metadata :orders))
+      (lib/join (-> (lib/join-clause (meta/table-metadata :products))
+                    (lib/with-join-alias "P1")
+                    (lib/with-join-conditions [(lib/= (meta/field-metadata :orders :product-id)
+                                                      (-> (meta/field-metadata :products :id)
+                                                          (lib/with-join-alias "P1")))])))
+      (lib/join (-> (lib/join-clause (meta/table-metadata :people))
+                    (lib/with-join-alias "People")
+                    (lib/with-join-conditions [(lib/= (meta/field-metadata :orders :user-id)
+                                                      (-> (meta/field-metadata :people :id)
+                                                          (lib/with-join-alias "People")))])))
+      (lib/breakout (-> (meta/field-metadata :products :category)
+                        (lib/with-join-alias "P1")))
+      (lib/breakout (-> (meta/field-metadata :people :source)
+                        (lib/with-join-alias "People")))
+      (lib/aggregate (lib/count))))
+
+(deftest ^:parallel metadata-for-breakouts-from-joins-test
+  (testing "metadata for breakouts of joined columns should be calculated correctly (#29907)"
+    (let [query (metadata-for-breakouts-from-joins-test-query)]
+      (is (= [{:name                     "CATEGORY"
+               :lib/source-column-alias  "CATEGORY"
+               ::lib.join/join-alias     "P1"
+               :lib/desired-column-alias "P1__CATEGORY"}
+              {:name                     "SOURCE"
+               :lib/source-column-alias  "SOURCE"
+               ::lib.join/join-alias     "People"
+               :lib/desired-column-alias "People__SOURCE"}
+              {:name                     "count"
+               :lib/source-column-alias  "count"
+               :lib/desired-column-alias "count"}]
+             (map #(select-keys % [:name :lib/source-column-alias ::lib.join/join-alias :lib/desired-column-alias])
+                  (lib.metadata.calculation/returned-columns query)))))))
+
+(defn- metadata-for-breakouts-from-joins-test-query-2
+  "A query against `REVIEWS` joining `PRODUCTS`."
+  []
+  (-> (lib/query meta/metadata-provider (meta/table-metadata :reviews))
+      (lib/join (-> (lib/join-clause (meta/table-metadata :products))
+                    (lib/with-join-alias "P2")
+                    (lib/with-join-conditions [(lib/= (meta/field-metadata :reviews :product-id)
+                                                      (-> (meta/field-metadata :products :id)
+                                                          (lib/with-join-alias "P2")))])))
+      (lib/breakout (-> (meta/field-metadata :products :category)
+                        (lib/with-join-alias "P2")))
+      (lib/aggregate (lib/avg (meta/field-metadata :reviews :rating)))
+      lib/append-stage))
+
+(deftest ^:parallel metadata-for-breakouts-from-joins-test-2
+  (testing "metadata for breakouts of joined columns should be calculated correctly (#29907)"
+    (let [query (metadata-for-breakouts-from-joins-test-query-2)]
+      (is (= [{:name "CATEGORY", :lib/source-column-alias "P2__CATEGORY", :lib/desired-column-alias "P2__CATEGORY"}
+              {:name "avg", :lib/source-column-alias "avg", :lib/desired-column-alias "avg"}]
+             (map #(select-keys % [:name :lib/source-column-alias ::lib.join/join-alias :lib/desired-column-alias])
+                  (lib.metadata.calculation/returned-columns query)))))))
+
+(defn- metadata-for-breakouts-from-joins-from-previous-stage-test-query
+  "[[metadata-for-breakouts-from-joins-test-query]] but with an additional stage and a join
+  against [[metadata-for-breakouts-from-joins-test-query-2]]. This means there are two joins against `PRODUCTS`, one
+  from the first stage and one from the nested query in the join in the second stage."
+  []
+  (-> (metadata-for-breakouts-from-joins-test-query)
+      lib/append-stage
+      (lib/join (-> (lib/join-clause (metadata-for-breakouts-from-joins-test-query-2))
+                    (lib/with-join-alias "Q2")
+                    (lib/with-join-conditions [(lib/= (-> (meta/field-metadata :products :category)
+                                                          (lib/with-join-alias "P1"))
+                                                      (-> (meta/field-metadata :products :category)
+                                                          (lib/with-join-alias "Q2")))])))))
+
+(deftest ^:parallel metadata-for-breakouts-from-joins-from-previous-stage-test
+  (testing "metadata for breakouts of columns from join in previous stage should be calculated correctly (#29907)"
+    (let [query (metadata-for-breakouts-from-joins-from-previous-stage-test-query)]
+      (is (= [{:name                     "CATEGORY"
+               :lib/source-column-alias  "P1__CATEGORY"
+               :lib/desired-column-alias "P1__CATEGORY"}
+              {:name                     "SOURCE"
+               :lib/source-column-alias  "People__SOURCE"
+               :lib/desired-column-alias "People__SOURCE"}
+              {:name                     "count"
+               :lib/source-column-alias  "count"
+               :lib/desired-column-alias "count"}
+              {:name                     "CATEGORY"
+               :lib/source-column-alias  "P2__CATEGORY"
+               ::lib.join/join-alias     "Q2"
+               :lib/desired-column-alias "Q2__P2__CATEGORY"}
+              {:name                     "avg"
+               :lib/source-column-alias  "avg"
+               ::lib.join/join-alias     "Q2"
+               :lib/desired-column-alias "Q2__avg"}]
+             (map #(select-keys % [:name :lib/source-column-alias ::lib.join/join-alias :lib/desired-column-alias])
+                  (lib.metadata.calculation/returned-columns query)))))))

test/metabase/query_processor/middleware/constraints_test.clj

@@ -7,12 +7,12 @@
 (defn- add-default-userland-constraints [query]
   (:pre (mt/test-qp-middleware qp.constraints/add-default-userland-constraints query)))
 
-(deftest no-op-without-middleware-options-test
+(deftest ^:parallel no-op-without-middleware-options-test
   (testing "don't do anything to queries without [:middleware :add-default-userland-constraints?] set"
     (is (= {}
            (add-default-userland-constraints {})))))
 
-(deftest add-constraints-test
+(deftest ^:parallel add-constraints-test
   (testing "if it is *truthy* add the constraints"
     (is (= {:middleware  {:add-default-userland-constraints? true},
             :constraints {:max-results           @#'qp.constraints/max-results
@@ -20,13 +20,13 @@
            (add-default-userland-constraints
             {:middleware {:add-default-userland-constraints? true}})))))
 
-(deftest no-op-if-option-is-false-test
+(deftest ^:parallel no-op-if-option-is-false-test
   (testing "don't do anything if it's not truthy"
     (is (= {:middleware {:add-default-userland-constraints? false}}
            (add-default-userland-constraints
             {:middleware {:add-default-userland-constraints? false}})))))
 
-(deftest dont-overwrite-existing-constraints-test
+(deftest ^:parallel dont-overwrite-existing-constraints-test
   (testing "if it already has constraints, don't overwrite those!"
     (is (= {:middleware  {:add-default-userland-constraints? true}
             :constraints {:max-results           @#'qp.constraints/max-results
@@ -35,7 +35,7 @@
             {:constraints {:max-results-bare-rows 1}
              :middleware  {:add-default-userland-constraints? true}})))))
 
-(deftest max-results-bare-rows-should-be-less-than-max-results-test
+(deftest ^:parallel max-results-bare-rows-should-be-less-than-max-results-test
   (testing "if you specify just `:max-results` it should make sure `:max-results-bare-rows` is <= `:max-results`"
     (is (= {:middleware  {:add-default-userland-constraints? true}
             :constraints {:max-results           5

test/metabase/query_processor/middleware/limit_test.clj

@@ -20,7 +20,7 @@
     (is (= test-max-results
            (-> (limit {:type :native}) mt/rows count)))))
 
-(deftest disable-max-results-test
+(deftest ^:parallel disable-max-results-test
   (testing "Apply `absolute-max-results` limit in the default case"
     (let [query {:type :query
                  :query {}}]

test/metabase/test_runner.clj

@@ -84,7 +84,7 @@
    "test_resources"])
 
 (defn- default-options []
-  {:namespace-pattern   #"^metabase.*test$"
+  {:namespace-pattern   #"^metabase.*"
    :exclude-directories excluded-directories
    :test-warn-time      1500})