With parent record

enterprise/backend/src/metabase_enterprise/query_reference_validation/api.clj

@@ -4,7 +4,7 @@
    [medley.core :as m]
    [metabase.api.common :as api]
    [metabase.api.routes.common :refer [+auth]]
-   [metabase.models.query-field :as query-field]
+   [metabase.models.query-analysis :as query-analysis]
    [metabase.server.middleware.offset-paging :as mw.offset-paging]
    [toucan2.core :as t2]))
 
@@ -41,8 +41,9 @@
                                               :u.first_name :u.last_name :u.email]
                                              sort-dir-kw]]
                               [(into sorting-selects [sort-dir-kw])])
-        card-query          (query-field/cards-with-reference-errors
+        card-query          (query-analysis/cards-with-reference-errors
                              (m/assoc-some
+                              ;; TODO this table has a lot of fields... we should whittle down to only the ones we need.
                               {:select    (into [:c.*] sorting-selects)
                                :from      [[(t2/table-name :model/Card) :c]]
                                :left-join sorting-joins
@@ -52,7 +53,7 @@
                               :limit  limit
                               :offset offset))
         cards               (t2/select :model/Card card-query)
-        id->errors          (query-field/reference-errors cards)
+        id->errors          (query-analysis/reference-errors cards)
         add-errors          (fn [{:keys [id] :as card}]
                               (assoc card :errors (id->errors id)))]
     (map add-errors (t2/hydrate cards :collection :creator))))
@@ -61,7 +62,7 @@
   []
   (:count
    (t2/query-one
-    (query-field/cards-with-reference-errors
+    (query-analysis/cards-with-reference-errors
      {:select [[[:count [:distinct :c.id]] :count]]
       :from   [[(t2/table-name :model/Card) :c]]
       :where  [:= :c.archived false]}))))

enterprise/backend/test/metabase_enterprise/query_reference_validation/api_test.clj

@@ -23,34 +23,82 @@
                             :model/Field      {field-1 :id}  {:active   false
                                                               :name     "FA"
                                                               :table_id table-1}
-                            :model/Field      {field-1b :id} {:active   false
-                                                              :name     "FAB"
-                                                              :table_id table-1}
                             :model/Field      {field-2 :id}  {:active   false
                                                               :name     "FB"
-                                                              :table_id table-1}
-                            :model/Field      {field-3 :id}  {:active   false
-                                                              :name     "FC"
                                                               :table_id table-2}
+
+                            :model/QueryAnalysis {qa-1 :id}  {:card_id card-1}
+                            :model/QueryAnalysis {qa-2 :id}  {:card_id card-2}
+                            :model/QueryAnalysis {qa-3 :id}  {:card_id card-3}
+
+                            ;; QTs not to include:
+                            ;; - Table is still active
+                            :model/QueryTable {}             {:card_id     card-1
+                                                              :analysis_id qa-1
+                                                              :table       "ORDERS"
+                                                              :table_id    (mt/id :orders)}
+
+                            :model/QueryTable {}              {:card_id     card-1
+                                                               :analysis_id qa-1
+                                                               :table       "T1"
+                                                               :table_id    table-1}
+                            :model/QueryTable {}              {:card_id     card-2
+                                                               :analysis_id qa-2
+                                                               :table       "T2"
+                                                               :table_id    table-2}
+                            :model/QueryTable {}              {:card_id     card-3
+                                                               :analysis_id qa-3
+                                                               :table       "T3"
+                                                               :table_id    nil}
+
                             ;; QFs not to include:
                             ;; - Field is still active
-                            :model/QueryField {}             {:card_id  card-1
-                                                              :field_id (mt/id :orders :tax)}
+                            :model/QueryField {}             {:card_id     card-1
+                                                              :analysis_id qa-3
+                                                              :table       "ORDERS"
+                                                              :column      "tax"
+                                                              :table_id    (mt/id :orders)
+                                                              :field_id    (mt/id :orders :tax)}
                             ;; - Implicit reference
                             :model/QueryField {}             {:card_id            card-2
+                                                              :analysis_id        qa-3
+                                                              :table              "T1"
+                                                              :column             "FA"
                                                               :field_id           field-1
                                                               :explicit_reference false}
                             ;; QFs to include:
-                            :model/QueryField {qf-1 :id}     {:card_id  card-1
-                                                              :field_id field-1}
-                            :model/QueryField {qf-1b :id}    {:card_id  card-1
-                                                              :field_id field-1b}
-                            :model/QueryField {qf-2 :id}     {:card_id  card-2
-                                                              :field_id field-2}
-                            :model/QueryField {qf-3 :id}     {:card_id  card-3
-                                                              :field_id field-3}]
+                            :model/QueryField {}             {:card_id     card-1
+                                                              :analysis_id qa-1
+                                                              :table       "T1"
+                                                              :column      "FA"
+                                                              :table_id    table-1
+                                                              :field_id    field-1}
+                            :model/QueryField {}             {:card_id     card-1
+                                                              :analysis_id qa-1
+                                                              :table       "T1"
+                                                              :column      "FAB"
+                                                              :table_id    table-1
+                                                              :field_id    nil}
+                            :model/QueryField {}             {:card_id     card-2
+                                                              :analysis_id qa-2
+                                                              :table       "T2"
+                                                              :column      "FB"
+                                                              :table_id    table-2
+                                                              :field_id    field-2}
+                            :model/QueryField {}             {:card_id     card-2
+                                                              :analysis_id qa-2
+                                                              :table       "T2"
+                                                              :column      "FBB"
+                                                              :table_id    table-2
+                                                              :field_id    nil}
+                            :model/QueryField {}             {:card_id     card-3
+                                                              :analysis_id qa-3
+                                                              :table       "T3"
+                                                              :column      "FC"
+                                                              :table_id    nil
+                                                              :field_id    nil}]
      (mt/with-premium-features #{:query-reference-validation}
-       (mt/call-with-map-params f [card-1 card-2 card-3 card-4 qf-1 qf-1b qf-2 qf-3])))))
+       (mt/call-with-map-params f [card-1 card-2 card-3 card-4])))))
 
 (defmacro ^:private with-test-setup
   "Creates some non-stale QueryFields and anaphorically provides stale QueryField IDs called `qf-{1-3}` and `qf-1b` and
@@ -60,7 +108,7 @@
   `card-4` is guaranteed not to have problems"
   [& body]
   `(do-with-test-setup
-    (mt/with-anaphora [qf-1 qf-1b qf-2 qf-3 card-1 card-2 card-3 card-4]
+    (mt/with-anaphora [card-1 card-2 card-3 card-4]
       ~@body)))
 
 (def ^:private url "ee/query-reference-validation/invalid-cards")
@@ -90,8 +138,7 @@
    {expected-total :total expected-limit :limit expected-offset :offset expected-data :data}
    {actual-total :total actual-limit :limit actual-offset :offset actual-data :data}
    unexpected]
-  (let [results [
-                 ;; Total
+  (let [results [;; Total
                  (merge {:expected {:total expected-total}
                          :actual   {:total actual-total}}
                         (if (<= expected-total actual-total)
@@ -113,17 +160,17 @@
                           {:type :fail
                            :diffs [actual-offset [expected-offset nil]]}))
                  ;; Data
-                 (merge {:expected {:data expected-data}
-                         :actual   {:data actual-data}}
-                        (let [trimmed-actual-data (map #(select-keys % (keys (first expected-data))) actual-data)
-                              actual-set          (into #{} trimmed-actual-data)
-                              expected-set        (into #{} expected-data)]
+                 (let [trimmed-actual-data (map #(select-keys % (keys (first expected-data))) actual-data)
+                       actual-set          (into #{} trimmed-actual-data)
+                       expected-set        (into #{} expected-data)]
+                   (merge {:expected {:data expected-data}
+                           :actual   {:data trimmed-actual-data}}
                           (if (and (some? actual-data)
                                    (mt/ordered-subset? expected-data trimmed-actual-data approx=))
                             {:type :pass}
-                            {:type :fail
-                             :diffs [actual-data [(set/difference expected-set actual-set)
-                                                  (set/difference actual-set expected-set)]]})))
+                            {:type  :fail
+                             :diffs [trimmed-actual-data [(set/difference expected-set actual-set)
+                                                          (set/difference actual-set expected-set)]]})))
                  ;; Unexpected
                  (if (none-found? unexpected actual-data)
                    {:type :pass}
@@ -155,6 +202,13 @@
   `(do-report
     (qv=-report ~message ~expected ~actual ~unexpected)))
 
+(deftest premium-feature-test
+  (testing "It requires the premium feature"
+    (mt/with-premium-features #{}
+      (is (= (str "Query Reference Validation is a paid feature not currently available to your instance. Please upgrade to"
+                  " use it. Learn more at metabase.com/upgrade/")
+             (mt/user-http-request :crowberto :get 402 url))))))
+
 (deftest list-invalid-cards-basic-test
   (testing "Only returns cards with problematic field refs"
     (with-test-setup
@@ -163,21 +217,16 @@
                 [{:id     card-1
                   :name   "A"
                   :errors [{:type "inactive-field", :table "T1", :field "FA"}
-                           {:type "inactive-field", :table "T1", :field "FAB"}]}
+                           {:type "unknown-field",  :table "T1", :field "FAB"}]}
                  {:id     card-2
                   :name   "B"
-                  :errors [{:type "inactive-field", :table "T1", :field "FB"}]}
+                  :errors [{:type "inactive-table", :table "T2"}]}
                  {:id     card-3
                   :name   "C"
-                  :errors [{:type "inactive-table", :table "T2", :field "FC"}]}]}
+                  :errors [{:type "unknown-table",  :table "T3"}]}]}
                (get!)
                [{:id   card-4
-                 :name "D"}]))))
-  (testing "It requires the premium feature"
-    (mt/with-premium-features #{}
-      (is (= (str "Query Reference Validation is a paid feature not currently available to your instance. Please upgrade to"
-                  " use it. Learn more at metabase.com/upgrade/")
-             (mt/user-http-request :crowberto :get 402 url))))))
+                 :name "D"}])))))
 
 (deftest pagination-test
   (testing "Lets you page results"
@@ -189,10 +238,10 @@
                 [{:id     card-1
                   :name   "A"
                   :errors [{:type "inactive-field", :table "T1", :field "FA"}
-                           {:type "inactive-field", :table "T1", :field "FAB"}]}
+                           {:type "unknown-field",  :table "T1", :field "FAB"}]}
                  {:id     card-2
                   :name   "B"
-                  :errors [{:type "inactive-field", :table "T1", :field "FB"}]}]}
+                  :errors [{:type "inactive-table", :table "T2"}]}]}
                (get! {:limit 2})
                [{:id   card-3
                  :name "C"}
@@ -204,7 +253,7 @@
                 :data
                 [{:id     card-3
                   :name   "C"
-                  :errors [{:type "inactive-table", :table "T2", :field "FC"}]}]}
+                  :errors [{:type "unknown-table", :table "T3"}]}]}
                (get! {:limit 1 :offset 2})
                [{:id   card-1
                  :name "A"}

resources/migrations/001_update_migrations.yaml

@@ -8505,6 +8505,209 @@ databaseChangeLog:
                   constraints:
                     nullable: true
 
+  - changeSet:
+      id: v51.2024-07-25T11:56:27
+      author: crisptrutski
+      comment: Wipe stale query fields, so we can add new non-nullable columns
+      rollback: # none, entries will be recreated by sweeper
+      changes:
+        - delete:
+            tableName: query_field
+            remarks: remove stale analysis, so the new fields can be non-nullable
+
+  - changeSet:
+      id: v51.2024-07-25T11:57:27
+      author: crisptrutski
+      comment: Add `column` column to query fields
+      preConditions:
+        - not:
+          - columnExists:
+              tableName: query_field
+              columnName: column
+      changes:
+        - addColumn:
+            tableName: query_field
+            columns:
+              - column:
+                  name: column
+                  type: varchar(254) # matching metabase_field.name
+                  remarks: name of the table or card being referenced
+                  constraints:
+                    nullable: false
+
+  - changeSet:
+      id: v51.2024-07-25T11:58:27
+      author: crisptrutski
+      comment: Add `table` column to query fields
+      preConditions:
+        - not:
+          - columnExists:
+              tableName: query_field
+              columnName: table
+      changes:
+        - addColumn:
+            tableName: query_field
+            columns:
+              - column:
+                  name: table
+                  type: varchar(254) # matching metabase_table.name
+                  remarks: name of the table or card being referenced
+                  constraints:
+                    nullable: false
+
+  - changeSet:
+      id: v51.2024-07-25T12:02:21
+      author: crisptrutski
+      comment: Make `field_id` nullable on query fields
+      changes:
+        - dropNotNullConstraint:
+            tableName: query_field
+            columnDataType: int
+            columnName: field_id
+            remarks: This value is null if the field does not exist, or is created within a model
+
+  - changeSet:
+      id: v51.2024-07-26T11:56:27
+      author: crisptrutski
+      comment: Add `table_id` column to query fields
+      preConditions:
+        - not:
+          - columnExists:
+              tableName: query_field
+              columnName: table_id
+      changes:
+        - addColumn:
+            tableName: query_field
+            columns:
+              - column:
+                  name: table_id
+                  type: int
+                  remarks: track the table directly, in case the field does not exist
+                  constraints:
+                    nullable: true
+
+
+  - changeSet:
+      id: v50.2024-07-29T09:22:12
+      author: crisptrutski
+      comment: Add the query_analysis table
+      changes:
+        - createTable:
+            tableName: query_analysis
+            remarks: Parent node for query analysis records
+            columns:
+              - column:
+                  name: id
+                  remarks: PK
+                  type: int
+                  autoIncrement: true
+                  constraints:
+                    primaryKey: true
+                    nullable: false
+              - column:
+                  name: card_id
+                  remarks: referenced card
+                  type: int
+                  constraints:
+                    nullable: false
+                    referencedTableName: report_card
+                    referencedColumnNames: id
+                    foreignKeyName: fk_query_analysis_card_id
+                    deleteCascade: true
+              - column:
+                  remarks: The timestamp of when the analysis was created
+                  name: created_at
+                  type: ${timestamp_type}
+                  defaultValueComputed: current_timestamp
+                  constraints:
+                    nullable: false
+              - column:
+                  remarks: The timestamp of when the analysis was updated
+                  name: updated_at
+                  type: ${timestamp_type}
+                  defaultValueComputed: current_timestamp
+                  constraints:
+                    nullable: false
+
+  - changeSet:
+      id: v50.2024-07-29T09:04:13
+      author: crisptrutski
+      comment: Add the query_table join table
+      changes:
+        - createTable:
+            tableName: query_table
+            remarks: Tables used by a card's query
+            columns:
+              - column:
+                  name: id
+                  remarks: PK
+                  type: int
+                  autoIncrement: true
+                  constraints:
+                    primaryKey: true
+                    nullable: false
+              - column:
+                  name: card_id
+                  remarks: referenced card
+                  type: int
+                  constraints:
+                    nullable: false
+                    referencedTableName: report_card
+                    referencedColumnNames: id
+                    foreignKeyName: fk_query_table_card_id
+                    deleteCascade: true
+              - column:
+                  name: analysis_id
+                  remarks: round of analysis
+                  type: int
+                  constraints:
+                    nullable: false
+                    referencedTableName: query_analysis
+                    referencedColumnNames: id
+                    foreignKeyName: fk_query_table_analysis_id
+                    deleteCascade: true
+              - column:
+                  name: table_id
+                  remarks: referenced field
+                  type: int
+                  constraints:
+                    nullable: true
+                    referencedTableName: metabase_table
+                    referencedColumnNames: id
+                    foreignKeyName: fk_query_table_table_id
+                    deleteCascade: true
+              - column:
+                  name: schema
+                  type: varchar(254) # matching metabase_table.schema
+                  remarks: name of the schema of the table being referenced
+                  constraints:
+                    nullable: true
+              - column:
+                  name: table
+                  type: varchar(254) # matching metabase_table.name
+                  remarks: name of the table or card being referenced
+                  constraints:
+                    nullable: false
+
+  - changeSet:
+      id: v50.2024-07-29T10:04:13
+      author: crisptrutski
+      changes:
+        - addColumn:
+            tableName: query_field
+            columns:
+              - column:
+                  name: analysis_id
+                  remarks: round of analysis
+                  type: int
+                  constraints:
+                    nullable: false
+                    referencedTableName: query_analysis
+                    referencedColumnNames: id
+                    foreignKeyName: fk_query_field_analysis_id
+                    deleteCascade: true
+
+
   # >>>>>>>>>> DO NOT ADD NEW MIGRATIONS BELOW THIS LINE! ADD THEM ABOVE <<<<<<<<<<
 
 ########################################################################################################################

src/metabase/models.clj

@@ -34,9 +34,11 @@
    [metabase.models.pulse-card :as pulse-card]
    [metabase.models.pulse-channel :as pulse-channel]
    [metabase.models.pulse-channel-recipient :as pulse-channel-recipient]
+   [metabase.models.query-analysis :as query-analysis]
    [metabase.models.query-cache :as query-cache]
    [metabase.models.query-execution :as query-execution]
    [metabase.models.query-field :as query-field]
+   [metabase.models.query-table :as query-table]
    [metabase.models.revision :as revision]
    [metabase.models.secret :as secret]
    [metabase.models.segment :as segment]
@@ -90,7 +92,9 @@
          pulse/keep-me
          query-cache/keep-me
          query-execution/keep-me
+         query-analysis/keep-me
          query-field/keep-me
+         query-table/keep-me
          revision/keep-me
          secret/keep-me
          segment/keep-me

src/metabase/models/query_analysis.clj

@@ -0,0 +1,111 @@
+(ns metabase.models.query-analysis
+  (:require
+   [metabase.util :as u]
+   [methodical.core :as methodical]
+   [toucan2.core :as t2]))
+
+(methodical/defmethod t2/table-name :model/QueryAnalysis [_model] :query_analysis)
+
+(doto :model/QueryAnalysis
+  (derive :metabase/model))
+
+(defn- coerce-boolean [x]
+  (if (= 0 x) false x))
+
+(defn- coerce-booleans [m]
+  (update-vals m coerce-boolean))
+
+(defn cards-with-reference-errors
+  "Given some HoneySQL query map with :model/Card bound as :c, restrict this query to only return cards
+  with invalid references."
+  [card-query-map]
+  (-> card-query-map
+      (update :join concat
+              [[(t2/table-name :model/QueryField) :qf]
+               [:and
+                [:= :qf.card_id :c.id]
+                [:= :qf.explicit_reference true]]])
+      (update :left-join concat
+              [[(t2/table-name :model/Field) :f]
+               [:= :qf.field_id :f.id]])
+      (update :where
+              (fn [existing-where]
+                [:and
+                 existing-where
+                 [:or
+                  [:= :f.id nil]
+                  [:= :f.active false]]]))))
+
+(defn- group-errors [errors]
+  (reduce (fn [acc [id error]]
+            (update acc id u/conjv error))
+          {}
+          errors))
+
+(defn field-reference-errors
+  "Given a seq of cards, return a map of card-id => reference errors"
+  [cards]
+  (when (seq cards)
+    (->> (t2/query {:select    [:qf.card_id
+                                [:qf.column :field]
+                                [:qf.table :table]
+                                [[:= :f.id nil] :field_unknown]
+                                [[:not [:coalesce :f.active true]] :field_inactive]]
+                    :from      [[(t2/table-name :model/QueryField) :qf]]
+                    :left-join [[(t2/table-name :model/Field) :f] [:= :f.id :qf.field_id]
+                                [(t2/table-name :model/Table) :t] [:= :t.id :qf.table_id]]
+                    :where     [:and
+                                [:= :qf.explicit_reference true]
+                                [:= :t.active true]
+                                [:or
+                                 [:= :f.id nil]
+                                 [:= :f.active false]]
+                                [:in :card_id (map :id cards)]]
+                    :order-by  [:qf.card_id :table :field]})
+         (map coerce-booleans)
+         (map (fn [{:keys [card_id table field
+                           field_unknown
+                           field_inactive]}]
+                [card_id {:type  (cond
+                                   field_unknown  :unknown-field
+                                   field_inactive :inactive-field
+                                   ;; This shouldn't be reachable
+                                   :else          :unknown-error)
+                          :table table
+                          :field field}]))
+         group-errors)))
+
+(defn- table-reference-errors
+  [cards]
+  (when (seq cards)
+    (->> (t2/query {:select    [:qt.card_id
+                                [:qt.table :table]
+                                [[:= :t.id nil] :table_unknown]
+                                [[:not [:coalesce :t.active true]] :table_inactive]]
+                    :from      [[(t2/table-name :model/QueryTable) :qt]]
+                    :left-join [[(t2/table-name :model/Table) :t] [:= :t.id :qt.table_id]]
+                    :where     [:and
+                                [:or
+                                 [:= :t.id nil]
+                                 [:= :t.active false]]
+                                [:in :card_id (map :id cards)]]
+                    :order-by  [:qt.card_id :table]})
+         (map coerce-booleans)
+         (map (fn [{:keys [card_id table
+                           table_unknown
+                           table_inactive]}]
+                [card_id {:type  (cond
+                                   table_unknown  :unknown-table
+                                   table_inactive :inactive-table
+                                   ;; This shouldn't be reachable
+                                   :else          :unknown-error)
+                          :table table}]))
+
+         group-errors)))
+
+(defn reference-errors
+  "blah"
+  [cards]
+  (merge-with concat
+    (field-reference-errors cards)
+    (table-reference-errors cards)))

src/metabase/models/query_field.clj

@@ -1,6 +1,5 @@
 (ns metabase.models.query-field
   (:require
-   [metabase.util :as u]
    [methodical.core :as methodical]
    [toucan2.core :as t2]))
 
@@ -8,76 +7,3 @@
 
 (doto :model/QueryField
   (derive :metabase/model))
-
-(def ^:private reference-error-joins
-  [[(t2/table-name :model/QueryField) :qf]
-   [:and
-    [:= :qf.card_id :c.id]
-    [:= :qf.explicit_reference true]]
-
-   [(t2/table-name :model/Field) :f]
-   [:and
-    [:= :qf.field_id :f.id]
-    [:= :f.active false]]])
-
-(defn cards-with-reference-errors
-  "Given some HoneySQL query map with :model/Card bound as :c, restrict this query to only return cards
-  with invalid references.
-  For now this only handles inactive references, but in future it will also handle unknown references too."
-  [card-query-map]
-  ;; NOTE: We anticipate a schema change to support missing references that will result in left-joins and
-  ;; where clauses being appended as well.
-  (update card-query-map :join concat reference-error-joins))
-
-(defn reference-errors
-  "Given a seq of cards, return a map of card-id => reference errors"
-  [cards]
-  (when (seq cards)
-    (->> (t2/select :model/QueryField
-                    {:select [:qf.card_id
-                              [:f.name :field]
-                              [:t.name :table]
-                              [:t.active :table_active]]
-                     :from   [[(t2/table-name :model/QueryField) :qf]]
-                     :join   [[(t2/table-name :model/Field) :f] [:= :f.id :qf.field_id]
-                              [(t2/table-name :model/Table) :t] [:= :t.id :f.table_id]]
-                     :where  [:and
-                              [:= :qf.explicit_reference true]
-                              [:= :f.active false]
-                              [:in :card_id (map :id cards)]]
-                     :order-by [:qf.card_id :t.name :f.name]})
-         (map (fn [{:keys [card_id table field table_active]}]
-                [card_id {:type  (if table_active
-                                   :inactive-field
-                                   :inactive-table)
-                          :table table
-                          :field field}]))
-         (reduce (fn [acc [id error]]
-                   (update acc id u/conjv error))
-                 {}))))
-
-;;; Updating QueryField from card
-
-(defn update-query-fields-for-card!
-  "Clears QueryFields associated with this card and creates fresh, up-to-date-ones.
-
-  Returns `nil` (and logs the error) if there was a parse error."
-  [card-id query-field-rows]
-  (t2/with-transaction [_conn]
-    (let [existing            (t2/select :model/QueryField :card_id card-id)
-          {:keys [to-update
-                  to-create
-                  to-delete]} (u/row-diff existing query-field-rows
-                                          {:id-fn      :field_id
-                                           :to-compare #(dissoc % :id :card_id :field_id)})]
-      (when (seq to-delete)
-        ;; this deletion seems to break transaction (implicit commit or something) on MySQL, and this `diff`
-        ;; algo drops its frequency by a lot - which should help with transactions affecting each other a
-        ;; lot. Parallel tests in `metabase.models.query.permissions-test` were breaking when delete was
-        ;; executed unconditionally on every query change.
-        (t2/delete! :model/QueryField :card_id card-id :field_id [:in (map :field_id to-delete)]))
-      (when (seq to-create)
-        (t2/insert! :model/QueryField to-create))
-      (doseq [item to-update]
-        (t2/update! :model/QueryField {:card_id card-id :field_id (:field_id item)}
-                    (select-keys item [:explicit_reference]))))))

src/metabase/models/query_table.clj

@@ -0,0 +1,9 @@
+(ns metabase.models.query-table
+  (:require
+   [methodical.core :as methodical]
+   [toucan2.core :as t2]))
+
+(methodical/defmethod t2/table-name :model/QueryTable [_model] :query_table)
+
+(doto :model/QueryField
+  (derive :metabase/model))

src/metabase/query_analysis.clj

@@ -10,7 +10,6 @@
    [metabase.legacy-mbql.util :as mbql.u]
    [metabase.lib.core :as lib]
    [metabase.lib.util :as lib.util]
-   [metabase.models.query-field :as query-field]
    [metabase.public-settings :as public-settings]
    [metabase.query-analysis.native-query-analyzer :as nqa]
    [metabase.query-analysis.native-query-analyzer.replacement :as nqa.replacement]
@@ -80,7 +79,7 @@
          :mbql/query true
          false)))
 
-(defn- explicit-references [field-ids]
+(defn- explicit-field-references [field-ids]
   (when (seq field-ids)
     ;; We add this on in code as `true` in MySQL-based drivers would be returned as 1.
     (map #(assoc % :explicit-reference true)
@@ -90,6 +89,11 @@
                                        :join   [[(t2/table-name :model/Table) :t] [:= :t.id :f.table_id]]
                                        :where  [:in :f.id field-ids]}))))
 
+(defn- explicit-references [field-ids]
+  (let [field-refs (explicit-field-references field-ids)]
+    {:fields field-refs
+     :tables (distinct (map #(dissoc % :field-id :field) field-refs))}))
+
 (defn- query-references
   "Find out ids of all fields used in a query. Conforms to the same protocol as [[query-analyzer/field-ids-for-sql]],
   so returns `{:explicit #{...int ids}}` map.
@@ -117,15 +121,28 @@
   [{card-id :id, query :dataset_query}]
   (let [query-type (lib/normalized-query-type query)]
     (when (enabled-type? query-type)
-      (let [references       (query-references query query-type)
-            reference->row   (fn [{:keys [field-id explicit-reference]}]
-                               ;; For now we only persist references which resolve to known fields
-                               (when field-id
+      (t2/with-transaction [_conn]
+        (let [analysis-id      (t2/insert-returning-pk! :model/QueryAnalysis {:card_id card-id})
+              references       (query-references query query-type)
+              table->row       (fn [{:keys [schema table table-id]}]
+                                 {:card_id     card-id
+                                  :analysis_id analysis-id
+                                  :schema      schema
+                                  :table       table
+                                  :table_id    table-id})
+              field->row       (fn [{:keys [table column table-id field-id explicit-reference]}]
                                  {:card_id            card-id
+                                  :analysis_id        analysis-id
+                                  :table              table
+                                  :column             column
+                                  :table_id           table-id
                                   :field_id           field-id
-                                  :explicit_reference explicit-reference}))
-            query-field-rows (map reference->row references)]
-        (query-field/update-query-fields-for-card! card-id query-field-rows)))))
+                                  :explicit_reference explicit-reference})
+              query-field-rows (map field->row (:fields references))
+              query-table-rows (map table->row (:tables references))]
+          (t2/insert! :model/QueryField query-field-rows)
+          (t2/insert! :model/QueryTable query-table-rows)
+          (t2/delete! :model/QueryAnalysis {:where [:not= :id analysis-id]}))))))
 
 (defn- replaced-inner-query-for-native-card
   "Substitute new references for certain fields and tables, based upon the given mappings."

src/metabase/query_analysis/native_query_analyzer.clj

@@ -30,11 +30,11 @@
 
 (def ^:private field-and-table-fragment
   "HoneySQL fragment to get the Field and Table"
-  {:select [[:f.id :field-id] [:f.name :column]
-            [:t.id :table-id] [:t.name :table]]
-   :from   [[:metabase_field :f]]
+  {:select    [[:f.id :field-id] [:f.name :column]
+               [:t.id :table-id] [:t.name :table]]
    ;; (t2/table-name :model/Table) doesn't work on CI since models/table.clj hasn't been loaded
-   :join   [[:metabase_table :t] [:= :table_id :t.id]]})
+   :from      [[:metabase_table :t]]
+   :left-join [[:metabase_field :f] [:= :f.table_id :t.id]]})
 
 ;; NOTE: be careful when adding square braces, as the rules for nesting them are different.
 (def ^:private quotes "\"`")
@@ -89,37 +89,23 @@
   "Generates the query for a column, incorporating its concrete table information (if known) or matching it against
   the provided list of all possible tables."
   [tables column]
-  (if (:table column)
-    [:and
-     (field-query :f.name (:column column))
-     (table-query column)]
-    [:and
-     (field-query :f.name (:column column))
-     (into [:or] (map table-query tables))]))
+  [:and
+   [:or
+    [:= :f.id nil]
+    (field-query :f.name (:column column))]
+   (if (:table column)
+     (table-query column)
+     (into [:or] (map table-query tables)))])
 
-(defn- strip-redundant-refs
-  "Strip out duplicate references, and unqualified references that are shadowed by found or qualified ones."
-  [references]
-  (let [qualified? (into #{} (comp (filter :table) (map :column)) references)]
-    (into #{}
-          (filter (fn [{:keys [table column]}]
-                    (or table (not (qualified? column)))))
-          references)))
-
-(defn- consolidate-columns
-  "Qualify analyzed columns with the corresponding database IDs, where we are able to resolve them."
-  [analyzed-columns database-columns]
-  (let [->tab-key             (comp u/lower-case-en :table)
-        ->col-key             (comp u/lower-case-en :column)
-        column->records       (group-by (comp ->col-key) database-columns)
-        table+column->records (group-by (juxt ->tab-key ->col-key) database-columns)]
-    (strip-redundant-refs
-     (mapcat (fn [{:keys [table column] :as reference}]
-               (or (if table
-                     (table+column->records [(normalized-key table) (normalized-key column)])
-                     (column->records (normalized-key column)))
-                   [(update-vals reference strip-quotes)]))
-             analyzed-columns))))
+(defn table-reference
+  "Used by tests"
+  [db-id table]
+  (t2/select-one :model/QueryTable
+                 {:select [[:t.id :table-id] [:t.name :table]]
+                  :from   [[(t2/table-name :model/Table) :t]]
+                  :where  [:and
+                           [:= :t.db_id db-id]
+                           (table-query {:table (name table)})]}))
 
 (defn field-reference
   "Used by tests"
@@ -130,17 +116,100 @@
                                                   (column-query nil {:table  (name table)
                                                                      :column (name column)})])))
 
-(defn- explicit-references-for-query
-  "Selects IDs of Fields that could be used in the query"
-  [{column-maps :columns table-maps :tables} db-id]
+(defn- strip-redundant-refs
+  "Strip out duplicate references, and unqualified references that are shadowed by found or qualified ones."
+  [references]
+  ;; TODO handle schema
+  (let [qualified? (into #{} (comp (filter :table) (map :column)) references)]
+    (into #{}
+          (filter (fn [{:keys [table column]}]
+                    (or table (not (qualified? column)))))
+          references)))
+
+(defn- strip-redundant-table-refs
+  "Strip out duplicate references, and unqualified references that are shadowed by found or qualified ones."
+  [references]
+  (let [qualified? (into #{} (comp (filter :schema) (map :table)) references)]
+    (into #{}
+          (filter (fn [{:keys [schema table]}]
+                    (or schema (not (qualified? table)))))
+          references)))
+
+(defn- consolidate-columns
+  "Qualify analyzed columns with the corresponding database IDs, where we are able to resolve them."
+  [analyzed-columns database-columns]
+  ;; TODO we should handle schema as well
+  (let [->tab-key             (comp u/lower-case-en :table)
+        ->col-key             (comp u/lower-case-en :column)
+        column->records       (group-by ->col-key database-columns)
+        table+column->records (group-by (juxt ->tab-key ->col-key) database-columns)]
+    (strip-redundant-refs
+     (mapcat (fn [{:keys [table column] :as reference}]
+               (or (if table
+                     (table+column->records [(normalized-key table) (normalized-key column)])
+                     (column->records (normalized-key column)))
+                   [(update-vals reference strip-quotes)]))
+             analyzed-columns))))
+
+(defn- consolidate-tables [analyzed-tables database-tables]
+  (let [->schema-key          (comp u/lower-case-en :schema)
+        ->table-key           (comp u/lower-case-en :table)
+        table->records        (group-by ->table-key database-tables)
+        schema+table->records (group-by (juxt ->schema-key ->table-key) database-tables)]
+    (strip-redundant-table-refs
+     (mapcat (fn [{:keys [schema table] :as reference}]
+               (or (if schema
+                     (schema+table->records [(normalized-key schema) (normalized-key table)])
+                     (table->records (normalized-key table)))
+                   [(update-vals reference strip-quotes)]))
+             analyzed-tables))))
+
+(defn- table-refs-for-query
+  "Given the results of query analysis, return references to the corresponding tables and cards."
+  [{table-maps :tables} db-id]
+  (let [tables (map :component table-maps)]
+    (consolidate-tables
+     tables
+     (t2/select :model/QueryTable
+                {:select [[:t.id :table-id] [:t.name :table]]
+                 :from   [[(t2/table-name :model/Table) :t]]
+                 :where  [:and
+                         [:= :t.db_id db-id]
+                         (into [:or] (map table-query tables))]}))))
+
+(defn- fill-missing-table-ids-hack
+  "See if we can qualify the schema and table-id for any explicit field refs which couldn't resolve their field"
+  [table-refs field-refs]
+  ;; Note, at this point we have given up on any pretense of respecting case sensitivity
+  (let [t->ids  (group-by (comp u/lower-case-en :table) table-refs)
+        s+t->id (u/index-by (juxt (comp u/lower-case-en :schema)
+                                  (comp u/lower-case-en :table))
+                            table-refs)]
+    (into (empty field-refs)
+          (mapcat (fn [{:keys [schema table table-id] :as ref}]
+                    (if table-id
+                      [ref]
+                      (or (if schema
+                            (when-let [table-ref (s+t->id [(u/lower-case-en schema)
+                                                           (u/lower-case-en table)])]
+                              [(merge ref table-ref)])
+                            (seq (map #(merge ref %) (t->ids (u/lower-case-en table)))))
+                          [ref]))))
+          field-refs)))
+
+(defn- explicit-field-refs-for-query
+  "Given the results of query analysis, return references to the corresponding fields and model outputs."
+  [{column-maps :columns table-maps :tables} db-id table-refs]
   (let [columns (map :component column-maps)
         tables  (map :component table-maps)]
-    (consolidate-columns
-     columns
-     (t2/select :model/QueryField (assoc field-and-table-fragment
-                                         :where [:and
-                                                 [:= :t.db_id db-id]
-                                                 (into [:or] (map (partial column-query tables) columns))])))))
+    (fill-missing-table-ids-hack
+     table-refs
+     (consolidate-columns
+      columns
+      (t2/select :model/QueryField (assoc field-and-table-fragment
+                                          :where [:and
+                                                  [:= :t.db_id db-id]
+                                                  (into [:or] (map (partial column-query tables) columns))]))))))
 
 (defn- wildcard-tables
   "Given a parsed query, return the list of tables we are selecting from using a wildcard."
@@ -160,11 +229,12 @@
   "Similar to explicit-field-ids-for-query, but for wildcard selects"
   [parsed-query db-id]
   (when-let [tables (wildcard-tables parsed-query)]
-    (t2/select :model/QueryField (merge field-and-table-fragment
-                                        {:where [:and
-                                                 [:= :t.db_id db-id]
-                                                 [:= :f.active true]
-                                                 (into [:or] (map table-query tables))]}))))
+    (set
+     (t2/select :model/QueryField (merge field-and-table-fragment
+                                         {:where [:and
+                                                  [:= :t.db_id db-id]
+                                                  [:= :f.active true]
+                                                  (into [:or] (map table-query tables))]})))))
 
 (defn- mark-reference [refs explicit?]
   (map #(assoc % :explicit-reference explicit?) refs))
@@ -181,12 +251,15 @@
         macaw-opts    (nqa.impl/macaw-options driver)
         sql-string    (:query (nqa.sub/replace-tags query))
         parsed-query  (macaw/query->components (macaw/parsed-query sql-string macaw-opts) macaw-opts)
-        explicit-refs (explicit-references-for-query parsed-query db-id)
-        implicit-refs (set/difference (set (implicit-references-for-query parsed-query db-id))
-                                      (set explicit-refs))]
-    ;; TODO: add table refs
-    (concat (mark-reference explicit-refs true)
-            (mark-reference implicit-refs false))))
+        table-refs    (table-refs-for-query parsed-query db-id)
+        explicit-refs (explicit-field-refs-for-query parsed-query db-id table-refs)
+        implicit-refs (-> (implicit-references-for-query parsed-query db-id)
+                          (set/difference explicit-refs))
+        field-refs    (concat (mark-reference explicit-refs true)
+                              (mark-reference implicit-refs false))
+        ]
+    {:tables table-refs
+     :fields field-refs}))
 
 (defn references-for-native
   "Returns a `{:explicit #{...} :implicit #{...}}` map with field IDs that (may) be referenced in the given card's

src/metabase/sync/analyze/query_results.clj

@@ -0,0 +1 @@
+(ns metabase.sync.analyze.query-results)

src/metabase/util.cljc

@@ -1031,3 +1031,10 @@
      "Return how many milliseconds have elapsed since the given timer was started."
      [timer]
      (/ (- (System/nanoTime) timer) 1e6)))
+
+(defn index-by
+  "(index-by first second [[1 3] [1 4] [2 5]]) => {1 4, 2 5}"
+ ([kf coll]
+  (reduce (fn [acc v] (assoc acc (kf v) v)) {} coll))
+ ([kf vf coll]
+  (reduce (fn [acc v] (assoc acc (kf v) (vf v))) {} coll)))

test/metabase/models/query_field_test.clj

@@ -4,18 +4,20 @@
    [clojure.test :refer :all]
    [metabase.query-analysis :as query-analysis]
    [metabase.test :as mt]
+   [metabase.util :as u]
    [toucan2.core :as t2]
    [toucan2.tools.with-temp :as t2.with-temp]))
 
-(def ^:private query-field-keys [:card_id :field_id :explicit_reference])
+(def ^:private query-field-keys [:card_id :table :column :table_id :field_id :explicit_reference])
 
 (defn- qf->map [query-field]
-  (select-keys query-field query-field-keys))
+  (-> (select-keys query-field query-field-keys)
+      (update :table u/lower-case-en)
+      (update :column u/lower-case-en)))
 
 (defn- query-fields-for-card
   [card-id]
-  (t2/select-fn-set qf->map :model/QueryField
-                    :card_id card-id))
+  (t2/select-fn-set qf->map :model/QueryField :card_id card-id))
 
 (defn- do-with-test-setup [f]
   (query-analysis/with-immediate-analysis
@@ -52,15 +54,33 @@
 
 (deftest query-fields-created-by-queries-test
   (with-test-setup
-    (let [total-qf {:card_id            card-id
-                    :field_id           total-id
-                    :explicit_reference true}
-          tax-qf   {:card_id            card-id
-                    :field_id           tax-id
-                    :explicit_reference true}]
+    (let [total-qf     {:card_id            card-id
+                        :table              "orders"
+                        :column             "total"
+                        :table_id           table-id
+                        :field_id           total-id
+                        :explicit_reference true}
+          tax-qf       {:card_id            card-id
+                        :table              "orders"
+                        :column             "tax"
+                        :table_id           table-id
+                        :field_id           tax-id
+                        :explicit_reference true}
+          not-total-qf {:card_id            card-id
+                        :table              "orders"
+                        :column             "not_total"
+                        :table_id           table-id
+                        :field_id           nil
+                        :explicit_reference true}
+          not-tax      {:card_id            card-id
+                        :table              "orders"
+                        :column             "not_tax"
+                        :table_id           table-id
+                        :field_id           nil
+                        :explicit_reference true}]
 
       (testing "A freshly created card has relevant corresponding QueryFields"
-        (is (= #{total-qf}
+        (is (= #{total-qf not-tax}
                (query-fields-for-card card-id))))
 
       (testing "Adding new columns to the query also adds the QueryFields"
@@ -70,7 +90,7 @@
 
       (testing "Removing columns from the query removes the QueryFields"
         (trigger-parse! card-id "SELECT tax, not_total FROM orders")
-        (is (= #{tax-qf}
+        (is (= #{tax-qf not-total-qf}
                (query-fields-for-card card-id))))
 
       (testing "Columns referenced via field filters are still found"
@@ -86,18 +106,40 @@
         (is (= #{tax-qf total-qf}
                (query-fields-for-card card-id)))))))
 
-(deftest bogus-queries-test
+(deftest unknown-test
   (with-test-setup
-    (testing "Updating a query with bogus columns does not create QueryFields"
-      (trigger-parse! card-id "SELECT DOES, NOT_EXIST FROM orders")
-      (is (empty? (t2/select :model/QueryField :card_id card-id))))))
+    (testing "selecting an unknown column from an known table"
+      (let [qux-qf {:card_id            card-id
+                    :table              "orders"
+                    :column             "qux"
+                    :table_id           table-id
+                    :field_id           nil
+                    :explicit_reference true}]
+        (trigger-parse! card-id "select qux from orders")
+        (is (= #{qux-qf} (query-fields-for-card card-id)))))
+
+    (testing "selecting an unknown column from an unknown table"
+      (let [qux-qf {:card_id            card-id
+                    :table              "borders"
+                    :column             "qux"
+                    :table_id           nil
+                    :field_id           nil
+                    :explicit_reference true}]
+        (trigger-parse! card-id "select qux from borders")
+        (is (= #{qux-qf} (query-fields-for-card card-id)))))))
 
 (deftest wildcard-test
   (with-test-setup
     (let [total-qf {:card_id          card-id
+                    :table            "orders"
+                    :column           "total"
+                    :table_id         table-id
                     :field_id         total-id
                     :explicit_reference false}
           tax-qf   {:card_id          card-id
+                    :table            "orders"
+                    :column           "tax"
+                    :table_id         table-id
                     :field_id         tax-id
                     :explicit_reference false}]
       (testing "simple select *"
@@ -110,9 +152,15 @@
 (deftest table-wildcard-test
   (with-test-setup
     (let [total-qf {:card_id          card-id
+                    :table            "orders"
+                    :column           "total"
+                    :table_id         table-id
                     :field_id         total-id
                     :explicit_reference true}
           tax-qf   {:card_id          card-id
+                    :table            "orders"
+                    :column           "tax"
+                    :table_id         table-id
                     :field_id         tax-id
                     :explicit_reference true}]
       (testing "mix of select table.* and named columns"

test/metabase/query_analysis/native_query_analyzer_test.clj

@@ -39,14 +39,21 @@
                       {:field-id 3 :table "t3" :column "c3"}]))))))
 
 (defn- refs [sql]
-  (->> (mt/native-query {:query sql})
-       (#'nqa/references-for-native)
-       (sort-by (juxt :table :column))))
+  (-> (mt/native-query {:query sql})
+      (#'nqa/references-for-native)
+      (update-vals
+       (partial sort-by (juxt :table :column)))))
 
-(defn- missing-reference [table column]
-  {:table              (name table)
-   :column             (name column)
-   :explicit-reference true})
+(defn- field-refs [sql]
+  (:fields (refs sql)))
+
+(defn- table-reference [table]
+  (let [reference (nqa/table-reference (mt/id) table)]
+    ;; sanity-check that this is the right reference
+    (assert (= (mt/id table) (:table-id reference)))
+    ;; sanity-check the names, whose case depends on the driver
+    (assert (= (name table) (u/lower-case-en (:table reference))))
+    reference))
 
 (defn- field-reference [table column]
   (let [reference (nqa/field-reference (mt/id) table column)]
@@ -59,51 +66,81 @@
     ;; tag it
     (assoc reference :explicit-reference true)))
 
+(defn- missing-reference [table column]
+  (merge
+   {:table              (name table)
+    :column             (name column)
+    :explicit-reference true}
+   ;; the table might be be resolved...
+   (nqa/table-reference (mt/id) table)))
+
 (deftest ^:parallel field-matching-simple-test
   (testing "simple query matches"
     (is (= [(field-reference :venues :id)]
-           (refs "select id from venues")))))
+           (field-refs "select id from venues")))))
 
 (deftest ^:parallel field-matching-case-test
   (testing "quotes stop case matching"
     (is (= [(missing-reference :venues :id)]
-           (refs "select \"id\" from venues")))
+           (field-refs "select \"id\" from venues")))
     (is (= [(field-reference :venues :id)]
-           (refs "select \"ID\" from venues"))))
+           (field-refs "select \"ID\" from venues"))))
   (testing "unresolved references use case verbatim"
     (is (= [{:column "id", :table "unKnown", :explicit-reference true}]
-           (refs "select \"id\" from unKnown")))
+           (field-refs "select \"id\" from unKnown")))
     (is (= [{:column "ID", :table "unknowN", :explicit-reference true}]
-           (refs "select ID from unknowN"))))
+           (field-refs "select ID from unknowN"))))
   (testing "resolved references normalize the case"
-    (is (not= "veNUES" (:table (first (refs "select id from veNUES")))))
-    (is (= "venues" (u/lower-case-en (:table (first (refs "select id from veNUES")))))))
+    (is (not= "veNUES" (:table (first (field-refs "select id from veNUES")))))
+    (is (= "venues" (u/lower-case-en (:table (first (field-refs "select id from veNUES")))))))
   (testing "you can mix quoted and unquoted names"
     (is (= [(field-reference :venues :id)
             (field-reference :venues :name)]
-           (refs "select v.\"ID\", v.name from venues v")))
+           (field-refs "select v.\"ID\", v.name from venues v")))
     (is (= [(field-reference :venues :id)
             (field-reference :venues :name)]
-           (refs "select v.`ID`, v.name from venues v")))))
+           (field-refs "select v.`ID`, v.name from venues v")))))
 
 (deftest ^:parallel field-matching-multi-test
   (testing "It will find all relevant columns if query is not specific"
     (is (= [(field-reference :checkins :id)
             (field-reference :venues :id)]
-           (refs "select id from venues join checkins"))))
+           (field-refs "select id from venues join checkins"))))
   (testing "But if you are specific - then it's a concrete field"
     (is (= [(field-reference :venues :id)]
-           (refs "select v.id from venues v join checkins"))))
+           (field-refs "select v.id from venues v join checkins"))))
   (testing "And wildcards are matching everything"
     (is (= {false 10}
-           (frequencies (map :explicit-reference (refs "select * from venues v join checkins")))))
+           (frequencies (map :explicit-reference (field-refs "select * from venues v join checkins")))))
     (is (= {false 6}
-           (frequencies (map :explicit-reference (refs "select v.* from venues v join checkins")))))))
+           (frequencies (map :explicit-reference (field-refs "select v.* from venues v join checkins")))))))
 
 (deftest ^:parallel field-matching-keywords-test
   (when (not (contains? #{:snowflake :oracle} driver/*driver*))
     (testing "Analysis does not fail due to keywords that are only reserved in other databases"
       (is (= [(field-reference :venues :id)]
-             (refs "select id as final from venues")))
+             (field-refs "select id as final from venues")))
       (is (= [(missing-reference :venues :final)]
-             (refs "select final from venues"))))))
+             (field-refs "select final from venues"))))))
+
+(deftest ^:parallel table-without-field-reference-test
+  (testing "We track table dependencies even when there are no fields being used"
+    (is (= {:tables [(table-reference :venues)]
+            :fields []}
+           (refs "select count(*) from venues")))))
+
+(deftest fill-missing-table-ids-hack-test
+  (testing "Where applicable, we insert the appropriate schemas and table-ids"
+    (is (=
+         [{:schema 4    :table "t1" :table-id 6 :column "a"}
+          {:schema 1    :table "t2" :table-id 3 :column "b"}
+          {:schema 8    :table "t2" :table-id 9 :column "b"}
+          {:schema 7    :table "t2"             :column "c"}]
+         (#'nqa/fill-missing-table-ids-hack
+          ;; tables
+          [{:schema 1   :table "t2" :table-id 3}
+           {:schema 8   :table "t2" :table-id 9}]
+          ;; columns
+          [{:schema 4   :table "t1" :table-id 6 :column "a"}
+           {:schema nil :table "t2"             :column "b"}
+           {:schema 7   :table "t2"             :column "c"}])))))

test/metabase/query_analysis_test.clj

@@ -28,13 +28,16 @@
     (is (false? (query-analysis/enabled-type? :unexpected)))))
 
 (defn- field-id-references [card-or-query]
-  (->> (:dataset_query card-or-query card-or-query)
+  (-> (:dataset_query card-or-query card-or-query)
        (#'query-analysis/query-references)
-       ;; lowercase names to avoid tests being driver-dependent
-       (map #(-> %
-                 (update :table u/lower-case-en)
-                 (update :column u/lower-case-en)))
-       (sort-by (juxt :table :column))))
+       (update-vals
+        (fn [refs]
+          (->> refs
+               ;; lowercase names to avoid tests being driver-dependent
+               (map #(-> %
+                         (update :table u/lower-case-en)
+                         (update :column u/lower-case-en)))
+               (sort-by (juxt :table :column)))))))
 
 (deftest parse-mbql-test
   (testing "Parsing MBQL query returns correct used fields"
@@ -52,12 +55,12 @@
       (is (= (mt/$ids
               [{:table-id (mt/id :venues), :table "venues", :field-id %venues.name, :column "name", :explicit-reference true}
                {:table-id (mt/id :venues), :table "venues", :field-id %venues.price, :column "price", :explicit-reference true}])
-             (field-id-references c1)))
-      (is (empty? (field-id-references c2)))
+             (:fields (field-id-references c1))))
+      (is (empty? (:fields (field-id-references c2))))
       (is (= (mt/$ids
               [{:table-id (mt/id :checkins), :table "checkins", :field-id %checkins.venue_id, :column "venue_id", :explicit-reference true}
                {:table-id (mt/id :venues), :table "venues", :field-id %venues.id, :column "id", :explicit-reference true}])
-             (field-id-references c3)))))
+             (:fields (field-id-references c3))))))
   (testing "Parsing pMBQL query returns correct used fields"
     (let [metadata-provider (lib.metadata.jvm/application-database-metadata-provider (mt/id))
           venues            (lib.metadata/table metadata-provider (mt/id :venues))
@@ -69,7 +72,7 @@
                :field-id (mt/id :venues :name)
                :column "name"
                :explicit-reference true}]
-             (field-id-references mlv2-query))))))
+             (:fields (field-id-references mlv2-query)))))))
 
 (deftest replace-fields-and-tables!-test
   (testing "fields and tables in a native card can be replaced"