rusackas commented on code in PR #44657:
URL: https://github.com/apache/superset/pull/44657#discussion_r4172324168
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -1051,8 +1252,54 @@ class PivotData {
);
const vals = this.props.vals as string[];
- const fractionType =
- FRACTION_TYPE_BY_SHOW_VALUES_AS[this.props.showValuesAs as string];
+ // Result aggregation (see resultAggregation.ts): a second aggregation pass
+ // over a metric's own grouped results (e.g. the median of a set of
+ // per-store SUM(sales) values), restoring the pre-SIP-216 "Aggregation
+ // function" choice, computed correctly this time -- `processResultRecord`
+ // below feeds each scope its own original contributing leaf records,
+ // never another scope's already-computed output. `aggregators` already
+ // has a real template for every choice (it's the same dict the
+ // pre-SIP-216 pivot table used); wrap whichever one is selected so a
+ // shared Total/corner slot that ends up seeing more than one metric (see
+ // `makeMixedMetricTracker`) blanks instead of quietly mixing them.
+ const resultAggregation = getResultAggregation(
+ this.props.aggregateFunction,
+ );
+ // `showValuesAs`'s control is hidden once a result aggregation is active
+ // (see controlPanel.tsx) because a result aggregation has its own
+ // "... as Fraction of ..." choices and takes over the computation
+ // entirely -- but hiding the control doesn't reset its stored value, so
+ // a percent choice selected before switching to e.g. "Average" stays in
+ // `this.props.showValuesAs`. Gate `fractionType` on `resultAggregation`
+ // being unset so that stale value can't leak in: left ungated, a leaf
+ // cell would wrap in `fractionOf` and look up a denominator built from
+ // the (non-fraction) result aggregator, which has no `.inner`, and
+ // render blank instead of its actual value.
+ const fractionType = resultAggregation
+ ? undefined
+ : FRACTION_TYPE_BY_SHOW_VALUES_AS[this.props.showValuesAs as string];
+ const resultFactory = resultAggregation
+ ? (...args: unknown[]): Aggregator => {
+ const build = aggregators[resultAggregation] as (
+ v: string[],
+ ) => (...a: unknown[]) => Aggregator;
+ const inner = build(vals)(...args);
+ const innerValue = inner.value.bind(inner);
+ const innerPush = inner.push.bind(inner);
+ const tracker = makeMixedMetricTracker();
+ let mixed = false;
+ return {
+ ...inner,
+ push(record: PivotRecord) {
+ mixed = tracker.sawMixedMetric(record);
+ innerPush(record);
+ },
+ value() {
+ return mixed ? null : innerValue();
Review Comment:
@sadpandajoe done: the shared formatter and the extrema formatter now return
an empty string for null, so custom formatters never see it. Added a rendered
mixed-metric Minimum total assertion.
##########
UPDATING.md:
##########
@@ -1519,9 +1517,25 @@ Fraction of ..." variants are **not** migrated: they
divided a record count,
while the new control divides the metric's own value, so translating them
automatically would silently change what the chart displays rather than
restore it; those charts need to be manually reconfigured if the value-based
-percentage is what's wanted. Charts that used any other non-fraction
-`aggregateFunction` value (Sum, Average, Count, ...) are unaffected, since
-that specific behavior remains unavailable per the above.
+percentage is what's wanted.
+
+The **"Aggregation function"** control itself (form_data field
+`aggregateFunction`) is also back, as a second aggregation pass over a
+metric's own grouped results -- e.g. the median of a set of per-store
+`SUM(sales)` values -- computed correctly this time: every scope (subtotal,
+row/column total, grand total) reduces its own original contributing query
+results, never another scope's already-displayed value, so the previous
+non-additive-metric bug this section originally removed the control for does
+not return. It reuses the exact same field name and value spellings as
+before, so a chart that still had e.g. `aggregateFunction: "Median"` sitting
+unused in its saved `params` starts computing totals with that aggregation
+again automatically, with no action required and no value to reconfigure. A
+one-time migration tags every such chart with a
`legacy-pivot-aggregation-restored`
+custom tag; opening a tagged chart in Explore shows a notice that its totals
+may now look different, which clears once you review and accept it (or save
+the chart). If you have alerts/reports built on one of these charts, validate
+its cached results after upgrading, since a scheduled report render does not
Review Comment:
@sadpandajoe reworded: UPDATING.md now says Accept only removes the tag and
affected charts need re-saving in Explore before validating scheduled outputs.
--
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]
