Merge pull request #8787 from metabase/fix-column-reordering-for-bucketed-breakouts

Fix column reordering for bucketed breakouts 

src/metabase/mbql/normalize.clj

@@ -31,7 +31,9 @@
   (:require [clojure.tools.logging :as log]
             [clojure.walk :as walk]
             [medley.core :as m]
-            [metabase.mbql.util :as mbql.u]
+            [metabase.mbql
+             [predicates :as mbql.pred]
+             [util :as mbql.u]]
             [metabase.util :as u]
             [metabase.util.i18n :refer [tru]]))
 
@@ -506,11 +508,27 @@
 (defn- remove-breakout-fields-from-fields
   "Remove any Fields specified in both `:breakout` and `:fields` from `:fields`; it is implied that any breakout Field
   will be returned, specifying it in both would imply it is to be returned twice, which tends to cause confusion for
-  the QP and drivers."
+  the QP and drivers. (This is done to work around historic bugs with the way queries were generated on the frontend;
+  I'm not sure this behavior makes sense, but removing it would break existing queries.)
+
+  We will remove either exact matches:
+
+    {:breakout [[:field-id 10]], :fields [[:field-id 10]]} ; -> {:breakout [[:field-id 10]]}
+
+  or unbucketed matches:
+
+    {:breakout [[:datetime-field [:field-id 10] :month]], :fields [[:field-id 10]]}
+    ;; -> {:breakout [[:field-id 10]]}"
   [{{:keys [breakout fields]} :query, :as query}]
   (if-not (and (seq breakout) (seq fields))
     query
-    (update-in query [:query :fields] (comp vec (partial remove (set breakout))))))
+    ;; get a set of all Field clauses (of any type) in the breakout. For `datetime-field` clauses, we'll include both
+    ;; the bucketed `[:datetime-field <field> ...]` clause and the `<field>` clause it wraps
+    (let [breakout-fields (set (reduce concat (mbql.u/match breakout
+                                                [:datetime-field field-clause _] [&match field-clause]
+                                                mbql.pred/Field?                 [&match])))]
+      ;; now remove all the Fields in `:fields` that match the ones in the set
+      (update-in query [:query :fields] (comp vec (partial remove breakout-fields))))))
 
 (defn- perform-whole-query-transformations
   "Perform transformations that operate on the query as a whole, making sure the structure as a whole is logical and

src/metabase/mbql/predicates.clj

@@ -17,3 +17,7 @@
 (def ^{:arglists '([ag-clause])} Aggregation?
   "Is this a valid Aggregation clause?"
   (complement (s/checker mbql.s/Aggregation)))
+
+(def ^{:arglists '([field-clause])} Field?
+  "Is this a valid Field clause?"
+  (complement (s/checker mbql.s/Field)))

src/metabase/mbql/schema.clj

@@ -181,16 +181,16 @@
 ;; TODO - binning strategy param is disallowed for `:default` and required for the others. For `num-bins` it must also
 ;; be an integer.
 (defclause ^{:requires-features #{:binning}} binning-strategy
-  field          BinnableField
-  strategy-name  BinningStrategyName
-  strategy-param (optional (s/constrained s/Num (complement neg?) "strategy param must be >= 0."))
+  field            BinnableField
+  strategy-name    BinningStrategyName
+  strategy-param   (optional (s/constrained s/Num (complement neg?) "strategy param must be >= 0."))
   ;; These are added in automatically by the `binning` middleware. Don't add them yourself, as they're just be
   ;; replaced. Driver implementations can rely on this being populated
   resolved-options (optional ResolvedBinningStrategyOptions))
 
 (def Field
   "Schema for anything that refers to a Field, from the common `[:field-id <id>]` to variants like `:datetime-field` or
-  `:fk->`."
+  `:fk->` or an expression reference `[:expression <name>]`."
   (one-of field-id field-literal fk-> datetime-field expression binning-strategy))
 
 ;; aggregate field reference refers to an aggregation, e.g.

test/metabase/mbql/normalize_test.clj

@@ -724,6 +724,45 @@
     :query {:breakout [[:field-id 1] [:field-id 2]]
             :fields   [[:field-id 2] [:field-id 3]]}}))
 
+;; should work with FKs
+(expect
+  {:type  :query
+   :query {:breakout [[:field-id 1]
+                      [:fk-> [:field-id 2] [:field-id 4]]]
+           :fields   [[:field-id 3]]}}
+  (#'normalize/perform-whole-query-transformations
+   {:type  :query
+    :query {:breakout [[:field-id 1]
+                       [:fk-> [:field-id 2] [:field-id 4]]]
+            :fields   [[:fk-> [:field-id 2] [:field-id 4]]
+                       [:field-id 3]]}}))
+
+;; should work if the Field is bucketed in the breakout & in fields
+(expect
+  {:type  :query
+   :query {:breakout [[:field-id 1]
+                      [:datetime-field [:fk-> [:field-id 2] [:field-id 4]] :month]]
+           :fields   [[:field-id 3]]}}
+  (#'normalize/perform-whole-query-transformations
+   {:type  :query
+    :query {:breakout [[:field-id 1]
+                       [:datetime-field [:fk-> [:field-id 2] [:field-id 4]] :month]]
+            :fields   [[:datetime-field [:fk-> [:field-id 2] [:field-id 4]] :month]
+                       [:field-id 3]]}}))
+
+;; should work if the Field is bucketed in the breakout but not in fields
+(expect
+  {:type  :query
+   :query {:breakout [[:field-id 1]
+                      [:datetime-field [:fk-> [:field-id 2] [:field-id 4]] :month]]
+           :fields   [[:field-id 3]]}}
+  (#'normalize/perform-whole-query-transformations
+   {:type  :query
+    :query {:breakout [[:field-id 1]
+                       [:datetime-field [:fk-> [:field-id 2] [:field-id 4]] :month]]
+            :fields   [[:fk-> [:field-id 2] [:field-id 4]]
+                       [:field-id 3]]}}))
+
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                              REMOVE EMPTY CLAUSES                                              |