Table Column Ordering Should Not Prevent Pivot Exports (#50599) (#50610)

* Table Column Ordering Should Not Prevent Pivot Exports

Prior, when viz-settings existed for table column ordering, or if some columns were hidden in a regular table viz, the
pivot export could fail.

Now, if a pivot table is being exported, the table column sorting is properly ignored and the pivot export should work.

* address feedback

Co-authored-by: adam-james <21064735+adam-james-v@users.noreply.github.com>

src/metabase/query_processor/streaming.clj

@@ -53,49 +53,59 @@
                   table-columns)
       table-columns)))
 
+(defn- pivot-grouping-exists?
+  "Returns `true` if there's a column with the :name \"pivot-grouping\",
+  which is an internal detail from the pivot qp."
+  [cols]
+  (some #(= (:name %) "pivot-grouping") cols))
+
 (defn- export-column-order
   "For each entry in `table-columns` that is enabled, finds the index of the corresponding
   entry in `cols` by name or id. If a col has been remapped, uses the index of the new column.
 
   The resulting list of indices determines the order of column names and data in exports."
   [cols table-columns]
-  (let [table-columns'     (or (validate-table-columms table-columns cols)
-                               ;; If table-columns is not provided (e.g. for saved cards), we can construct a fake one
-                               ;; that retains the original column ordering in `cols`
-                               (for [col cols]
-                                 (let [col-name   (:name col)
-                                       id-or-name (or (:id col) col-name)
-                                       field-ref  (:field_ref col)]
-                                   {::mb.viz/table-column-field-ref (or field-ref [:field id-or-name nil])
-                                    ::mb.viz/table-column-enabled true
-                                    ::mb.viz/table-column-name col-name})))
-        enabled-table-cols (filter ::mb.viz/table-column-enabled table-columns')
-        cols-vector        (into [] cols)
-        ;; cols-index is a map from keys representing fields to their indices into `cols`
-        cols-index         (reduce-kv (fn [m i col]
-                                        ;; Always add col-name as a key, so that native queries and remapped fields work correctly
-                                        (let [m' (assoc m (:name col) i)]
-                                          (if-let [field-ref (:field_ref col)]
-                                            ;; Add a map key based on the column's field-ref, if available
-                                            (assoc m' field-ref i)
-                                            m')))
-                                      {}
-                                      cols-vector)]
-    (->> (map
-          (fn [{field-ref ::mb.viz/table-column-field-ref, col-name ::mb.viz/table-column-name}]
-            (let [index              (or (get cols-index field-ref)
-                                         (get cols-index col-name))
-                  col                (get cols-vector index)
-                  remapped-to-name   (:remapped_to col)
-                  remapped-from-name (:remapped_from col)]
-              (cond
-                remapped-to-name
-                (get cols-index remapped-to-name)
-
-                (not remapped-from-name)
-                index)))
-          enabled-table-cols)
-         (remove nil?))))
+  (if (pivot-grouping-exists? cols)
+    ;; If the columns contain a pivot-grouping, we're exporting a pivot and the cols order is not used,
+    ;; so we can just pass the indices in order.
+    (range (count cols))
+    (let [table-columns'     (or (validate-table-columms table-columns cols)
+                                 ;; If table-columns is not provided (e.g. for saved cards), we can construct a fake one
+                                 ;; that retains the original column ordering in `cols`
+                                 (for [col cols]
+                                   (let [col-name   (:name col)
+                                         id-or-name (or (:id col) col-name)
+                                         field-ref  (:field_ref col)]
+                                     {::mb.viz/table-column-field-ref (or field-ref [:field id-or-name nil])
+                                      ::mb.viz/table-column-enabled   true
+                                      ::mb.viz/table-column-name      col-name})))
+          enabled-table-cols (filter ::mb.viz/table-column-enabled table-columns')
+          cols-vector        (into [] cols)
+          ;; cols-index is a map from keys representing fields to their indices into `cols`
+          cols-index         (reduce-kv (fn [m i col]
+                                          ;; Always add col-name as a key, so that native queries and remapped fields work correctly
+                                          (let [m' (assoc m (:name col) i)]
+                                            (if-let [field-ref (:field_ref col)]
+                                              ;; Add a map key based on the column's field-ref, if available
+                                              (assoc m' field-ref i)
+                                              m')))
+                                        {}
+                                        cols-vector)]
+      (->> (map
+            (fn [{field-ref ::mb.viz/table-column-field-ref, col-name ::mb.viz/table-column-name}]
+              (let [index              (or (get cols-index field-ref)
+                                           (get cols-index col-name))
+                    col                (get cols-vector index)
+                    remapped-to-name   (:remapped_to col)
+                    remapped-from-name (:remapped_from col)]
+                (cond
+                  remapped-to-name
+                  (get cols-index remapped-to-name)
+
+                  (not remapped-from-name)
+                  index)))
+            enabled-table-cols)
+           (remove nil?)))))
 
 (defn order-cols
   "Dedups and orders `cols` based on the contents of table-columns in the provided viz settings. Also

test/metabase/api/downloads_exports_test.clj

@@ -15,6 +15,7 @@
    [clojure.data :as data]
    [clojure.data.csv :as csv]
    [clojure.java.io :as io]
+   [clojure.math.combinatorics :as math.combo]
    [clojure.set :as set]
    [clojure.string :as str]
    [clojure.test :refer :all]
@@ -267,7 +268,7 @@
                       :visualization_settings {:pivot_table.column_split
                                                {:rows    ["CATEGORY"]
                                                 :columns ["CREATED_AT"]
-                                                :values  ["sum" "avg"]}
+                                                :values  ["sum"]}
                                                :column_settings
                                                {"[\"name\",\"sum\"]" {:number_style       "currency"
                                                                       :currency_in_header false}}}
@@ -437,6 +438,53 @@
                         ((fn [m] (update-vals m #(into #{} (mapv first %)))))
                         (apply concat))))))))))
 
+(deftest ^:parallel simple-pivot-export-works-even-with-table-column-ordering-test
+  (testing "Pivot table exports are not affected by table sort settings"
+    (testing "Try some permutations with csv"
+      (doseq [col-order   (math.combo/permutations [{:name "CATEGORY"}
+                                                    {:name "CREATED_AT"}
+                                                    {:name "sum"}])
+              col-enabled [true false]]
+        (mt/dataset test-data
+          (mt/with-temp [:model/Card card
+                         {:display                :pivot
+                          :visualization_settings {:table.columns
+                                                   ;; the :table.columns key specifies order/enabled status of columns for regular table viz
+                                                   ;; and should not cause pivot exports to fail.
+                                                   (mapv #(assoc % :enabled col-enabled) col-order)
+                                                   :pivot_table.column_split
+                                                   {:rows    ["CATEGORY"]
+                                                    :columns ["CREATED_AT"]
+                                                    :values  ["sum"]}
+                                                   :column_settings
+                                                   {"[\"name\",\"sum\"]" {:number_style       "currency"
+                                                                          :currency_in_header false}}}
+                          :dataset_query          {:database (mt/id)
+                                                   :type     :query
+                                                   :query
+                                                   {:source-table (mt/id :products)
+                                                    :aggregation  [[:sum [:field (mt/id :products :price) {:base-type :type/Float}]]]
+                                                    :breakout     [[:field (mt/id :products :category) {:base-type :type/Text}]
+                                                                   [:field (mt/id :products :created_at) {:base-type :type/DateTime :temporal-unit :year}]]}}}]
+            (testing "they work regardless of the table.columns setting"
+              (is (= [["Category" "2016" "2017" "2018" "2019" "Row totals"]
+                      ["Doohickey" "$632.14" "$854.19" "$496.43" "$203.13" "$2,185.89"]
+                      ["Gadget" "$679.83" "$1,059.11" "$844.51" "$435.75" "$3,019.20"]
+                      ["Gizmo" "$529.70" "$1,080.18" "$997.94" "$227.06" "$2,834.88"]
+                      ["Widget" "$987.39" "$1,014.68" "$912.20" "$195.04" "$3,109.31"]
+                      ["Grand totals" "$2,829.06" "$4,008.16" "$3,251.08" "$1,060.98" "$11,149.28"]]
+                     (card-download card {:export-format :csv :format-rows true :pivot true}))))
+            (testing "the xlsx export has a pivot table"
+              (let [result (mt/user-http-request :crowberto :post 200
+                                                 (format "card/%d/query/xlsx" (:id card))
+                                                 :format_rows   true
+                                                 :pivot_results true)
+                    pivot  (with-open [in (io/input-stream result)]
+                             (->> (spreadsheet/load-workbook in)
+                                  (spreadsheet/select-sheet "pivot")
+                                  ((fn [s] (.getPivotTables ^XSSFSheet s)))))]
+                (is (some? pivot))))))))))
+
 (deftest ^:parallel pivot-export-test
   []
   (mt/dataset test-data