Skip to content

Commit 490356d

Browse files
rusackasclaude
andcommitted
fix(pivot-table): fetch percent-mode denominator levels regardless of totals toggle
`showValuesAs` percent modes (% of row/column/grand total) divide by a rollup level that `buildGroupbyCombinations` only queried when the matching Total/subtotal display toggle was on, so picking a percent mode without enabling that toggle could crash on a missing denominator aggregator. Force the required level(s) in and fall back to a blank cell instead of throwing if one is ever still missing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent cdb52e5 commit 490356d

4 files changed

Lines changed: 110 additions & 11 deletions

File tree

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

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,12 @@
1818
*/
1919

2020
import { QueryFormColumn, QueryFormMetric } from '@superset-ui/core';
21-
import { Groupby, MetricsLayoutEnum, PivotTableQueryFormData } from '../types';
21+
import {
22+
Groupby,
23+
MetricsLayoutEnum,
24+
PivotTableQueryFormData,
25+
ShowValuesAsEnum,
26+
} from '../types';
2227

2328
// Aggregates whose group total can be derived from per-group results
2429
// (decomposable): summing sub-sums, counting sub-counts, min-of-mins,
@@ -189,6 +194,12 @@ export function splitGroupingSetsResult(
189194
* - intermediate column prefix -> column subtotal -> colSubTotals
190195
* A full-length prefix (the leaf level) is always emitted; when a dimension
191196
* list is empty, `[]` *is* the full level and is therefore always kept.
197+
*
198+
* A "% of row/column/grand total" `showValuesAs` selection needs its
199+
* denominator level even when the corresponding Total/subtotal display
200+
* toggle is off (that toggle only controls whether the totals row/column is
201+
* *rendered*; see `PivotData` in `react-pivottable/utilities.ts`), so the
202+
* required collapsed level(s) are forced in regardless of the toggle.
192203
*/
193204
export default function buildGroupbyCombinations(
194205
formData: PivotTableQueryFormData,
@@ -207,14 +218,26 @@ export default function buildGroupbyCombinations(
207218
...columns.map((_, i) => columns.slice(0, i + 1)),
208219
];
209220

221+
// "% of column total" divides each cell by its column's grand total, which
222+
// is computed with all rows collapsed; "% of grand total" needs the same.
223+
const needsRowsCollapsed =
224+
formData.showValuesAs === ShowValuesAsEnum.PERCENT_OF_COLUMN ||
225+
formData.showValuesAs === ShowValuesAsEnum.PERCENT_OF_TOTAL;
226+
// "% of row total" divides each cell by its row's grand total, which is
227+
// computed with all columns collapsed; "% of grand total" needs the same.
228+
const needsColumnsCollapsed =
229+
formData.showValuesAs === ShowValuesAsEnum.PERCENT_OF_ROW ||
230+
formData.showValuesAs === ShowValuesAsEnum.PERCENT_OF_TOTAL;
231+
210232
const rowPrefixNeeded = (prefix: QueryFormColumn[]): boolean => {
211233
if (prefix.length === rows.length) return true; // leaf / full level
212-
if (prefix.length === 0) return !!formData.colTotals; // bottom Total row
234+
if (prefix.length === 0) return !!formData.colTotals || needsRowsCollapsed; // bottom Total row
213235
return !!formData.rowSubTotals; // row subtotal
214236
};
215237
const colPrefixNeeded = (prefix: QueryFormColumn[]): boolean => {
216238
if (prefix.length === columns.length) return true; // leaf / full level
217-
if (prefix.length === 0) return !!formData.rowTotals; // right Total column
239+
if (prefix.length === 0)
240+
return !!formData.rowTotals || needsColumnsCollapsed; // right Total column
218241
return !!formData.colSubTotals; // column subtotal
219242
};
220243

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

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -753,9 +753,18 @@ const baseAggregatorTemplates = {
753753
},
754754
format: fmtNonString(formatter),
755755
value() {
756-
const acc = data
757-
.getAggregator(...Array.from(this.selector || []))
758-
.inner.value();
756+
// `buildGroupbyCombinations` requests the denominator's rollup
757+
// level whenever a percent `showValuesAs` is selected, but fall
758+
// back to `null` (rendered blank) instead of throwing if it is
759+
// ever missing -- e.g. a denominator aggregator with no matching
760+
// rows in the response.
761+
const denominatorAggregator = data.getAggregator(
762+
...Array.from(this.selector || []),
763+
);
764+
if (!denominatorAggregator.inner) {
765+
return null;
766+
}
767+
const acc = denominatorAggregator.inner.value();
759768

760769
if (typeof acc === 'string') {
761770
return acc;

superset-frontend/plugins/plugin-chart-pivot-table/src/types.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,12 @@ export enum MetricsLayoutEnum {
5252

5353
/**
5454
* Display mode for pivot cell values: raw values, or each value expressed as
55-
* a fraction of its row/column/grand total. Purely a presentational transform
56-
* applied at render time against the already DB-correct rollup totals (see
57-
* `PivotData` in `react-pivottable/utilities.ts`); it does not change what
58-
* gets queried.
55+
* a fraction of its row/column/grand total. The division itself is a
56+
* presentational transform applied at render time against the already
57+
* DB-correct rollup totals (see `PivotData` in
58+
* `react-pivottable/utilities.ts`), but a percent mode does need its
59+
* denominator level queried even if the corresponding Total/subtotal display
60+
* toggle is off; see `buildGroupbyCombinations` in `plugin/utilities.ts`.
5961
*/
6062
export enum ShowValuesAsEnum {
6163
ACTUAL = 'actual',

superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/utilities.test.ts

Lines changed: 66 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,11 @@ import buildGroupbyCombinations, {
2626
splitGroupingSetsResult,
2727
groupingMarkerLabel,
2828
} from '../../src/plugin/utilities';
29-
import { PivotTableQueryFormData, MetricsLayoutEnum } from '../../src/types';
29+
import {
30+
PivotTableQueryFormData,
31+
MetricsLayoutEnum,
32+
ShowValuesAsEnum,
33+
} from '../../src/types';
3034

3135
const baseFormData = {
3236
groupbyRows: ['row1', 'row2'],
@@ -364,6 +368,67 @@ test('pruning: empty column dims keep the [] (leaf) level regardless of rowTotal
364368
]);
365369
});
366370

371+
test('pruning: percent_row forces the row-total (columns collapsed) level even with rowTotals off', () => {
372+
const combinations = buildGroupbyCombinations({
373+
...baseFormData,
374+
colTotals: false,
375+
rowTotals: false,
376+
colSubTotals: false,
377+
rowSubTotals: false,
378+
showValuesAs: ShowValuesAsEnum.PERCENT_OF_ROW,
379+
});
380+
expect(combinations).toEqual([
381+
{ rows: ['row1', 'row2'], columns: [] },
382+
{ rows: ['row1', 'row2'], columns: ['col1', 'col2'] },
383+
]);
384+
});
385+
386+
test('pruning: percent_col forces the column-total (rows collapsed) level even with colTotals off', () => {
387+
const combinations = buildGroupbyCombinations({
388+
...baseFormData,
389+
colTotals: false,
390+
rowTotals: false,
391+
colSubTotals: false,
392+
rowSubTotals: false,
393+
showValuesAs: ShowValuesAsEnum.PERCENT_OF_COLUMN,
394+
});
395+
expect(combinations).toEqual([
396+
{ rows: [], columns: ['col1', 'col2'] },
397+
{ rows: ['row1', 'row2'], columns: ['col1', 'col2'] },
398+
]);
399+
});
400+
401+
test('pruning: percent_total forces the grand-total level even with both totals off', () => {
402+
const combinations = buildGroupbyCombinations({
403+
...baseFormData,
404+
colTotals: false,
405+
rowTotals: false,
406+
colSubTotals: false,
407+
rowSubTotals: false,
408+
showValuesAs: ShowValuesAsEnum.PERCENT_OF_TOTAL,
409+
});
410+
expect(combinations).toEqual([
411+
{ rows: [], columns: [] },
412+
{ rows: ['row1', 'row2'], columns: [] },
413+
{ rows: [], columns: ['col1', 'col2'] },
414+
{ rows: ['row1', 'row2'], columns: ['col1', 'col2'] },
415+
]);
416+
});
417+
418+
test('pruning: actual (no percent mode) does not force any extra level', () => {
419+
const combinations = buildGroupbyCombinations({
420+
...baseFormData,
421+
colTotals: false,
422+
rowTotals: false,
423+
colSubTotals: false,
424+
rowSubTotals: false,
425+
showValuesAs: ShowValuesAsEnum.ACTUAL,
426+
});
427+
expect(combinations).toEqual([
428+
{ rows: ['row1', 'row2'], columns: ['col1', 'col2'] },
429+
]);
430+
});
431+
367432
test('additiveReducerFor maps aggregates to reducers', () => {
368433
const mk = (aggregate: string) =>
369434
({

0 commit comments

Comments
 (0)