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

Filter out Empty Rows from Pivot Exports (#49512) 

* Pivot Measures Order Used in Pivot Exports

Fixes #48442

A pivot table can have any number of measures, and the user can order these by dragging in the UI. Before this PR,
that order was ignored and measures would alway be in index order, which is confusing for any user who needs the
measures to be displayed in a particular order, especially if they've re-ordered them in the pivot viz settings UI.

A test has been added to check that measure order is used.

A few minor changes to the pivot qp and post-processor
 - measure indices are looked up in the pivot qp and added in viz-settings order
 - the pivot measures are only added if the qp has not already added them.
 - pivot-opts Malli spec has been made in the namespace, adjusted to allow `nil` as a valid pivot-opts output, and
 used in relevant functions

* address review points.

* add a rows order test

* Filter out Empty Rows from Pivot Exports

Fixes #49353

The linked issue is not actually related to pivot export size but is instead related to the 'Min of Created At: Month'
aggregation; the default aggregation function was `+`, so it broke when the date string was encountered.

That was fixed.

As I was trynig to keep the export small if possible, I noticed that in some cases empty rows are appended, so I added
the filter so that if a pivot row's values are completely empty, it doesn't add it.

Finally, this PR also adds the 'sub section totals' which I noticed were missing from the exports. This comes up when
you have 3+ pivot-rows configured, so you can see the subtotals for the first pivot row and the subtotals nested
within those sections for the second pivot row, and so on.

* add test for non-numeric values

* Make sure the new name refs match on aggregations not just breakouts

* cljfmt
parent 39534602
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/query_processor/pivot/postprocess.clj
+ 143
− 55
View file @ 63151f4e
......@@ -7,6 +7,7 @@
(:require
[clojure.math.combinatorics :as math.combo]
[clojure.set :as set]
[clojure.string :as str]
[metabase.query-processor.streaming.common :as common]
[metabase.util.malli :as mu]
[metabase.util.malli.registry :as mr]))
......@@ -103,6 +104,13 @@
[m k v]
(update m k conj v))
(defn- default-agg-fn
[agg v]
(when v
(if (number? v)
(+ agg v)
v)))
(defn- update-aggregate
"Update the given `measure-aggregations` with `new-values` using the appropriate function in the `agg-fns` map.
......@@ -116,7 +124,7 @@
(into {}
(map
(fn [[measure-key agg]]
(let [agg-fn (get agg-fns measure-key +) ; default aggregation is just summation
(let [agg-fn (get agg-fns measure-key default-agg-fn)
new-v (get new-values measure-key)]
[measure-key (if new-v
(agg-fn agg new-v)
......@@ -133,22 +141,27 @@
row-path (mapv row pivot-rows)
col-path (mapv row pivot-cols)
measure-vals (select-keys row pivot-measures)
total-fn (fn [m path]
total-fn* (fn [m path]
(if (seq path)
(update-in m path
#(update-aggregate (or % (zipmap pivot-measures (repeat 0))) measure-vals measures))
m))]
m))
total-fn (fn [m paths]
(reduce total-fn* m paths))]
(-> pivot
(update :row-count (fn [v] (if v (inc v) 0)))
(update :data update-in (concat row-path col-path)
#(update-aggregate (or % (zipmap pivot-measures (repeat 0))) measure-vals measures))
(update :totals (fn [totals]
(-> totals
(total-fn [:grand-total])
(total-fn row-path)
(total-fn col-path)
(total-fn [:section-totals (first row-path)])
(total-fn (concat [:column-totals (first row-path)] col-path)))))
(total-fn [[:grand-total]])
(total-fn [row-path])
(total-fn [col-path])
(total-fn [[:section-totals (first row-path)]])
#_(total-fn [(concat [:column-totals (first row-path)] col-path)])
(total-fn (map (fn [part]
(concat [:column-totals] part col-path))
(rest (reductions conj [] row-path)))))))
(update :row-values #(reduce-kv update-set % (select-keys row pivot-rows)))
(update :column-values #(reduce-kv update-set % (select-keys row pivot-cols))))))
......@@ -188,37 +201,43 @@
(defn- build-row
"Build a single row of the pivot table."
[row-combo col-combos pivot-measures data totals row-totals? ordered-formatters row-formatters]
(let [row-path row-combo]
(concat
(when-not (seq row-formatters) (repeat (count pivot-measures) nil))
(mapv fmt row-formatters row-combo)
(concat
(for [col-combo col-combos
measure-key pivot-measures]
(fmt (get ordered-formatters measure-key)
(get-in data (concat row-path col-combo [measure-key]))))
(when row-totals?
(for [measure-key pivot-measures]
(fmt (get ordered-formatters measure-key)
(get-in totals (concat row-path [measure-key])))))))))
(let [row-path row-combo
measure-values (for [col-combo col-combos
measure-key pivot-measures]
(fmt (get ordered-formatters measure-key)
(get-in data (concat row-path col-combo [measure-key]))))]
(when (some #(and (some? %) (not= "" %)) measure-values)
(concat
(when-not (seq row-formatters) (repeat (count pivot-measures) nil))
row-combo
#_(mapv fmt row-formatters row-combo)
(concat
measure-values
(when row-totals?
(for [measure-key pivot-measures]
(fmt (get ordered-formatters measure-key)
(get-in totals (concat row-path [measure-key]))))))))))
(defn- build-column-totals
"Build column totals for a section."
[section col-combos pivot-measures totals row-totals? ordered-formatters pivot-rows]
(concat
(cons (format "Totals for %s" (fmt (get ordered-formatters (first pivot-rows)) section))
(repeat (dec (count pivot-rows)) nil))
(for [col-combo col-combos
measure-key pivot-measures]
(fmt (get ordered-formatters measure-key)
(get-in totals (concat
[:column-totals section]
col-combo
[measure-key]))))
(when row-totals?
(for [measure-key pivot-measures]
(fmt (get ordered-formatters measure-key)
(get-in totals [:section-totals section measure-key]))))))
[section-path col-combos pivot-measures totals row-totals? ordered-formatters pivot-rows]
(let [totals-row (distinct (for [col-combo col-combos
measure-key pivot-measures]
(fmt (get ordered-formatters measure-key)
(get-in totals (concat
[:column-totals]
section-path
col-combo
[measure-key])))))]
(when (some #(and (some? %) (not= "" %)) totals-row)
(concat
(cons (format "Totals for %s" (fmt (get ordered-formatters (first pivot-rows)) (last section-path)))
(repeat (dec (count pivot-rows)) nil))
totals-row
(when row-totals?
(for [measure-key pivot-measures]
(fmt (get ordered-formatters measure-key)
(get-in totals (concat [:section-totals] section-path [measure-key])))))))))
(defn- build-grand-totals
"Build grand totals row."
......@@ -236,29 +255,98 @@
(fmt (get ordered-formatters measure-key)
(get-in totals [:grand-total measure-key])))))
(defn- append-totals-to-subsections
[pivot section col-combos ordered-formatters]
(let [{:keys [config
totals]} pivot
{:keys [pivot-rows
pivot-measures
row-totals?]} config]
(concat
(reduce
(fn [section pivot-row-idx]
(mapcat
(fn [[k rows]]
(let [partial-path (take pivot-row-idx (first rows))
subtotal-path (concat partial-path [k])
total-row (vec (build-column-totals
subtotal-path
col-combos
pivot-measures
totals
row-totals?
ordered-formatters pivot-rows))
;; inside a subsection, we know that the 'parent' subsection values will all be the same
;; so we can just grab it from the first row
next-subsection-value (nth (first rows) (dec pivot-row-idx))]
(vec (concat
rows
;; assoc the next subsection's value into the row so it stays grouped in the next reduction
[(if (<= (dec pivot-row-idx) 0)
total-row
(assoc total-row (dec pivot-row-idx) next-subsection-value))]))))
(group-by (fn [r] (nth r pivot-row-idx)) section)))
section
(reverse (range 1 (dec (count pivot-rows)))))
[(vec (build-column-totals
[(ffirst section)]
col-combos
pivot-measures
totals
false #_row-totals?
ordered-formatters pivot-rows))])))
(defn build-pivot-output
"Arrange and format the aggregated `pivot` data."
[pivot ordered-formatters]
(let [{:keys [config data totals row-values column-values]} pivot
{:keys [pivot-rows pivot-cols pivot-measures column-titles row-totals? col-totals?]} config
row-formatters (mapv #(get ordered-formatters %) pivot-rows)
col-formatters (mapv #(get ordered-formatters %) pivot-cols)
row-combos (apply math.combo/cartesian-product (map row-values pivot-rows))
col-combos (apply math.combo/cartesian-product (map column-values pivot-cols))
row-totals? (and row-totals? (boolean (seq pivot-cols)))
column-headers (build-column-headers config col-combos col-formatters)
headers (or (seq (build-headers column-headers config))
[(concat
(map #(get column-titles %) pivot-rows)
(map #(get column-titles %) pivot-measures))])]
(let [{:keys [config
data
totals
row-values
column-values]} pivot
{:keys [pivot-rows
pivot-cols
pivot-measures
column-titles
row-totals?
col-totals?]} config
row-formatters (mapv #(get ordered-formatters %) pivot-rows)
col-formatters (mapv #(get ordered-formatters %) pivot-cols)
row-combos (apply math.combo/cartesian-product (map row-values pivot-rows))
col-combos (apply math.combo/cartesian-product (map column-values pivot-cols))
row-totals? (and row-totals? (boolean (seq pivot-cols)))
column-headers (build-column-headers config col-combos col-formatters)
headers (or (seq (build-headers column-headers config))
[(concat
(map #(get column-titles %) pivot-rows)
(map #(get column-titles %) pivot-measures))])]
(concat
headers
(apply concat
(for [section-row-combos (sort-by ffirst (vals (group-by first row-combos)))]
(concat
(for [row-combo (sort-by first section-row-combos)]
(build-row row-combo col-combos pivot-measures data totals row-totals? ordered-formatters row-formatters))
(when (and col-totals? (> (count pivot-rows) 1))
[(build-column-totals (ffirst section-row-combos) col-combos pivot-measures totals row-totals? ordered-formatters pivot-rows)]))))
(filter seq
(apply concat
(let [sections-rows
(for [section-row-combos (sort-by ffirst (vals (group-by first row-combos)))]
(concat
(remove nil?
(for [row-combo (sort-by first section-row-combos)]
(build-row row-combo col-combos pivot-measures data totals row-totals? ordered-formatters row-formatters)))))]
(mapv
(fn [section-rows]
(->>
;; section rows are either enriched with column-totals rows or left as is
(if col-totals?
(append-totals-to-subsections pivot section-rows col-combos ordered-formatters)
section-rows)
;; then, we apply the row-formatters to the pivot-rows portion of each row,
;; filtering out any rows that begin with "Totals ..."
(mapv
(fn [row]
(let [[row-part vals-part] (split-at (count pivot-rows) row)]
(if (or
(not (seq row-part))
(str/starts-with? (first row-part) "Totals"))
row
(vec (concat (map fmt row-formatters row-part) vals-part))))))))
sections-rows))))
(when col-totals?
[(build-grand-totals config col-combos totals row-totals? ordered-formatters)]))))
test/metabase/api/downloads_exports_test.clj
+ 55
− 4
View file @ 63151f4e
......@@ -280,9 +280,13 @@
(testing "formatted"
(is (= [[["Category" "2016" "2017" "2018" "2019" "Row totals"]
["Doohickey" "$632.14" "$854.19" "$496.43" "$203.13" "$2,185.89"]
["Totals for Doohickey" "$632.14" "$854.19" "$496.43" "$203.13"]
["Gadget" "$679.83" "$1,059.11" "$844.51" "$435.75" "$3,019.20"]
["Totals for Gadget" "$679.83" "$1,059.11" "$844.51" "$435.75"]
["Gizmo" "$529.70" "$1,080.18" "$997.94" "$227.06" "$2,834.88"]
["Totals for Gizmo" "$529.70" "$1,080.18" "$997.94" "$227.06"]
["Widget" "$987.39" "$1,014.68" "$912.20" "$195.04" "$3,109.31"]
["Totals for Widget" "$987.39" "$1,014.68" "$912.20" "$195.04"]
["Grand totals" "$2,829.06" "$4,008.16" "$3,251.08" "$1,060.98" "$11,149.28"]]
#{:unsaved-card-download :card-download :dashcard-download
:alert-attachment :subscription-attachment
......@@ -299,9 +303,13 @@
"2019-01-01T00:00:00Z"
"Row totals"]
["Doohickey" "632.14" "854.19" "496.43" "203.13" "2185.89"]
["Totals for Doohickey" "632.14" "854.19" "496.43" "203.13"]
["Gadget" "679.83" "1059.11" "844.51" "435.75" "3019.20"]
["Totals for Gadget" "679.83" "1059.11" "844.51" "435.75"]
["Gizmo" "529.7" "1080.18" "997.94" "227.06" "2834.88"]
["Totals for Gizmo" "529.7" "1080.18" "997.94" "227.06"]
["Widget" "987.39" "1014.68" "912.2" "195.04" "3109.31"]
["Totals for Widget" "987.39" "1014.68" "912.2" "195.04"]
["Grand totals" "2829.06" "4008.16" "3251.08" "1060.98" "11149.28"]]
#{:unsaved-card-download :card-download :dashcard-download
:alert-attachment :subscription-attachment
......@@ -513,12 +521,15 @@
:pivot_results true)
csv/read-csv)]
(is (= [["Created At" "Category" "Sum of Price"]
["April, 2016" "Doohickey" ""]
["April, 2016" "Gadget" "49.54"]
["April, 2016" "Gizmo" "87.29"]
["April, 2016" "Widget" ""]
["Totals for April, 2016" "" "136.83"]]
(take 6 result))))))))
["Totals for April, 2016" "" "136.83"]
["May, 2016" "Doohickey" "144.12"]
["May, 2016" "Gadget" "81.58"]
["May, 2016" "Gizmo" "75.09"]
["May, 2016" "Widget" "90.21"]
["Totals for May, 2016" "" "391"]]
(take 9 result))))))))
(deftest ^:parallel zero-row-pivot-tables-test
(testing "Pivot tables with zero rows download correctly."
......@@ -1155,3 +1166,43 @@
:dashcard-download expected-header
:public-dashcard-download expected-header}
(update-vals formatted-results first)))))))))
(deftest pivot-non-numeric-values-in-aggregations
(testing "A pivot table with an aggegation that results in non-numeric values (eg. Dates) will still worl (#49353)."
(mt/dataset test-data
(mt/with-temp [:model/Card card {:display :pivot
:dataset_query {:database (mt/id)
:type :query
:query
{:source-table (mt/id :products)
:aggregation [[:count]
[:min
[:field (mt/id :products :created_at)
{:base-type :type/DateTime :temporal-unit :year}]]]
:breakout [[:field (mt/id :products :category) {:base-type :type/Text}]
[:field (mt/id :products :created_at)
{:base-type :type/DateTime :temporal-unit :year}]]}}
:visualization_settings {:pivot_table.column_split
{:rows [[:field (mt/id :products :created_at) {:base-type :type/DateTime :temporal-unit :year}]
[:field (mt/id :products :category) {:base-type :type/Text}]]
:columns []
:values [[:aggregation 0] [:aggregation 1]]}
:column_settings
{"[\"name\",\"count\"]" {:column_title "Count Renamed"}}}}]
(let [expected-header ["Created At" "Category" "Count" "Min of Created At: Year"]
formatted-results (all-downloads card {:export-format :csv :format-rows false :pivot true})]
(is (= {:unsaved-card-download expected-header
:card-download expected-header
:public-question-download expected-header
:dashcard-download expected-header
:public-dashcard-download expected-header}
(update-vals formatted-results first))))
(testing "The column title changes are used when format-rows is true"
(let [expected-header ["Created At" "Category" "Count Renamed" "Min of Created At: Year"]
formatted-results (all-downloads card {:export-format :csv :format-rows true :pivot true})]
(is (= {:unsaved-card-download expected-header
:card-download expected-header
:public-question-download expected-header
:dashcard-download expected-header
:public-dashcard-download expected-header}
(update-vals formatted-results first)))))))))
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