rusackas commented on code in PR #44657:
URL: https://github.com/apache/superset/pull/44657#discussion_r4140526347
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -1223,7 +1446,124 @@ class PivotData {
return this.rowKeys;
}
+ /**
+ * Result aggregation (see resultAggregation.ts): unlike `processRecord`
+ * below, which places each DB-precomputed record into exactly one rollup
+ * slot, every leaf record here is fed directly to every scope it
+ * contributes to -- one cell, one subtotal per enabled row/column depth,
+ * and the grand total -- so each aggregator reduces real original results,
+ * never another aggregator's already-computed value.
+ */
+ processResultRecord(record: PivotRecord): void {
+ const rows = this.props.rows as string[];
+ const cols = this.props.cols as string[];
+ const rowKey = rows.map(key =>
+ String(key in record ? record[key] : 'null'),
+ );
+ const colKey = cols.map(key =>
+ String(key in record ? record[key] : 'null'),
+ );
+ // A per-metric total (see `rowMetricTotals`/`colMetricTotals`), needed by
+ // "... as Fraction of ..." result aggregations independently of whether
+ // the corresponding subtotal is enabled, and of where the Metric
+ // pseudo-dimension sits in `rows`/`cols` (`combineMetric` can place it
+ // first or last): keyed purely by the metric's own value, not by depth,
+ // so it doesn't matter which position it collapses from.
+ const metricDim = record.__metricKey as unknown as string | undefined;
+ if (metricDim) {
+ const colMetricIndex = cols.indexOf(metricDim);
+ if (colMetricIndex !== -1) {
+ const metricValue = colKey[colMetricIndex];
+ this.colMetricTotals[metricValue] ??= this.getFormattedAggregator(
+ record,
+ )(this, [], [metricValue]);
+ this.colMetricTotals[metricValue].push(record);
+
+ // Row+metric scope: this record's own row, just this metric, across
+ // every column that shares it -- the 'row' fraction type's
+ // denominator when Metric sits on columns. Independent of
+ // `subtotals.colEnabled`, unlike the depth-gated tree.
+ const flatRk = flatKey(rowKey);
Review Comment:
Good catch, fixed. `rowGroupMetricTotals`/`colGroupMetricTotals` now record
every enabled depth, not just the leaf, so a region subtotal's own prefix key
finds a denominator instead of coming up empty. Added a regression test for
that exact rows/store shape.
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -1053,6 +1230,41 @@ 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,
+ );
+ const resultFactory = resultAggregation
+ ? (...args: unknown[]): Aggregator => {
+ const build = aggregators[resultAggregation] as (
Review Comment:
Good catch, fixed. Only the "... as Fraction of ..." choices still need the
unformatted default. The others now reuse the same reducer and just swap in the
metric's own formatter instead of falling back to a differently-reduced
passthrough.
##########
superset/charts/client_processing.py:
##########
@@ -1069,11 +1069,20 @@ def pivot_table_v2(
percent_mode = (
show_values_as if show_values_as in SHOW_VALUES_AS_PERCENT_MODES 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).
+ # Treat it, and any other value this map doesn't recognize, the same way
+ # an absent field always has been: fall back to "Sum".
+ aggregate_function = form_data.get("aggregateFunction")
Review Comment:
The full result-aggregation gap here is already tracked in #44625, that's
more than this PR should take on. Fixed the narrower half though: this path was
still applying a stale `showValuesAs` even once `aggregateFunction` takes over
and Explore hides that control. Now it ignores it the same way the chart does.
--
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]
