rusackas commented on code in PR #44657:
URL: https://github.com/apache/superset/pull/44657#discussion_r4179104177
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -408,7 +461,7 @@ const cellValue =
}
},
value() {
- return this.val;
+ return this.mixedMetrics ? null : this.val;
Review Comment:
@sadpandajoe good catch. Value sorting now uses a separate sort value on the
aggregator, so a blanked mixed-metric total still orders by its last-metric
value as before. Added a test with your blue/red case.
##########
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:
@sadpandajoe fixed. `flatKey` now prefixes non-empty scopes, so depth zero
and a single empty-string category get distinct keys on both axes. Added a
regression test.
##########
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:
@sadpandajoe fixed. The sample variance/std summary fallback now zeroes any
scope with exactly one non-null observation, rather than keying off the
physical column count. Added a sparse US/FR regression test.
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/resultAggregation.ts:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+
+import { t } from '@apache-superset/core/translation';
+
+/**
+ * A "result aggregation" is a second aggregation pass over a metric's own
+ * grouped results (e.g. the median of a set of per-store SUM(sales) values),
+ * distinct from -- and independent of -- the metric's own SQL aggregate. This
+ * is the pre-SIP-216 "Aggregation function" control's actual job: it never
+ * touched leaf cells (those were always the metric's own aggregate), only
+ * how subtotals/totals summarized the leaf cells beneath them. SIP-216
+ * removed the control because that summarization was computed by re-folding
+ * already-displayed cell values client-side, which is wrong for non-additive
+ * reducers (see SIP.md). This module restores the same choice, computed
+ * correctly: every scope (cell, subtotal, grand total) is reduced from its
+ * own original contributing query results, never from another scope's
+ * already-computed output.
+ */
+export const RESULT_AGGREGATIONS = [
+ 'Count',
Review Comment:
@sadpandajoe fixed. Count, Count Unique Values, List Unique Values, First
and Last now go through a per-axis summary helper instead of the Series-only
reducer. Added `pivot_table_v2` tests for each summary axis.
--
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]
