sadpandajoe commented on code in PR #42761:
URL: https://github.com/apache/superset/pull/42761#discussion_r3718239305
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx:
##########
@@ -243,6 +243,29 @@ const config: ControlPanelConfig = {
},
},
],
+ [
+ {
+ name: 'showValuesAs',
+ config: {
+ type: 'SelectControl',
+ label: t('Show values as'),
+ default: ShowValuesAsEnum.ACTUAL,
+ renderTrigger: true,
Review Comment:
This is render-only, but the selection now changes which `grouping_sets` are
queried. For a non-additive metric with totals disabled, switching from actual
values leaves the cached response without the denominator level and the
percentage body renders blank; should this trigger a new query instead?
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -970,32 +989,53 @@ class PivotData {
);
const vals = this.props.vals as string[];
+ const fractionType =
+ FRACTION_TYPE_BY_SHOW_VALUES_AS[this.props.showValuesAs as string];
// Values come pre-aggregated from the database (one query per rollup
level),
// so the pivot stores them verbatim via `cellValue` instead of
aggregating.
- this.aggregator = cellValue(this.props.defaultFormatter as
Formatter)(vals);
- this.formattedAggregators = this.props.customFormatters
- ? Object.entries(
- this.props.customFormatters as Record<
- string,
- Record<string, unknown>
- >,
- ).reduce(
- (
- acc: Record<
+ // When "Show values as" a fraction is active, wrap that passthrough with
+ // the same fractionOf template the pre-SIP-216 "Sum as Fraction of ..."
+ // aggregators used: it divides a cell's own value by the value at the
+ // requested scope (row/column/grand total), each of which is itself one
+ // of these uniformly-wrapped aggregators, so `fractionOf`'s cross-lookup
+ // (`data.getAggregator(...).inner.value()`) resolves correctly no matter
+ // which scope it's called for -- including the totals dividing by
+ // themselves to read 100%. This needs no new query and no per-metric
+ // aggregator-override control (that control is gone, see SIP.md); it's a
+ // pure display transform over values that are already DB-correct.
+ this.aggregator = fractionType
+ ? aggregatorTemplates.fractionOf(
Review Comment:
With multiple metrics, this collapsed lookup uses the shared total slot
populated by the metric-collapse path, where each push overwrites the prior
value and the last metric wins. Consequently earlier metrics are divided by the
last metric's total (for example Revenue can become a percentage of Profit);
should the denominator retain the metric key or should these modes be limited
to one metric?
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/utilities.ts:
##########
@@ -207,14 +218,26 @@ export default function buildGroupbyCombinations(
...columns.map((_, i) => columns.slice(0, i + 1)),
];
+ // "% of column total" divides each cell by its column's grand total, which
+ // is computed with all rows collapsed; "% of grand total" needs the same.
+ const needsRowsCollapsed =
+ formData.showValuesAs === ShowValuesAsEnum.PERCENT_OF_COLUMN ||
+ formData.showValuesAs === ShowValuesAsEnum.PERCENT_OF_TOTAL;
+ // "% of row total" divides each cell by its row's grand total, which is
+ // computed with all columns collapsed; "% of grand total" needs the same.
+ const needsColumnsCollapsed =
+ formData.showValuesAs === ShowValuesAsEnum.PERCENT_OF_ROW ||
+ formData.showValuesAs === ShowValuesAsEnum.PERCENT_OF_TOTAL;
+
const rowPrefixNeeded = (prefix: QueryFormColumn[]): boolean => {
if (prefix.length === rows.length) return true; // leaf / full level
- if (prefix.length === 0) return !!formData.colTotals; // bottom Total row
+ if (prefix.length === 0) return !!formData.colTotals ||
needsRowsCollapsed; // bottom Total row
return !!formData.rowSubTotals; // row subtotal
};
const colPrefixNeeded = (prefix: QueryFormColumn[]): boolean => {
if (prefix.length === columns.length) return true; // leaf / full level
- if (prefix.length === 0) return !!formData.rowTotals; // right Total column
+ if (prefix.length === 0)
+ return !!formData.rowTotals || needsColumnsCollapsed; // right Total
column
Review Comment:
The `combineMetric` filter below removes this forced collapsed level again
(`columns: []` for the default COLUMNS layout). That makes `% of row total` and
`% of grand total` render every cell blank when Combine metrics is enabled;
should the forced denominator levels be exempt from that filter?
--
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]
