abhinav-phi opened a new pull request, #44815:
URL: https://github.com/apache/superset/pull/44815
Fixes #44724.
## Root cause
`PivotData` builds one aggregator per cell slot, and every slot resolves its
number
formatter through `getFormattedAggregator()`, which looks up the per-metric
formatter
in `props.customFormatters` (built by `PivotTableChart` from each metric's
own value
format / currency configuration).
The grand-total slot was the one exception. It was created eagerly in the
constructor:
```ts
this.allTotal = this.aggregator(this, [], []);
```
`this.aggregator` is built from `props.defaultFormatter`, and because the
slot
already existed before any record was seen, `processRecord` had nothing to
resolve a
formatter from when it later filled the slot. So the grand total was pinned
to the
chart-level default formatter regardless of how the metric was configured: a
currency metric rendered its body cells as `$100.00` / `$200.00` and its
column total
as `$300.00`, but the grand-total corner as a bare `300.00`.
## The fix
`allTotal` is now created lazily by a new `pushAllTotal()` on the first
record that
lands in it — the same registration pattern `rowTotals` and `colTotals`
already use
in `processRecord`:
```ts
pushAllTotal(record: PivotRecord) {
if (!this.allTotal) {
this.allTotal = this.getFormattedAggregator(record)(this, [], []);
}
this.allTotal.push(record);
}
```
`allTotal` is now typed `Aggregator | null`. `getAggregator([], [])` already
falls
back to a blank aggregator when a slot is unset, so an empty pivot renders
as before.
## Before / after
Single `sales` metric with a currency formatter, against a 2-decimal
`defaultFormatter` — the visible table text, captured by rendering the
fixture:
```diff
Metric sales Total
color
blue $100.00 100.00
red $200.00 200.00
- Total $300.00 300.00
+ Total $300.00 $300.00
```
Only the grand-total corner changes. Body cells and the column total already
used
the metric's formatter and are asserted too, so a regression anywhere in the
chain is
caught.
## Tests
`utilities.test.ts` (unit, on `PivotData`) and `tableRenders.test.tsx`
(rendered cell
text) both gained cases using two metrics with different formatters —
currency vs.
a custom 3-decimal-with-suffix format — so a cell that borrows the other
metric's
formatter, or the default, fails loudly. All four fail on the unpatched tree
with
exactly the reported symptom:
```
● grand total uses the metric formatter, not the default formatter
Expected: "$300.00"
Received: "300.00"
```
`npx jest plugins/plugin-chart-pivot-table` → **9 suites, 118 tests, all
passing**
(up from 112; 6 new). `oxfmt --list-different`, `oxlint` and `tsc --noEmit`
are clean
on the changed files.
## Scope
Two things I deliberately did **not** change, both because they are the
mixed-metric
question tracked by #44725 rather than a formatting bug:
- **Cross-axis row totals.** `getFormattedAggregator` falls back to the
default
formatter when the metric is not part of the key being totalled, so a row
total that
spans the whole metric axis keeps the default format. Loosening that guard
would
make a total that mixes `MAX(sales)` with `MEDIAN(msrp)` display a
confident-looking
format for whichever metric happened to be found first. The test documents
this
boundary explicitly.
- **Which metric a multi-metric grand total belongs to.** With one metric on
the
pseudo-dimension the grand total is unambiguous and this patch fixes it;
with
several, "last metric wins" is a separate decision.
## Note on the code path named in the issue
The issue points at the `aggregators` dict / `resultFactory` in
`utilities.ts` and at
`processResultRecord`. Those live on the branch behind #44660, not on
`master`:
`processResultRecord` does not appear anywhere in the default branch
(`gh api "search/code?q=processResultRecord+repo:apache/superset"` →
`total_count: 0`),
and #44660 is merged into `fix/pivot-grand-total-mixed-metrics`, not
`master`.
Per @rusackas's review of #44660 ("fixing it means threading a formatter
through every
template rather than a small patch. Opened apache/superset#44724 to track it
separately"), that follow-up is knowingly deferred to the result-aggregation
feature.
This PR therefore fixes the equivalent defect that is reproducible on
`master` today —
the aggregation cell that bypasses per-metric formatting — rather than
duplicating a
refactor of code that is not on this branch. If the intent is to also re-do
the
`aggregators` templates on the result-aggregation branch, that should land
there, on
top of #44660.
AI tools were used assistively; I reviewed and tested every change and take
responsibility for it.
--
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]
