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

Pivot Measures Order Used in Pivot Exports (#49367) (#49503) 


* 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

Co-authored-by: default avataradam-james <21064735+adam-james-v@users.noreply.github.com>
parent 84dceb4e
Branches
Tags
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 154 additions and 69 deletions
src/metabase/query_processor/pivot.clj
+ 50
− 47
View file @ 8bdb22c2
......@@ -45,8 +45,15 @@
(mr/def ::num-breakouts ::lib.schema.common/int-greater-than-or-equal-to-zero)
(mr/def ::index ::lib.schema.common/int-greater-than-or-equal-to-zero)
(mr/def ::pivot-rows [:sequential ::index])
(mr/def ::pivot-cols [:sequential ::index])
(mr/def ::pivot-rows [:sequential ::index])
(mr/def ::pivot-cols [:sequential ::index])
(mr/def ::pivot-measures [:sequential ::index])
(mr/def ::pivot-opts [:maybe
[:map
[:pivot-rows {:optional true} [:maybe ::pivot-rows]]
[:pivot-cols {:optional true} [:maybe ::pivot-cols]]
[:pivot-measures {:optional true} [:maybe ::pivot-measures]]]])
(mu/defn- group-bitmask :- ::bitmask
"Come up with a display name given a combination of breakout `indexes` e.g.
......@@ -173,9 +180,7 @@
(mu/defn- generate-queries :- [:sequential ::lib.schema/query]
"Generate the additional queries to perform a generic pivot table"
[query :- ::lib.schema/query
{:keys [pivot-rows pivot-cols], :as _pivot-options} :- [:map
[:pivot-rows {:optional true} [:maybe ::pivot-rows]]
[:pivot-cols {:optional true} [:maybe ::pivot-cols]]]]
{:keys [pivot-rows pivot-cols], :as _pivot-options} :- ::pivot-opts]
(try
(let [all-breakouts (lib/breakouts query)
all-queries (for [breakout-indexes (u/prog1 (breakout-combinations (count all-breakouts) pivot-rows pivot-cols)
......@@ -342,48 +347,45 @@
qp.pipeline/*reduce* (or reduce qp.pipeline/*reduce*)]
(qp/process-query first-query rff))))
(mu/defn- pivot-options :- [:map
[:pivot-rows [:maybe [:sequential [:int {:min 0}]]]]
[:pivot-cols [:maybe [:sequential [:int {:min 0}]]]]]
(mu/defn- pivot-options :- ::pivot-opts
"Given a pivot table query and a card ID, looks at the `pivot_table.column_split` key in the card's visualization
settings and generates pivot-rows and pivot-cols to use for generating subqueries."
[query :- [:map
[:database ::lib.schema.id/database]]
viz-settings :- [:maybe :map]]
(let [column-split (:pivot_table.column_split viz-settings)
column-split-rows (seq (:rows column-split))
column-split-columns (seq (:columns column-split))
index-in-breakouts (when (or column-split-rows
column-split-columns)
(let [metadata-provider (or (:lib/metadata query)
(lib.metadata.jvm/application-database-metadata-provider (:database query)))
mlv2-query (lib/query metadata-provider query)
breakouts (into []
(map-indexed (fn [i col]
(cond-> col
true (assoc ::i i)
;; if the col has a card-id, we swap the :lib/source to say source/card
;; this allows `lib/find-matching-column` to properly match a column that has a join-alias
;; but whose source is a model
(contains? col :lib/card-id) (assoc :lib/source :source/card))))
(lib/breakouts-metadata mlv2-query))]
(fn [legacy-ref]
(try
(::i (lib.equality/find-column-for-legacy-ref
mlv2-query
-1
legacy-ref
breakouts))
(catch Throwable e
(log/errorf e "Error finding matching column for ref %s" (pr-str legacy-ref))
nil)))))
pivot-rows (when column-split-rows
(into [] (keep index-in-breakouts) column-split-rows))
pivot-cols (when column-split-columns
(into [] (keep index-in-breakouts) column-split-columns))]
{:pivot-rows pivot-rows
:pivot-cols pivot-cols}))
(let [{:keys [rows columns values]} (:pivot_table.column_split viz-settings)
metadata-provider (or (:lib/metadata query)
(lib.metadata.jvm/application-database-metadata-provider (:database query)))
mlv2-query (lib/query metadata-provider query)
breakouts (into []
(map-indexed (fn [i col]
(cond-> col
true (assoc ::idx i)
;; if the col has a card-id, we swap the :lib/source to say
;; source/card this allows `lib/find-matching-column` to properly
;; match a column that has a join-alias but whose source is a
;; model
(contains? col :lib/card-id) (assoc :lib/source :source/card))))
(concat (lib/breakouts-metadata mlv2-query)
(lib/aggregations-metadata mlv2-query)))
index-in-breakouts (fn index-in-breakouts [legacy-ref]
(try
(::idx (lib.equality/find-column-for-legacy-ref
mlv2-query
-1
legacy-ref
breakouts))
(catch Throwable e
(log/errorf e "Error finding matching column for ref %s" (pr-str legacy-ref))
nil)))
process-refs (fn process-refs [refs]
(when (seq refs)
(into [] (keep index-in-breakouts) refs)))
pivot-opts {:pivot-rows (process-refs rows)
:pivot-cols (process-refs columns)
:pivot-measures (process-refs values)}]
(when (some some? (vals pivot-opts))
pivot-opts)))
(mu/defn- column-mapping-for-subquery :- ::pivot-column-mapping
[num-canonical-cols :- ::lib.schema.common/int-greater-than-or-equal-to-zero
......@@ -529,10 +531,11 @@
(qp.setup/with-qp-setup [query query]
(let [rff (or rff qp.reducible/default-rff)
query (lib/query (qp.store/metadata-provider) query)
pivot-options (or
(not-empty (select-keys query [:pivot-rows :pivot-cols]))
(pivot-options query (get-in query [:info :visualization-settings])))
query (assoc-in query [:middleware :pivot-options] pivot-options)
all-queries (generate-queries query pivot-options)
pivot-opts (or
(pivot-options query (get query :viz-settings))
(pivot-options query (get-in query [:info :visualization-settings]))
(not-empty (select-keys query [:pivot-rows :pivot-cols :pivot-measures])))
query (assoc-in query [:middleware :pivot-options] pivot-opts)
all-queries (generate-queries query pivot-opts)
column-mapping-fn (make-column-mapping-fn query)]
(process-multiple-queries all-queries rff column-mapping-fn))))))
src/metabase/query_processor/pivot/postprocess.clj
+ 13
− 4
View file @ 8bdb22c2
......@@ -62,10 +62,19 @@
(mu/defn add-pivot-measures :- ::pivot-spec
"Given a pivot-spec map without the `:pivot-measures` key, determine what key(s) the measures will be and assoc that value into `:pivot-measures`."
[pivot-spec :- ::pivot-spec]
(-> pivot-spec
(assoc :pivot-measures (pivot-measures pivot-spec))
(assoc :pivot-grouping (pivot-grouping-key (:column-titles pivot-spec)))))
[{measure-indices :pivot-measures :as pivot-spec} :- ::pivot-spec]
(let [pivot-grouping-key (pivot-grouping-key (:column-titles pivot-spec))]
(cond-> pivot-spec
;; if pivot-measures don't already exist (from the pivot qp), we add them ourselves, assuming lowest ID -> highest ID sort order
(not (seq measure-indices)) (assoc :pivot-measures (pivot-measures pivot-spec))
;; otherwise, we modify indices to skip over whatever the pivot-grouping idx is, so we pull the correct values per row
(seq measure-indices) (update :pivot-measures (fn [indices]
(mapv (fn [idx]
(if (>= idx pivot-grouping-key)
(inc idx)
idx))
indices)))
true (assoc :pivot-grouping pivot-grouping-key))))
(mu/defn add-totals-settings :- ::pivot-spec
"Given a pivot-spec map and `viz-settings`, add the `:row-totals?` and `:col-totals?` keys."
......
src/metabase/query_processor/streaming/csv.clj
+ 4
− 5
View file @ 8bdb22c2
......@@ -78,15 +78,14 @@
(begin! [_ {{:keys [ordered-cols results_timezone format-rows? pivot-export-options pivot?]
:or {format-rows? true
pivot? false}} :data} viz-settings]
(let [opts (when (and pivot? pivot-export-options)
(let [col-names (vec (common/column-titles ordered-cols (::mb.viz/column-settings viz-settings) format-rows?))
opts (when (and pivot? pivot-export-options)
(-> (merge {:pivot-rows []
:pivot-cols []}
pivot-export-options)
(assoc :column-titles (mapv :display_name ordered-cols))
(assoc :column-titles col-names)
(qp.pivot.postprocess/add-totals-settings viz-settings)
qp.pivot.postprocess/add-pivot-measures))
;; col-names are created later when exporting a pivot table, so only create them if there are no pivot options
col-names (when-not opts (common/column-titles ordered-cols (::mb.viz/column-settings viz-settings) format-rows?))
pivot-grouping-key (qp.pivot.postprocess/pivot-grouping-key col-names)]
;; initialize the pivot-data
......@@ -101,7 +100,7 @@
(mapv #(formatter/create-formatter results_timezone % viz-settings format-rows?) ordered-cols))
;; write the column names for non-pivot tables
(when col-names
(when (not opts)
(let [header (m/remove-nth (or pivot-grouping-key (inc (count col-names))) col-names)]
(write-csv writer [header])
(.flush writer)))))
......
src/metabase/query_processor/streaming/json.clj
+ 0
− 2
View file @ 8bdb22c2
......@@ -41,8 +41,6 @@
(reify qp.si/StreamingResultsWriter
(begin! [_ {{:keys [ordered-cols results_timezone format-rows?]
:or {format-rows? true}} :data} viz-settings]
;; TODO -- wouldn't it make more sense if the JSON downloads used `:name` preferentially? Seeing how JSON is
;; probably going to be parsed programmatically
(let [cols (common/column-titles ordered-cols (::mb.viz/column-settings viz-settings) format-rows?)
pivot-grouping (qp.pivot.postprocess/pivot-grouping-key cols)]
(when pivot-grouping (vreset! pivot-grouping-idx pivot-grouping))
......
test/metabase/api/downloads_exports_test.clj
+ 84
− 8
View file @ 8bdb22c2
......@@ -1061,32 +1061,108 @@
:type :native
:native {:query "SELECT 1234.567 as A"}}}]
(testing "CSV downloads respect the formatted/unformatted setting"
(let [formatted-json-results (all-downloads card {:export-format :csv :format-rows true})
unformatted-json-results (all-downloads card {:export-format :csv :format-rows false})]
(let [formatted-results (all-downloads card {:export-format :csv :format-rows true})
unformatted-results (all-downloads card {:export-format :csv :format-rows false})]
(is (= {:unsaved-card-download [["A"] ["1,234.57"]]
:card-download [["A"] ["1,234.57"]]
:public-question-download [["A"] ["1,234.57"]]
:dashcard-download [["A"] ["1,234.57"]]
:public-dashcard-download [["A"] ["1,234.57"]]}
formatted-json-results))
formatted-results))
(is (= {:unsaved-card-download [["A"] ["1234.567"]]
:card-download [["A"] ["1234.567"]]
:public-question-download [["A"] ["1234.567"]]
:dashcard-download [["A"] ["1234.567"]]
:public-dashcard-download [["A"] ["1234.567"]]}
unformatted-json-results))))
unformatted-results))))
(testing "JSON downloads respect the formatted/unformatted setting"
(let [formatted-json-results (all-downloads card {:export-format :json :format-rows true})
unformatted-json-results (all-downloads card {:export-format :json :format-rows false})]
(let [formatted-results (all-downloads card {:export-format :json :format-rows true})
unformatted-results (all-downloads card {:export-format :json :format-rows false})]
(is (= {:unsaved-card-download [["A"] ["1,234.57"]]
:card-download [["A"] ["1,234.57"]]
:public-question-download [["A"] ["1,234.57"]]
:dashcard-download [["A"] ["1,234.57"]]
:public-dashcard-download [["A"] ["1,234.57"]]}
formatted-json-results))
formatted-results))
(is (= {:unsaved-card-download [["A"] [1234.567]]
:card-download [["A"] [1234.567]]
:public-question-download [["A"] [1234.567]]
:dashcard-download [["A"] [1234.567]]
:public-dashcard-download [["A"] [1234.567]]}
unformatted-json-results))))))))
unformatted-results))))))))
(deftest pivot-measures-order-test
(testing "A pivot download will use the user-configured measures order (#48442)."
(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]
[:sum [:field (mt/id :products :price) {:base-type :type/Float}]]
[:avg [:field (mt/id :products :rating) {:base-type :type/Float}]]]
:breakout [[:field (mt/id :products :category) {:base-type :type/Text}]]}}
:visualization_settings {:pivot_table.column_split
{:rows [[:field (mt/id :products :category) {:base-type :type/Text}]]
:columns []
:values [[:aggregation 1]
[:aggregation 0]
[:aggregation 2]]}
:column_settings
{"[\"name\",\"count\"]" {:column_title "Count Renamed"}
"[\"name\",\"sum\"]" {:column_title "Sum Renamed"}
"[\"name\",\"avg\"]" {:column_title "Average Renamed"}}}}]
(let [expected-header ["Category" "Sum of Price" "Count" "Average of Rating"]
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 ["Category" "Sum Renamed" "Count Renamed" "Average Renamed"]
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)))))))))
(deftest pivot-rows-order-test
(testing "A pivot download will use the user-configured rows order."
(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]]
: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]]}
:column_settings
{"[\"name\",\"count\"]" {:column_title "Count Renamed"}}}}]
(let [expected-header ["Created At" "Category" "Count"]
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"]
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)))))))))
test/metabase/query_processor/pivot_test.clj
+ 3
− 3
View file @ 8bdb22c2
......@@ -191,7 +191,7 @@
(testing "`pivot-options` correctly generates pivot-rows and pivot-cols from a card's viz settings"
(let [query (api.pivots/pivot-query false)
viz-settings (:visualization_settings (api.pivots/pivot-card))
pivot-options {:pivot-rows [1 0], :pivot-cols [2]}]
pivot-options {:pivot-rows [1 0], :pivot-cols [2] :pivot-measures nil}]
(is (= pivot-options
(#'qp.pivot/pivot-options query viz-settings)))
(are [num-breakouts expected] (= expected
......@@ -217,7 +217,7 @@
:columns [[:field "RATING" {:base-type :type/Integer}]]}})
[:pivot_table.column_split])
pivot-options (#'qp.pivot/pivot-options query viz-settings)]
(is (= {:pivot-rows [], :pivot-cols []}
(is (= {:pivot-rows [], :pivot-cols [] :pivot-measures nil}
pivot-options))
(is (= [[0 1] [1] [0] []]
(#'qp.pivot/breakout-combinations 2 (:pivot-rows pivot-options) (:pivot-cols pivot-options)))))))
......@@ -245,7 +245,7 @@
{:rows [$category]
:columns [$created_at]}})
pivot-options (#'qp.pivot/pivot-options query viz-settings)]
(is (= {:pivot-rows [0], :pivot-cols [1]}
(is (= {:pivot-rows [0], :pivot-cols [1] :pivot-measures nil}
pivot-options))
(is (= [[0 1] [1] [0] []]
(#'qp.pivot/breakout-combinations 2 (:pivot-rows pivot-options) (:pivot-cols pivot-options))))
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment