sadpandajoe commented on code in PR #44657:
URL: https://github.com/apache/superset/pull/44657#discussion_r4139859230
##########
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:
This still reduces post-processed CSV/XLSX/report data with the old backend
contract: `pivot_table_v2` accepts `Median`/`Sum as Fraction…` but never
implements the new client-side result aggregation, and it can still apply a
persisted `showValuesAs` value that Explore now hides. That leaves
exports/reports disagreeing with the chart. Could this path either implement
the same aggregation semantics or reject those settings until it can?
##########
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:
This map only records full leaf row keys, but `fractionOf` looks up the
prefix key for a row subtotal. With rows `[region, store]`, Metric on columns,
row subtotals enabled, and column subtotals disabled, a `Sum as Fraction of
Rows` region subtotal cannot find its per-metric denominator and renders blank.
Could the per-metric totals also be recorded for the subtotal prefixes (with a
regression test for that case)?
##########
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:
The restored result-aggregation path always builds from `aggregators`, whose
formatters are the fixed US defaults, and then disables `customFormatters`
below. Existing currency or custom-formatted pivots therefore lose their
configured format when a legacy `aggregateFunction` becomes active. Could this
preserve the chart/default formatter for non-fraction result aggregations?
--
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]
