Skip to content
Snippets Groups Projects
Unverified Commit c382f56e authored by Braden Shepherdson's avatar Braden Shepherdson Committed by GitHub
Browse files

[MLv2] Drop redundant :fields clauses from stages and joins (#36959)

If they're equivalent to the default, they can be removed (for stages)
or replaced by `:all` (on joins).
parent a1bc6366
No related branches found
No related tags found
No related merge requests found
......@@ -43,8 +43,9 @@ export function findColumnIndexForColumnSetting(columns, columnSetting) {
const fieldRef = normalizeFieldRef(columnSetting.fieldRef);
// first try to find by fieldRef
if (fieldRef != null) {
const dimension = Dimension.parseMBQL(fieldRef);
const index = _.findIndex(columns, col =>
_.isEqual(fieldRef, normalizeFieldRef(fieldRefForColumn(col))),
dimension.isSameBaseDimension(fieldRefForColumn(col)),
);
if (index >= 0) {
return index;
......
......@@ -384,3 +384,23 @@
(map #(find-matching-column query stage-number % cols))
selected-refs)]
(mapv #(assoc % :selected? (contains? matching-selected-cols %)) cols)))))
(mu/defn matching-column-sets? :- :boolean
"Returns true if the provided `refs` is the same set as the provided `columns`.
Order is ignored. Only returns true if each of the `refs` matches a column, and each of the `columns` is matched by
exactly 1 of the `refs`. (A bijection, in math terms.)"
[query :- ::lib.schema/query
stage-number :- :int
refs :- [:sequential ::lib.schema.ref/ref]
columns :- [:sequential ::lib.schema.metadata/column]]
;; The lists match iff:
;; - Each ref matches a column; AND
;; - Each column was matched by exactly one ref
;; So we return true if nil is not a key in the matching, AND all vals in the matching have length 1,
;; AND the matching has as many elements as `columns` (usually the list of columns returned by default).
(and (= (count refs) (count columns))
(let [matching (group-by #(find-matching-column query stage-number % columns) refs)]
(and (not (contains? matching nil))
(= (count matching) (count columns))
(every? #(= (count %) 1) (vals matching))))))
......@@ -543,13 +543,8 @@
This is exactly [[lib.metadata.calculation/returned-columns]] filtered by the `:lib/source`.
Fields from explicit joins are listed on the join itself and should not be listed in `:fields`."
[query stage-number]
(lib.util/update-query-stage query stage-number
(fn [stage]
(assoc stage :fields
(into [] (comp (remove (comp #{:source/joins :source/implicitly-joinable}
:lib/source))
(map lib.ref/ref))
(lib.metadata.calculation/returned-columns query stage-number stage))))))
(let [defaults (lib.metadata.calculation/default-columns-for-stage query stage-number)]
(lib.util/update-query-stage query stage-number assoc :fields (mapv lib.ref/ref defaults))))
(defn- query-with-fields
"If the given stage already has a `:fields` clause, do nothing. If it doesn't, populate the `:fields` clause with the
......@@ -609,23 +604,25 @@
column :- lib.metadata.calculation/ColumnMetadataWithSource]
(let [stage (lib.util/query-stage query stage-number)
source (:lib/source column)]
(case source
(:source/table-defaults
:source/fields
:source/card
:source/previous-stage
:source/expressions
:source/aggregations
:source/breakouts) (cond-> query
(contains? stage :fields) (include-field stage-number column))
:source/joins (add-field-to-join query stage-number column)
:source/implicitly-joinable (include-field query stage-number column)
:source/native (throw (ex-info (native-query-fields-edit-error) {:query query :stage stage-number}))
;; Default case - do nothing if we don't know about the incoming value.
;; Generates a warning, as we should aim to capture all the :source/* values here.
(do
(log/warn (i18n/tru "Cannot add-field with unknown source {0}" (pr-str source)))
query))))
(-> (case source
(:source/table-defaults
:source/fields
:source/card
:source/previous-stage
:source/expressions
:source/aggregations
:source/breakouts) (cond-> query
(contains? stage :fields) (include-field stage-number column))
:source/joins (add-field-to-join query stage-number column)
:source/implicitly-joinable (include-field query stage-number column)
:source/native (throw (ex-info (native-query-fields-edit-error) {:query query :stage stage-number}))
;; Default case - do nothing if we don't know about the incoming value.
;; Generates a warning, as we should aim to capture all the :source/* values here.
(do
(log/warn (i18n/tru "Cannot add-field with unknown source {0}" (pr-str source)))
query))
;; Then drop any redundant :fields clauses.
lib.remove-replace/normalize-fields-clauses)))
(defn- remove-matching-ref [column refs]
(let [match (lib.equality/find-matching-ref column refs)]
......@@ -673,22 +670,25 @@
stage-number :- :int
column :- lib.metadata.calculation/ColumnMetadataWithSource]
(let [source (:lib/source column)]
(case source
(:source/table-defaults
:source/fields
:source/breakouts
:source/aggregations
:source/expressions
:source/card
:source/previous-stage
:source/implicitly-joinable) (exclude-field query stage-number column)
:source/joins (remove-field-from-join query stage-number column)
:source/native (throw (ex-info (native-query-fields-edit-error) {:query query :stage stage-number}))
;; Default case: do nothing and return the query unchaged.
;; Generate a warning - we should aim to capture every `:source/*` value above.
(do
(log/warn (i18n/tru "Cannot remove-field with unknown source {0}" (pr-str source)))
query))))
(-> (case source
(:source/table-defaults
:source/fields
:source/breakouts
:source/aggregations
:source/expressions
:source/card
:source/previous-stage
:source/implicitly-joinable) (exclude-field query stage-number column)
:source/joins (remove-field-from-join query stage-number column)
:source/native (throw (ex-info (native-query-fields-edit-error)
{:query query :stage stage-number}))
;; Default case: do nothing and return the query unchaged.
;; Generate a warning - we should aim to capture every `:source/*` value above.
(do
(log/warn (i18n/tru "Cannot remove-field with unknown source {0}" (pr-str source)))
query))
;; Then drop any redundant :fields clauses.
lib.remove-replace/normalize-fields-clauses)))
;; TODO: Refactor this away? The special handling for aggregations is strange.
(mu/defn find-visible-column-for-ref :- [:maybe ::lib.schema.metadata/column]
......
......@@ -358,15 +358,14 @@
(when-let [effective-type ((some-fn :effective-type :base-type) x-metadata)]
{:effective-type effective-type})
(when-let [table-id (:table-id x-metadata)]
{:table (display-info
query
stage-number
;; TODO: only ColumnMetadatas should possibly have legacy `card__<id>` `:table-id`s... we should
;; probably move this special casing into [[metabase.lib.field]] instead of having it be part of the
;; `:default` method.
(cond
(integer? table-id) (lib.metadata/table query table-id)
(string? table-id) (lib.metadata/card query (lib.util/legacy-string-table-id->card-id table-id))))})
;; TODO: only ColumnMetadatas should possibly have legacy `card__<id>` `:table-id`s... we should
;; probably move this special casing into [[metabase.lib.field]] instead of having it be part of the
;; `:default` method.
(when-let [inner-metadata (cond
(integer? table-id) (lib.metadata/table query table-id)
(string? table-id) (lib.metadata/card
query (lib.util/legacy-string-table-id->card-id table-id)))]
{:table (display-info query stage-number inner-metadata)}))
(when-let [source (:lib/source x-metadata)]
{:is-from-previous-stage (= source :source/previous-stage)
:is-from-join (= source :source/joins)
......@@ -575,3 +574,17 @@
(assoc field :lib/desired-column-alias (unique-name-fn
(lib.join.util/desired-alias query field))))))))
column-metadatas)))
(mu/defn default-columns-for-stage :- ColumnsWithUniqueAliases
"Given a query and stage, returns the columns which would be selected by default.
This is exactly [[lib.metadata.calculation/returned-columns]] filtered by the `:lib/source`.
(Fields from explicit joins are listed on the join itself and should not be listed in `:fields`.)
If there is already a `:fields` list on that stage, it is ignored for this calculation."
[query :- ::lib.schema/query
stage-number :- :int]
(let [no-fields (lib.util/update-query-stage query stage-number dissoc :fields)]
(into [] (remove (comp #{:source/joins :source/implicitly-joinable}
:lib/source))
(returned-columns no-fields stage-number (lib.util/query-stage no-fields stage-number)))))
......@@ -4,6 +4,7 @@
[malli.core :as mc]
[medley.core :as m]
[metabase.lib.common :as lib.common]
[metabase.lib.equality :as lib.equality]
[metabase.lib.join :as lib.join]
[metabase.lib.join.util :as lib.join.util]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
......@@ -31,6 +32,7 @@
(declare remove-local-references)
(declare remove-stage-references)
(declare normalize-fields-clauses)
(defn- find-matching-order-by-index
[query stage-number [target-op {:keys [temporal-unit binning]} target-ref-id]]
......@@ -175,7 +177,9 @@
:else
query)]
(if location
(remove-replace-location query stage-number query location target-clause remove-replace-fn)
(-> query
(remove-replace-location stage-number query location target-clause remove-replace-fn)
normalize-fields-clauses)
query))))
(declare remove-join)
......@@ -366,7 +370,9 @@
:joins
(fn [joins]
(mapv #(lib.join/add-default-alias $q stage-number %) joins))))))]
(remove-invalidated-refs query-after query stage-number)))
(-> query-after
(remove-invalidated-refs query stage-number)
normalize-fields-clauses)))
query)))
(defn- has-field-from-join? [form join-alias]
......@@ -424,3 +430,40 @@
new-join
%)
joins))))))
(defn- specifies-default-fields? [query stage-number]
(let [fields (:fields (lib.util/query-stage query stage-number))]
(and fields
;; Quick first check: if there are any implicitly-joined fields, it's not the default list.
(not (some (comp :source-field lib.options/options) fields))
(lib.equality/matching-column-sets? query stage-number fields
(lib.metadata.calculation/default-columns-for-stage query stage-number)))))
(defn- normalize-fields-for-join [query stage-number join]
(if (#{:none :all} (:fields join))
;; Nothing to do if it's already a keyword.
join
(cond-> join
(lib.equality/matching-column-sets?
query stage-number (:fields join)
(lib.metadata.calculation/returned-columns query stage-number (assoc join :fields :all)))
(assoc :fields :all))))
(defn- normalize-fields-for-stage [query stage-number]
(let [stage (lib.util/query-stage query stage-number)]
(cond-> query
(specifies-default-fields? query stage-number)
(lib.util/update-query-stage stage-number dissoc :fields)
(:joins stage)
(lib.util/update-query-stage stage-number update :joins
(partial mapv #(normalize-fields-for-join query stage-number %))))))
(mu/defn normalize-fields-clauses :- :metabase.lib.schema/query
"Check all the `:fields` clauses in the query - on the stages and any joins - and drops them if they are equal to the
defaults.
- For stages, if the `:fields` list is identical to the default fields for this stage.
- For joins, replace it with `:all` if it's all the fields that are in the join by default.
- For joins, remove it if the list is empty (the default for joins is no fields)."
[query :- :metabase.lib.schema/query]
(reduce normalize-fields-for-stage query (range (count (:stages query)))))
......@@ -709,7 +709,7 @@
(defn- fields-of
([query] (fields-of query -1))
([query stage-number]
(sorted-fields (lib/fields query stage-number))))
(some-> (lib/fields query stage-number) sorted-fields)))
(deftest ^:parallel populate-fields-for-stage-test
(testing "simple table query"
......@@ -941,6 +941,11 @@
(is (=? implied-query
(lib/add-field implied-query -1 (nth implicit-columns 6)))))))))))
(defn- clean-ref [column]
(-> column
lib/ref
(lib.options/update-options dissoc :lib/uuid)))
(deftest ^:parallel remove-field-tests
(testing "simple table query"
(let [query (lib/query meta/metadata-provider (meta/table-metadata :orders))
......@@ -1057,30 +1062,44 @@
viz-columns (lib/visible-columns query)
table-columns (lib/fieldable-columns query -1)
implicit-columns (filter #(= (:lib/source %) :source/implicitly-joinable) viz-columns)
implied-query (lib/add-field query -1 (first implicit-columns))]
implied-query (-> query
(lib/add-field -1 (first implicit-columns))
(lib/add-field -1 (second implicit-columns)))
table-fields (map clean-ref table-columns)
implied1 (clean-ref (first implicit-columns))
implied2 (clean-ref (second implicit-columns))]
(is (= (map #(dissoc % :selected?) table-columns)
(lib/returned-columns query)))
(testing "attaching the implicitly joined field should alter the query"
(testing "attaching implicitly joined fields should alter the query"
(is (not= query implied-query))
(is (nil? (lib.equality/find-matching-ref (first implicit-columns)
(map lib/ref (lib/returned-columns query))))))
(testing "with no :fields set does nothing"
(is (=? query
(lib/remove-field query -1 (first implicit-columns)))))
(lib/remove-field query -1 (first implicit-columns))))
(is (=? query
(lib/remove-field query -1 (second implicit-columns)))))
(testing "with explicit :fields list"
(is (=? (->> table-columns
(map lib/ref)
(map #(lib.options/update-options % dissoc :lib/uuid))
sorted-fields)
(is (=? (sorted-fields (conj table-fields implied2))
(-> implied-query
(lib/remove-field -1 (first implicit-columns))
fields-of)))
(is (=? (sorted-fields (conj table-fields implied1))
(-> implied-query
(lib/remove-field -1 (second implicit-columns))
fields-of))))
(is (not= query
(lib/remove-field implied-query -1 (first implicit-columns)))
"even though the :fields list is now the default again, it's still an explicit list"))))
(testing "drops the :fields clause when it becomes the defaults"
(is (= query
(-> implied-query
(lib/remove-field -1 (first implicit-columns))
(lib/remove-field -1 (second implicit-columns)))))
(is (= query
(-> implied-query
(lib/remove-field -1 (second implicit-columns))
(lib/remove-field -1 (first implicit-columns)))))))))
(deftest ^:parallel add-remove-fields-source-card-test
(testing "query with a source card"
......@@ -1096,18 +1115,14 @@
(lib/remove-field -1 (first columns))
fields-of))))
(testing "allows adding back the removed field"
(is (=? [[:field {} "USER_ID"]
[:field {} "count"]]
(-> query
(lib/remove-field -1 (second columns))
(lib/add-field -1 (second columns))
fields-of)))
(is (=? [[:field {} "USER_ID"]
[:field {} "count"]]
(-> query
(lib/remove-field -1 (first columns))
(lib/add-field -1 (first columns))
fields-of)))))))
(is (nil? (-> query
(lib/remove-field -1 (second columns))
(lib/add-field -1 (second columns))
fields-of)))
(is (nil? (-> query
(lib/remove-field -1 (first columns))
(lib/add-field -1 (first columns))
fields-of)))))))
(deftest ^:parallel add-remove-fields-multi-stage-test
(testing "multi-stage query"
......@@ -1137,18 +1152,14 @@
fields-of))))
(testing "removing and adding each field"
(is (=? [[:field {} "CREATED_AT"]
[:field {} "sum"]]
(-> query
(lib/remove-field 1 sum)
(lib/add-field 1 sum)
fields-of)))
(is (=? [[:field {} "CREATED_AT"]
[:field {} "sum"]]
(-> query
(lib/remove-field 1 created-at)
(lib/add-field 1 created-at)
fields-of)))))))
(is (nil? (-> query
(lib/remove-field 1 sum)
(lib/add-field 1 sum)
fields-of)))
(is (nil? (-> query
(lib/remove-field 1 created-at)
(lib/add-field 1 created-at)
fields-of)))))))
(deftest ^:parallel add-remove-fields-native-query-test
(testing "native query"
......@@ -1174,18 +1185,14 @@
fields-of))))
(testing "removing and adding each field"
(is (=? [[:field {} "abc"]
[:field {} "sum"]]
(-> query
(lib/remove-field 1 (first columns))
(lib/add-field 1 (first columns))
fields-of)))
(is (=? [[:field {} "abc"]
[:field {} "sum"]]
(-> query
(lib/remove-field 1 (second columns))
(lib/add-field 1 (second columns))
fields-of)))))))))
(is (nil? (-> query
(lib/remove-field 1 (first columns))
(lib/add-field 1 (first columns))
fields-of)))
(is (nil? (-> query
(lib/remove-field 1 (second columns))
(lib/add-field 1 (second columns))
fields-of)))))))))
(deftest ^:parallel add-remove-fields-aggregation-breakout-test
(testing "aggregations and breakouts"
......@@ -1207,18 +1214,16 @@
fields-of))))
(testing "removing and adding each field"
(is (=? [created-at sum]
(-> query
(lib/remove-field -1 (first columns))
(lib/add-field -1 (first columns))
(lib.util/query-stage -1)
:fields)))
(is (=? [sum created-at]
(-> query
(lib/remove-field -1 (second columns))
(lib/add-field -1 (second columns))
(lib.util/query-stage -1)
:fields)))))))
(is (nil? (-> query
(lib/remove-field -1 (first columns))
(lib/add-field -1 (first columns))
(lib.util/query-stage -1)
:fields)))
(is (nil? (-> query
(lib/remove-field -1 (second columns))
(lib/add-field -1 (second columns))
(lib.util/query-stage -1)
:fields)))))))
(deftest ^:parallel find-visible-column-for-ref-test
(testing "precise references"
......@@ -1393,7 +1398,7 @@
(testing "can have that dropped field added back"
(let [added (lib/add-field query -1 vis-price)]
(is (=? (map #(assoc % :lib/source :source/fields) columns)
(is (=? columns #_(map #(assoc % :lib/source :source/fields) columns)
(lib.metadata.calculation/returned-columns added)))
(testing "and removed again"
(is (=? (map #(assoc % :lib/source :source/fields) no-price)
......
......@@ -5,6 +5,7 @@
[medley.core :as m]
[metabase.lib.core :as lib]
[metabase.lib.options :as lib.options]
[metabase.lib.remove-replace :as lib.remove-replace]
[metabase.lib.test-metadata :as meta]
[metabase.lib.test-util :as lib.tu]
[metabase.lib.test-util.macros :as lib.tu.macros]))
......@@ -1044,3 +1045,82 @@
(lib/replace-clause (first (lib/expressions query))
(lib/with-expression-name 2 "evaluated expr"))
lib/expressions)))))
(deftest ^:parallel normalize-fields-clauses-test
(testing "queries with no :fields clauses should not be changed"
(are [query] (= query (lib.remove-replace/normalize-fields-clauses query))
lib.tu/query-with-join
lib.tu/query-with-self-join
lib.tu/venues-query))
(testing "a :fields clause with extras is retained"
(let [base (lib/query meta/metadata-provider (meta/table-metadata :orders))
viz (lib/visible-columns base)
category (m/find-first #(= (:name %) "CATEGORY") viz)
query (lib/add-field base 0 category)]
(is (= base (lib.remove-replace/normalize-fields-clauses base)))
(is (= query (lib.remove-replace/normalize-fields-clauses query)))))
(testing "a :fields clause with a default field removed is retained"
(let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :orders))
(lib/remove-field 0 (assoc (meta/field-metadata :orders :tax)
:lib/source :source/table-defaults)))]
(is (= 8 (-> query :stages first :fields count)))
(is (= query (lib.remove-replace/normalize-fields-clauses query)))))
(testing "if :fields clause matches the defaults it is dropped"
(testing "removing then restoring a field"
(let [tax (assoc (meta/field-metadata :orders :tax)
:lib/source :source/table-defaults)
tax-ref (lib/ref tax)
query (-> (lib/query meta/metadata-provider (meta/table-metadata :orders))
(lib/remove-field 0 tax)
;; Can't use lib/add-field here because it will normalize the :fields clause.
(update-in [:stages 0 :fields] conj tax-ref))]
(is (= 9 (-> query :stages first :fields count)))
(is (nil? (-> query
lib.remove-replace/normalize-fields-clauses
:stages
first
:fields)))))
(testing "adding and dropping and implicit join field"
(let [category (assoc (meta/field-metadata :products :category)
:lib/source :source/implicitly-joinable)
query (-> (lib/query meta/metadata-provider (meta/table-metadata :orders))
(lib/add-field 0 category)
;; Can't use remove-field itself; it will normalize the fields clause.
(update-in [:stages 0 :fields] #(remove (comp #{(meta/id :products :category)} last) %)))]
(is (= 9 (-> query :stages first :fields count)))
(is (nil? (-> query
lib.remove-replace/normalize-fields-clauses
:stages
first
:fields))))))
(testing ":fields clauses on joins"
(testing "are preserved if :none or :all"
(let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :orders))
(lib/join (lib/join-clause (meta/table-metadata :products)
[(lib/= (meta/field-metadata :orders :product-id)
(meta/field-metadata :products :id))])))
none (assoc-in query [:stages 0 :joins 0 :fields] :none)]
(is (= :all (-> query :stages first :joins first :fields)))
(is (= query (lib.remove-replace/normalize-fields-clauses query)))
(is (= none (lib.remove-replace/normalize-fields-clauses none)))))
(testing "are preserved if they do not match the defaults"
(let [join (-> (lib/join-clause (meta/table-metadata :products)
[(lib/= (meta/field-metadata :orders :product-id)
(meta/field-metadata :products :id))])
(lib/with-join-fields (for [field (take 4 (meta/fields :products))]
(meta/field-metadata :products field))))
query (-> (lib/query meta/metadata-provider (meta/table-metadata :orders))
(lib/join join))]
(is (= 4 (-> query :stages first :joins first :fields count)))
(is (= query (lib.remove-replace/normalize-fields-clauses query)))))
(testing "are replaced with :all if they include all the defaults"
(let [join (-> (lib/join-clause (meta/table-metadata :products)
[(lib/= (meta/field-metadata :orders :product-id)
(meta/field-metadata :products :id))])
(lib/with-join-fields (for [field (meta/fields :products)]
(meta/field-metadata :products field))))
query (-> (lib/query meta/metadata-provider (meta/table-metadata :orders))
(lib/join join))]
(is (= 8 (-> query :stages first :joins first :fields count)))
(is (= (assoc-in query [:stages 0 :joins 0 :fields] :all)
(lib.remove-replace/normalize-fields-clauses query)))))))
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment