From 37abc084499d5b1a83c1d17f1bb18576d0b54fa2 Mon Sep 17 00:00:00 2001
From: adam-james <21064735+adam-james-v@users.noreply.github.com>
Date: Tue, 18 Jun 2024 13:20:00 -0700
Subject: [PATCH] Pivot Tables with no Pivot Columns should still download
(#44329)
* Pivot Tables with no Pivot Columns should still download
Fixes #44159
Pivot Tables can still be valid without columns, and therefore the download should respect such a query and be
successfully exported.
In this PR, the postprocessing of pivot results into 'visual pivot' exports for csv takes into account the zero-column
configuration and the download no longer fails.
* add a test to confirm that zero cols and >1 measures works as well
* Handle the case where there are zero pivot-rows.
This is a similar problem to the zero pivot-cols case, just with rows.
The app actually doesn't render these tables correctly, but that's a separate frontend bug
---
src/metabase/api/embed/common.clj | 21 -----
.../query_processor/pivot/postprocess.clj | 67 ++++++++-----
.../query_processor/streaming/csv.clj | 3 +-
test/metabase/api/downloads_exports_test.clj | 94 ++++++++++++++++++-
4 files changed, 136 insertions(+), 49 deletions(-)
diff --git a/src/metabase/api/embed/common.clj b/src/metabase/api/embed/common.clj
index 3f25b55db1c..ed2ce7b7d9e 100644
--- a/src/metabase/api/embed/common.clj
+++ b/src/metabase/api/embed/common.clj
@@ -251,27 +251,6 @@
(assoc (select-keys param [:type :target :slug])
:value value))))
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
;;; ---------------------------- Card Fns used by both /api/embed and /api/preview_embed -----------------------------
(defn card-for-unsigned-token
diff --git a/src/metabase/query_processor/pivot/postprocess.clj b/src/metabase/query_processor/pivot/postprocess.clj
index 1865b6a17a0..c0cdfb56033 100644
--- a/src/metabase/query_processor/pivot/postprocess.clj
+++ b/src/metabase/query_processor/pivot/postprocess.clj
@@ -44,8 +44,10 @@
(concat (vec (remove nil? all-vals)) (when include-nil? [nil]))))
(mu/defn ^:private pivot-row-titles
- [{:keys [column-titles pivot-rows]} :- ::pivot-spec]
- (mapv #(get column-titles %) pivot-rows))
+ [{:keys [column-titles pivot-rows pivot-cols]} :- ::pivot-spec]
+ (if (seq pivot-rows)
+ (mapv #(get column-titles %) pivot-rows)
+ [(get column-titles (first pivot-cols) "")]))
(mu/defn ^:private pivot-measure-titles
[{:keys [column-titles pivot-measures]} :- ::pivot-spec]
@@ -54,27 +56,28 @@
(mu/defn ^:private header-builder
"Construct the export-style pivot headers from the raw pivot rows, according to the indices specified in `pivot-spec`."
[rows {:keys [pivot-cols pivot-measures] :as pivot-spec} :- ::pivot-spec]
- (let [row-titles (pivot-row-titles pivot-spec)
- measure-titles (pivot-measure-titles pivot-spec)
- n-measures (count pivot-measures)
- multiple-measures? (< 1 n-measures)
+ (let [row-titles (pivot-row-titles pivot-spec)
+ measure-titles (pivot-measure-titles pivot-spec)
+ n-measures (count pivot-measures)
+ multiple-measures? (< 1 n-measures)
+ include-row-totals-header? (seq pivot-cols)
;; For each pivot column, get the possible values for that column
- ;; Then, get the cartesian product of each to for all of the value groups
+ ;; Then, get the cartesian product of each for all of the value groups
;; Each group will have (count pivot-cols) entries and the values
;; will be from the columns in the same order as presented in pivot-cols.
;; So, if pivot-cols is [0 1], the first col-value-group will have [first-value-from-first-col first-value-from-second-col]
- col-value-groups (apply math.combo/cartesian-product (concat
- (map (fn [col-k]
- (all-values-for rows col-k false))
- pivot-cols)
- (when (seq measure-titles)
- [measure-titles])))
- header-indices (if multiple-measures?
- ;; when there are more than 1 pivot-measures, we need to
- ;; add one more header row that holds the titles of the measure columns
- ;; and we know it's always just one more row, so we can inc the count.
- (range (inc (count pivot-cols)))
- (range (count pivot-cols)))]
+ col-value-groups (apply math.combo/cartesian-product (concat
+ (map (fn [col-k]
+ (all-values-for rows col-k false))
+ pivot-cols)
+ (when (seq measure-titles)
+ [measure-titles])))
+ header-indices (if (or multiple-measures? (not (seq pivot-cols)))
+ ;; when there are more than 1 pivot-measures, we need to
+ ;; add one more header row that holds the titles of the measure columns
+ ;; and we know it's always just one more row, so we can inc the count.
+ (range (inc (count pivot-cols)))
+ (range (count pivot-cols)))]
;; Each Header (1 header row per pivot-col) will first start with the Pivot Row Titles. There will be (count pivot-rows) entries.
;; Then, Get all of the nth entries in the col-value-gropus for the nth header, and then append "Row Totals" label.
(mapv
@@ -84,9 +87,11 @@
(map #(nth % col-idx) col-value-groups)
(if (and
multiple-measures?
+ (seq pivot-cols)
(= col-idx (last header-indices)))
measure-titles
- (repeat (max 1 n-measures) "Row totals")))))
+ (when include-row-totals-header?
+ (repeat (max 1 n-measures) "Row totals"))))))
header-indices)))
(mu/defn ^:private col-grouper
@@ -98,8 +103,9 @@
This is used inside `row-grouper` on a subset of the total list of raw pivot rows."
[rows {:keys [pivot-cols]} :- ::pivot-spec]
- (let [cols-groups (group-by (apply juxt (map (fn [k] #(get % k)) pivot-cols)) rows)]
- cols-groups))
+ (when (seq pivot-cols)
+ (let [cols-groups (group-by (apply juxt (map (fn [k] #(get % k)) pivot-cols)) rows)]
+ cols-groups)))
(mu/defn ^:private row-grouper
"Map of raw pivot rows keyed by [pivot-rows]. The logic for how the map is initially constructed is the same
@@ -114,7 +120,9 @@
`(get-in m [[row-idx1 row-idx2] [col-idx1 col-idx2]])`"
[rows {:keys [pivot-rows pivot-cols pivot-measures] :as pivot-spec} :- ::pivot-spec]
- (let [rows-groups (group-by (apply juxt (map (fn [k] #(get % k)) pivot-rows)) rows)
+ (let [rows-groups (if (seq pivot-rows)
+ (group-by (apply juxt (map (fn [k] #(get % k)) pivot-rows)) rows)
+ {[nil] rows})
sub-rows-fn (fn [sub-rows]
(let [cols-groups (col-grouper sub-rows pivot-spec)
padded-sub-rows (vec
@@ -131,7 +139,12 @@
[(vec (repeat (count pivot-cols) nil))])))]
(vec (mapcat (fn [row]
(mapv #(get row %) pivot-measures))
- padded-sub-rows))))]
+ ;; cols-groups will be nil if there are no pivot columns
+ ;; In such a case, we don't need to modify the rows with padding
+ ;; we only need to grab the pivot-measures directly
+ (if cols-groups
+ padded-sub-rows
+ (take 1 sub-rows))))))]
(-> rows-groups
(update-vals sub-rows-fn))))
@@ -192,8 +205,10 @@
- Run the `totals-row-fn` to add the Row totals and Grand totals labels in the right spots."
[rows {:keys [pivot-rows] :as pivot-spec} :- ::pivot-spec]
(let [row-groups (row-grouper rows pivot-spec)
- ks (mapv vec (concat
- (apply math.combo/cartesian-product (map #(all-values-for rows % true) pivot-rows))))]
+ ks (if (seq pivot-rows)
+ (mapv vec (concat
+ (apply math.combo/cartesian-product (map #(all-values-for rows % true) pivot-rows))))
+ [[nil]])]
(->> (map (fn [k] (vec (concat k (get row-groups k)))) ks)
(filter #(< (count pivot-rows) (count %)))
(map #(totals-row-fn % pivot-spec)))))
diff --git a/src/metabase/query_processor/streaming/csv.clj b/src/metabase/query_processor/streaming/csv.clj
index 957403b6275..6ea4be571e6 100644
--- a/src/metabase/query_processor/streaming/csv.clj
+++ b/src/metabase/query_processor/streaming/csv.clj
@@ -40,7 +40,8 @@
col-names (when-not opts (common/column-titles ordered-cols (::mb.viz/column-settings viz-settings) format-rows?))]
;; when pivot options exist, we want to save them to access later when processing the complete set of results for export.
(when opts
- (reset! pivot-options opts))
+ (reset! pivot-options (merge {:pivot-rows []
+ :pivot-cols []} opts)))
(vreset! ordered-formatters
(if format-rows?
(mapv #(formatter/create-formatter results_timezone % viz-settings) ordered-cols)
diff --git a/test/metabase/api/downloads_exports_test.clj b/test/metabase/api/downloads_exports_test.clj
index ef855543087..57dc7754a19 100644
--- a/test/metabase/api/downloads_exports_test.clj
+++ b/test/metabase/api/downloads_exports_test.clj
@@ -137,7 +137,7 @@
(is (= "Grand Totals"
(first (last result)))))))))))
-(deftest multi-measure-pivot-tables-headers-test
+(deftest ^:parallel multi-measure-pivot-tables-headers-test
(testing "Pivot tables with multiple measures correctly include the measure titles in the final header row."
(mt/dataset test-data
(mt/with-temp [:model/Card {pivot-card-id :id}
@@ -170,3 +170,95 @@
"Sum of Price"
"Average of Rating"]]
(take 2 result))))))))
+
+(deftest ^:parallel zero-column-pivot-tables-test
+ (testing "Pivot tables with zero columns download correctly."
+ (mt/dataset test-data
+ (mt/with-temp [:model/Card {pivot-card-id :id}
+ {:display :pivot
+ :visualization_settings {:pivot_table.column_split
+ {:rows [[:field (mt/id :products :created_at) {:base-type :type/DateTime :temporal-unit :month}]
+ [:field (mt/id :products :category) {:base-type :type/Text}]]
+ :columns []
+ :values [[:aggregation 0]]}}
+ :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 :month}]]}}}]
+ (let [result (->> (mt/user-http-request :crowberto :post 200 (format "card/%d/query/csv?format_rows=false" pivot-card-id))
+ csv/read-csv)]
+ (is (= [["Created At" "Category" "Sum of Price"]
+ ["2016-05-01T00:00:00Z" "Doohickey" "144.12"]
+ ["2016-05-01T00:00:00Z" "Gadget" "81.58"]
+ ["2016-05-01T00:00:00Z" "Gizmo" "75.09"]
+ ["2016-05-01T00:00:00Z" "Widget" "90.21"]
+ ["Totals for 2016-05-01T00:00:00Z" "" "391"]]
+ (take 6 result))))))))
+
+(deftest ^:parallel zero-row-pivot-tables-test
+ (testing "Pivot tables with zero rows download correctly."
+ (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]]}}
+ :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}]]}}}]
+ (let [result (->> (mt/user-http-request :crowberto :post 200 (format "card/%d/query/csv?format_rows=false" pivot-card-id))
+ csv/read-csv)]
+ (is (= [["Category" "Doohickey" "Gadget" "Gizmo" "Widget" "Row totals"]
+ ["Grand Totals" "2185.89" "3019.2" "2834.88" "3109.31" "11149.28"]]
+ result)))))))
+
+(deftest ^:parallel zero-column-multiple-meausres-pivot-tables-test
+ (testing "Pivot tables with zero columns and multiple measures download correctly."
+ (mt/dataset test-data
+ (mt/with-temp [:model/Card {pivot-card-id :id}
+ {:display :pivot
+ :visualization_settings {:pivot_table.column_split
+ {:rows [[:field (mt/id :products :created_at) {:base-type :type/DateTime :temporal-unit :month}]
+ [:field (mt/id :products :category) {:base-type :type/Text}]]
+ :columns []
+ :values [[:aggregation 0] [:aggregation 1]]}}
+ :dataset_query {:database (mt/id)
+ :type :query
+ :query
+ {:source-table (mt/id :products)
+ :aggregation [[:sum [:field (mt/id :products :price) {:base-type :type/Float}]]
+ [: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}]]}}}]
+ (let [result (->> (mt/user-http-request :crowberto :post 200 (format "card/%d/query/csv?format_rows=false" pivot-card-id))
+ csv/read-csv)]
+ (is (= [["Created At" "Category" "Sum of Price" "Sum of Price"]
+ ["2016-01-01T00:00:00Z" "Doohickey" "632.14" "632.14"]
+ ["2016-01-01T00:00:00Z" "Gadget" "679.83" "679.83"]
+ ["2016-01-01T00:00:00Z" "Gizmo" "529.7" "529.7"]
+ ["2016-01-01T00:00:00Z" "Widget" "987.39" "987.39"]
+ ["Totals for 2016-01-01T00:00:00Z" "" "2829.06" "2829.06"]
+ ["2017-01-01T00:00:00Z" "Doohickey" "854.19" "854.19"]
+ ["2017-01-01T00:00:00Z" "Gadget" "1059.11" "1059.11"]
+ ["2017-01-01T00:00:00Z" "Gizmo" "1080.18" "1080.18"]
+ ["2017-01-01T00:00:00Z" "Widget" "1014.68" "1014.68"]
+ ["Totals for 2017-01-01T00:00:00Z" "" "4008.16" "4008.16"]
+ ["2018-01-01T00:00:00Z" "Doohickey" "496.43" "496.43"]
+ ["2018-01-01T00:00:00Z" "Gadget" "844.51" "844.51"]
+ ["2018-01-01T00:00:00Z" "Gizmo" "997.94" "997.94"]
+ ["2018-01-01T00:00:00Z" "Widget" "912.2" "912.2"]
+ ["Totals for 2018-01-01T00:00:00Z" "" "3251.08" "3251.08"]
+ ["2019-01-01T00:00:00Z" "Doohickey" "203.13" "203.13"]
+ ["2019-01-01T00:00:00Z" "Gadget" "435.75" "435.75"]
+ ["2019-01-01T00:00:00Z" "Gizmo" "227.06" "227.06"]
+ ["2019-01-01T00:00:00Z" "Widget" "195.04" "195.04"]
+ ["Totals for 2019-01-01T00:00:00Z" "" "1060.98" "1060.98"]
+ ["Grand Totals" "" "11149.28" "11149.28"]]
+ result)))))))
--
GitLab