Skip to content
Snippets Groups Projects
Unverified Commit feb534e3 authored by metamben's avatar metamben Committed by GitHub
Browse files

Make the MLv2 display-name include the path of nested fields (#38054) 

* Make the MLv2 display-name include the path of nested fields
* Hide binning and bucketing info when displaying breakoutables

Fixes #35938.
parent 6b6fe582
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/lib/breakout.cljc
+ 1
− 1
View file @ feb534e3
......@@ -88,7 +88,7 @@
(mapv #(let [[pos a-breakout] (matching %)
binning (lib.binning/binning a-breakout)
{:keys [unit]} (lib.temporal-bucket/temporal-bucket a-breakout)]
(cond-> %
(cond-> (assoc % :lib/hide-bin-bucket? true)
binning (lib.binning/with-binning binning)
unit (lib.temporal-bucket/with-temporal-bucket unit)
pos (assoc :breakout-position pos)))
......
src/metabase/lib/field.cljc
+ 68
− 26
View file @ feb534e3
......@@ -131,11 +131,19 @@
"If this is a nested column, add metadata about the parent column."
[query :- ::lib.schema/query
metadata :- ::lib.schema.metadata/column]
(let [parent-metadata (lib.metadata/field query (:parent-id metadata))
{parent-name :name} (cond->> parent-metadata
(:parent-id parent-metadata) (add-parent-column-metadata query))]
(update metadata :name (fn [field-name]
(str parent-name \. field-name)))))
(let [parent-metadata
(lib.metadata/field query (:parent-id metadata))
{parent-name :name, parent-display-name :display-name}
(cond->> parent-metadata
(:parent-id parent-metadata) (add-parent-column-metadata query))]
(-> metadata
(assoc :lib/simple-name (:name metadata))
(update :name (fn [field-name]
(str parent-name \. field-name)))
(assoc ::simple-display-name (:display-name metadata))
(update :display-name (fn [display-name]
(str parent-display-name ": " display-name))))))
(defn- column-metadata-effective-type
"Effective type of a column when taking the `::temporal-unit` into account. If we have a temporal extraction like
......@@ -190,20 +198,44 @@
(cond->> metadata
(:parent-id metadata) (add-parent-column-metadata query))))
(defn- field-nesting-path
[metadata-providerable {:keys [display-name parent-id] :as _field-metadata}]
(loop [field-id parent-id, path (list display-name)]
(if field-id
(let [{:keys [display-name parent-id]} (lib.metadata/field metadata-providerable field-id)]
(recur parent-id (conj path display-name)))
path)))
(defn- nest-display-name
[metadata-providerable field-metadata]
(let [path (field-nesting-path metadata-providerable field-metadata)]
(when (every? some? path)
(str/join ": " path))))
;;; this lives here as opposed to [[metabase.lib.metadata]] because that namespace is more of an interface namespace
;;; and moving this there would cause circular references.
(defmethod lib.metadata.calculation/display-name-method :metadata/column
[query stage-number {field-display-name :display-name
field-name :name
temporal-unit :unit
binning ::binning
join-alias :source-alias
fk-field-id :fk-field-id
table-id :table-id
:as field-metadata} style]
(let [field-display-name (or field-display-name
[query stage-number {field-display-name :display-name
field-name :name
temporal-unit :unit
binning ::binning
join-alias :source-alias
fk-field-id :fk-field-id
table-id :table-id
parent-id :parent-id
simple-display-name ::simple-display-name
hide-bin-bucket? :lib/hide-bin-bucket?
:as field-metadata} style]
(let [humanized-name (u.humanization/name->human-readable-name :simple field-name)
field-display-name (or simple-display-name
(when (and parent-id
;; check that we haven't nested yet
(or (nil? field-display-name)
(= field-display-name humanized-name)))
(nest-display-name query field-metadata))
field-display-name
(if (string? field-name)
(u.humanization/name->human-readable-name :simple field-name)
humanized-name
(str field-name)))
join-display-name (when (and (= style :long)
;; don't prepend a join display name if `:display-name` already contains one!
......@@ -214,11 +246,11 @@
(not (str/includes? field-display-name " → ")))
(or
(when fk-field-id
;; Implicitly joined column pickers don't use the target table's name, they use the FK field's name with
;; "ID" dropped instead.
;; This is very intentional: one table might have several FKs to one foreign table, each with different
;; meaning (eg. ORDERS.customer_id vs. ORDERS.supplier_id both linking to a PEOPLE table).
;; See #30109 for more details.
;; Implicitly joined column pickers don't use the target table's name, they use the FK field's name with
;; "ID" dropped instead.
;; This is very intentional: one table might have several FKs to one foreign table, each with different
;; meaning (eg. ORDERS.customer_id vs. ORDERS.supplier_id both linking to a PEOPLE table).
;; See #30109 for more details.
(if-let [field (lib.metadata/field query fk-field-id)]
(-> (lib.metadata.calculation/display-info query stage-number field)
:display-name
......@@ -228,13 +260,19 @@
(or join-alias (lib.join.util/current-join-alias field-metadata))))
display-name (if join-display-name
(str join-display-name " → " field-display-name)
field-display-name)]
field-display-name)
temporal-format (fn [display-name]
(lib.util/format "%s: %s" display-name (-> (name temporal-unit)
(str/replace \- \space)
u/capitalize-en)))
bin-format (fn [display-name]
(lib.util/format "%s: %s" display-name (lib.binning/binning-display-name binning field-metadata)))]
;; temporal unit and binning formatting are only applied if they haven't been applied yet
(cond
temporal-unit (lib.util/format "%s: %s" display-name (-> (name temporal-unit)
(str/replace \- \space)
u/capitalize-en))
binning (lib.util/format "%s: %s" display-name (lib.binning/binning-display-name binning field-metadata))
:else display-name)))
(and (not= style :long) hide-bin-bucket?) display-name
(and temporal-unit (not= display-name (temporal-format humanized-name))) (temporal-format display-name)
(and binning (not= display-name (bin-format humanized-name))) (bin-format display-name)
:else display-name)))
(defmethod lib.metadata.calculation/display-name-method :field
[query
......@@ -265,6 +303,10 @@
[query stage-number field-metadata]
(merge
((get-method lib.metadata.calculation/display-info-method :default) query stage-number field-metadata)
;; These have to be calculated even if the metadata has display-name to support nested fields
;; because the query processor doesn't produce nested display-names.
{:display-name (lib.metadata.calculation/display-name query stage-number field-metadata)
:long-display-name (lib.metadata.calculation/display-name query stage-number field-metadata :long)}
;; if this column comes from a source Card (Saved Question/Model/etc.) use the name of the Card as the 'table' name
;; rather than the ACTUAL table name.
(when (= (:lib/source field-metadata) :source/card)
......
test/metabase/lib/field_test.cljc
+ 69
− 14
View file @ feb534e3
......@@ -47,20 +47,23 @@
(def ^:private grandparent-parent-child-metadata-provider
"A MetadataProvider for a Table that nested Fields: grandparent, parent, and child"
(let [grandparent {:lib/type :metadata/column
:name "grandparent"
:id (grandparent-parent-child-id :grandparent)
:base-type :type/Text}
parent {:lib/type :metadata/column
:name "parent"
:parent-id (grandparent-parent-child-id :grandparent)
:id (grandparent-parent-child-id :parent)
:base-type :type/Text}
child {:lib/type :metadata/column
:name "child"
:parent-id (grandparent-parent-child-id :parent)
:id (grandparent-parent-child-id :child)
:base-type :type/Text}]
(let [grandparent {:lib/type :metadata/column
:name "grandparent"
:display-name "Grandparent"
:id (grandparent-parent-child-id :grandparent)
:base-type :type/Text}
parent {:lib/type :metadata/column
:name "parent"
:display-name "Parent"
:parent-id (grandparent-parent-child-id :grandparent)
:id (grandparent-parent-child-id :parent)
:base-type :type/Text}
child {:lib/type :metadata/column
:name "child"
:display-name "Child"
:parent-id (grandparent-parent-child-id :parent)
:id (grandparent-parent-child-id :child)
:base-type :type/Text}]
(lib.tu/mock-metadata-provider
{:database meta/database
:tables [(meta/table-metadata :venues)]
......@@ -96,6 +99,58 @@
:visibility-type :normal}
(col-info [:field {:lib/uuid (str (random-uuid))} (grandparent-parent-child-id :child)]))))))
(deftest ^:parallel nested-field-display-name-test
(let [base (lib/query grandparent-parent-child-metadata-provider (meta/table-metadata :venues))]
(testing "simple queries"
(doseq [query [base (lib/append-stage base)]
:let [cols (lib/visible-columns query)]]
(is (= ["Grandparent: Parent: Child" "Grandparent" "Grandparent: Parent"]
(map #(lib/display-name query %) cols)))
(is (= ["Grandparent: Parent: Child" "Grandparent" "Grandparent: Parent"]
(map #(lib/display-name query -1 % :long) cols)))
(is (=? [{:display-name "Grandparent: Parent: Child"
:long-display-name "Grandparent: Parent: Child"}
{:display-name "Grandparent"
:long-display-name "Grandparent"}
{:display-name "Grandparent: Parent"
:long-display-name "Grandparent: Parent"}]
(map #(lib/display-info query -1 %) cols)))))
(testing "breakout"
(is (=? [{:display-name "Grandparent: Parent: Child"
:long-display-name "Grandparent: Parent: Child"}]
(->> base
lib/breakoutable-columns
first
(lib/breakout base)
lib/append-stage
lib/visible-columns
(map #(lib/display-info base -1 %))))))
(testing "join"
(let [join-column (second (lib/visible-columns base))
base (-> base
(lib/join
(-> (lib/join-clause
base [(lib/= join-column
(lib/with-join-alias join-column
"alias"))])
(lib/with-join-alias "alias"))))]
(doseq [query [base (lib/append-stage base)]]
(is (=? [{:display-name "Grandparent: Parent: Child"
:long-display-name "Grandparent: Parent: Child"}
{:display-name "Grandparent"
:long-display-name "Grandparent"}
{:display-name "Grandparent: Parent"
:long-display-name "Grandparent: Parent"}
{:display-name "Grandparent: Parent: Child"
:long-display-name "alias → Grandparent: Parent: Child"}
{:display-name "Grandparent"
:long-display-name "alias → Grandparent"}
{:display-name "Grandparent: Parent"
:long-display-name "alias → Grandparent: Parent"}]
(->> query
lib/visible-columns
(map #(lib/display-info base -1 %))))))))))
(deftest ^:parallel col-info-field-literals-test
(testing "field literals should get the information from the matching `:lib/stage-metadata` if it was supplied"
(is (=? {:name "sum"
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment