source-field populated in aggregates with foreign refs (#38624)

* source-field populated in aggregates with foreign refs When aggregates are populated that reference other tables, either a `:source-field` must be provided in the field metadata or a `:join-alias` needs to be provided. The former is preferred. Prior to this PR, the first situation in the below simplified case could happen: ```clojure ;;Simplified case ;; busted (qp/process-query {:database 1, ;; 59 is "PRODUCTS" -> "PRICE" which is not in the "REVIEWS" table :query {:aggregation [["sum" ["field" 59 {:base-type "type/Float"}]]] ;; "REVIEWS" :source-table 8}, :type "query"}) ; Works (qp/process-query {:database 1, ;; The `:source-field` metadata fixes the above query :query {:aggregation [["sum" ["field" 59 {:base-type "type/Float" ;; "REVIEWS" -> "PRODUCT_ID" :source-field 71}]]] :source-table 8}, :type "query"}) ``` This PR updates `metabase.automagic-dashboards.interestin/ground-metric` to use the `->reference` function for fields when building the `decoder` for transforming field names to references, which properly adds sources for external fields. Fixes #38618 * Updating `grounded-metrics-test` to reflect updated field referencing.
metabase · Feb 12, 2024 · 06d3215 · 06d3215
1 parent 3d86160
commit 06d3215
Show file tree

Hide file tree

Showing 3 changed files with 110 additions and 95 deletions.
diff --git a/src/metabase/automagic_dashboards/interesting.clj b/src/metabase/automagic_dashboards/interesting.clj
@@ -93,6 +93,88 @@
            (group-by :semantic_type))
       set)))
 
+(defmulti
+  ^{:doc      "Get a reference for a given model to be injected into a template
+          (either MBQL, native query, or string)."
+    :arglists '([template-type model])}
+  ->reference (fn [template-type model]
+                [template-type (mi/model model)]))
+
+(defn- optimal-datetime-resolution
+  [field]
+  (let [[earliest latest] (some->> field
+                                   :fingerprint
+                                   :type
+                                   :type/DateTime
+                                   ((juxt :earliest :latest))
+                                   (map u.date/parse))]
+    (if (and earliest latest)
+      ;; e.g. if 3 hours > [duration between earliest and latest] then use `:minute` resolution
+      (condp u.date/greater-than-period-duration? (u.date/period-duration earliest latest)
+        (t/hours 3) :minute
+        (t/days 7) :hour
+        (t/months 6) :day
+        (t/years 10) :month
+        :year)
+      :day)))
+
+(defmethod ->reference [:mbql Field]
+  [_ {:keys [fk_target_field_id id link aggregation name base_type] :as field}]
+  (let [reference (mbql.normalize/normalize
+                    (cond
+                      link [:field id {:source-field link}]
+                      fk_target_field_id [:field fk_target_field_id {:source-field id}]
+                      id [:field id nil]
+                      :else [:field name {:base-type base_type}]))]
+    (cond
+      (isa? base_type :type/Temporal)
+      (mbql.u/with-temporal-unit reference (keyword (or aggregation
+                                                        (optimal-datetime-resolution field))))
+
+      (and aggregation
+           (isa? base_type :type/Number))
+      (mbql.u/update-field-options reference assoc-in [:binning :strategy] (keyword aggregation))
+
+      :else
+      reference)))
+
+(defmethod ->reference [:string Field]
+  [_ {:keys [display_name full-name link]}]
+  (cond
+    full-name full-name
+    link (format "%s → %s"
+                 (-> (t2/select-one Field :id link) :display_name (str/replace #"(?i)\sid$" ""))
+                 display_name)
+    :else display_name))
+
+(defmethod ->reference [:string Table]
+  [_ {:keys [display_name full-name]}]
+  (or full-name display_name))
+
+(defmethod ->reference [:string Metric]
+  [_ {:keys [name full-name]}]
+  (or full-name name))
+
+(defmethod ->reference [:mbql Metric]
+  [_ {:keys [id definition]}]
+  (if id
+    [:metric id]
+    (-> definition :aggregation first)))
+
+(defmethod ->reference [:native Field]
+  [_ field]
+  (field/qualified-name field))
+
+(defmethod ->reference [:native Table]
+  [_ {:keys [name]}]
+  name)
+
+(defmethod ->reference :default
+  [_ form]
+  (or (cond-> form
+        (map? form) ((some-fn :full-name :name) form))
+      form))
+
 (defn transform-metric-aggregate
   "Map a metric aggregate definition from nominal types to semantic types."
   [m decoder]
@@ -118,8 +200,7 @@
          (apply math.combo/cartesian-product)
          (map (partial zipmap named-dimensions))
          (map (fn [nm->field]
-                (let [xform (update-vals nm->field (fn [{field-id :id}]
-                                                     [:field field-id nil]))]
+                (let [xform (update-vals nm->field (partial ->reference :mbql))]
                   {:metric-name           metric-name
                    :metric-title          metric-name
                    :metric-score          metric-score
@@ -309,88 +390,6 @@
                              -1 b))
               {})))
 
-(defmulti
-  ^{:doc      "Get a reference for a given model to be injected into a template
-          (either MBQL, native query, or string)."
-    :arglists '([template-type model])}
-  ->reference (fn [template-type model]
-                [template-type (mi/model model)]))
-
-(defn- optimal-datetime-resolution
-  [field]
-  (let [[earliest latest] (some->> field
-                                   :fingerprint
-                                   :type
-                                   :type/DateTime
-                                   ((juxt :earliest :latest))
-                                   (map u.date/parse))]
-    (if (and earliest latest)
-      ;; e.g. if 3 hours > [duration between earliest and latest] then use `:minute` resolution
-      (condp u.date/greater-than-period-duration? (u.date/period-duration earliest latest)
-        (t/hours 3) :minute
-        (t/days 7) :hour
-        (t/months 6) :day
-        (t/years 10) :month
-        :year)
-      :day)))
-
-(defmethod ->reference [:mbql Field]
-  [_ {:keys [fk_target_field_id id link aggregation name base_type] :as field}]
-  (let [reference (mbql.normalize/normalize
-                    (cond
-                      link [:field id {:source-field link}]
-                      fk_target_field_id [:field fk_target_field_id {:source-field id}]
-                      id [:field id nil]
-                      :else [:field name {:base-type base_type}]))]
-    (cond
-      (isa? base_type :type/Temporal)
-      (mbql.u/with-temporal-unit reference (keyword (or aggregation
-                                                        (optimal-datetime-resolution field))))
-
-      (and aggregation
-           (isa? base_type :type/Number))
-      (mbql.u/update-field-options reference assoc-in [:binning :strategy] (keyword aggregation))
-
-      :else
-      reference)))
-
-(defmethod ->reference [:string Field]
-  [_ {:keys [display_name full-name link]}]
-  (cond
-    full-name full-name
-    link (format "%s → %s"
-                 (-> (t2/select-one Field :id link) :display_name (str/replace #"(?i)\sid$" ""))
-                 display_name)
-    :else display_name))
-
-(defmethod ->reference [:string Table]
-  [_ {:keys [display_name full-name]}]
-  (or full-name display_name))
-
-(defmethod ->reference [:string Metric]
-  [_ {:keys [name full-name]}]
-  (or full-name name))
-
-(defmethod ->reference [:mbql Metric]
-  [_ {:keys [id definition]}]
-  (if id
-    [:metric id]
-    (-> definition :aggregation first)))
-
-(defmethod ->reference [:native Field]
-  [_ field]
-  (field/qualified-name field))
-
-(defmethod ->reference [:native Table]
-  [_ {:keys [name]}]
-  name)
-
-(defmethod ->reference :default
-  [_ form]
-  (or (cond-> form
-        (map? form) ((some-fn :full-name :name) form))
-      form))
-
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;; TODO - Deduplicate from core
 (def ^:private ^{:arglists '([source])} source->db

diff --git a/test/metabase/automagic_dashboards/core_test.clj b/test/metabase/automagic_dashboards/core_test.clj
@@ -1765,3 +1765,19 @@
                           {:group-name (get-in dashcard [:visualization_settings :text])
                            :card-name  (get-in dashcard [:card :name])})
                         comparison-dashcards))))))))))
+
+(deftest source-fields-are-populated-for-aggregations-38618-test
+  (testing "X-ray aggregates (metrics) with source fields in external tables should properly fill in `:source-field` (#38618)"
+    (mt/dataset test-data
+      (let [dashcard  (->> (magic/automagic-analysis (t2/select-one Table :id (mt/id :reviews)) {:show :all})
+                           :dashcards
+                           (filter (fn [dashcard]
+                                     (= "Distinct Product ID"
+                                        (get-in dashcard [:card :name]))))
+                           first)
+            aggregate (get-in dashcard [:card :dataset_query :query :aggregation])]
+        (testing "Fields requiring a join should have :source-field populated in the aggregate."
+          (is (= [["distinct" [:field (mt/id :products :id)
+                               ;; This should be present vs. nil (value before issue)
+                               {:source-field (mt/id :reviews :product_id)}]]]
+                 aggregate)))))))
diff --git a/test/metabase/automagic_dashboards/interesting_test.clj b/test/metabase/automagic_dashboards/interesting_test.clj
@@ -544,9 +544,9 @@
                                   ["dimension" "Income"]], :score 100, :metric-name "AvgDiscount"}
                         {:metric ["sum" ["dimension" "GenericNumber"]], :score 100, :metric-name "Sum"}
                         {:metric ["avg" ["dimension" "GenericNumber"]], :score 100, :metric-name "Avg"}]
-          {total-id :id :as total-field} {:id 1 :name "TOTAL"}
-          {discount-id :id :as discount-field} {:id 2 :name "DISCOUNT"}
-          {income-id :id :as income-field} {:id 3 :name "INCOME"}]
+          total-field {:id 1 :name "TOTAL"}
+          discount-field {:id 2 :name "DISCOUNT"}
+          income-field {:id 3 :name "INCOME"}]
       (testing "When no dimensions are provided, we produce grounded dimensionless metrics"
         (is (= [{:metric-name       "Count"
                  :metric-title      "Count"
@@ -557,10 +557,10 @@
                  test-metrics
                  {"Count" {:matches []}}))))
       (testing "When we can match on a dimension, we produce every matching metric (2 for GenericNumber)"
-        (is (=? [{:metric-name       "Sum"
-                  :metric-definition {:aggregation [["sum" [:field total-id nil]]]}}
-                 {:metric-name       "Avg"
-                  :metric-definition {:aggregation [["avg" [:field total-id nil]]]}}]
+        (is (=? [{:metric-name       "Sum",
+                  :metric-definition {:aggregation [["sum" "TOTAL"]]}}
+                 {:metric-name       "Avg",
+                  :metric-definition {:aggregation [["avg" "TOTAL"]]}}]
                 (interesting/grounded-metrics
                   ;; Drop Count
                   (rest test-metrics)
@@ -569,17 +569,17 @@
       (testing "The addition of Discount doesn't add more matches as we need
                  Income as well to add the metric that uses Discount"
         (is (=? [{:metric-name       "Sum"
-                  :metric-definition {:aggregation [["sum" [:field total-id nil]]]}}
+                  :metric-definition {:aggregation [["sum" "TOTAL"]]}}
                  {:metric-name       "Avg"
-                  :metric-definition {:aggregation [["avg" [:field total-id nil]]]}}]
+                  :metric-definition {:aggregation [["avg" "TOTAL"]]}}]
                 (interesting/grounded-metrics
                   (rest test-metrics)
                   {"Count"         {:matches []}
                    "GenericNumber" {:matches [total-field]}
                    "Discount"      {:matches [discount-field]}}))))
       (testing "Discount and Income will add the satisfied AvgDiscount grounded metric"
-        (is (=? [{:metric-name "AvgDiscount",
-                  :metric-definition {:aggregation [["/" [:field discount-id nil] [:field income-id nil]]]}}
+        (is (=? [{:metric-name       "AvgDiscount",
+                  :metric-definition {:aggregation [["/" "DISCOUNT" "INCOME"]]}}
                  {:metric-name "Sum"}
                  {:metric-name "Avg"}]
                 (interesting/grounded-metrics