Merge pull request #8870 from metabase/fix-sparksql-nested-query-support

Simplify SparkSQL driver; fix nested query support

src/metabase/driver/bigquery.clj

@@ -22,7 +22,9 @@
             [metabase.mbql
              [schema :as mbql.s]
              [util :as mbql.u]]
-            [metabase.models.table :as table]
+            [metabase.models
+             [field :refer [Field]]
+             [table :as table]]
             [metabase.query-processor
              [store :as qp.store]
              [util :as qputil]]
@@ -317,7 +319,8 @@
 ;; TODO - this is totally unnecessary now, we can just override `->honeysql` for `Field` and `Table` instead. FIXME!
 (defrecord ^:private BigQueryIdentifier [dataset-name ; optional; will use (dataset-name-for-current-query) otherwise
                                          table-name
-                                         field-name]
+                                         field-name
+                                         alias?]
   honeysql.format/ToSql
   (to-sql [{:keys [dataset-name table-name field-name], :as bq-id}]
     ;; Check to make sure the identifiers are valid and don't contain any sorts of escape characters since we are
@@ -333,9 +336,13 @@
       (assert (valid-bigquery-identifier? field-name)
         (tru "Invalid BigQuery identifier: ''{0}''" field-name)))
     ;; BigQuery identifiers should look like `dataset.table` or `dataset.table`.`field` (SAD!)
-    (str (format "`%s.%s`" (or dataset-name (dataset-name-for-current-query)) table-name)
-         (when (seq field-name)
-           (format ".`%s`" field-name)))))
+    (let [dataset-name (or dataset-name (dataset-name-for-current-query))]
+      (str
+       (if alias?
+         (format "`%s`" table-name)
+         (format "`%s.%s`" dataset-name table-name))
+       (when (seq field-name)
+         (format ".`%s`" field-name))))))
 
 (defn- honeysql-form->sql ^String [honeysql-form]
   {:pre [(map? honeysql-form)]}
@@ -393,17 +400,14 @@
   [driver [_ field unit]]
   (sql/date driver unit (sqlqp/->honeysql driver field)))
 
-(defmethod sqlqp/->honeysql [BigQueryDriver :field-id]
-  [_ [_ field-id]]
-  (let [{field-name :name, special-type :special_type, table-id :table_id} (qp.store/field field-id)
-        {table-name :name}                                                 (qp.store/table table-id)
-        field                                                              (map->BigQueryIdentifier
-                                                                            {:table-name table-name
-                                                                             :field-name field-name})]
-    (cond
-      (isa? special-type :type/UNIXTimestampSeconds)      (unix-timestamp->timestamp field :seconds)
-      (isa? special-type :type/UNIXTimestampMilliseconds) (unix-timestamp->timestamp field :milliseconds)
-      :else                                               field)))
+(defmethod sqlqp/->honeysql [BigQueryDriver (class Field)]
+  [driver field]
+  (let [{table-name :name, :as table} (qp.store/table (:table_id field))
+        field-identifier              (map->BigQueryIdentifier
+                                       {:table-name table-name
+                                        :field-name (:name field)
+                                        :alias?     (:alias? table)})]
+    (sqlqp/cast-unix-timestamp-field-if-needed driver field field-identifier)))
 
 (defn- field->identifier
   "Generate appropriate identifier for a Field for SQL parameters. (NOTE: THIS IS ONLY USED FOR SQL PARAMETERS!)"
@@ -448,10 +452,10 @@
             honeysql-form
             (h/merge-left-join honeysql-form
               [(map->BigQueryIdentifier {:table-name table-name})
-               (map->BigQueryIdentifier {:table-name join-alias})]
+               (map->BigQueryIdentifier {:table-name join-alias, :alias? true})]
               [:=
                (map->BigQueryIdentifier {:table-name source-table-name, :field-name (:name source-field)})
-               (map->BigQueryIdentifier {:table-name join-alias, :field-name (:name pk-field)})])]
+               (map->BigQueryIdentifier {:table-name join-alias, :field-name (:name pk-field), :alias? true})])]
         (if (seq more)
           (recur honeysql-form more)
           honeysql-form)))))

src/metabase/driver/sparksql.clj

@@ -15,7 +15,6 @@
              [generic-sql :as sql]
              [hive-like :as hive-like]]
             [metabase.driver.generic-sql.query-processor :as sqlqp]
-            [metabase.mbql.util :as mbql.u]
             [metabase.models.field :refer [Field]]
             [metabase.query-processor
              [store :as qp.store]
@@ -34,54 +33,18 @@
 
 ;;; ------------------------------------------ Custom HoneySQL Clause Impls ------------------------------------------
 
-(def ^:private source-table-alias "t1")
-
-(defn- resolve-table-alias [{field-name :name, special-type :special_type, table-id :table_id, :as field}]
-  (let [{schema-name :schema, table-name :name} (qp.store/table table-id)
-        source-table                            (qp.store/table (mbql.u/query->source-table-id sqlqp/*query*))
-        matching-join-table                     (some (fn [{:keys [table-id]}]
-                                                        (let [join-table (qp.store/table table-id)]
-                                                          (when (and (= schema-name (:schema join-table))
-                                                                     (= table-name (:name join-table)))
-                                                            join-table)))
-                                                      (get-in sqlqp/*query* [:query :join-tables]))]
-    (cond
-      (and (= schema-name (:schema source-table))
-           (= table-name (:name source-table)))
-      (assoc field :schema nil, :table source-table-alias)
-
-      matching-join-table
-      (assoc field :schema nil, :table (:join-alias matching-join-table))
-
-      :else
-      field)))
-
-(defmethod  sqlqp/->honeysql [SparkSQLDriver (class Field)]
-  [driver field-before-aliasing]
-  (let [{schema-name  :schema
-         table-name   :table
-         special-type :special_type
-         field-name   :name} (resolve-table-alias field-before-aliasing)
-        field                (keyword (hx/qualify-and-escape-dots schema-name table-name field-name))]
-    (cond
-      (isa? special-type :type/UNIXTimestampSeconds)      (sql/unix-timestamp->timestamp driver field :seconds)
-      (isa? special-type :type/UNIXTimestampMilliseconds) (sql/unix-timestamp->timestamp driver field :milliseconds)
-      :else                                               field)))
-
-(defn- apply-join-tables
-  [honeysql-form {join-tables :join-tables}]
-  (loop [honeysql-form honeysql-form, [{:keys [table-id pk-field-id fk-field-id schema join-alias]} & more] join-tables]
-    (let [{table-name :name} (qp.store/table table-id)
-          source-field       (qp.store/field fk-field-id)
-          pk-field           (qp.store/field pk-field-id)
-          honeysql-form      (h/merge-left-join honeysql-form
-                                                [(hx/qualify-and-escape-dots schema table-name) (keyword join-alias)]
-                                                [:=
-                                                 (hx/qualify-and-escape-dots source-table-alias (:field-name source-field))
-                                                 (hx/qualify-and-escape-dots join-alias         (:field-name pk-field))])]
-      (if (seq more)
-        (recur honeysql-form more)
-        honeysql-form))))
+(def ^:private source-table-alias
+  "Default alias for all source tables. (Not for source queries; those still use the default SQL QP alias of `source`.)"
+  "t1")
+
+(defmethod sqlqp/->honeysql [SparkSQLDriver (class Field)]
+  [driver field]
+  (let [table            (qp.store/table (:table_id field))
+        table-name       (if (:alias? table)
+                           (:name table)
+                           source-table-alias)
+        field-identifier (keyword (hx/qualify-and-escape-dots table-name (:name field)))]
+    (sqlqp/cast-unix-timestamp-field-if-needed driver field field-identifier)))
 
 (defn- apply-page-using-row-number-for-offset
   "Apply `page` clause to HONEYSQL-FROM, using row_number() for drivers that do not support offsets"
@@ -203,7 +166,6 @@
   (merge (sql/ISQLDriverDefaultsMixin)
          {:apply-page                (u/drop-first-arg apply-page-using-row-number-for-offset)
           :apply-source-table        (u/drop-first-arg apply-source-table)
-          :apply-join-tables         (u/drop-first-arg apply-join-tables)
           :column->base-type         (u/drop-first-arg hive-like/column->base-type)
           :connection-details->spec  (u/drop-first-arg connection-details->spec)
           :date                      (u/drop-first-arg hive-like/date)

test/metabase/driver/bigquery_test.clj

@@ -151,10 +151,11 @@
 ;; alias, e.g. something like `categories__via__category_id`, which is considerably different from what other SQL
 ;; databases do. (#4218)
 (expect-with-engine :bigquery
-  (str "SELECT `test_data.categories__via__category_id`.`name` AS `name`,"
-       " count(*) AS `count` FROM `test_data.venues` "
-       "LEFT JOIN `test_data.categories` `test_data.categories__via__category_id`"
-       " ON `test_data.venues`.`category_id` = `test_data.categories__via__category_id`.`id` "
+  (str "SELECT `categories__via__category_id`.`name` AS `name`,"
+       " count(*) AS `count` "
+       "FROM `test_data.venues` "
+       "LEFT JOIN `test_data.categories` `categories__via__category_id`"
+       " ON `test_data.venues`.`category_id` = `categories__via__category_id`.`id` "
        "GROUP BY `name` "
        "ORDER BY `name` ASC")
   ;; normally for test purposes BigQuery doesn't support foreign keys so override the function that checks that and

test/metabase/query_processor_test/nested_queries_test.clj

@@ -646,3 +646,22 @@
                       Collection [dest-card-collection]]
         (perms/grant-collection-read-permissions! (group/all-users) source-card-collection)
         (save-card-via-API-with-native-source-query! 403 db source-card-collection dest-card-collection)))))
+
+;; make sure that if we refer to a Field that is actually inside the source query, the QP is smart enough to figure
+;; out what you were referring to and behave appropriately
+(datasets/expect-with-engines (non-timeseries-engines-with-feature :nested-queries)
+  [[10]]
+  (format-rows-by [int]
+    (rows
+      (qp/process-query
+        {:database (data/id)
+         :type     :query
+         :query    {:source-query {:source-table (data/id :venues)
+                                   :fields       [(data/id :venues :id)
+                                                  (data/id :venues :name)
+                                                  (data/id :venues :category_id)
+                                                  (data/id :venues :latitude)
+                                                  (data/id :venues :longitude)
+                                                  (data/id :venues :price)]}
+                    :aggregation  [[:count]]
+                    :filter       [:= [:field-id (data/id :venues :category_id)] 50]}}))))