rusackas commented on code in PR #42761:
URL: https://github.com/apache/superset/pull/42761#discussion_r3716735961
##########
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,
+ choices: [
+ [ShowValuesAsEnum.ACTUAL, t('Actual values')],
+ [ShowValuesAsEnum.PERCENT_OF_ROW, t('% of row total')],
+ [ShowValuesAsEnum.PERCENT_OF_COLUMN, t('% of column total')],
+ [ShowValuesAsEnum.PERCENT_OF_TOTAL, t('% of grand total')],
+ ],
Review Comment:
Confirmed — `buildGroupbyCombinations` only queried the collapsed rollup
level when the matching `rowTotals`/`colTotals` toggle was on, so a percent
mode without that toggle could look up a denominator that was never fetched.
Pushed a fix that forces the required level(s) in whenever `showValuesAs` needs
them, independent of the display toggle, plus a fallback in `fractionOf` so a
still-missing denominator renders blank instead of throwing.
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -970,32 +980,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(
+ cellValue(),
+ fractionType,
+ usFmtPct,
+ )(vals)
+ : cellValue(this.props.defaultFormatter as Formatter)(vals);
Review Comment:
Same root cause as the other thread — fixed there (forcing the denominator
rollup level regardless of the totals toggle, plus a safe fallback in
`fractionOf` instead of dereferencing a missing `.inner`).
--
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]
