This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch fix/pivot-grand-total-mixed-metrics in repository https://gitbox.apache.org/repos/asf/superset.git
commit d15dc7d394b0cec34160d9c9db3c98b635d7ad55 Author: Evan Rusackas <[email protected]> AuthorDate: Fri Sep 25 09:23:03 2026 -0700 fix(pivot-table): stop the grand-total corner from silently picking one metric With `Apply metrics on: Columns` and more than one metric, the rightmost "Total" column and bottom-right grand-total corner collapse the Metric pseudo-dimension, so that slot receives one DB-computed record per metric (e.g. MAX(sales) and MEDIAN(msrp) both landing in the same cell). The rollup pivot's passthrough aggregator (`cellValue` in react-pivottable/utilities.ts) assumed exactly one record per cell and just overwrote its stored value on every push, so the cell silently showed whichever metric happened to be pushed last -- with the same number mislabeled "Total", and the other metric's contribution discarded with no indication anything was dropped. There's no single number that means anything for "max of sales combined with median of msrp" regardless of which metric wins, so `push()` now detects when a second, different metric lands in the same slot (via the existing `__metricKey` tagging already used elsewhere in this file for the same pseudo-dimension) and `value()` renders that cell blank instead -- the same blank-cell convention already used for a genuine DB-computed null. This also fixes the equivalent "self-consistent but still meaningless" 100% that "Show values as" percent modes produced for the same cell, since `fractionOf` wraps this aggregator and already treats a null numerator as blank. `processRecord`'s "Metric-collapse totals" comment is updated to match -- it previously documented "last metric wins" as a known, deliberate gap ("left as future work"); this is that follow-up. Co-Authored-By: Evan Rusackas <[email protected]> Co-Authored-By: Claude Sonnet 5 <[email protected]> --- .../src/react-pivottable/utilities.ts | 31 ++++++++++++++--- .../test/react-pivottable/tableRenders.test.tsx | 24 +++++++++---- .../test/react-pivottable/utilities.test.ts | 40 +++++++++++++++++++++- 3 files changed, 83 insertions(+), 12 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..4d351b625c3 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 @@ -391,14 +391,34 @@ const fmtNonString = * rather than re-aggregating. This is what makes non-additive totals correct. * See SIP.md. Currency tracking mirrors the real aggregators for AUTO-mode * detection. + * + * The "exactly one record per metric" invariant doesn't hold for the Total + * axis/corner opposite the Metric pseudo-dimension when there's more than one + * metric (see `processRecord`'s "Metric-collapse totals"): that slot receives + * one record per metric, e.g. MAX(sales) and MEDIAN(msrp) both landing in the + * same grand-total cell. There's no single number that means anything for + * "max of sales combined with median of msrp", so once a second, different + * metric is pushed into the same cell, `value()` renders blank instead of + * silently keeping whichever metric happened to be pushed last. */ const cellValue = (formatter: Formatter = usFmt) => ([attr]: string[]) => () => ({ val: null as string | number | null, + seenMetric: undefined as string | undefined, + mixedMetrics: false, currencySet: new Set<string>(), push(record: PivotRecord) { + const metricDim = record.__metricKey as unknown as string | undefined; + if (metricDim) { + const metric = String(record[metricDim]); + if (this.seenMetric === undefined) { + this.seenMetric = metric; + } else if (metric !== this.seenMetric) { + this.mixedMetrics = true; + } + } this.val = record[attr] as string | number | null; if ( record.__currencyColumn && @@ -408,7 +428,7 @@ const cellValue = } }, value() { - return this.val; + return this.mixedMetrics ? null : this.val; }, getCurrencies() { return Array.from(this.currencySet); @@ -1312,9 +1332,12 @@ class PivotData { // would leave the opposite "Total" axis and the grand-total corner empty. // When a record's axis holds only the metric (no real dims there), its value // is also the collapsed total for that axis, so mirror it into rowTotals / - // colTotals / allTotal. (For a single metric this equals the metric column; - // for multiple metrics it is the last metric -- a cross-metric total is not - // well defined and is left as future work.) + // colTotals / allTotal. For a single metric this equals the metric column. + // For multiple metrics, more than one record lands in the same slot; a + // cross-metric total (e.g. MAX(sales) combined with MEDIAN(msrp)) has no + // single meaningful value, so `cellValue`'s `push()` detects the mismatch + // and renders that slot blank rather than showing whichever metric was + // pushed last. const metricKey = record.__metricKey as unknown as string | undefined; if (metricKey) { const realColCount = levelColumns.filter(c => c !== metricKey).length; 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..54b852d10e0 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 @@ -226,6 +226,13 @@ test('TableRenderer renders grand total when both totals are enabled', () => { * empty key on the metric axis. Records carry `__metricKey` so PivotData can * mirror the value into rowTotals / allTotal. (Regression guard for the gap that * in-app verification surfaced: a null right-hand "Total" column.) + * + * This fixture uses a single metric ("m1") throughout, so every record + * mirrored into a shared total slot agrees on the metric -- the case this + * guards is "no data at all reaches the slot", not a metric mismatch. See + * `TAGGED_MULTI_METRIC_ON_COLUMNS` below for the genuinely-mixed-metric case, + * where the same slot receiving two different metrics' records correctly + * blanks instead of picking one. */ const TAGGED_METRIC_ON_COLUMNS = [ // leaf cells: rows = [color], columns = [Metric] (metric on the column axis) @@ -1001,13 +1008,16 @@ test('TableRenderer shows actual values when showValuesAs is unset (default)', ( /** * Regression guard: the grand-total corner cell is a single shared aggregator * slot that "Metric-collapse totals" mirrors every metric's grand-total - * record into (see `processRecord`), so both its own value and `metricAxis` - * reflect the last metric pushed (m2). The denominator lookup must resolve - * against that same metric's own total (m2's 300, not m1's 30) so the corner - * cell reads a self-consistent 100% instead of an obviously wrong - * cross-metric ratio (300 / 30 = "1000.0%"). + * record into (see `processRecord`), so it receives one record per metric + * (m1 then m2 here). `cellValue`'s `push()` detects that the records disagree + * on which metric they belong to and renders the cell blank -- there's no + * meaningful single ratio for "m1 as a fraction of m1's total, or is it m2's, + * mixed with m2's own numerator" -- rather than a fabricated number: neither + * an obviously wrong cross-metric ratio (m2's 300 / m1's 30 = "1000.0%") nor + * a self-consistent-looking but still meaningless 100% (m2 / m2, discarding + * m1's contribution) is actually correct. */ -test('TableRenderer keeps the grand-total corner cell self-consistent when it mixes multiple metrics in fraction mode', () => { +test('TableRenderer blanks the grand-total corner cell instead of fabricating a cross-metric ratio in fraction mode', () => { const props = buildDefaultProps({ data: TAGGED_MULTI_METRIC_ON_COLUMNS, rows: ['color'], @@ -1022,7 +1032,7 @@ test('TableRenderer keeps the grand-total corner cell self-consistent when it mi .getAllByRole('gridcell') .filter(cell => cell.classList.contains('pvtGrandTotal')); expect(grandTotalCells).toHaveLength(1); - expect(grandTotalCells[0]).toHaveTextContent('100.0%'); + expect(grandTotalCells[0]).toHaveTextContent(''); }); /** 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..908f0b13e61 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,41 @@ test('Minimum and Maximum still compute extremes regardless of order', () => { expect(aggregate('Minimum', records)).toBe(1); expect(aggregate('Maximum', records)).toBe(5); }); + +// Records shaped like PivotTableChart.tsx's real output: the "Metric" pseudo +// -dimension is the sole column, so each record's own rollup level has no +// "real" columns -- which is exactly the condition that also mirrors its +// value into the row-total/grand-total slots (see `processRecord`'s +// "Metric-collapse totals"). +const metricRecord = (metric: string, value: number): PivotRecord => + ({ + Metric: metric, + value, + __metricKey: 'Metric', + __rows: [], + __columns: ['Metric'], + }) as unknown as PivotRecord; + +test('grand total renders blank when it would combine two different metrics', () => { + const pivotData = new PivotData({ + data: [metricRecord('MAX(sales)', 100), metricRecord('MEDIAN(msrp)', 50)], + rows: [], + cols: ['Metric'], + vals: ['value'], + }); + + // Neither metric's own value -- there's no single number that means + // "max of sales combined with median of msrp". + expect(pivotData.getAggregator([], []).value()).toBeNull(); +}); + +test('grand total still passes through the value for a single metric', () => { + const pivotData = new PivotData({ + data: [metricRecord('MAX(sales)', 100)], + rows: [], + cols: ['Metric'], + vals: ['value'], + }); + + expect(pivotData.getAggregator([], []).value()).toBe(100); +});
