fitzee commented on issue #44725:
URL: https://github.com/apache/superset/issues/44725#issuecomment-5900379667
Adversarial pass on this ticket. I checked it against `master` @ `9a5faa8`
and ran `pivot_df` directly: rows=`country`, cols=`gender`, metrics=`SUM(num)`,
`MAX(num)`, both totals on.
**1. The premise depends on a PR that hasn't merged yet.** #44657 is still
open (review required). On current `master` the browser still keeps whichever
metric was pushed last: `cellValue.push()` overwrites, and `PivotTableChart`
unpivots `metricNames` in configured order. So for the GROUPING SETS path,
browser and export **agree** today. They'll only disagree once #44657 lands.
Step 2 of the repro ("live Explore view (blank, per #44657)") doesn't reproduce
on `master`. #44730 needs to merge **with or after** #44657. If it merges
first, the drift just flips: exports go blank while Explore still shows a
number.
**2. This isn't limited to percent modes.** `_apply_rollup_totals` runs in
Actual Values mode too (since #44631) and calls the same `_collapsed_metric`.
On `master`, with rollups, the Actual Values `Total (Sum)` column exports
`MAX(num)`'s values (3 / 10 / 10), and #44657 blanks that same cell in the
browser. The repro should drop the "percent mode" requirement. `metricsLayout:
Rows` hits it the same way.
**3. The most common case never reaches `_collapsed_metric`.** When every
metric is additive (SUM/COUNT/MIN/MAX), `buildQuery` sends no `grouping_sets`.
`rollup_levels` comes back empty and the export builds totals from the leaf
cells:
| mode (no rollups) | export `Total` column (UK / US / grand) | browser on
`master` |
|---|---|---|
| Actual Values | **14 / 26 / 40** (SUM + MAX added together) | 3 / 10 / 10
(last metric) |
| % of total / row / col, grand-total corner | **1.3** (130%); **1.6** with
metrics on rows | 100% |
In that path the export doesn't use the last metric at all. It sums across
metrics, and in percent modes the grand-total corner goes above 100%. The
browser and export disagree **today**, whatever happens with #44657. #44730
blanks the percent-mode cells here, but the Actual Values column still exports
14 / 26 / 40: `pivot_v2_aggfunc_map[aggfunc](block, axis=1)` sums across every
metric column, and nothing on the additive path overwrites it.
**4. Adjacent, probably its own ticket:** on the additive path in Actual
Values mode, a *single* `MAX(num)` metric exports sum-of-maxes subtotals and
totals (4 / 16 / 20), where the browser's `synthesizeAdditiveLevels` gives 3 /
10 / 10. Totals only follow `metric_rollup_reducers` in percent mode. Otherwise
they use `aggregateFunction`, which the chart ignores after SIP-216.
**On #44730:** its branch is 81 commits behind and predates #44631 (it still
has `if percent_mode and rollup_levels`), so its green CI ran against the old
code. The diff does apply cleanly to `master`. Rebased, it blanks the mixed
Total/corner in every mode on the rollup path, which is what #44657 does.
`min_count=1` also matches the browser, which returns `null` for an all-empty
group. Before merge it still needs: a rebase, a merge-order dependency on
#44657, and a fix (or a separate ticket) for the Actual Values sum on the
additive path from (3).
Suggested scope for this ticket: "exports must blank any total that
collapses 2+ metrics, in every `showValuesAs` mode and on both the GROUPING
SETS and additive paths", with a test for each combination.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
