[MLv2] Drop redundant :fields clauses from stages and joins

If they're equivalent to the default, they can be removed (for stages)
or replaced by `:all` (on joins).

src/metabase/lib/equality.cljc

@@ -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))))))

src/metabase/lib/field.cljc

@@ -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]

src/metabase/lib/metadata/calculation.cljc

@@ -575,3 +575,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)))))

src/metabase/lib/remove_replace.cljc

@@ -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)))))