rusackas opened a new pull request, #44657:
URL: https://github.com/apache/superset/pull/44657
### SUMMARY
With `Apply metrics on: Columns` and more than one metric on a pivot table,
the rightmost "Total" column and the bottom-right grand-total corner silently
show the **wrong number** — not an error, not blank, a plausible-looking number
that just happens to be one metric's total mislabeled as the combined "Total".
Repro: two metrics, e.g. `MAX(sales)` and `MEDIAN(msrp)`, row/column totals
enabled. The "Total" column shows exactly `MEDIAN(msrp)`'s own subtotal for
every row — `MAX(sales)`'s contribution is silently discarded. Swap the metric
order and the Total column would show `MAX(sales)`'s numbers instead.
**Root cause:** the pivot table's rollup values come from a "passthrough"
aggregator (`cellValue` in `react-pivottable/utilities.ts`) — since the
database already computed every rollup level, each cell stores its one
DB-computed record verbatim instead of re-aggregating. That's correct
everywhere *except* the Total axis/corner opposite the Metric pseudo-dimension:
when there's more than one metric, that slot receives **one record per metric**
(e.g. both `MAX(sales)` and `MEDIAN(msrp)`'s grand-total records), and `push()`
just overwrote its stored value on every call — "last write wins," with no
indication anything was dropped. `processRecord`'s own comment already flagged
this as a known, deliberate gap ("a cross-metric total is not well defined and
is left as future work") — this PR is that follow-up.
**Fix:** `push()` now tracks which metric each pushed record belongs to (via
the existing `__metricKey` tagging already used elsewhere in this file for the
same pseudo-dimension). When a second, different metric lands in the same slot,
the cell renders blank instead of picking one — matching the existing
convention already used for a genuine DB-computed `NULL`. There's no single
number that means anything for "max of sales combined with median of msrp," so
blank is the honest answer, not a fabricated one.
This also fixes the equivalent bug in "Show values as" percent modes:
`fractionOf` wraps this same aggregator, and an existing test locked in a
"self-consistent but still meaningless 100%" for the same mixed-metric cell
(both numerator and denominator secretly reading the same last-metric value,
canceling out to exactly 1.0). That test is updated to assert blank instead,
since `fractionOf` already treats a null numerator as blank.
### TESTING INSTRUCTIONS
```
npx jest plugins/plugin-chart-pivot-table --runInBand
```
New coverage: two unit tests in `react-pivottable/utilities.test.ts`
exercising `PivotData` directly (grand total blanks when two metrics mix, still
passes through correctly for a single metric), and the existing
`tableRenders.test.tsx` percent-mode regression test updated to assert the new
(correct) blank behavior instead of the old self-consistent-but-meaningless
100%.
Manually: build a pivot table with 2+ metrics, `Apply metrics on: Columns`,
row/column totals on. The Total column/corner should render blank instead of
one metric's numbers.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
