You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/10/06 20:22:29 UTC
(superset) branch master updated: fix(pivot-table): respect per-metric formatters in result aggregation (#44815)
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new fcee985241b fix(pivot-table): respect per-metric formatters in result aggregation (#44815)
fcee985241b is described below
commit fcee985241be7925dd82a3148c91e4c00578147f
Author: Abhinav <al...@gmail.com>
AuthorDate: Wed Oct 7 01:52:21 2026 +0530
fix(pivot-table): respect per-metric formatters in result aggregation (#44815)
Co-authored-by: Evan Rusackas <ev...@preset.io>
---
.../src/react-pivottable/utilities.ts | 46 ++++-
.../test/react-pivottable/tableRenders.test.tsx | 100 ++++++++++
.../test/react-pivottable/utilities.test.ts | 217 ++++++++++++++++++++-
3 files changed, 357 insertions(+), 6 deletions(-)
diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts b/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts
index 520f343a17e..2c59941de7f 100644
--- a/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts
+++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts
@@ -1026,7 +1026,8 @@ class PivotData {
colKeys: string[][];
rowTotals: Record<string, Aggregator>;
colTotals: Record<string, Aggregator>;
- allTotal: Aggregator;
+ allTotal: Aggregator | null;
+ allTotalFormatter: ((...args: unknown[]) => Aggregator) | null;
subtotals: SubtotalOptions;
sorted: boolean;
@@ -1103,7 +1104,11 @@ class PivotData {
this.colKeys = [];
this.rowTotals = {};
this.colTotals = {};
- this.allTotal = this.aggregator(this, [], []);
+ // Created lazily by pushAllTotal() on the first record that lands in it, so
+ // it resolves its formatter through getFormattedAggregator like every other
+ // slot -- see the note there.
+ this.allTotal = null;
+ this.allTotalFormatter = null;
this.subtotals = subtotals;
this.sorted = false;
@@ -1130,6 +1135,33 @@ class PivotData {
return fmtAggs[groupName][String(groupValue)] || this.aggregator;
}
+ /*
+ * Push a record into the grand-total slot, (re)building that slot's aggregator
+ * whenever the record resolves to a different formatter.
+ *
+ * The grand total has no key of its own, so -- exactly like rowTotals and
+ * colTotals -- its aggregator is derived from the records that reach it rather
+ * than fixed up front. Building it in the constructor from `this.aggregator`
+ * (as this used to) pinned it to `defaultFormatter`, so a metric configured
+ * with a currency or custom d3 format had its total render as a bare number
+ * while its body cells rendered as money.
+ *
+ * Rebuilding on a formatter change is what keeps that honest when several
+ * metrics share the grand total: `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. `getFormattedAggregator` returns a
+ * stable function per metric, so an identity check is enough to spot the
+ * switch, and records that resolve to the same formatter still accumulate.
+ */
+ 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);
+ }
+
arrSort(attrs: string[], partialOnTop: boolean | undefined, reverse = false) {
const sortersArr = attrs.map(a =>
getSort(
@@ -1292,7 +1324,7 @@ class PivotData {
// Place the value in exactly one slot, determined by the level.
if (rowKey.length === 0 && colKey.length === 0) {
- this.allTotal.push(record);
+ this.pushAllTotal(record);
} else if (rowKey.length === 0) {
this.colTotals[flatColKey].push(record);
this.colTotals[flatColKey].isSubtotal = isColSubtotal;
@@ -1320,11 +1352,11 @@ class PivotData {
const realColCount = levelColumns.filter(c => c !== metricKey).length;
const realRowCount = levelRows.filter(r => r !== metricKey).length;
if (levelColumns.includes(metricKey) && realColCount === 0) {
- if (rowKey.length === 0) this.allTotal.push(record);
+ if (rowKey.length === 0) this.pushAllTotal(record);
else this.rowTotals[flatRowKey]?.push(record);
}
if (levelRows.includes(metricKey) && realRowCount === 0) {
- if (colKey.length === 0) this.allTotal.push(record);
+ if (colKey.length === 0) this.pushAllTotal(record);
else this.colTotals[flatColKey]?.push(record);
}
}
@@ -1345,6 +1377,10 @@ class PivotData {
}
return (
agg || {
+ // Blank cell: no record ever reached this slot. `push` is a no-op so
+ // this satisfies `Aggregator` -- the slot being unset is only observable
+ // through value()/format().
+ push() {},
value() {
return null;
},
diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx b/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx
index 511d55d5d81..e77fe7876e3 100644
--- a/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx
+++ b/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx
@@ -1220,3 +1220,103 @@ test('TableRenderer ignores customFormatters while showValuesAs is a percentage'
expect.arrayContaining(['50.0%', '50.0%', '50.0%', '50.0%']),
);
});
+
+/**
+ * #44724: an aggregation cell must be formatted with the formatter of the metric
+ * it aggregates, not with the chart-level `defaultFormatter`. `sales` is a
+ * currency metric, so every one of its cells -- body, column total and the
+ * grand-total corner -- has to read like money. Before the fix the grand-total
+ * corner was built straight from `defaultFormatter` and rendered as a bare
+ * "300.00".
+ */
+const CURRENCY_TAGGED_DATA = [
+ // leaf cells: rows = [color], columns = [Metric]
+ {
+ color: 'blue',
+ Metric: 'sales',
+ value: 100,
+ __rows: ['color'],
+ __columns: ['Metric'],
+ __metricKey: 'Metric',
+ },
+ {
+ color: 'red',
+ Metric: 'sales',
+ value: 200,
+ __rows: ['color'],
+ __columns: ['Metric'],
+ __metricKey: 'Metric',
+ },
+ // column total: rows = [], columns = [Metric]
+ {
+ Metric: 'sales',
+ value: 300,
+ __rows: [],
+ __columns: ['Metric'],
+ __metricKey: 'Metric',
+ },
+ // grand total: rows = [], columns = []
+ { Metric: 'sales', value: 300, __rows: [], __columns: [] },
+];
+
+const defaultNumberFormatter = (x: number) => x.toFixed(2);
+const currencyFormatter = (x: number) => `$${x.toFixed(2)}`;
+
+test('TableRenderer formats the grand total with the metric formatter', () => {
+ const props = buildDefaultProps({
+ data: CURRENCY_TAGGED_DATA,
+ rows: ['color'],
+ cols: ['Metric'],
+ vals: ['value'],
+ tableOptions: { rowTotals: true, colTotals: true },
+ defaultFormatter: defaultNumberFormatter,
+ customFormatters: { Metric: { sales: currencyFormatter } },
+ });
+ renderWithTheme(<TableRenderer {...props} />);
+
+ // Body cells and the column total already used the metric formatter; assert
+ // them so a regression anywhere in the chain is caught here too.
+ expect(getCellTexts('pvtVal')).toEqual(['$100.00', '$200.00']);
+ // `pvtTotal` covers both the right-hand row totals and the bottom column
+ // total. Only the column total is scoped to `sales` here: a row total spans
+ // the whole metric axis, so with several metrics on it there is no single
+ // correct formatter to apply -- deciding that is #44725, and `getFormattedAggregator`
+ // deliberately falls back to the default for that case.
+ expect(getCellTexts('pvtTotal')).toEqual(expect.arrayContaining(['$300.00']));
+
+ // The grand-total corner aggregates that same single metric, so it must use
+ // the same formatter rather than falling back to the default.
+ const grandTotalCells = screen
+ .getAllByRole('gridcell')
+ .filter(cell => cell.classList.contains('pvtGrandTotal'));
+ expect(grandTotalCells.length).toBe(1);
+ expect(grandTotalCells[0]).toHaveTextContent('$300.00');
+ expect(grandTotalCells[0].textContent).not.toMatch(/^300\.00$/);
+});
+
+test('TableRenderer keeps each metric aggregation on its own formatter', () => {
+ const rateFormatter = (x: number) => `${x.toFixed(3)} r`;
+ const props = buildDefaultProps({
+ data: CURRENCY_TAGGED_DATA.map(record =>
+ record.Metric === 'sales'
+ ? { ...record, Metric: 'rate', value: record.value / 100 }
+ : record,
+ ),
+ rows: ['color'],
+ cols: ['Metric'],
+ vals: ['value'],
+ tableOptions: { rowTotals: true, colTotals: true },
+ defaultFormatter: defaultNumberFormatter,
+ customFormatters: { Metric: { rate: rateFormatter } },
+ });
+ renderWithTheme(<TableRenderer {...props} />);
+
+ expect(getCellTexts('pvtVal')).toEqual(['1.000 r', '2.000 r']);
+ expect(getCellTexts('pvtTotal')).toEqual(expect.arrayContaining(['3.000 r']));
+
+ const grandTotalCells = screen
+ .getAllByRole('gridcell')
+ .filter(cell => cell.classList.contains('pvtGrandTotal'));
+ expect(grandTotalCells.length).toBe(1);
+ expect(grandTotalCells[0]).toHaveTextContent('3.000 r');
+});
diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/utilities.test.ts b/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/utilities.test.ts
index add7e37d9df..ad6c77f4b63 100644
--- a/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/utilities.test.ts
+++ b/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/utilities.test.ts
@@ -17,7 +17,7 @@
* under the License.
*/
-import { aggregators } from '../../src/react-pivottable/utilities';
+import { aggregators, PivotData } from '../../src/react-pivottable/utilities';
import type { PivotRecord } from '../../src/react-pivottable/utilities';
// Records may legitimately carry null values for an attribute; PivotRecord only
@@ -59,3 +59,218 @@ test('Minimum and Maximum still compute extremes regardless of order', () => {
expect(aggregate('Minimum', records)).toBe(1);
expect(aggregate('Maximum', records)).toBe(5);
});
+
+/*
+ * #44724 -- per-metric formatters must reach the aggregation (total) cells too.
+ *
+ * A metric can be configured with its own value format (currency, d3 format,
+ * decimal precision). `PivotData` receives those as `customFormatters`, keyed by
+ * the metric pseudo-dimension and then by metric name, and resolves them through
+ * `getFormattedAggregator`. Every slot must go through that resolution, otherwise
+ * a total silently renders with the chart-level `defaultFormatter` instead -- so
+ * a $300 total shows up as a bare "300.00".
+ *
+ * Two metrics with deliberately different formatters make a wrong pick obvious:
+ * the assertions below read correctly only when each cell uses its own metric's
+ * formatter rather than the other metric's or the default.
+ */
+const currencyFmt = (x: number) => `$${x.toFixed(2)}`;
+const rateFmt = (x: number) => `${x.toFixed(3)} r`;
+const defaultFmt = (x: number) => x.toFixed(2);
+
+// Keyed exactly as PivotTableChart builds `metricFormatters`: METRIC_KEY first,
+// then metric name.
+const perMetricFormatters = {
+ Metric: { sales: currencyFmt, rate: rateFmt },
+};
+
+const buildPivot = (data: Record<string, unknown>[]) =>
+ new PivotData(
+ {
+ data,
+ rows: ['color'],
+ cols: ['Metric'],
+ vals: ['value'],
+ defaultFormatter: defaultFmt,
+ customFormatters: perMetricFormatters,
+ } as unknown as Record<string, unknown>,
+ { colEnabled: true, rowEnabled: true },
+ );
+
+const rendered = (pivotData: PivotData, rowKey: string[], colKey: string[]) => {
+ const agg = pivotData.getAggregator(rowKey, colKey);
+ return agg.format(agg.value(), agg);
+};
+
+/**
+ * Single metric, so every total unambiguously belongs to `sales` and must be
+ * currency-formatted. Multi-metric grand totals are a separate question (which
+ * metric a cross-metric total belongs to is #44725, not this issue).
+ */
+const SINGLE_METRIC_DATA = [
+ {
+ color: 'blue',
+ Metric: 'sales',
+ value: 100,
+ __rows: ['color'],
+ __columns: ['Metric'],
+ __metricKey: 'Metric',
+ },
+ {
+ color: 'red',
+ Metric: 'sales',
+ value: 200,
+ __rows: ['color'],
+ __columns: ['Metric'],
+ __metricKey: 'Metric',
+ },
+ {
+ Metric: 'sales',
+ value: 300,
+ __rows: [],
+ __columns: ['Metric'],
+ __metricKey: 'Metric',
+ },
+ { Metric: 'sales', value: 300, __rows: [], __columns: [] },
+];
+
+type TaggedRecord = Record<string, unknown>;
+
+/**
+ * Drop the explicit grand-total record -- the one tagged with an empty row *and*
+ * column rollup. Selecting on those tags rather than a position keeps the
+ * metric-collapse test correct if records are appended to or reordered in the
+ * shared fixture.
+ */
+const withoutGrandTotalRecord = (records: TaggedRecord[]): TaggedRecord[] =>
+ records.filter(
+ record =>
+ (record.__rows as string[]).length > 0 ||
+ (record.__columns as string[]).length > 0,
+ );
+
+const isGrandTotalRecord = (record: TaggedRecord) =>
+ (record.__rows as string[]).length === 0 &&
+ (record.__columns as string[]).length === 0;
+
+test('grand total uses the metric formatter, not the default formatter', () => {
+ const pivotData = buildPivot(SINGLE_METRIC_DATA);
+
+ // The body cell and the column total already resolve per-metric...
+ expect(rendered(pivotData, ['blue'], ['sales'])).toBe('$100.00');
+ expect(rendered(pivotData, [], ['sales'])).toBe('$300.00');
+ // ...and the grand total is the same `sales` value, so it must match.
+ // Before the fix this was "300.00" -- defaultFormatter, the reported bug.
+ expect(rendered(pivotData, [], [])).toBe('$300.00');
+});
+
+test('metric-collapse total uses the metric formatter', () => {
+ // No rollup level produces an empty key on the metric axis, so the collapsed
+ // total is mirrored into rowTotals/allTotal from the metric-only records.
+ // Removing the grand-total record from the shared fixture is exactly that case.
+ 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);
+ const pivotData = buildPivot(fixture);
+
+ // There is no explicit rows=[]/columns=[] record here, so the grand total is
+ // fed purely by the metric-collapse mirror -- and it still has to pick up the
+ // metric's formatter.
+ expect(rendered(pivotData, [], [])).toBe('$300.00');
+ // The per-row total is deliberately left alone: it spans the whole metric
+ // axis, so `getFormattedAggregator` falls back to the default formatter there.
+ // With several metrics on that axis no single formatter is correct, and
+ // picking one anyway is the mixed-metric-total question tracked by #44725.
+ expect(rendered(pivotData, ['blue'], [])).toBe('100.00');
+});
+
+test('each aggregation cell uses the formatter of its own metric', () => {
+ const pivotData = buildPivot([
+ {
+ color: 'blue',
+ Metric: 'sales',
+ value: 100,
+ __rows: ['color'],
+ __columns: ['Metric'],
+ __metricKey: 'Metric',
+ },
+ {
+ color: 'blue',
+ Metric: 'rate',
+ value: 5,
+ __rows: ['color'],
+ __columns: ['Metric'],
+ __metricKey: 'Metric',
+ },
+ {
+ Metric: 'sales',
+ value: 100,
+ __rows: [],
+ __columns: ['Metric'],
+ __metricKey: 'Metric',
+ },
+ {
+ Metric: 'rate',
+ value: 5,
+ __rows: [],
+ __columns: ['Metric'],
+ __metricKey: 'Metric',
+ },
+ ]);
+
+ // Column totals, one per metric: neither may borrow the other's formatter.
+ expect(rendered(pivotData, [], ['sales'])).toBe('$100.00');
+ expect(rendered(pivotData, [], ['rate'])).toBe('5.000 r');
+ // Body cells, same rule.
+ expect(rendered(pivotData, ['blue'], ['sales'])).toBe('$100.00');
+ expect(rendered(pivotData, ['blue'], ['rate'])).toBe('5.000 r');
+});
+
+/**
+ * Which metric a multi-metric grand total belongs to is #44725's question, and
+ * this PR does not settle it. What must never happen is the slot being built
+ * once from the first metric and then going on to render the *last* metric's
+ * value in the *first* metric's format -- a regression that reads as correct
+ * formatting of a plainly wrong number.
+ *
+ * Whichever metric's value the slot ends up showing, it has to be formatted as
+ * that same metric. Asserted for both orders, since `push` overwrites the stored
+ * value and the last record wins.
+ */
+test.each([
+ ['sales first', ['sales', 'rate'], '5.000 r'],
+ ['rate first', ['rate', 'sales'], '$300.00'],
+])(
+ 'multi-metric grand total formats the value it shows as that metric (%s)',
+ (_label, order, expected) => {
+ const grandTotals: Record<string, number> = { sales: 300, rate: 5 };
+ const pivotData = buildPivot(
+ order.map(metric => ({
+ Metric: metric,
+ value: grandTotals[metric],
+ __rows: [],
+ __columns: [],
+ __metricKey: 'Metric',
+ })),
+ );
+
+ // The value is the last record's; the formatter has to be that metric's too.
+ const agg = pivotData.getAggregator([], []);
+ expect(agg.value()).toBe(grandTotals[order[order.length - 1]]);
+ expect(agg.format(agg.value(), agg)).toBe(expected);
+ },
+);
+
+test('a total with no per-metric formatter still falls back to the default', () => {
+ const pivotData = new PivotData({
+ data: SINGLE_METRIC_DATA,
+ rows: ['color'],
+ cols: ['Metric'],
+ vals: ['value'],
+ defaultFormatter: defaultFmt,
+ } as unknown as Record<string, unknown>);
+
+ expect(rendered(pivotData, [], [])).toBe('300.00');
+});