fix(pivot-table): respect per-metric formatters in result aggregation

[abhinav-phi]

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:

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 built from the records that reach it, by a new pushAllTotal() — the same pattern rowTotals and colTotals already use in processRecord:

pushAllTotal(record: PivotRecord): void {
  const formatter = this.getFormattedAggregator(record);
  if (!this.allTotal || formatter !== this.allTotalFormatter) {
    this.allTotalFormatter = formatter;
    this.allTotal = formatter(this, [], []);
  }
  this.allTotal.push(record);
}

getFormattedAggregator returns a stable function per metric, so the identity check is enough to detect a switch, and records that resolve to the same formatter still accumulate into the same slot. allTotal is typed Aggregator | null; getAggregator([], []) already falls back to a blank aggregator when a slot is unset, so an empty pivot renders as before.

The rebuild-on-change is not cosmetic. cellValue.push() overwrites the stored value, so a slot built once from the first metric would go on to render the last metric's value in the first metric's format — e.g. a rate of 5 shown as $5.00. That is the same class of bug this issue is about, just harder to spot, so it is now covered by a test for both record orders.

Before / after

Single sales metric with a currency formatter, against a 2-decimal defaultFormatter — the visible table text, captured by rendering the fixture:

                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 of them 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, 120 tests, all passing (baseline 112; 8 new). oxfmt --list-different, oxlint and tsc are clean on the changed files, and CI is green.

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. This patch only guarantees the weaker invariant that the value shown is formatted as the metric it came from.

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 #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.

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	e3cb176
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6ac0902eb724b80008a8b194
😎 Deploy Preview	https://deploy-preview-44815--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[bito-code-review]

The flagged issue is already resolved in the provided PR diff. The changes in superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts introduce a pushAllTotal method that lazily initializes allTotal using getFormattedAggregator(record). This ensures the grand total correctly adopts the formatter of the metric being aggregated, rather than being pinned to a default formatter in the constructor. No further implementation is required for this specific issue.

superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts

pushAllTotal(record: PivotRecord) {
    if (!this.allTotal) {
      this.allTotal = this.getFormattedAggregator(record)(this, [], []);
    }
    this.allTotal.push(record);
  }

[codecov]

Codecov Report

❌ Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.18%. Comparing base (8e66145) to head (3c776fa).
⚠️ Report is 520 commits behind head on master.

Files with missing lines	Patch %	Lines
...hart-pivot-table/src/react-pivottable/utilities.ts	83.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #44815      +/-   ##
==========================================
+ Coverage   80.17%   80.18%   +0.01%     
==========================================
  Files        2925     2925              
  Lines      172670   172678       +8     
  Branches    40087    40088       +1     
==========================================
+ Hits       138440   138468      +28     
+ Misses      31633    31613      -20     
  Partials     2597     2597              

Flag	Coverage Δ
javascript	75.68% <83.33%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bito-code-review]

Code Review Agent Run #f6fa46

Actionable Suggestions - 2

• superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts - 1

  • Missing return type · Line 1147-1147
• superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/utilities.test.ts - 1

  • Duplicated test data constant · Line 151-151

Review Details

• Files reviewed - 3 · Commit Range: f9d5934..f9d5934

  • superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts
  • superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx
  • superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/utilities.test.ts
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • Eslint (Linter) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[bito-code-review]

Code Review Agent Run #bea010

Actionable Suggestions - 0

Additional Suggestions - 1

• superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx - 1

  • Unguarded grand-total cell index · Line 1296-1299
    This test extracts `pvtGrandTotal` cells exactly like the test above (lines 1268-1271) but drops that test's `expect(grandTotalCells.length).toBe(1)` guard. On a regression, `grandTotalCells[0]` is undefined and `toHaveTextContent` fails with a confusing matcher error instead of a clear count mismatch. Reuse the guard so both tests fail diagnostically.

Review Details

• Files reviewed - 3 · Commit Range: e3ab8f5..e3ab8f5

  • superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts
  • superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx
  • superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/utilities.test.ts
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • Eslint (Linter) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[abhinav-phi]

Fixed — the second rendered grand-total test (TableRenderer keeps each metric aggregation on its own formatter) now has the same expect(grandTotalCells.length).toBe(1) guard as the one above it, so both fail on a clear count mismatch instead of a matcher error against undefined.

Also addressed in the same push, for completeness:

• CodeAnt's Major finding on pushAllTotal — a real bug in my first pass, now fixed. The slot was built from the first record and never revisited, but cellValue.push() overwrites the stored value, so a grand total shared by several metrics rendered the last metric's value in the first metric's format. Verified against the old behaviour: a rate of 5 came out as $5.00, and $300 came out as 300.000 r. The slot now rebuilds when a record resolves to a different formatter, with a test.each covering both record orders.
• Missing : void on pushAllTotal — added, matching sortKeys() and processRecord().
• Duplicated test fixture — the metric-collapse test now derives from the shared SINGLE_METRIC_DATA via .slice(0, -1) rather than repeating the records inline.

npx jest plugins/plugin-chart-pivot-table → 9 suites, 120 tests, all passing. oxfmt and oxlint clean.

[bito-code-review]

Code Review Agent Run #efd35b

Actionable Suggestions - 0

Additional Suggestions - 1

• superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/utilities.test.ts - 1

  • Magic index coupling · Line 153-153
    `SINGLE_METRIC_DATA.slice(0, -1)` positionally assumes the last fixture record is the grand total. If a record is appended to or reordered in `SINGLE_METRIC_DATA`, this test silently drops the wrong record and its assertions change meaning. Consider a dedicated fixture or a filter on `__columns`/`__rows` instead of the magic `-1` index.

Review Details

• Files reviewed - 3 · Commit Range: e3ab8f5..d9eea95

  • superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts
  • superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx
  • superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/utilities.test.ts
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • Eslint (Linter) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[abhinav-phi]

Agreed, and it was a worse problem than it looked. slice(0, -1) was encoding "the grand total is the last record" as a positional assumption, so appending or reordering anything in the fixture would change what the test was actually exercising.

Replaced with a filter on the rollup tags themselves — the grand-total record is the one with an empty row and column rollup:

const withoutGrandTotalRecord = (records: TaggedRecord[]): TaggedRecord[] =>
  records.filter(
    record =>
      (record.__rows as string[]).length > 0 ||
      (record.__columns as string[]).length > 0,
  );

While checking this I found the test would not actually have caught a wrong selection: the grand total would still be fed directly by the surviving record, so expect(rendered(pivotData, [], [])).toBe('$300.00') would have stayed green. Added an assertion that pins the premise the test depends on:

const fixture = withoutGrandTotalRecord(SINGLE_METRIC_DATA);
// The whole point of this test is that the grand total arrives via the mirror
// and not from a record, so assert the fixture really is in that shape --
// otherwise dropping the wrong record would leave the test silently green.
expect(fixture.some(isGrandTotalRecord)).toBe(false);

Verified both directions. A filter that keeps the grand-total record now fails on that assertion:

× metric-collapse total uses the metric formatter
  Expected: false
  Received: true
  at utilities.test.ts:175

and a filter that drops the wrong record instead fails on the value:

  Expected: "$300.00"
  Received: ""

npx jest plugins/plugin-chart-pivot-table → 9 suites, 120 tests, all passing. oxfmt clean.

[bito-code-review]

Code Review Agent Run #14e70e

Actionable Suggestions - 0

Review Details

• Files reviewed - 1 · Commit Range: d9eea95..3c776fa

  • superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/utilities.test.ts
• Files skipped - 0
• Tools

  • Eslint (Linter) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[abhinav-phi]

hi @rusackas, sadpandajoe approved this on oct 2 and all checks are green, the two open bot threads are answered and resolved.. heads up that i pushed one more commit (e3cb176) after the approval so the approve predates the current head, though that commit is only a master merge and touches no file in this plugin.. could you take a look or land it when convenient