sadpandajoe commented on code in PR #44657:
URL: https://github.com/apache/superset/pull/44657#discussion_r4175563082
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -1223,7 +1571,156 @@ 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'),
+ );
+ // Depth 0 is the fully collapsed (grand total/opposite-axis) scope;
+ // depth === length is the leaf; anything between is a subtotal, included
+ // only when that axis's subtotals are enabled. Computed before the
+ // metric-scope block below so
`rowGroupMetricTotals`/`colGroupMetricTotals`
+ // can be recorded at every depth a subtotal denominator might need, not
+ // just the leaf.
+ const rowDepths = [
+ 0,
+ ...rows
+ .map((_, i) => i + 1)
+ .filter(depth => depth === rows.length || this.subtotals.rowEnabled),
+ ];
+ const colDepths = [
+ 0,
+ ...cols
+ .map((_, i) => i + 1)
+ .filter(depth => depth === cols.length || this.subtotals.colEnabled),
+ ];
+ // 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. Recorded at
+ // every enabled row depth (not just the leaf) so a row subtotal's
+ // own (shorter) prefix key finds a denominator too, instead of only
+ // the full leaf-level row ever getting an entry.
+ rowDepths.forEach(ri => {
+ const rowPrefix = rowKey.slice(0, ri);
+ const flatRk = flatKey(rowPrefix);
Review Comment:
An empty-string category shares its denominator key with the collapsed scope
because flatKey([]) equals flatKey(['']). With region='' values A=10/B=20 and
another region A=30, Sum as Fraction of Rows displays 11.1%/22.2% instead of
33.3%/66.7%; could these scope keys distinguish depth zero from an empty
category, on both axes?
##########
superset/charts/client_processing.py:
##########
@@ -810,17 +810,47 @@ def union_currency_context(
)
+def _sample_dispersion(
+ data: Union[pd.DataFrame, pd.Series],
+ method: str,
+ axis: Optional[int] = None,
+) -> Any:
+ """
+ Sample variance/standard deviation that is 0 for a single observation.
+
+ Accepts a Series (cell aggregation) or a DataFrame reduced along ``axis``
+ (row/column summaries), mirroring how the other reducers are invoked.
+ """
+ if isinstance(data, pd.DataFrame):
+ axis = 0 if axis is None else axis
+ result = getattr(data, method)(axis=axis)
+ return result.fillna(0) if data.shape[axis] <= 1 else result
Review Comment:
Sparse pivots still produce blank sample-variance/std summaries: with
US/boy=10 and FR/girl=20, each row has one observation but two physical
columns, so this returns NaN rather than the single-observation zero. Could the
fallback use each scope's non-null observation count, with a sparse
row/column-summary regression?
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx:
##########
@@ -243,6 +250,34 @@ const config: ControlPanelConfig = {
},
},
],
+ [
+ {
+ name: 'aggregateFunction',
+ config: {
+ type: 'SelectControl',
+ label: () => t('Aggregation function'),
+ default: 'Metric',
Review Comment:
The live-row cleanup fixes the immediate rollback, but slices_version.params
still retains "Metric" from charts saved before downgrade. Restoring one of
those versions copies it back into live params, so the older export path raises
KeyError again; could downgrade normalize the restorable snapshots too, with a
save/downgrade/restore regression?
--
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]
