sadpandajoe commented on code in PR #44657:
URL: https://github.com/apache/superset/pull/44657#discussion_r4246908774
##########
superset-frontend/src/explore/components/ExploreChartPanel/index.tsx:
##########
@@ -383,6 +384,9 @@ const ExploreChartPanel = ({
padding-top: ${theme.sizeUnit * 2}px;
`}
>
+ {vizType === 'pivot_table_v2' && (
+ <LegacyAggregationAlert sliceId={slice?.slice_id} />
Review Comment:
The upgrade notice is only exercised by rendering `LegacyAggregationAlert`
directly, so removing this line or no longer forwarding `slice?.slice_id` would
leave every current test green while upgraded users never see the warning or
the Accept button. Could `ExploreChartPanel.test.tsx` render a saved
`pivot_table_v2` chart with a slice ID whose tags response includes the
migration tag and assert the notice is shown?
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -1255,7 +1639,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),
Review Comment:
Dropping `|| this.subtotals.colEnabled` here would silently remove
intermediate column subtotals for result aggregations, and nothing in the suite
would notice: the only `colEnabled` cases in `utilities.test.ts` use a single
column level or assert just leaf cells. For example, Average with rows
`[product]`, columns `[region, store]`, and P's North/A=10, North/B=20,
South/C=100 should give P/North = 15 and a P total of about 43.33 when column
subtotals are on, and no `North` prefix when they are off. Could a `PivotData`
test pin that?
--
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]
