From 885905ebe5d0c912e63b6376464f3f35be3aedd5 Mon Sep 17 00:00:00 2001
From: github-automation-metabase
<166700802+github-automation-metabase@users.noreply.github.com>
Date: Tue, 26 Nov 2024 17:57:09 -0500
Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=96=20backported=20"remove=20pivot-gro?=
=?UTF-8?q?uping=20row=20and=20columns=20in=20tandem"=20(#50530)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* remove pivot-grouping row and columns in tandem (#50476)
* remove pivot-grouping row and columns in tandem
* add pivot card scalar test for csvs and xlsx.
* improve test feedback
* add a 'todo' for pivoted json exports
* indent
* tap tap tap
---------
Co-authored-by: bryan <bryan.maass@gmail.com>
Co-authored-by: Adam James <adam.vermeer2@gmail.com>
---
.../query_processor/streaming/xlsx.clj | 40 +++++++++-------
test/metabase/api/downloads_exports_test.clj | 46 +++++++++++++++++++
2 files changed, 70 insertions(+), 16 deletions(-)
diff --git a/src/metabase/query_processor/streaming/xlsx.clj b/src/metabase/query_processor/streaming/xlsx.clj
index 7d2ef31ac93..7f9bc06418a 100644
--- a/src/metabase/query_processor/streaming/xlsx.clj
+++ b/src/metabase/query_processor/streaming/xlsx.clj
@@ -440,12 +440,14 @@
(defmulti ^:private add-row!
"Adds a row of values to the spreadsheet. Values with the `scaled` viz setting are scaled prior to being added.
- This is based on the equivalent function in Docjure, but adapted to support Metabase viz settings."
+ This is based on the equivalent function in Docjure: [[spreadsheet/add-row!]], but adapted to support Metabase viz
+ settings."
{:arglists '([sheet values cols col-settings cell-styles typed-cell-styles]
[sheet row-num values cols col-settings cell-styles typed-cell-styles])}
(fn [sheet & _args]
(class sheet)))
+;; TODO this add-row! and the one below (For XSSFSheet) should be consolidated
(defmethod add-row! org.apache.poi.xssf.streaming.SXSSFSheet
([^SXSSFSheet sheet values cols col-settings cell-styles typed-cell-styles]
(let [row-num (if (= 0 (.getPhysicalNumberOfRows sheet))
@@ -466,7 +468,8 @@
id-or-name (or (:id col) (:name col))
settings (or (get col-settings {::mb.viz/field-id id-or-name})
(get col-settings {::mb.viz/column-name id-or-name}))
- scaled-val (if (and value (::mb.viz/scale settings))
+ ;; value can be a column header (a string), so if the column is scaled, it'll try to do (* "count" 7)
+ scaled-val (if (and (number? value) (::mb.viz/scale settings))
(* value (::mb.viz/scale settings))
value)
;; Temporal values are converted into strings in the format-rows QP middleware, which is enabled during
@@ -499,7 +502,8 @@
id-or-name (or (:id col) (:name col))
settings (or (get col-settings {::mb.viz/field-id id-or-name})
(get col-settings {::mb.viz/column-name id-or-name}))
- scaled-val (if (and value (::mb.viz/scale settings))
+ ;; value can be a column header (a string), so if the column is scaled, it'll try to do (* "count" 7)
+ scaled-val (if (and (number? value) (::mb.viz/scale settings))
(* value (::mb.viz/scale settings))
value)
;; Temporal values are converted into strings in the format-rows QP middleware, which is enabled during
@@ -618,7 +622,7 @@
(.addColumnLabel pivot-table DataConsolidateFunction/SUM #_(get aggregation-functions idx DataConsolidateFunction/SUM) idx))
(doseq [[idx sort-setting] column-sort-order]
(let [setting (case sort-setting
- :ascending STFieldSortType/ASCENDING
+ :ascending STFieldSortType/ASCENDING
:descending STFieldSortType/DESCENDING)]
(when setting
(-> pivot-table
@@ -698,26 +702,30 @@
(vreset! typed-cell-styles (compute-typed-cell-styles workbook data-format)))))
(write-row! [_ row row-num ordered-cols {:keys [output-order] :as viz-settings}]
- (let [ordered-row (vec (if output-order
- (let [row-v (into [] row)]
- (for [i output-order] (row-v i)))
- row))
- col-settings (::mb.viz/column-settings viz-settings)
- pivot-grouping-key @pivot-grouping-idx
- group (get row pivot-grouping-key)
- modified-row (cond->> ordered-row
- pivot-grouping-key (m/remove-nth pivot-grouping-key))
- {:keys [sheet]} @workbook-data]
+ (let [ordered-row (vec (if output-order
+ (let [row-v (into [] row)]
+ (for [i output-order] (row-v i)))
+ row))
+ col-settings (::mb.viz/column-settings viz-settings)
+ pivot-grouping-key @pivot-grouping-idx
+ group (get row pivot-grouping-key)
+ [row' ordered-cols'] (cond->> [ordered-row ordered-cols]
+ pivot-grouping-key
+ ;; We need to remove the pivot-grouping key if it's there, because we don't show
+ ;; it in the export. `ordered-cols` is a parallel array, so we must remove the
+ ;; corresponding col.
+ (map #(m/remove-nth pivot-grouping-key %)))
+ {:keys [sheet]} @workbook-data]
(when (or (not group)
(= 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)
+ (add-row! sheet (inc row-num) row' ordered-cols' col-settings @cell-styles @typed-cell-styles)
(when (= (inc row-num) *auto-sizing-threshold*)
(autosize-columns! sheet)))))
(finish! [_ {:keys [row_count]}]
(let [{:keys [workbook sheet]} @workbook-data]
(when (or (nil? row_count) (< row_count *auto-sizing-threshold*))
- ;; Auto-size columns if we never hit the row threshold, or a final row count was not provided
+ ;; Auto-size columns if we never hit the row threshold, or a final row count was not provided
(autosize-columns! sheet))
(try
(spreadsheet/save-workbook-into-stream! os workbook)
diff --git a/test/metabase/api/downloads_exports_test.clj b/test/metabase/api/downloads_exports_test.clj
index 00ed156a14f..16d1771fdd5 100644
--- a/test/metabase/api/downloads_exports_test.clj
+++ b/test/metabase/api/downloads_exports_test.clj
@@ -12,6 +12,7 @@
#_{:clj-kondo/ignore [:deprecated-namespace]}
(:require
[cheshire.core :as json]
+ [clojure.data :as data]
[clojure.data.csv :as csv]
[clojure.java.io :as io]
[clojure.set :as set]
@@ -1339,3 +1340,48 @@
:dashcard-download expected-header
:public-dashcard-download expected-header}
(update-vals formatted-results first)))))))))
+
+(defn- pivot-card-with-scalar [scalar]
+ {:display :pivot
+ :visualization_settings {:pivot_table.column_split
+ {:rows ["CATEGORY"]
+ :columns ["CREATED_AT"]
+ :values ["sum"]}
+ :column_settings
+ {"[\"name\",\"sum\"]" (merge {:number_style "currency"
+ :currency_in_header false}
+ (when scalar {:scale scalar}))}}
+ :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}]]}}})
+
+(deftest pivot-with-scale-test
+ (testing "Pivot table exports work with \"Multiply by a number\" (scale)"
+ (mt/dataset test-data
+ (mt/with-temp [:model/Card {:keys [created_at] :as no-scale-card} (pivot-card-with-scalar nil)
+ :model/Card one-scale-card (assoc (pivot-card-with-scalar 1) :created_at created_at)
+ :model/Card zero-scale-card (assoc (pivot-card-with-scalar 0) :created_at created_at)]
+ (let [named-cards {:one-scale-card one-scale-card
+ :two-scale-card zero-scale-card
+ :no-scale-card no-scale-card}]
+ (doseq [[c1-name c2-name export-format expected] [[:one-scale-card :no-scale-card :csv true]
+ [:one-scale-card :two-scale-card :csv false]
+ [:no-scale-card :two-scale-card :csv false]
+ [:one-scale-card :no-scale-card :xlsx true]
+ [:one-scale-card :two-scale-card :xlsx false]
+ [:no-scale-card :two-scale-card :xlsx false]
+ ;; TODO: We don't support JSON for pivot tables, once we
+ ;; do, we should add them here
+ ]]
+ (testing (str "> " (name c1-name) " and " (name c2-name) " with export-format: '" (name export-format) "' should be " expected)
+ (let [c1 (get named-cards c1-name)
+ c2 (get named-cards c2-name)
+ [unique-to-a unique-to-b _both]
+ (data/diff (all-outputs! c1 {:export-format export-format :format-rows true :pivot true})
+ (all-outputs! c2 {:export-format export-format :format-rows true :pivot true}))]
+ (if expected
+ (is (= [nil nil] [unique-to-a unique-to-b]))
+ (is (or (some? unique-to-a) (some? unique-to-b))))))))))))
--
GitLab