MBQL Explict Joins support (#10011)

* Explict Joins 
[ci drivers]

* Fix Snowflake tests 

frontend/test/metabase/services/__snapshots__/MetabaseApi.e2e.spec.js.snap

@@ -14,11 +14,14 @@ Object {
       "standard-deviation-aggregations",
       "expression-aggregations",
       "foreign-keys",
+      "right-join",
+      "left-join",
       "native-parameters",
       "nested-queries",
       "expressions",
       "case-sensitivity-string-filter-options",
       "binning",
+      "inner-join",
     ],
     "id": 1,
     "is_full_sync": true,
@@ -1110,11 +1113,14 @@ Object {
       "standard-deviation-aggregations",
       "expression-aggregations",
       "foreign-keys",
+      "right-join",
+      "left-join",
       "native-parameters",
       "nested-queries",
       "expressions",
       "case-sensitivity-string-filter-options",
       "binning",
+      "inner-join",
     ],
     "id": 1,
     "is_full_sync": true,
@@ -2311,11 +2317,14 @@ Object {
       "standard-deviation-aggregations",
       "expression-aggregations",
       "foreign-keys",
+      "right-join",
+      "left-join",
       "native-parameters",
       "nested-queries",
       "expressions",
       "case-sensitivity-string-filter-options",
       "binning",
+      "inner-join",
     ],
     "id": 1,
     "is_full_sync": true,
@@ -3155,11 +3164,14 @@ Object {
       "standard-deviation-aggregations",
       "expression-aggregations",
       "foreign-keys",
+      "right-join",
+      "left-join",
       "native-parameters",
       "nested-queries",
       "expressions",
       "case-sensitivity-string-filter-options",
       "binning",
+      "inner-join",
     ],
     "id": 1,
     "is_full_sync": true,

modules/drivers/googleanalytics/test/metabase/driver/googleanalytics_test.clj

@@ -169,10 +169,10 @@
 
 (defn- do-with-some-fields [f]
   (tt/with-temp* [Database [db                 {:engine :googleanalytics}]
-                  Table    [table              {:name "98765432"}]
-                  Field    [event-action-field {:name "ga:eventAction", :base_type "type/Text"}]
-                  Field    [event-label-field  {:name "ga:eventLabel", :base_type "type/Text"}]
-                  Field    [date-field         {:name "ga:date", :base_type "type/Date"}]]
+                  Table    [table              {:name "98765432", :db_id (u/get-id db)}]
+                  Field    [event-action-field {:name "ga:eventAction", :base_type "type/Text", :table_id (u/get-id table)}]
+                  Field    [event-label-field  {:name "ga:eventLabel", :base_type "type/Text", :table_id (u/get-id table)}]
+                  Field    [date-field         {:name "ga:date", :base_type "type/Date", :table_id (u/get-id table)}]]
     (f {:db                 db
         :table              table
         :event-action-field event-action-field
@@ -257,6 +257,7 @@
 
 ;; ok, now do the same query again, but run the entire QP pipeline, swapping out a few things so nothing is actually
 ;; run externally.
+;; TODO - Saw random test failure
 (expect
   {:row_count 1
    :status    :completed

modules/drivers/oracle/test/metabase/test/data/oracle.clj

@@ -37,6 +37,8 @@
 
 (defmethod tx/dbdef->connection-details :oracle [& _] @connection-details)
 
+(defmethod tx/sorts-nil-first? :oracle [_] false)
+
 (defmethod sql.tx/field-base-type->sql-type [:oracle :type/BigInteger] [_ _] "NUMBER(*,0)")
 (defmethod sql.tx/field-base-type->sql-type [:oracle :type/Boolean]    [_ _] "NUMBER(1)")
 (defmethod sql.tx/field-base-type->sql-type [:oracle :type/Date]       [_ _] "DATE")

modules/drivers/snowflake/project.clj

@@ -2,7 +2,7 @@
   :min-lein-version "2.5.0"
 
   :dependencies
-  [[net.snowflake/snowflake-jdbc "3.6.27"]]
+  [[net.snowflake/snowflake-jdbc "3.8.0"]]
 
   :profiles
   {:provided

modules/drivers/snowflake/src/metabase/driver/snowflake.clj

@@ -136,8 +136,10 @@
       (throw (Exception. (str (tru "Invalid Snowflake connection details: missing DB name."))))))
 
 (defn- query-db-name []
-  (or (-> (qp.store/database) db-name)
-      (throw (Exception. "Missing DB name"))))
+  ;; the store is always initialized when running QP queries; for some stuff like the test extensions DDL statements
+  ;; it won't be, *but* they should already be qualified by database name anyway
+  (when (qp.store/initialized?)
+    (db-name (qp.store/database))))
 
 ;; unless we're currently using a table alias, we need to prepend Table and Field identifiers with the DB name for the
 ;; query
@@ -154,6 +156,10 @@
     sql.qp/*table-alias*
     false
 
+    ;;; `query-db-name` is not currently set, e.g. because we're generating DDL statements for tests
+    (empty? (query-db-name))
+    false
+
     ;; already qualified
     (= (first components) (query-db-name))
     false

modules/drivers/snowflake/test/metabase/driver/snowflake_test.clj

 (ns metabase.driver.snowflake-test
-  (:require [clojure.java.jdbc :as jdbc]
-            [clojure.set :as set]
+  (:require [clojure.set :as set]
+            [expectations :refer [expect]]
             [metabase.driver :as driver]
             [metabase.models.table :refer [Table]]
             [metabase.test
              [data :as data]
              [util :as tu]]
-            [metabase.test.data.datasets :refer [expect-with-driver]]))
+            [metabase.test.data
+             [dataset-definitions :as dataset-defs]
+             [datasets :refer [expect-with-driver]]
+             [interface :as tx]
+             [sql :as sql.tx]]
+            [metabase.test.data.sql.ddl :as ddl]))
+
+;; make sure we didn't break the code that is used to generate DDL statements when we add new test datasets
+(expect
+  "DROP DATABASE IF EXISTS \"test-data\"; CREATE DATABASE \"test-data\";"
+  (sql.tx/create-db-sql :snowflake (tx/get-dataset-definition dataset-defs/test-data)))
+
+(expect
+  ["DROP TABLE IF EXISTS \"test-data\".\"PUBLIC\".\"users\";"
+   (str "CREATE TABLE \"test-data\".\"PUBLIC\".\"users\" (\"name\" TEXT  ,\"last_login\" TIMESTAMPLTZ"
+        "  ,\"password\" TEXT , \"id\" INTEGER AUTOINCREMENT, PRIMARY KEY (\"id\")) ;")
+   "DROP TABLE IF EXISTS \"test-data\".\"PUBLIC\".\"categories\";"
+   (str "CREATE TABLE \"test-data\".\"PUBLIC\".\"categories\" (\"name\" TEXT , \"id\" INTEGER AUTOINCREMENT,"
+        " PRIMARY KEY (\"id\")) ;")
+   "DROP TABLE IF EXISTS \"test-data\".\"PUBLIC\".\"venues\";"
+   (str "CREATE TABLE \"test-data\".\"PUBLIC\".\"venues\" (\"name\" TEXT  ,\"latitude\" FLOAT  ,\"longitude\" FLOAT"
+        "  ,\"price\" INTEGER  ,\"category_id\" INTEGER , \"id\" INTEGER AUTOINCREMENT, PRIMARY KEY (\"id\")) ;")
+   "DROP TABLE IF EXISTS \"test-data\".\"PUBLIC\".\"checkins\";"
+   (str "CREATE TABLE \"test-data\".\"PUBLIC\".\"checkins\" (\"user_id\" INTEGER  ,\"venue_id\" INTEGER  ,\"date\" DATE ,"
+        " \"id\" INTEGER AUTOINCREMENT, PRIMARY KEY (\"id\")) ;")
+   (str "ALTER TABLE \"test-data\".\"PUBLIC\".\"venues\" ADD CONSTRAINT \"fk_venues_category_id_categori\" FOREIGN KEY"
+        " (\"category_id\") REFERENCES \"test-data\".\"PUBLIC\".\"categories\" (\"id\");")
+   (str "ALTER TABLE \"test-data\".\"PUBLIC\".\"checkins\" ADD CONSTRAINT \"fk_checkins_user_id_users\""
+        " FOREIGN KEY (\"user_id\") REFERENCES \"test-data\".\"PUBLIC\".\"users\" (\"id\");")
+   (str "ALTER TABLE \"test-data\".\"PUBLIC\".\"checkins\" ADD CONSTRAINT \"fk_checkins_venue_id_venues\""
+        " FOREIGN KEY (\"venue_id\") REFERENCES \"test-data\".\"PUBLIC\".\"venues\" (\"id\");")]
+  (ddl/create-db-ddl-statements :snowflake (tx/get-dataset-definition dataset-defs/test-data)))
 
 (expect-with-driver :snowflake
   "UTC"

modules/drivers/snowflake/test/metabase/test/data/snowflake.clj

@@ -4,6 +4,7 @@
             [metabase.driver.sql-jdbc
              [connection :as sql-jdbc.conn]
              [sync :as sql-jdbc.sync]]
+            [metabase.driver.sql.util.unprepare :as unprepare]
             [metabase.test.data
              [interface :as tx]
              [sql :as sql.tx]
@@ -11,10 +12,13 @@
             [metabase.test.data.sql-jdbc
              [execute :as execute]
              [load-data :as load-data]]
+            [metabase.test.data.sql.ddl :as ddl]
             [metabase.util :as u]))
 
 (sql-jdbc.tx/add-test-extensions! :snowflake)
 
+(defmethod tx/sorts-nil-first? :snowflake [_] false)
+
 (defmethod sql.tx/field-base-type->sql-type [:snowflake :type/BigInteger] [_ _] "BIGINT")
 (defmethod sql.tx/field-base-type->sql-type [:snowflake :type/Boolean]    [_ _] "BOOLEAN")
 (defmethod sql.tx/field-base-type->sql-type [:snowflake :type/Date]       [_ _] "DATE")
@@ -90,10 +94,19 @@
       ;; load it next time around
       (catch Throwable e
         (let [drop-db-sql (format "DROP DATABASE \"%s\";" database-name)]
-          (println "Creating DB failed; executing" drop-db-sql)
+          (println "Creating DB failed:" e)
+          (println "Executing" drop-db-sql)
           (jdbc/execute! (no-db-connection-spec) [drop-db-sql]))
         (throw e)))))
 
+;; For reasons I don't understand the Snowflake JDBC driver doesn't seem to work when trying to use parameterized
+;; INSERT statements, even though the documentation suggests it should. Just go ahead and deparameterize all the
+;; statements for now.
+(defmethod ddl/insert-rows-ddl-statements :snowflake
+  [driver table-identifier row-or-rows]
+  (for [sql+args ((get-method ddl/insert-rows-ddl-statements :sql-jdbc/test-extensions) driver table-identifier row-or-rows)]
+    (unprepare/unprepare driver sql+args)))
+
 (defmethod execute/execute-sql! :snowflake [& args]
   (apply execute/sequentially-execute-sql! args))
 

modules/drivers/vertica/test/metabase/test/data/vertica.clj

@@ -13,6 +13,8 @@
 
 (sql-jdbc.tx/add-test-extensions! :vertica)
 
+(defmethod tx/sorts-nil-first? :vertica [_] false)
+
 (defmethod sql.tx/field-base-type->sql-type [:vertica :type/BigInteger] [_ _] "BIGINT")
 (defmethod sql.tx/field-base-type->sql-type [:vertica :type/Boolean]    [_ _] "BOOLEAN")
 (defmethod sql.tx/field-base-type->sql-type [:vertica :type/Char]       [_ _] "VARCHAR(254)")

src/metabase/driver.clj

@@ -528,7 +528,12 @@
     ;; SQLite, SQLServer, and MySQL do not support this -- `LIKE` clauses are always case-insensitive.
     ;;
     ;; DEFAULTS TO TRUE.
-    :case-sensitivity-string-filter-options})
+    :case-sensitivity-string-filter-options
+
+    :left-join
+    :right-join
+    :inner-join
+    :full-join})
 
 (defmulti supports?
   "Does this driver support a certain `feature`? (A feature is a keyword, and can be any of the ones listed above in

src/metabase/driver/h2.clj

@@ -26,6 +26,8 @@
 ;;; |                                             metabase.driver impls                                              |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
+(defmethod driver/supports? [:h2 :full-join] [_ _] false)
+
 (defmethod driver/connection-properties :h2 [_]
   [{:name         "db"
     :display-name (tru "Connection String")

src/metabase/driver/mysql.clj

@@ -40,6 +40,8 @@
 ;;; |                                             metabase.driver impls                                              |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
+(defmethod driver/supports? [:mysql :full-join] [_ _] false)
+
 (defmethod driver/connection-properties :mysql [_]
   (ssh/with-tunnel-config
     [driver.common/default-host-details

src/metabase/driver/sql.clj

@@ -18,6 +18,11 @@
 (defmethod driver/supports? [:sql :nested-queries]                  [_ _] true)
 (defmethod driver/supports? [:sql :binning]                         [_ _] true)
 
+(defmethod driver/supports? [:sql :left-join]  [driver _] (driver/supports? driver :foreign-keys))
+(defmethod driver/supports? [:sql :right-join] [driver _] (driver/supports? driver :foreign-keys))
+(defmethod driver/supports? [:sql :inner-join] [driver _] (driver/supports? driver :foreign-keys))
+(defmethod driver/supports? [:sql :full-join]  [driver _] (driver/supports? driver :foreign-keys))
+
 (defmethod driver/mbql->native :sql [driver query]
   (sql.qp/mbql->native driver query))
 

src/metabase/driver/sql/util/unprepare.clj

@@ -51,6 +51,7 @@
   (hformat/to-sql (hx/->time (hsql/raw (unprepare-value driver (du/->Timestamp value))))))
 
 
+;; TODO - I think a name like `deparameterize` would be more appropriate here
 (defmulti ^String unprepare
   "Convert a normal SQL `[statement & prepared-statement-args]` vector into a flat, non-prepared statement.
   Implementations should return a plain SQL string.

src/metabase/mbql/normalize.clj

@@ -140,7 +140,7 @@
   (let [clause-name (mbql.u/normalize-token clause-name)]
     (if-let [f (mbql-clause->special-token-normalization-fn clause-name)]
       (apply f clause-name args)
-      (vec (cons clause-name (map #(normalize-tokens % :ignore-path) args))))))
+      (into [clause-name] (map #(normalize-tokens % :ignore-path) args)))))
 
 
 (defn- aggregation-subclause? [x]
@@ -211,12 +211,26 @@
 (defn- normalize-source-query [{native? :native, :as source-query}]
   (normalize-tokens source-query [(if native? :native :query)]))
 
+(defn- normalize-join [join]
+  ;; path in call to `normalize-tokens` is [:query] so it will normalize `:source-query` as appropriate
+  (let [{:keys [strategy fields alias], :as join} (normalize-tokens join :query)]
+    (cond-> join
+      strategy
+      (update :strategy mbql.u/normalize-token)
+
+      ((some-fn keyword? string?) fields)
+      (update :fields mbql.u/normalize-token)
+
+      alias
+      (update :alias u/keyword->qualified-name))))
+
 (defn- normalize-source-metadata [metadata]
   (-> metadata
       (update :base_type    keyword)
       (update :special_type keyword)
       (update :fingerprint  walk/keywordize-keys)))
 
+;; TODO - why not make this a multimethod of some sort?
 (def ^:private path->special-token-normalization-fn
   "Map of special functions that should be used to perform token normalization for a given path. For example, the
   `:expressions` key in an MBQL query should preserve the case of the expression names; this custom behavior is
@@ -228,7 +242,8 @@
    :query           {:aggregation  normalize-ag-clause-tokens
                      :expressions  normalize-expressions-tokens
                      :order-by     normalize-order-by-tokens
-                     :source-query normalize-source-query}
+                     :source-query normalize-source-query
+                     :joins        {::sequence normalize-join}}
    :parameters      {::sequence normalize-query-parameter}
    :context         #(some-> % mbql.u/normalize-token)
    :source-metadata {::sequence normalize-source-metadata}})

src/metabase/mbql/schema.clj

@@ -527,6 +527,14 @@
     (set/rename-keys NativeQuery {:query :native})
     (s/recursive #'MBQLQuery)))
 
+(def ^java.util.regex.Pattern source-table-card-id-regex
+  "Pattern that matches `card__id` strings that can be used as the `:source-table` of MBQL queries."
+  #"^card__[1-9]\d*$")
+
+(def SourceTable
+  "Schema for a valid value for the `:source-table` clause of an MBQL query."
+  (s/cond-pre su/IntGreaterThanZero source-table-card-id-regex))
+
 (def JoinStrategy
   "Strategy that should be used to perform the equivalent of a SQL `JOIN` against another table or a nested query.
   These correspond 1:1 to features of the same name in driver features lists; e.g. you should check that the current
@@ -539,21 +547,22 @@
   In the top-level query, you can reference Fields from the joined table or nested query by the `:fk->` clause for
   implicit joins; for explicit joins, you *must* specify `:alias` yourself; you can then reference Fields by using a
   `:joined-field` clause, e.g.
+
     [:joined-field \"my_join_alias\" [:field-id 1]]                 ; for joins against other Tabless
     [:joined-field \"my_join_alias\" [:field-literal \"my_field\"]] ; for joins against nested queries"
   (->
-   { ;; The condition on which to JOIN. Can be anything that is a valid `:filter` clause. For automatically-generated
+   {;; *What* to JOIN. Self-joins can be done by using the same `:source-table` as in the query where this is specified.
+    ;; YOU MUST SUPPLY EITHER `:source-table` OR `:source-query`, BUT NOT BOTH!
+    (s/optional-key :source-table) SourceTable
+    (s/optional-key :source-query) SourceQuery
+    ;;
+    ;; The condition on which to JOIN. Can be anything that is a valid `:filter` clause. For automatically-generated
     ;; JOINs this is always
     ;;
     ;;    [:= <source-table-fk-field> [:joined-field <join-table-alias> <dest-table-pk-field>]]
     ;;
     :condition                     Filter
     ;;
-    ;; *What* to JOIN. Self-joins can be done by using the same `:source-table` as in the query where this is specified.
-    ;; YOU MUST SUPPLY EITHER `:source-table` OR `:source-query`, BUT NOT BOTH!
-    (s/optional-key :source-table) su/IntGreaterThanZero
-    (s/optional-key :source-query) (s/recursive #'Query)
-    ;;
     ;; Defaults to `:left-join`; used for all automatically-generated JOINs
     ;;
     ;; Driver implementations: this is guaranteed to be present after pre-processing.
@@ -575,7 +584,7 @@
     ;; with appropriate aliases.
     (s/optional-key :fields)       (s/cond-pre
                                     (s/enum :all :none)
-                                    (su/distinct (su/non-empty [Field])))
+                                    (su/distinct (su/non-empty [joined-field])))
     ;;
     ;; The name used to alias the joined table or query. This is usually generated automatically and generally looks
     ;; like `table__via__field`. You can specify this yourself if you need to reference a joined field in a
@@ -603,13 +612,9 @@
    #(su/empty-or-distinct? (filter some? (map :alias %)))
    "All join aliases must be unique."))
 
-(def ^java.util.regex.Pattern source-table-card-id-regex
-  "Pattern that matches `card__id` strings that can be used as the `:source-table` of MBQL queries."
-  #"^card__[1-9]\d*$")
-
-(def SourceTable
-  "Schema for a valid value for the `:source-table` clause of an MBQL query."
-  (s/cond-pre su/IntGreaterThanZero source-table-card-id-regex))
+(def Fields
+  "Schema for valid values of the `:fields` clause."
+  (su/distinct (su/non-empty [Field])))
 
 (def MBQLQuery
   "Schema for a valid, normalized MBQL [inner] query."
@@ -620,7 +625,7 @@
     (s/optional-key :breakout)     (su/non-empty [Field])
     ;; TODO - expressions keys should be strings; fix this when we get a chance
     (s/optional-key :expressions)  {s/Keyword FieldOrExpressionDef}
-    (s/optional-key :fields)       (su/distinct (su/non-empty [Field]))
+    (s/optional-key :fields)       Fields
     (s/optional-key :filter)       Filter
     (s/optional-key :limit)        su/IntGreaterThanZero
     (s/optional-key :order-by)     (su/distinct (su/non-empty [OrderBy]))

src/metabase/query_processor.clj

@@ -8,6 +8,7 @@
             [metabase.query-processor.middleware
              [add-dimension-projections :as add-dim]
              [add-implicit-clauses :as implicit-clauses]
+             [add-implicit-joins :as add-implicit-joins]
              [add-row-count-and-status :as row-count-and-status]
              [add-settings :as add-settings]
              [annotate :as annotate]
@@ -38,7 +39,7 @@
              [resolve-database :as resolve-database]
              [resolve-driver :as resolve-driver]
              [resolve-fields :as resolve-fields]
-             [resolve-joined-tables :as resolve-joined-tables]
+             [resolve-joins :as resolve-joins]
              [resolve-source-table :as resolve-source-table]
              [results-metadata :as results-metadata]
              [splice-params-in-response :as splice-params-in-response]
@@ -104,7 +105,8 @@
       perms/check-query-permissions
       cumulative-ags/handle-cumulative-aggregations
       ;; ▲▲▲ NO FK->s POINT ▲▲▲ Everything after this point will not see `:fk->` clauses, only `:joined-field`
-      resolve-joined-tables/resolve-joined-tables
+      resolve-joins/resolve-joins
+      add-implicit-joins/add-implicit-joins
       dev/check-results-format
       limit/limit
       results-metadata/record-and-return-metadata!

src/metabase/query_processor/middleware/add_implicit_clauses.clj

@@ -41,7 +41,7 @@
     ;; I suppose if we wanted to we could make the `order-by` rules swappable with something other set of rules
     {:order-by (default-sort-rules)}))
 
-(s/defn ^:private sorted-implicit-fields-for-table :- [mbql.s/Field]
+(s/defn sorted-implicit-fields-for-table :- [mbql.s/Field]
   "For use when adding implicit Field IDs to a query. Return a sequence of field clauses, sorted by the rules listed
   in `metabase.query-processor.sort`, for all the Fields in a given Table."
   [table-id :- su/IntGreaterThanZero]

src/metabase/query_processor/middleware/resolve_joined_tables.clj → src/metabase/query_processor/middleware/add_implicit_joins.clj

-(ns metabase.query-processor.middleware.resolve-joined-tables
-  "Middleware that fetches tables that will need to be joined, referred to by `fk->` clauses, and adds information to
-  the query about what joins should be done and how they should be performed."
+(ns metabase.query-processor.middleware.add-implicit-joins
+  "Middleware that creates corresponding `:joins` for Tables referred to by `:fk->` clauses and replaces those clauses
+  with `:joined-field` clauses."
   (:require [metabase
              [db :as mdb]
-             [driver :as driver]]
+             [driver :as driver]
+             [util :as u]]
             [metabase.mbql
              [schema :as mbql.s]
              [util :as mbql.u]]
@@ -64,7 +65,9 @@
 (s/defn ^:private store-join-tables! [fk-clauses :- [FKClauseWithFieldIDArgs]]
   (let [table-ids-to-fetch (fks->dest-table-ids fk-clauses)]
     (when (seq table-ids-to-fetch)
-      (doseq [table (db/select (vec (cons Table qp.store/table-columns-to-fetch)), :id [:in table-ids-to-fetch])]
+      (doseq [table (db/select (into [Table] qp.store/table-columns-to-fetch)
+                      :db_id (u/get-id (qp.store/database))
+                      :id    [:in table-ids-to-fetch])]
         (qp.store/store-table! table)))))
 
 
@@ -74,7 +77,7 @@
   [pk-info :- [PKInfo]]
   (let [pk-field-ids (set (map :pk-id pk-info))
         pk-fields    (when (seq pk-field-ids)
-                       (db/select (vec (cons Field qp.store/field-columns-to-fetch)) :id [:in pk-field-ids]))]
+                       (db/select (into [Field] qp.store/field-columns-to-fetch) :id [:in pk-field-ids]))]
     (doseq [field pk-fields]
       (qp.store/store-field! field))))
 
@@ -152,7 +155,7 @@
 
 ;;; -------------------------------------------- PUTTING it all together ---------------------------------------------
 
-(defn- resolve-joined-tables-in-top-level-query
+(defn- add-implicit-joins-in-top-level-query
   "Resolve JOINs at the top-level of the query."
   [{mbql-query :query, :as query}]
   ;; find fk-> clauses in the query AT THE TOP LEVEL
@@ -171,32 +174,32 @@
               (add-implicit-join-clauses resolved-join-info)
               (replace-fk-clauses resolved-join-info)))))))
 
-(defn- resolve-joined-tables-in-query-all-levels
+(defn- add-implicit-joins-in-query-all-levels
   "Resolve JOINs at all levels of the query, including the top level and nested queries at any level of nesting."
   [{{source-query :source-query} :query, :as query}]
   ;; first, resolve JOINs for the top-level
-  (let [query                          (resolve-joined-tables-in-top-level-query query)
+  (let [query                          (add-implicit-joins-in-top-level-query query)
         ;; then recursively resolve JOINs for any nested queries by pulling the query up a level and then getting the
         ;; result
         {resolved-source-query :query} (when source-query
-                                         (resolve-joined-tables-in-query-all-levels (assoc query :query source-query)))]
+                                         (add-implicit-joins-in-query-all-levels (assoc query :query source-query)))]
     ;; finally, merge the resolved source-query into the top-level query as appropriate
     (cond-> query
       resolved-source-query (assoc-in [:query :source-query] resolved-source-query))))
 
-(defn- resolve-joined-tables* [{query-type :type, :as query}]
+(defn- add-implicit-joins* [{query-type :type, :as query}]
   ;; if this is a native query, or if `driver/*driver*` is bound *and* it DOES NOT support `:foreign-keys`, return
   ;; query as is. Otherwise add implicit joins for `fk->` clauses
   (if (or (= query-type :native)
           (some-> driver/*driver* ((complement driver/supports?) :foreign-keys)))
     query
-    (resolve-joined-tables-in-query-all-levels query)))
+    (add-implicit-joins-in-query-all-levels query)))
 
-(defn resolve-joined-tables
+(defn add-implicit-joins
   "Fetch and store any Tables other than the source Table referred to by `fk->` clauses in an MBQL query, and add a
   `:join-tables` key inside the MBQL inner query containing information about the `JOIN`s (or equivalent) that need to
   be performed for these tables.
 
   This middleware also replaces all `fk->` clauses with `joined-field` clauses, which are easier to work with."
   [qp]
-  (comp qp resolve-joined-tables*))
+  (comp qp add-implicit-joins*))

src/metabase/query_processor/middleware/fetch_source_query.clj

@@ -51,14 +51,17 @@
                   ;; No need to include result metadata here, it can be large and will clutter the logs
                   (u/pprint-to-str 'yellow (dissoc <> :result_metadata)))))))
 
-(defn- expand-card-source-tables
+
+(declare resolve-card-id-source-tables)
+
+(defn- resolve-top-level-card-id-source-tables
   "If `source-table` is a Card reference (a string like `card__100`) then replace that with appropriate
   `:source-query` information. Does nothing if `source-table` is a normal ID. Recurses for nested-nested queries."
   [{:keys [source-table], :as inner-query}]
   (if-not (string? source-table)
     inner-query
     ;; (recursively) expand the source query
-    (let [source-query (expand-card-source-tables (source-table-str->source-query source-table))]
+    (let [source-query (resolve-card-id-source-tables (source-table-str->source-query source-table))]
       (-> inner-query
           ;; remove `source-table` `card__id` key
           (dissoc :source-table)
@@ -69,19 +72,34 @@
               :database        (:database source-query)
               :source-metadata (:source-metadata source-query))))))
 
+(s/defn ^:private resolve-joins-card-id-source-tables :- mbql.s/Joins
+  [joins :- mbql.s/Joins]
+  (for [{:keys [source-table], :as join} joins]
+    (if (string? source-table)
+      (let [source-query (resolve-card-id-source-tables (source-table-str->source-query source-table))]
+        (-> join
+            (dissoc :source-table)
+            (assoc :source-query source-query)))
+      join)))
+
+(defn- resolve-card-id-source-tables [{:keys [joins], :as inner-query}]
+  (cond-> (resolve-top-level-card-id-source-tables inner-query)
+    (seq joins) (update :joins resolve-joins-card-id-source-tables)))
+
 (s/defn ^:private fetch-source-query* :- mbql.s/Query
   [{inner-query :query, :as outer-query} :- mbql.s/Query]
   (if-not inner-query
     ;; for non-MBQL queries there's nothing to do since they have nested queries
     outer-query
     ;; otherwise attempt to expand any source queries as needed
-    (let [expanded-inner-query (expand-card-source-tables inner-query)]
-      (merge outer-query
-             {:query (dissoc expanded-inner-query :database :source-metadata)}
-             (when-let [database (:database expanded-inner-query)]
-               {:database database})
-             (when-let [source-metadata (:source-metadata expanded-inner-query)]
-               {:source-metadata source-metadata})))))
+    (let [expanded-inner-query (resolve-card-id-source-tables inner-query)]
+      (merge
+       outer-query
+       {:query (dissoc expanded-inner-query :database :source-metadata)}
+       (when-let [database (:database expanded-inner-query)]
+         {:database database})
+       (when-let [source-metadata (:source-metadata expanded-inner-query)]
+         {:source-metadata source-metadata})))))
 
 (defn fetch-source-query
   "Middleware that assocs the `:source-query` for this query if it was specified using the shorthand `:source-table`

src/metabase/query_processor/middleware/resolve_joins.clj

+(ns metabase.query-processor.middleware.resolve-joins
+  "Middleware that fetches tables that will need to be joined, referred to by `fk->` clauses, and adds information to
+  the query about what joins should be done and how they should be performed."
+  (:refer-clojure :exclude [alias])
+  (:require [metabase.mbql
+             [schema :as mbql.s]
+             [util :as mbql.u]]
+            [metabase.models
+             [field :refer [Field]]
+             [table :refer [Table]]]
+            [metabase.query-processor.middleware.add-implicit-clauses :as add-implicit-clauses]
+            [metabase.query-processor.store :as qp.store]
+            [metabase.util :as u]
+            [metabase.util
+             [i18n :refer [tru]]
+             [schema :as su]]
+            [schema.core :as s]
+            [toucan.db :as db]))
+
+(def ^:private Joins
+  "Schema for a non-empty sequence of Joins. Unlike `mbql.s/Joins`, this does not enforce the constraint that all join
+  aliases be unique; that is not guaranteeded until `deduplicate-aliases` transforms the joins."
+  (su/non-empty [mbql.s/Join]))
+
+(def ^:private MBQLQuery
+  "Schema for the parts of the 'inner' query we're transforming in this middleware that we care about validating (no
+  point in validating stuff we're not changing.)"
+  {:joins                   (s/constrained mbql.s/Joins vector?)
+   (s/optional-key :fields) (s/constrained mbql.s/Fields vector?)
+   s/Keyword                s/Any})
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                 Resolving Tables & Fields / Saving in QP Store                                 |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(s/defn ^:private resolve-fields! :- (s/eq nil)
+  [joins :- Joins]
+  (when-let [field-ids (->> (mbql.u/match joins [:field-id id] id)
+                            (remove qp.store/has-field?)
+                            seq)]
+    (doseq [field (db/select (into [Field] qp.store/field-columns-to-fetch) :id [:in field-ids])]
+      (qp.store/store-field! field))))
+
+(s/defn ^:private resolve-tables! :- (s/eq nil)
+  [joins :- Joins]
+  (when-let [source-table-ids (->> (map :source-table joins)
+                                   (filter some?)
+                                   (remove qp.store/has-table?)
+                                   seq)]
+    (let [resolved-tables (db/select (into [Table] qp.store/table-columns-to-fetch)
+                            :id    [:in source-table-ids]
+                            :db_id (u/get-id (qp.store/database)))
+          resolved-ids (set (map :id resolved-tables))]
+      ;; make sure all IDs were resolved, otherwise someone is probably trying to Join a table that doesn't exist
+      (doseq [id source-table-ids
+              :when (not (resolved-ids id))]
+        (throw
+         (IllegalArgumentException.
+          (str (tru "Could not find Table {0} in Database {1}." id (u/get-id (qp.store/database)))))))
+      ;; cool, now store the Tables in the DB
+      (doseq [table resolved-tables]
+        (qp.store/store-table! table)))))
+
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                             :joins Transformations                                             |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(s/defn ^:private deduplicate-aliases :- mbql.s/Joins
+  [joins :- Joins]
+  (let [joins          (for [join joins]
+                         (update join :alias #(or % "source")))
+        unique-aliases (mbql.u/uniquify-names (map :alias joins))]
+    (mapv
+     (fn [join alias]
+       (assoc join :alias alias))
+     joins
+     unique-aliases)))
+
+(s/defn ^:private merge-defaults :- mbql.s/Join
+  [join]
+  (merge {:strategy :left-join} join))
+
+(s/defn ^:private handle-all-fields :- mbql.s/Join
+  [{:keys [source-table alias fields], :as join} :- mbql.s/Join]
+  (merge
+   join
+   (if (= fields :all)
+     (if source-table
+       {:fields (for [field (add-implicit-clauses/sorted-implicit-fields-for-table source-table)]
+                  [:joined-field alias field])}
+       (throw
+        (UnsupportedOperationException.
+         "TODO - fields = all is not yet implemented for joins with source queries."))))))
+
+
+(s/defn ^:private resolve-references-and-deduplicate :- mbql.s/Joins
+  [joins :- Joins]
+  (resolve-tables! joins)
+  (let [joins (->> joins
+                   deduplicate-aliases
+                   (map merge-defaults)
+                   (mapv handle-all-fields))]
+    (resolve-fields! joins)
+    joins))
+
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                           MBQL-Query Transformations                                           |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(defn- joins->fields [joins]
+  (for [{:keys [fields]} joins
+        :when            (sequential? fields)
+        field            fields]
+    field))
+
+(defn- remove-joins-fields [joins]
+  (vec (for [join joins]
+         (dissoc join :fields))))
+
+(s/defn ^:private merge-joins-fields :- MBQLQuery
+  [{:keys [joins], :as query} :- MBQLQuery]
+  (let [join-fields (joins->fields joins)
+        query       (update query :joins remove-joins-fields)]
+    (cond-> query
+      (seq join-fields) (update :fields (comp vec distinct concat) join-fields))))
+
+(defn- check-join-aliases [{:keys [joins], :as query}]
+  (let [aliases (set (map :alias joins))]
+    (doseq [alias (mbql.u/match query [:joined-field alias _] alias)]
+      (when-not (aliases alias)
+        (throw
+         (IllegalArgumentException.
+          (str (tru "Bad :joined-field clause: join with alias ''{0}'' does not exist. Found: {1}"
+                    alias aliases))))))))
+
+(s/defn ^:private resolve-joins-in-mbql-query :- MBQLQuery
+  [{:keys [joins], :as query} :- MBQLQuery]
+  (u/prog1 (-> query
+               (update :joins resolve-references-and-deduplicate)
+               merge-joins-fields)
+    (check-join-aliases <>)))
+
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                Middleware & Boring Recursive Application Stuff                                 |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(defn- resolve-joins-in-mbql-query-all-levels
+  [{:keys [joins source-query], :as query}]
+  (cond-> query
+    (seq joins)
+    resolve-joins-in-mbql-query
+
+    source-query
+    (update :source-query resolve-joins-in-mbql-query-all-levels)))
+
+(defn- resolve-joins* [{inner-query :query, :as outer-query}]
+  (cond-> outer-query
+    inner-query (update :query resolve-joins-in-mbql-query-all-levels)))
+
+(defn resolve-joins
+  "Add any Tables and Fields referenced by the `:joins` clause to the QP store."
+  [qp]
+  (fn
+    ([query]
+     (qp (resolve-joins* query)))
+
+    ([query respond raise canceled-chan]
+     (qp (resolve-joins* query) respond raise canceled-chan))))