Skip to content
Snippets Groups Projects
Unverified Commit e8c63c32 authored by github-automation-metabase's avatar github-automation-metabase Committed by GitHub
Browse files

Pivot Exports Can Handle `nil` in Breakout Col, and Multiple Cols of the Same... 

Pivot Exports Can Handle `nil` in Breakout Col, and Multiple Cols of the Same Aggregation Type (#50672) (#50780)

* Pivot Exports Can Handle `nil` in Breakout Col, and Multiple Cols of the Same Aggregation Type

Fixes #50551

If a pivot table has several measures configured, they might both use the same kind of
aggregation (eg. 'sum'). Previously, this would lead to columns with the same name preventing the pivot-measures from
properly making it through the export post processing. Now, the correct aliased/deduped name is used and all columns
can be properly included in the pivot export.

Also fixes the case where a breakout column (pivot-row) contains `nil` values, which caused the pivot export to
fail. Now, all nil values are grouped and handled appropriately.

* when pivot is disabled, header should still be included

* min and max don't default to 0, add a test for the aggregations

Co-authored-by: default avataradam-james <21064735+adam-james-v@users.noreply.github.com>
parent 1d19852b
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.clj
+ 5
− 1
View file @ e8c63c32
......@@ -361,12 +361,16 @@
query (lib/query metadata-provider query)
returned-columns (lib/returned-columns query)
{:source/keys [aggregations breakouts]} (group-by :lib/source returned-columns)
column-alias->index (into {}
(map-indexed (fn [i column] [(:lib/desired-column-alias column) i]))
(concat breakouts aggregations))
column-name->index (into {}
(map-indexed (fn [i column] [(:name column) i]))
(concat breakouts aggregations))
process-columns (fn process-columns [column-names]
(when (seq column-names)
(into [] (keep column-name->index) column-names)))
(into [] (keep (fn [n] (or (column-alias->index n)
(column-name->index n)))) column-names)))
pivot-opts {:pivot-rows (process-columns rows)
:pivot-cols (process-columns columns)
:pivot-measures (process-columns values)}]
......
src/metabase/query_processor/pivot/postprocess.clj
+ 11
− 6
View file @ e8c63c32
......@@ -129,15 +129,19 @@
:min
(fn [prev v]
(if (number? v)
(-> (merge {:min 0} prev)
(update :min #(min % v)))
(update prev :min
(fn [x] (if x
(min x v)
v)))
v))
:max
(fn [prev v]
(if (number? v)
(-> (merge {:min 0} prev)
(update :min #(max % v)))
(update prev :max
(fn [x] (if x
(max x v)
v)))
v))
;; else
......@@ -442,10 +446,11 @@
;; filtering out any rows that begin with "Totals ..."
(mapv
(fn [row]
(let [[row-part vals-part] (split-at (count pivot-rows) row)]
(let [[row-part vals-part] (split-at (count pivot-rows) row)
first-entry (first row-part)]
(if (or
(not (seq row-part))
(str/starts-with? (first row-part) "Totals"))
(and (string? first-entry) (str/starts-with? first-entry "Totals")))
row
(into (mapv fmt row-formatters row-part) vals-part)))))))
sections-rows))))
......
src/metabase/query_processor/streaming/csv.clj
+ 1
− 1
View file @ e8c63c32
......@@ -116,7 +116,7 @@
(mapv #(formatter/create-formatter results_timezone % viz-settings format-rows?) ordered-cols))
;; write the column names for non-pivot tables
(when (not opts)
(when (or (not opts) (not (public-settings/enable-pivoted-exports)))
(let [header (m/remove-nth (or pivot-grouping-key (inc (count col-names))) col-names)]
(write-csv writer [header])
(.flush writer)))))
......
test/metabase/api/downloads_exports_test.clj
+ 189
− 1
View file @ e8c63c32
......@@ -313,7 +313,8 @@
((fn [m] (update-vals m #(into #{} (mapv first %)))))
(apply concat)))))
(testing "only when `public-settings/enable-pivoted-exports` is true (true by default)."
(is (= [[["Doohickey" "2016" "$632.14"]
(is (= [[["Category" "Created At: Year" "Sum of Price"]
["Doohickey" "2016" "$632.14"]
["Doohickey" "2017" "$854.19"]
["Doohickey" "2018" "$496.43"]
["Doohickey" "2019" "$203.13"]
......@@ -1433,3 +1434,190 @@
(if expected
(is (= [nil nil] [unique-to-a unique-to-b]))
(is (or (some? unique-to-a) (some? unique-to-b))))))))))))
(deftest ^:parallel pivot-exports-handle-nil-in-breakout-column
(testing "Pivot Exports will still work if the breakout column contains `nil` values."
(let [q "SELECT A,
CASE
WHEN A = 2 THEN NULL
ELSE A
END AS MEASURE
FROM ( SELECT 1 AS A UNION ALL SELECT 2 UNION ALL SELECT 3 )"]
(mt/dataset test-data
(mt/with-temp [:model/Card {pivot-data-card-id :id}
{:dataset_query {:database (mt/id)
:type :native
:native
{:template-tags {}
:query q}}
:result_metadata
(into [] (for [[_ field-name {:keys [base-type]}] pivot-fields]
{:name field-name
:display_name field-name
:field_ref [:field field-name {:base-type base-type}]
:base_type base-type}))}
:model/Card pivot-card
{:display :pivot
:visualization_settings {:pivot_table.column_split
{:rows ["MEASURE"]
:columns []
:values ["count" "sum"]}}
:dataset_query {:database (mt/id)
:type :query
:query
{:breakout [[:field "MEASURE" {:base-type :type/Integer}]],
:aggregation
[[:count]
[:sum [:field "A" {:base-type :type/Integer}]]]
:source-table (format "card__%s" pivot-data-card-id)}}}]
(let [result (card-download pivot-card {:export-format :csv :pivot true})]
(is (= [["MEASURE" "Count" "Sum of A"]
["" "1" "2"]
["1" "1" "1"]
["3" "1" "3"]
["Grand totals" "3" "6"]]
result))))))))
(deftest ^:parallel pivot-exports-handle-aggregations-with-the-same-base-name
(testing "Pivot Exports with multiple of the same kind of aggregation will include all of the data."
(let [q "SELECT A, B, MEASURE
FROM (
SELECT 1 as A, 1 as B, 1 as MEASURE UNION ALL
SELECT 2, 2, 2 UNION ALL
SELECT 3, 3, 3 UNION ALL
SELECT 4, 4, 4 UNION ALL
SELECT 5, 5, 5
)"]
(mt/dataset test-data
(mt/with-temp [:model/Card {pivot-data-card-id :id}
{:dataset_query {:database (mt/id)
:type :native
:native
{:template-tags {}
:query q}}
:result_metadata
(into [] (for [[_ field-name {:keys [base-type]}] pivot-fields]
{:name field-name
:display_name field-name
:field_ref [:field field-name {:base-type base-type}]
:base_type base-type}))}
:model/Card pivot-card
{:display :pivot
:visualization_settings {:pivot_table.column_split
{:rows ["MEASURE"]
:columns []
:values ["count" "sum" "sum_2"]}}
:dataset_query {:database (mt/id)
:type :query
:query
{:breakout [[:field "MEASURE" {:base-type :type/Integer}]],
:aggregation
[[:count]
[:sum [:field "A" {:base-type :type/Integer}]]
[:sum [:field "B" {:base-type :type/Integer}]]]
:source-table (format "card__%s" pivot-data-card-id)}}}
:model/Card reordered-card
{:display :pivot
:visualization_settings {:pivot_table.column_split
{:rows ["MEASURE"]
:columns []
:values ["sum_2" "count" "sum"]}}
:dataset_query {:database (mt/id)
:type :query
:query
{:breakout [[:field "MEASURE" {:base-type :type/Integer}]],
:aggregation
[[:count]
[:sum [:field "A" {:base-type :type/Integer}]]
[:sum [:field "B" {:base-type :type/Integer}]]]
:source-table (format "card__%s" pivot-data-card-id)}}}]
(let [result (card-download pivot-card {:export-format :csv :pivot true})
reordered-result (card-download reordered-card {:export-format :csv :pivot true})]
(testing "Both Sums are correctly included."
(is (= [["MEASURE" "Count" "Sum of A" "Sum of B"]
["1" "1" "1" "1"]
["2" "1" "2" "2"]
["3" "1" "3" "3"]
["4" "1" "4" "4"]
["5" "1" "5" "5"]
["Grand totals" "5" "15" "15"]]
result)))
(testing "and different order still works."
(is (= [["MEASURE" "Sum of B" "Count" "Sum of A"]
["1" "1" "1" "1"]
["2" "2" "1" "2"]
["3" "3" "1" "3"]
["4" "4" "1" "4"]
["5" "5" "1" "5"]
["Grand totals" "15" "5" "15"]]
reordered-result)))))))))
(deftest ^:parallel pivot-exports-aggregations-work
(testing "Pivot Exports have correct aggregations."
(let [q "SELECT A, B
FROM (
SELECT 1 as A, 1 as B UNION ALL
SELECT 1, 2 UNION ALL
SELECT 1, 3 UNION ALL
SELECT 1, 4 UNION ALL
SELECT 1, 5 UNION ALL
SELECT 2, 10 UNION ALL
SELECT 2, 20 UNION ALL
SELECT 2, 30 UNION ALL
SELECT 2, 40 UNION ALL
SELECT 2, 50 UNION ALL
SELECT 3, -1 UNION ALL
SELECT 3, -2 UNION ALL
SELECT 3, -3 UNION ALL
SELECT 3, -4 UNION ALL
SELECT 3, -5 UNION ALL
SELECT 4, 15 UNION ALL
SELECT 4, 25 UNION ALL
SELECT 4, 35 UNION ALL
SELECT 4, 45 UNION ALL
SELECT 4, 55 UNION ALL
SELECT 5, 11 UNION ALL
SELECT 5, 22 UNION ALL
SELECT 5, 33 UNION ALL
SELECT 5, 44 UNION ALL
SELECT 5, 55
)"]
(mt/dataset test-data
(mt/with-temp [:model/Card {pivot-data-card-id :id}
{:dataset_query {:database (mt/id)
:type :native
:native
{:template-tags {}
:query q}}
:result_metadata
(into [] (for [[_ field-name {:keys [base-type]}] pivot-fields]
{:name field-name
:display_name field-name
:field_ref [:field field-name {:base-type base-type}]
:base_type base-type}))}
:model/Card pivot-card
{:display :pivot
:visualization_settings {:pivot_table.column_split
{:rows ["A"]
:columns []
:values ["count" "sum" "avg" "min" "max"]}}
:dataset_query {:database (mt/id)
:type :query
:query
{:breakout [[:field "A" {:base-type :type/Integer}]],
:aggregation
[[:count]
[:sum [:field "B" {:base-type :type/Integer}]]
[:avg [:field "B" {:base-type :type/Integer}]]
[:min [:field "B" {:base-type :type/Integer}]]
[:max [:field "B" {:base-type :type/Integer}]]]
:source-table (format "card__%s" pivot-data-card-id)}}}]
(let [result (card-download pivot-card {:export-format :csv :pivot true})]
(is (= [["A" "Count" "Sum of B" "Average of B" "Min of B" "Max of B"]
["1" "5" "15" "3.0" "1" "5"]
["2" "5" "150" "30.0" "10" "50"]
["3" "5" "-15" "-3.0" "-5" "-1"]
["4" "5" "175" "35.0" "15" "55"]
["5" "5" "165" "33.0" "11" "55"]
["Grand totals" "25" "490" "19.6" "-5" "55"]]
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