diff --git a/src/metabase/query_processor/middleware/annotate.clj b/src/metabase/query_processor/middleware/annotate.clj
index 2194441b46dcf54e168e17916ad7ced95db7b2d2..31440e1236739bafa0be0630f7b4fc81107fcb74 100644
--- a/src/metabase/query_processor/middleware/annotate.clj
+++ b/src/metabase/query_processor/middleware/annotate.clj
@@ -114,41 +114,54 @@
{:base_type :type/Float
:special_type :type/Number}))
-(s/defn ^:private col-info-for-field-clause :- su/Map
+(s/defn ^:private col-info-for-field-clause :- {:field_ref mbql.s/Field, s/Keyword s/Any}
[{:keys [source-metadata expressions], :as inner-query} :- su/Map, clause :- mbql.s/Field]
;; for various things that can wrap Field clauses recurse on the wrapped Field but include a little bit of info
;; about the clause doing the wrapping
(mbql.u/match-one clause
[:binning-strategy field strategy _ resolved-options]
- (assoc (col-info-for-field-clause inner-query field)
- :binning_info (assoc (u/snake-keys resolved-options)
- :binning_strategy strategy))
+ (let [recursive-info (col-info-for-field-clause inner-query field)]
+ (assoc recursive-info
+ :binning_info (assoc (u/snake-keys resolved-options)
+ :binning_strategy strategy)
+ :field_ref (assoc (vec &match) 1 (:field_ref recursive-info))))
[:datetime-field field unit]
- (assoc (col-info-for-field-clause inner-query field) :unit unit)
-
- [:joined-field alias field]
- (let [{:keys [fk-field-id], :as join} (join-with-alias inner-query alias)]
- (-> (col-info-for-field-clause inner-query field)
- (assoc :fk_field_id fk-field-id)
- (update :display_name display-name-for-joined-field join)))
+ (let [recursive-info (col-info-for-field-clause inner-query field)]
+ (assoc recursive-info
+ :unit unit
+ :field_ref (assoc (vec &match) 1 (:field_ref recursive-info))))
+
+ [:joined-field join-alias field]
+ (let [{:keys [fk-field-id], :as join} (join-with-alias inner-query join-alias)]
+ (let [recursive-info (col-info-for-field-clause inner-query field)]
+ (-> recursive-info
+ (merge (when fk-field-id {:fk_field_id fk-field-id}))
+ (assoc :field_ref (if fk-field-id
+ [:fk-> [:field-id fk-field-id] field]
+ (assoc (vec &match) 2 (:field_ref recursive-info))))
+ (update :display_name display-name-for-joined-field join))))
;; TODO - should be able to remove this now
[:fk-> [:field-id source-field-id] field]
- (assoc (col-info-for-field-clause inner-query field) :fk_field_id source-field-id)
+ (assoc (col-info-for-field-clause inner-query field)
+ :field_ref &match
+ :fk_field_id source-field-id)
;; TODO - should be able to remove this now
;; for FKs where source is a :field-literal don't include `:fk_field_id`
[:fk-> _ field]
- (recur field)
+ (assoc (col-info-for-field-clause inner-query field)
+ :field_ref &match)
;; for field literals, look for matching `source-metadata`, and use that if we can find it; otherwise generate
;; basic info based on the content of the field literal
[:field-literal field-name field-type]
- (or (some #(when (= (:name %) field-name) %) source-metadata)
- {:name field-name
- :base_type field-type
- :display_name (humanization/name->human-readable-name field-name)})
+ (assoc (or (some #(when (= (:name %) field-name) %) source-metadata)
+ {:name field-name
+ :base_type field-type
+ :display_name (humanization/name->human-readable-name field-name)})
+ :field_ref &match)
[:expression expression-name]
(merge
@@ -158,14 +171,16 @@
{:name expression-name
:display_name expression-name
;; provided so the FE can add easily add sorts and the like when someone clicks a column header
- :expression_name expression-name})
+ :expression_name expression-name
+ :field_ref &match})
[:field-id id]
(let [{parent-id :parent_id, :as field} (dissoc (qp.store/field id) :database_type)]
- (if-not parent-id
- field
- (let [parent (col-info-for-field-clause inner-query [:field-id parent-id])]
- (update field :name #(str (:name parent) \. %)))))
+ (assoc (if-not parent-id
+ field
+ (let [parent (col-info-for-field-clause inner-query [:field-id parent-id])]
+ (update field :name #(str (:name parent) \. %))))
+ :field_ref &match))
;; we should never reach this if our patterns are written right so this is more to catch code mistakes than
;; something the user should expect to see
@@ -358,16 +373,14 @@
[{:keys [fields], :as inner-query} :- su/Map]
(for [field fields]
(assoc (col-info-for-field-clause inner-query field)
- :source :fields
- :field_ref field)))
+ :source :fields)))
(s/defn ^:private cols-for-ags-and-breakouts
[{aggregations :aggregation, breakouts :breakout, :as inner-query} :- su/Map]
(concat
(for [breakout breakouts]
(assoc (col-info-for-field-clause inner-query breakout)
- :source :breakout
- :field_ref breakout))
+ :source :breakout))
(for [[i aggregation] (m/indexed aggregations)]
(assoc (col-info-for-aggregation-clause inner-query aggregation)
:source :aggregation
diff --git a/test/metabase/query_processor/middleware/annotate_test.clj b/test/metabase/query_processor/middleware/annotate_test.clj
index 30071e986bdd7859494b1204296040efe5b47a6e..9f97366e3369f40cb357d653f5902092b30c8c8e 100644
--- a/test/metabase/query_processor/middleware/annotate_test.clj
+++ b/test/metabase/query_processor/middleware/annotate_test.clj
@@ -91,14 +91,14 @@
{:columns [:name]}))))
;; we should get `:fk_field_id` and information where possible when using `:joined-field` clauses; display_name should
-;; include the joined table
+;; include the joined table (for IMPLICIT JOINS)
(expect
[(data/$ids venues
(assoc (info-for-field :categories :name)
:display_name "Venues → Name"
- :fk_field_id %category_id
:source :fields
- :field_ref &CATEGORIES__via__CATEGORY_ID.categories.name))]
+ :field_ref $category_id->categories.name
+ :fk_field_id %category_id))]
(qp.test-util/with-everything-store
(data/$ids venues
(doall
@@ -112,14 +112,34 @@
:fk-field-id %category_id}]}}
{:columns [:name]})))))
+;; for EXPLICIT JOINS (which do not include an `:fk-field-id` in the Join info) the returned `:field_ref` should be a
+;; `joined-field` clause instead of an `fk->` clause
+(expect
+ [(data/$ids venues
+ (assoc (info-for-field :categories :name)
+ :display_name "Venues → Name"
+ :source :fields
+ :field_ref &CATEGORIES__via__CATEGORY_ID.categories.name))]
+ (qp.test-util/with-everything-store
+ (data/$ids venues
+ (doall
+ (annotate/column-info
+ {:type :query
+ :query {:fields [&CATEGORIES__via__CATEGORY_ID.categories.name]
+ :joins [{:alias "CATEGORIES__via__CATEGORY_ID"
+ :source-table $$venues
+ :condition [:= $category_id &CATEGORIES__via__CATEGORY_ID.categories.id]
+ :strategy :left-join}]}}
+ {:columns [:name]})))))
+
;; we shuld use the `display_name` of a Table instead of its `name` in joined display names
(expect
[(data/$ids venues
(assoc (info-for-field :categories :name)
:display_name "Geographical locations to share Tips about → Name" ; RIP GeoTips
- :fk_field_id %category_id
:source :fields
- :field_ref &CATEGORIES__via__CATEGORY_ID.categories.name))]
+ :field_ref $category_id->categories.name
+ :fk_field_id %category_id))]
(qp.test-util/with-everything-store
(data/$ids venues
(tu/with-temp-vals-in-db Table $$venues {:display_name "Geographical locations to share Tips about"}
@@ -140,9 +160,9 @@
[(data/$ids venues
(assoc (info-for-field :categories :name)
:display_name "cats → Name"
- :fk_field_id %category_id
:source :fields
- :field_ref &cats.categories.name))]
+ :field_ref $category_id->categories.name
+ :fk_field_id %category_id))]
(qp.test-util/with-everything-store
(data/$ids venues
(doall
@@ -220,7 +240,7 @@
;; For fields with parents we should return them with a combined name including parent's name
(tt/expect-with-temp [Field [parent {:name "parent", :table_id (data/id :venues)}]
- Field [child {:name "child", :table_id (data/id :venues), :parent_id (u/get-id parent)}]]
+ Field [child {:name "child", :table_id (data/id :venues), :parent_id (u/get-id parent)}]]
{:description nil
:table_id (data/id :venues)
:special_type nil
@@ -231,14 +251,15 @@
:visibility_type :normal
:display_name "Child"
:fingerprint nil
+ :field_ref [:field-id (u/get-id child)]
:base_type :type/Text}
(qp.test-util/with-everything-store
(#'annotate/col-info-for-field-clause {} [:field-id (u/get-id child)])))
;; nested-nested fields should include grandparent name (etc)
(tt/expect-with-temp [Field [grandparent {:name "grandparent", :table_id (data/id :venues)}]
- Field [parent {:name "parent", :table_id (data/id :venues), :parent_id (u/get-id grandparent)}]
- Field [child {:name "child", :table_id (data/id :venues), :parent_id (u/get-id parent)}]]
+ Field [parent {:name "parent", :table_id (data/id :venues), :parent_id (u/get-id grandparent)}]
+ Field [child {:name "child", :table_id (data/id :venues), :parent_id (u/get-id parent)}]]
{:description nil
:table_id (data/id :venues)
:special_type nil
@@ -249,6 +270,7 @@
:visibility_type :normal
:display_name "Child"
:fingerprint nil
+ :field_ref [:field-id (u/get-id child)]
:base_type :type/Text}
(qp.test-util/with-everything-store
(#'annotate/col-info-for-field-clause {} [:field-id (u/get-id child)])))
@@ -258,10 +280,11 @@
{:name "sum"
:display_name "sum of User ID"
:base_type :type/Integer
+ :field_ref [:field-literal "sum" :type/Integer]
:special_type :type/FK}
(qp.test-util/with-everything-store
(#'annotate/col-info-for-field-clause
- {:source-metadata [{:name "abc", :display_name "another Field", :base_type :type/Integer, :special_type :type/FK}
+ {:source-metadata [{:name "abc", :display_name "another Field", :base_type :type/Integer, :special_type :type/FK}
{:name "sum", :display_name "sum of User ID", :base_type :type/Integer, :special_type :type/FK}]}
[:field-literal "sum" :type/Integer])))
diff --git a/test/metabase/query_processor_test.clj b/test/metabase/query_processor_test.clj
index 7dee4e9bf434871d653a67333dde29753a1961f1..6cd400ef0517840817da07e8f7123e12f9c74226 100644
--- a/test/metabase/query_processor_test.clj
+++ b/test/metabase/query_processor_test.clj
@@ -12,7 +12,6 @@
[metabase.models
[field :refer [Field]]
[table :refer [Table]]]
- [metabase.query-processor.middleware.add-implicit-joins :as add-implicit-joins]
[metabase.test.data :as data]
[metabase.test.data
[datasets :as datasets]
@@ -172,11 +171,10 @@
(let [source-col (col source-table-kw source-field-kw)
dest-col (col dest-table-kw dest-field-kw)
dest-table-display-name (db/select-one-field :display_name Table :id (data/id dest-table-kw))
- dest-table-name (db/select-one-field :name Table :id (data/id dest-table-kw))
- join-alias (#'add-implicit-joins/join-alias dest-table-name (:name source-col))]
+ dest-table-name (db/select-one-field :name Table :id (data/id dest-table-kw))]
(-> dest-col
(update :display_name (partial format "%s → %s" (or dest-table-display-name dest-table-name)))
- (assoc :field_ref [:joined-field join-alias [:field-id (:id dest-col)]]
+ (assoc :field_ref [:fk-> [:field-id (:id source-col)] [:field-id (:id dest-col)]]
:fk_field_id (:id source-col)))))
(declare cols)
diff --git a/test/metabase/query_processor_test/implicit_joins_test.clj b/test/metabase/query_processor_test/implicit_joins_test.clj
index 7033de5f17714a1a62794110bb4ae3e513982f33..458916505d811818d7653d31a9c136a46e840f74 100644
--- a/test/metabase/query_processor_test/implicit_joins_test.clj
+++ b/test/metabase/query_processor_test/implicit_joins_test.clj
@@ -1,6 +1,7 @@
(ns metabase.query-processor-test.implicit-joins-test
"Test for JOIN behavior."
- (:require [metabase
+ (:require [expectations :refer [expect]]
+ [metabase
[driver :as driver]
[query-processor-test :as qp.test]]
[metabase.test.data :as data]
@@ -108,6 +109,17 @@
:limit 10}))
[:status :error]))
+;; Implicit joins should come back with `:fk->` field refs
+(expect
+ (data/$ids venues $category_id->categories.name)
+ (-> (qp.test/cols
+ (data/run-mbql-query :venues
+ {:fields [$category_id->categories.name]
+ :order-by [[:asc $id]]
+ :limit 1}))
+ first
+ :field_ref))
+
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | MULTIPLE JOINS |
diff --git a/test/metabase/query_processor_test/remapping_test.clj b/test/metabase/query_processor_test/remapping_test.clj
index d4878f69be9fa8c7347217da203845145b0069d6..66674e1be7f0a34ab29105c538ea216248f82d10 100644
--- a/test/metabase/query_processor_test/remapping_test.clj
+++ b/test/metabase/query_processor_test/remapping_test.clj
@@ -7,7 +7,6 @@
[dimension :refer [Dimension]]
[field :refer [Field]]]
[metabase.query-processor.middleware.add-dimension-projections :as add-dimension-projections]
- [metabase.query-processor.test-util :as qp.test-util]
[metabase.test.data :as data]
[metabase.test.data.datasets :as datasets]
[toucan.db :as db]))
@@ -59,9 +58,7 @@
:display_name "Foo"
:name (data/format-name "name_2")
:remapped_from (data/format-name "category_id")
- :field_ref [:joined-field
- (qp.test-util/fk-table-alias-name $$categories %category_id)
- $categories.name]))]}
+ :field_ref $category_id->categories.name))]}
(data/with-temp-objects
(data/create-venue-category-fk-remapping "Foo")
(select-columns (set (map data/format-name ["name" "price" "name_2"]))
@@ -84,9 +81,7 @@
:display_name "Foo"
:name (data/format-name "name_2")
:remapped_from (data/format-name "category_id")
- :field_ref [:joined-field
- (qp.test-util/fk-table-alias-name $$categories %category_id)
- $categories.name]))]}
+ :field_ref $category_id->categories.name))]}
(data/with-temp-objects
(data/create-venue-category-fk-remapping "Foo")
(select-columns (set (map data/format-name ["name" "price" "name_2"]))