aminghadersohi commented on code in PR #44657:
URL: https://github.com/apache/superset/pull/44657#discussion_r4179290040
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -1051,8 +1273,57 @@ 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();
+ },
+ sortValue() {
+ return innerValue();
Review Comment:
Blanking this `sortValue` (`mixed ? null : innerValue()`) leaves the suite
green; only the `cellValue` sort key is tested. The blue/red value-sort test
rerun with `aggregateFunction: 'Sum'` fails on that mutant and passes at head.
##########
superset/migrations/versions/2026-09-25_00-00_141b8ada7731_tag_pivot_tables_with_restored_.py:
##########
@@ -0,0 +1,264 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""tag pivot tables with a restored aggregateFunction for review
+
+PR #41184 (SIP-216) removed the Pivot Table's per-table "Aggregation
+function" control (form_data field ``aggregateFunction``) and left it as
+orphaned, ignored dead data on any chart that had it set -- per that PR's
+own UPDATING.md note, no migration was done at the time.
+
+A later change restores the same control as "result aggregation": a second
+aggregation pass over a metric's own grouped results, computed correctly
+this time (see docs/sip -- not repeating the pre-SIP-216 bug of
+re-aggregating already-displayed values). It deliberately reuses the exact
+same form_data field name and the exact same value spellings the old
+control used, so no value needs to change for a saved chart to pick it back
+up -- the moment that code ships, any chart with e.g.
+``aggregateFunction: "Median"`` already sitting in ``params`` starts
+computing its totals with the new, correct "Median" result aggregation
+again, automatically.
+
+That is by design and does not need admin review to be *safe*, but a
+chart's totals silently changing shape on upgrade is still worth a
+customer-facing heads-up, not silence. This migration does not touch
+``aggregateFunction`` or any other value -- there is nothing to migrate --
+it only:
+
+1. Tags every ``pivot_table_v2`` chart whose ``aggregateFunction`` is one of
+ the pre-SIP-216 values (i.e. would now activate result aggregation) with
+ a ``LEGACY_AGGREGATION_TAG`` custom tag, so affected charts are
+ queryable and the frontend can surface a one-time "these totals were
+ restored, please validate" notice (removed automatically the first time
+ the chart is opened and accepted, or saved).
+
+This migration deliberately leaves the chart's cached ``query_context``
+snapshot alone rather than clearing it. That cache predates the feature
+entirely (it was built under the old, GROUPING-SETS-only query shape), so it
+will keep serving a stale, pre-restoration result to a report or alert until
+the chart is next opened and re-saved in Explore -- the same caveat #42761's
+migration accepted for its own, narrower fraction-value migration. Unlike
+that stale-but-working tradeoff, clearing ``query_context`` outright (an
+earlier version of this migration did) turns that into a hard failure
+instead: ``ChartWarmUpCacheCommand`` requires a query context to exist and
+raises rather than falling back to a fresh build, so every tagged chart
+would error out of scheduled cache warm-up -- which never opens Explore --
+until a human manually opens and re-saves it. A stale result is exactly what
+the tag/notice already exist to prompt a human to fix; an outage isn't.
+
+Revision ID: 141b8ada7731
+Revises: 884a2115ebd3
+Create Date: 2026-09-25 00:00:00.000000
+
+"""
+
+from datetime import datetime
+
+from alembic import op
+from sqlalchemy import Column, DateTime, Enum, Integer, String, Text
+from sqlalchemy.orm import declarative_base
+
+from superset import db
+from superset.migrations.shared.utils import paginated_update
+from superset.tags.models import ObjectType, TagType
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "141b8ada7731"
+down_revision = "884a2115ebd3"
+
+Base = declarative_base()
+
+_VIZ_TYPE = "pivot_table_v2"
+_FIELD = "aggregateFunction"
+# The control's default, meaning "no second aggregation pass".
+_DEFAULT_RESULT_AGGREGATION = "Metric"
+
+# Name of the custom tag applied to an affected chart. Kept in sync with
+# `LEGACY_AGGREGATION_TAG` in
+# superset-frontend/src/explore/components/LegacyAggregationAlert.tsx,
+# which reads and clears it.
+LEGACY_AGGREGATION_TAG = "legacy-pivot-aggregation-restored"
+
+# The pre-SIP-216 "Aggregation function" values -- kept in sync with
+# RESULT_AGGREGATIONS in
+#
superset-frontend/.../plugin-chart-pivot-table/src/plugin/resultAggregation.ts.
+# Any chart whose orphaned `aggregateFunction` is one of these will start
+# computing its totals with that result aggregation again, automatically,
+# the moment the restoring code ships -- this migration only tags it.
+_LEGACY_AGGREGATE_FUNCTIONS = frozenset(
+ {
+ "Count",
+ "Count Unique Values",
+ "List Unique Values",
+ "Sum",
Review Comment:
`Sum` was the old default, so this tags nearly every pre-SIP-216 pivot.
https://github.com/apache/superset/pull/41184 isn't in 6.0 or 6.1, so release
upgraders' Sum totals don't change, yet the notice says the setting was
previously ignored. Intended, or skip Sum/reword?
##########
superset/charts/client_processing.py:
##########
@@ -1066,14 +1133,34 @@ def pivot_table_v2(
# totals.
df, rollup_levels = split_grouping_sets_levels(df)
show_values_as = form_data.get("showValuesAs")
+ # A result aggregation (anything but the default "Metric") takes over the
+ # cell/summary computation on the frontend and hides this control in
+ # Explore (see `aggregateFunction`'s `visibility` in controlPanel.tsx and
+ # `resultFactory ?? fractionType` in utilities.ts, where the result
+ # aggregation always wins) -- ignore a stale persisted `showValuesAs`
+ # the same way once a result aggregation is active, rather than applying
+ # a percent transform the live chart no longer shows.
+ aggregate_function_raw = form_data.get("aggregateFunction")
percent_mode = (
- show_values_as if show_values_as in SHOW_VALUES_AS_PERCENT_MODES else
None
+ show_values_as
+ if show_values_as in SHOW_VALUES_AS_PERCENT_MODES
+ and aggregate_function_raw in (None, "Metric")
+ else None
)
+ # "Metric" (the new result-aggregation control's default, meaning "use the
+ # metric's own definition, no second aggregation pass") isn't a key in
+ # pivot_v2_aggfunc_map -- it never needed to be, since this backend path
+ # has no result-aggregation support yet (see #44625's follow-up scope).
Review Comment:
Exports still diverge from the chart under a result aggregation. Same leaves
through `pivot_df` vs `PivotData`: Median corner 4.0 vs 2.5; Count leaves all 1
vs DB values; Sample Variance totals 0 vs 0.5/24.5.
https://github.com/apache/superset/issues/44625 is closed. Track this?
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -796,20 +933,81 @@ const baseAggregatorTemplates = {
string[],
string[],
];
+ // `type`'s selector (above) collapses one or both axes to `[]`,
+ // meaning "sum across everything on that axis". When Metric
+ // itself lives on the collapsed axis, "everything" would mean
+ // "every metric", silently adding unlike units together (e.g.
+ // SUM and MAX in the same denominator) -- substitute the
+ // metric's own key back in so the lookup stays scoped to this
+ // cell's own metric, the same way it's already scoped to this
+ // cell's own row/column. This applies to 'row'/'col' just as
+ // much as 'total': a row-fraction denominator still needs to
+ // stay within one metric, not sum across the metrics sharing
+ // that row.
+ let metricSubstituted: 'row' | 'col' | undefined;
if (this.metricAxis) {
if (this.metricAxis.axis === 'col' && selCol.length === 0) {
selCol = [this.metricAxis.value];
+ metricSubstituted = 'col';
} else if (
this.metricAxis.axis === 'row' &&
selRow.length === 0
) {
selRow = [this.metricAxis.value];
+ metricSubstituted = 'row';
}
}
- const denominatorAggregator = data.getAggregator(selRow, selCol);
- if (!denominatorAggregator.inner) {
+ // A metric-substituted lookup must go straight to the per-metric
+ // maps (`rowMetricTotals`/`colMetricTotals` for 'total',
+ // `rowGroupMetricTotals`/`colGroupMetricTotals` for 'row'/'col'),
+ // never through the generic keyed tree below: the tree's flat
+ // keys aren't namespaced by dimension, so a real category value
+ // that happens to equal the metric's own name (e.g. a "sales"
+ // metric alongside a "sales" category) flattens to the same key
+ // as the substituted metric address and would silently return
+ // that category's subtotal instead of the metric's total.
+ // `processRecord` (the DB-precomputed path `showValuesAs` drives)
+ // populates these same maps itself -- see its "Metric-collapse
+ // totals" section -- so they resolve there too, not just under
+ // `processResultRecord`'s result-aggregation reducer.
+ let denominatorAggregator: any;
+ if (metricSubstituted === 'col') {
+ denominatorAggregator =
+ type === 'total'
+ ? data.colMetricTotals[this.metricAxis!.value]
+ : data.rowGroupMetricTotals[flatKey(selRow)]?.[
+ this.metricAxis!.value
+ ];
+ } else if (metricSubstituted === 'row') {
+ denominatorAggregator =
+ type === 'total'
Review Comment:
Single-metric % of column still blanks the Total column (and % of row the
Total row): `selRow`/`selCol` is `[]` there, and `processRecord` never fills
that key. Rows [color], cols [Metric], blue 10/red 30: master shows
25%/75%/100%, head shows blank. This matches master; plugin suite stays green:
```suggestion
selRow.length === 0
? data.colMetricTotals[this.metricAxis!.value]
: data.rowGroupMetricTotals[flatKey(selRow)]?.[
this.metricAxis!.value
];
} else if (metricSubstituted === 'row') {
denominatorAggregator =
selCol.length === 0
```
--
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]
