Skip to content
Snippets Groups Projects
Unverified Commit 37abc084 authored by adam-james's avatar adam-james Committed by GitHub
Browse files

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
parent 66224704
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/api/embed/common.clj
+ 0
− 21
View file @ 37abc084
......@@ -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
......
src/metabase/query_processor/pivot/postprocess.clj
+ 41
− 26
View file @ 37abc084
......@@ -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)))))
......
src/metabase/query_processor/streaming/csv.clj
+ 2
− 1
View file @ 37abc084
......@@ -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)
......
test/metabase/api/downloads_exports_test.clj
+ 93
− 1
View file @ 37abc084
......@@ -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)))))))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment