Fix a Couple bugs related to pivot exports (#48867)

* Fix a Couple bugs related to pivot exports

 - pivot-grouping value, when not an int (in the case of Oracle dbs), would cause exports to contain zero rows. This
 fixes that
 - json formatted/unformatted for the dataset API endpoints (unsaved questions) was not being used, so all exports
 were formatted

* json and csv use (int group) to check row inclusion

* grouping change fix tests

* json formatted/unformatted test

* xlsx formatters are correct when pivot-grouping col is removed

* cljfmt

* fix up failing tests

* fix test

* Update src/metabase/api/dataset.clj

src/metabase/api/dataset.clj

@@ -129,10 +129,12 @@
 
 (api/defendpoint POST ["/:export-format", :export-format export-format-regex]
   "Execute a query and download the result data as a file in the specified format."
-  [export-format :as {{:keys [query visualization_settings]
+  [export-format :as {{:keys [query visualization_settings pivot_results format_rows]
                        :or   {visualization_settings "{}"}} :params}]
   {query                  ms/JSONString
    visualization_settings ms/JSONString
+   format_rows            [:maybe ms/BooleanValue]
+   pivot_results          [:maybe ms/BooleanValue]
    export-format          ExportFormat}
   (let [{:keys [was-pivot] :as query} (json/parse-string query keyword)
         query                         (dissoc query :was-pivot)
@@ -144,7 +146,9 @@
                                           (dissoc :constraints)
                                           (update :middleware #(-> %
                                                                    (dissoc :add-default-userland-constraints? :js-int-to-string?)
-                                                                   (assoc :process-viz-settings? true
+                                                                   (assoc :format-rows?          (or format_rows false)
+                                                                          :pivot?                (or pivot_results false)
+                                                                          :process-viz-settings? true
                                                                           :skip-results-metadata? true))))]
     (run-streaming-query
      (qp/userland-query query)

src/metabase/query_processor/streaming/csv.clj

@@ -109,25 +109,26 @@
               (.flush writer)))))
 
       (write-row! [_ row _row-num _ {:keys [output-order]}]
-        (let [ordered-row (if output-order
-                            (let [row-v (into [] row)]
-                              (into [] (for [i output-order] (row-v i))))
-                            row)]
+        (let [ordered-row              (if output-order
+                                         (let [row-v (into [] row)]
+                                           (into [] (for [i output-order] (row-v i))))
+                                         row)
+              {:keys [pivot-grouping]} (or (:config @pivot-data) @pivot-data)
+              group                    (get ordered-row pivot-grouping)]
           (if (contains? @pivot-data :config)
             ;; if we're processing a pivot result, we don't write it out yet, just aggregate it
             ;; so that we can post process the data in finish!
-            (when (= 0 (nth ordered-row (get-in @pivot-data [:config :pivot-grouping])))
-              (swap! pivot-data (fn [a] (qp.pivot.postprocess/add-row a ordered-row))))
+            (when (= qp.pivot.postprocess/NON_PIVOT_ROW_GROUP (int group))
+              (swap! pivot-data (fn [pivot-data] (qp.pivot.postprocess/add-row pivot-data ordered-row))))
 
-            (if-let [{:keys [pivot-grouping]} @pivot-data]
-              (let [group (get ordered-row pivot-grouping)]
-                (when (= 0 group)
-                  (let [formatted-row (->> (perf/mapv (fn [formatter r]
-                                                        (formatter (common/format-value r)))
-                                                      @ordered-formatters ordered-row)
-                                           (m/remove-nth pivot-grouping))]
-                    (write-csv writer [formatted-row])
-                    (.flush writer))))
+            (if group
+              (when (= qp.pivot.postprocess/NON_PIVOT_ROW_GROUP (int group))
+                (let [formatted-row (->> (perf/mapv (fn [formatter r]
+                                                      (formatter (common/format-value r)))
+                                                    @ordered-formatters ordered-row)
+                                         (m/remove-nth pivot-grouping))]
+                  (write-csv writer [formatted-row])
+                  (.flush writer)))
               (let [formatted-row (perf/mapv (fn [formatter r]
                                                (formatter (common/format-value r)))
                                              @ordered-formatters ordered-row)]

src/metabase/query_processor/streaming/json.clj

@@ -67,8 +67,8 @@
                                    pivot-grouping-key (m/remove-nth pivot-grouping-key))]
           ;; when a pivot-grouping col exists, we check its group number. When it's zero,
           ;; we keep it, otherwise don't include it in the results as it's a row representing a subtotal of some kind
-          (when (or (= qp.pivot.postprocess/NON_PIVOT_ROW_GROUP group)
-                    (not group))
+          (when (or (not group)
+                    (= qp.pivot.postprocess/NON_PIVOT_ROW_GROUP (int group)))
             (when-not (zero? row-num)
               (.write writer ",\n"))
             (json/generate-stream

src/metabase/query_processor/streaming/xlsx.clj

@@ -661,9 +661,11 @@
               (vreset! workbook-data wb)))
 
           (let [{:keys [workbook sheet]} @workbook-data
-                data-format              (. ^SXSSFWorkbook workbook createDataFormat)]
+                data-format              (. ^SXSSFWorkbook workbook createDataFormat)
+                cols                     (cond->> ordered-cols
+                                           pivot-grouping-key (m/remove-nth pivot-grouping-key))]
             (set-no-style-custom-helper sheet)
-            (vreset! cell-styles (compute-column-cell-styles workbook data-format viz-settings ordered-cols format-rows?))
+            (vreset! cell-styles (compute-column-cell-styles workbook data-format viz-settings cols format-rows?))
             (vreset! typed-cell-styles (compute-typed-cell-styles workbook data-format)))))
 
       (write-row! [_ row row-num ordered-cols {:keys [output-order] :as viz-settings}]
@@ -678,7 +680,7 @@
                                    pivot-grouping-key (m/remove-nth pivot-grouping-key))
               {:keys [sheet]}    @workbook-data]
           (when (or (not group)
-                    (= group 0))
+                    (= qp.pivot.postprocess/NON_PIVOT_ROW_GROUP (int group)))
             (add-row! sheet (inc row-num) modified-row ordered-cols col-settings @cell-styles @typed-cell-styles)
             (when (= (inc row-num) *auto-sizing-threshold*)
               (autosize-columns! sheet)))))

test/metabase/api/dataset_test.clj

@@ -727,7 +727,8 @@
                  (->> (mt/user-http-request
                        :crowberto :post 200
                        (format "dataset/%s" (name export-format))
-                       :query (json/generate-string (assoc q :middleware {:format-rows? apply-formatting?})))
+                       :query (json/generate-string q)
+                       :format_rows apply-formatting?)
                       ((get output-helper export-format))))))))))
 
 (deftest ^:parallel query-metadata-test

test/metabase/api/downloads_exports_test.clj

@@ -75,9 +75,9 @@
                                             :was-pivot (boolean pivot)
                                             :info {:visualization-settings (:visualization_settings card)}
                                             :middleware
-                                            {:format-rows?    format-rows
-                                             :pivot?          (boolean pivot)
-                                             :userland-query? true})))
+                                            {:userland-query? true}))
+                             :format_rows   format-rows
+                             :pivot_results (boolean pivot))
        (process-results pivot export-format)))
 
 (defn public-question-download
@@ -158,8 +158,7 @@
                                                   :alert_condition "rows"}
                    :model/PulseCard _ (merge
                                        (when (= :csv  export-format) {:include_csv true})
-                                       (when (= :json export-format) {:include_json true})
-                                       (when (= :xlsx export-format) {:include_xlsx true})
+                                       (when (= :xlsx export-format) {:include_xls true})
                                        {:format_rows format-rows}
                                        {:pivot_results pivot}
                                        {:pulse_id pulse-id
@@ -183,8 +182,7 @@
                                                     :dashboard_id (:dashboard_id card-or-dashcard)}
                      :model/PulseCard _ (merge
                                          (when (= :csv  export-format) {:include_csv true})
-                                         (when (= :json export-format) {:include_json true})
-                                         (when (= :xlsx export-format) {:include_xlsx true})
+                                         (when (= :xlsx export-format) {:include_xls true})
                                          {:format_rows format-rows}
                                          {:pivot_results pivot}
                                          {:pulse_id          pulse-id
@@ -205,8 +203,7 @@
                                                     :dashboard_id dashboard-id}
                      :model/PulseCard _ (merge
                                          (when (= :csv  export-format) {:include_csv true})
-                                         (when (= :json export-format) {:include_json true})
-                                         (when (= :xlsx export-format) {:include_xlsx true})
+                                         (when (= :xlsx export-format) {:include_xls true})
                                          {:format_rows format-rows}
                                          {:pivot_results pivot}
                                          {:pulse_id          pulse-id
@@ -1003,6 +1000,28 @@
               (is (= 2
                      (count (second data)))))))))))
 
+(deftest unpivoted-pivot-results-use-correct-formatters-in-xlsx
+  (testing "If a pivot question is downloaded or exported unpivoted as XLSX, the formatters are set up properly (#48158)"
+    (mt/dataset test-data
+      (mt/with-temp [:model/Card {pivot-card-id :id}
+                     {:display                :pivot
+                      :visualization_settings {:pivot_table.column_split
+                                               {:rows    []
+                                                :columns [[:field (mt/id :products :category) {:base-type :type/Text}]]
+                                                :values  [[:aggregation 0]]}
+                                               :column_settings {"[\"name\",\"count\"]" {:number_style "percent"}}}
+                      :dataset_query          {:database (mt/id)
+                                               :type     :query
+                                               :query
+                                               {:source-table (mt/id :products)
+                                                :aggregation  [[:count] #_[:sum [:field (mt/id :products :price) {:base-type :type/Float}]]]
+                                                :breakout     [[:field (mt/id :products :category) {:base-type :type/Text}]]}}}]
+        (let [result   (mt/user-http-request :crowberto :post 200
+                                             (format "card/%d/query/xlsx?format_rows=true" pivot-card-id)
+                                             {})
+              data   (process-results false :xlsx result)]
+          (is (= ["Doohickey" "4,200.00%"] (second data))))))))
+
 (deftest format-rows-value-affects-xlsx-exports
   (testing "Format-rows true/false is respected for xlsx exports."
     (mt/dataset test-data
@@ -1023,11 +1042,49 @@
                                                 :breakout     [[:field (mt/id :products :category) {:base-type :type/Text}]
                                                                [:field (mt/id :products :created_at) {:base-type :type/DateTime :temporal-unit :year}]]}}}]
         (is (= [["Category" "Created At" "Sum of Price"]
-                ["Doohickey" "2016" "632.14"]
-                ["Doohickey" "2017" "854.19"]]
+                ["Doohickey" "2016" "[$$]632.14"]
+                ["Doohickey" "2017" "[$$]854.19"]]
                (take 3 (card-download card {:export-format :xlsx :format-rows true :pivot true})))
             ;; Excel will apply a default format which is seen here. The 'actual' data in the cells is unformatted.
             (= [["Category" "Created At" "Sum of Price"]
                 ["Doohickey" "January 1, 2016, 12:00 AM" "632.14"]
                 ["Doohickey" "January 1, 2017, 12:00 AM" "854.19"]]
                (take 3 (card-download card {:export-format :xlsx :format-rows false :pivot true}))))))))
+
+(deftest unformatted-downloads-and-exports-keep-numbers-as-numbers
+  (testing "Unformatted numbers in downloads remain numbers."
+    (mt/dataset test-data
+      (mt/with-temp [:model/Card card {:display       :table
+                                       :dataset_query {:database (mt/id)
+                                                       :type     :native
+                                                       :native   {:query "SELECT 1234.567 as A"}}}]
+        (testing "CSV downloads respect the formatted/unformatted setting"
+          (let [formatted-json-results   (all-downloads card {:export-format :csv :format-rows true})
+                unformatted-json-results (all-downloads card {:export-format :csv :format-rows false})]
+            (is (= {:unsaved-card-download    [["A"] ["1,234.57"]]
+                    :card-download            [["A"] ["1,234.57"]]
+                    :public-question-download [["A"] ["1,234.57"]]
+                    :dashcard-download        [["A"] ["1,234.57"]]
+                    :public-dashcard-download [["A"] ["1,234.57"]]}
+                   formatted-json-results))
+            (is (= {:unsaved-card-download    [["A"] ["1234.567"]]
+                    :card-download            [["A"] ["1234.567"]]
+                    :public-question-download [["A"] ["1234.567"]]
+                    :dashcard-download        [["A"] ["1234.567"]]
+                    :public-dashcard-download [["A"] ["1234.567"]]}
+                   unformatted-json-results))))
+        (testing "JSON downloads respect the formatted/unformatted setting"
+          (let [formatted-json-results   (all-downloads card {:export-format :json :format-rows true})
+                unformatted-json-results (all-downloads card {:export-format :json :format-rows false})]
+            (is (= {:unsaved-card-download    [["A"] ["1,234.57"]]
+                    :card-download            [["A"] ["1,234.57"]]
+                    :public-question-download [["A"] ["1,234.57"]]
+                    :dashcard-download        [["A"] ["1,234.57"]]
+                    :public-dashcard-download [["A"] ["1,234.57"]]}
+                   formatted-json-results))
+            (is (= {:unsaved-card-download    [["A"] [1234.567]]
+                    :card-download            [["A"] [1234.567]]
+                    :public-question-download [["A"] [1234.567]]
+                    :dashcard-download        [["A"] [1234.567]]
+                    :public-dashcard-download [["A"] [1234.567]]}
+                   unformatted-json-results))))))))

test/metabase/query_processor/streaming/csv_test.clj

@@ -47,7 +47,8 @@
             ["4" "March 11, 2014"     "5" "4"]
             ["5" "May 5, 2013"        "3" "49"]]
            (let [result (mt/user-http-request :crowberto :post 200 "dataset/csv" :query
-                                              (json/generate-string (mt/mbql-query checkins {:order-by [[:asc $id]], :limit 5})))]
+                                              (json/generate-string (mt/mbql-query checkins {:order-by [[:asc $id]], :limit 5}))
+                                              :format_rows true)]
              (take 5 (parse-and-sort-csv result)))))))
 
 (deftest errors-not-include-visualization-settings
@@ -71,7 +72,8 @@
   (testing "NULL values should be written correctly"
     (mt/dataset defs/test-data-null-date
       (let [result (mt/user-http-request :crowberto :post 200 "dataset/csv" :query
-                                         (json/generate-string (mt/mbql-query checkins {:order-by [[:asc $id]], :limit 5})))]
+                                         (json/generate-string (mt/mbql-query checkins {:order-by [[:asc $id]], :limit 5}))
+                                         :format_rows true)]
         (is (= [["1" "April 7, 2014"      "" "5" "12"]
                 ["2" "September 18, 2014" "" "1" "31"]
                 ["3" "September 15, 2014" "" "8" "56"]
@@ -81,7 +83,8 @@
 
 (deftest datetime-fields-are-untouched-when-exported
   (let [result (mt/user-http-request :crowberto :post 200 "dataset/csv" :query
-                                     (json/generate-string (mt/mbql-query users {:order-by [[:asc $id]], :limit 5})))]
+                                     (json/generate-string (mt/mbql-query users {:order-by [[:asc $id]], :limit 5}))
+                                     :format_rows true)]
     (is (= [["1" "Plato Yeshua" "April 1, 2014, 8:30 AM"]
             ["2" "Felipinho Asklepios" "December 5, 2014, 3:15 PM"]
             ["3" "Kaneonuskatew Eiran" "November 6, 2014, 4:15 PM"]
@@ -101,7 +104,8 @@
                                               [:field (mt/id :venues :longitude) {:base-type :type/Float}]
                                               [:field (mt/id :venues :latitude) {:base-type :type/Float}]]
                                :order-by     [[:asc (mt/id :venues :id)]]
-                               :limit        5}}))]
+                               :limit        5}})
+                  :format_rows true)]
       (is (= [["1" "165.37400000° W" "10.06460000° N"]
               ["2" "118.32900000° W" "34.09960000° N"]
               ["3" "118.42800000° W" "34.04060000° N"]

test/metabase/query_processor/streaming/json_test.clj

@@ -22,7 +22,8 @@
                                               [:field (mt/id :venues :longitude) {:base-type :type/Float}]
                                               [:field (mt/id :venues :latitude) {:base-type :type/Float}]]
                                :order-by     [[:asc (mt/id :venues :id)]]
-                               :limit        5}}))]
+                               :limit        5}})
+                  :format_rows true)]
       (is (= [{:ID "1", :Longitude "165.37400000° W", :Latitude "10.06460000° N"}
               {:ID "2", :Longitude "118.32900000° W", :Latitude "34.09960000° N"}
               {:ID "3", :Longitude "118.42800000° W", :Latitude "34.04060000° N"}

test/metabase/query_processor/streaming/test_util.clj

@@ -78,7 +78,8 @@
                                        (assoc-in query [:middleware :js-int-to-string?] false))
                  (mt/user-real-request :crowberto :post (format "dataset/%s" (name export-format))
                                        {:request-options {:as :byte-array}}
-                                       :query (json/generate-string query)))]
+                                       :query (json/generate-string query)
+                                       :format_rows true))]
     (with-open [is (ByteArrayInputStream. byytes)]
       (apply parse-result export-format is args))))