This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch feat/pivot-result-aggregation in repository https://gitbox.apache.org/repos/asf/superset.git
commit 2fc9f76dae7e8905740e066df30e526d04d3b010 Author: Evan Rusackas <[email protected]> AuthorDate: Fri Sep 25 11:30:32 2026 -0700 feat(pivot-table): restore result aggregation as a second aggregation pass Restores the pre-SIP-216 "Aggregation function" choice (Sum/Average/ Median/Sample Variance/Sample Standard Deviation/Min/Max/Count/Count Unique Values/List Unique Values/First/Last, and the three "... as Fraction of ..." pairs), computed correctly this time: every cell, subtotal, and grand total reduces its own original contributing leaf query results, never another scope's already-computed output -- the correctness property SIP-216 (#41184) established, just applied to a genuinely different question than metric-definition totals answer. This is not a rehash of the pre-SIP-216 bug. The old "Aggregation function" control applied a chart-wide reducer to already-*displayed* subtotal values (re-aggregating aggregates, wrong for non-additive reducers). This is a second aggregation pass over a metric's own grouped results -- e.g. showing the median of a set of per-store SUM(sales) values -- a genuinely different, useful question the current metric-definition-only model has no way to express at all: rewriting a metric's own SQL aggregate changes its leaf cells too, which the SIP-216 architecture was specifically built to keep consistent between cells and totals. New `aggregateFunction` SelectControl, defaulting to "Use metric definition" (today's unchanged behavior). Selecting a result aggregation switches the query to full leaf detail (buildQuery.ts skips `grouping_sets` entirely, same as the additive fast path) and `PivotData.processResultRecord` (react-pivottable/utilities.ts) feeds each leaf record to every rollup scope it belongs to, reusing the existing `aggregators` template dict -- the same reducers the pre-SIP-216 pivot table used, never removed, just unused since the control was. Guards against the one real risk this reintroduces: a shared Total/ corner slot opposite the Metric pseudo-dimension can receive records from more than one metric when a chart has 2+ metrics (the same class of bug #44657, this branch's base, fixes for metric-definition mode). Extracted `cellValue`'s mixed-metric detection into a shared `makeMixedMetricTracker` helper and applied it to every result aggregator instance too, so a slot that would otherwise blend e.g. MAX(sales) with MEDIAN(msrp) blanks instead of guessing. Backend (report/export parity via client_processing.py) intentionally left for a follow-up folding into #44625, to keep this reviewable; the live browser view is the primary surface and where the bug this whole effort started from actually lives. Co-Authored-By: Evan Rusackas <[email protected]> Co-Authored-By: Claude Sonnet 5 <[email protected]> --- .../src/PivotTableChart.tsx | 2 + .../src/plugin/buildQuery.ts | 27 +-- .../src/plugin/controlPanel.tsx | 35 +++- .../src/plugin/resultAggregation.ts | 65 +++++++ .../src/plugin/transformProps.ts | 35 +++- .../src/react-pivottable/utilities.ts | 189 ++++++++++++++++++--- .../plugins/plugin-chart-pivot-table/src/types.ts | 1 + .../test/plugin/buildQuery.test.ts | 21 +++ .../test/plugin/transformProps.test.ts | 32 ++++ .../test/react-pivottable/utilities.test.ts | 55 ++++++ 10 files changed, 417 insertions(+), 45 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx b/superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx index f4b91259a42..cad03cb4b83 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx +++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx @@ -250,6 +250,7 @@ export default function PivotTableChart(props: PivotTableProps) { currencyFormats, metricsLayout, showValuesAs, + aggregateFunction, metricColorFormatters, dateFormatters, onContextMenu, @@ -756,6 +757,7 @@ export default function PivotTableChart(props: PivotTableProps) { onContextMenu={handleContextMenu} allowRenderHtml={allowRenderHtml} showValuesAs={showValuesAs} + aggregateFunction={aggregateFunction} /> </PivotTableWrapper> </Styles> diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/buildQuery.ts b/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/buildQuery.ts index ff8caaa98a2..ba60bebbe11 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/buildQuery.ts +++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/buildQuery.ts @@ -27,6 +27,7 @@ import { TimeGranularity, } from '@superset-ui/core'; import { Groupby, PivotTableQueryFormData } from '../types'; +import { getResultAggregation } from './resultAggregation'; import buildGroupbyCombinations, { allMetricsAdditive } from './utilities'; // Build the query `columns` for a single rollup level (one prefix of row dims @@ -86,17 +87,23 @@ export default function buildQuery(formData: PivotTableQueryFormData) { // per displayed total/subtotal) so the database computes every level; the // backend falls back to per-level queries on engines without native // support. transformProps splits the combined result by level. + // - result aggregation (see resultAggregation.ts): always a full-detail + // query, regardless of additivity -- every scope reduces its own + // original leaf records client-side, so there is nothing for the + // database to roll up in advance. const additive = allMetricsAdditive(ensureIsArray(formData.metrics)); - const groupingSets = additive - ? undefined - : buildGroupbyCombinations(formData).map(level => - // A dimension placed on both axes (a valid, if unusual, config) would - // otherwise appear twice in the same level, producing a duplicate - // column in the GROUPING SETS tuple sent to the database. - Array.from( - new Set([...level.rows, ...level.columns].map(getColumnLabel)), - ), - ); + const resultAggregation = getResultAggregation(formData.aggregateFunction); + const groupingSets = + additive || resultAggregation + ? undefined + : buildGroupbyCombinations(formData).map(level => + // A dimension placed on both axes (a valid, if unusual, config) would + // otherwise appear twice in the same level, producing a duplicate + // column in the GROUPING SETS tuple sent to the database. + Array.from( + new Set([...level.rows, ...level.columns].map(getColumnLabel)), + ), + ); return buildQueryContext(formData, baseQueryObject => { const { series_limit_metric, metrics, order_desc } = baseQueryObject; diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx b/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx index 4107466f80d..a46e4f58191 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx +++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx @@ -34,6 +34,7 @@ import { QueryFormColumn, } from '@superset-ui/core'; import { MetricsLayoutEnum, ShowValuesAsEnum } from '../types'; +import { RESULT_AGGREGATIONS } from './resultAggregation'; const config: ControlPanelConfig = { controlPanelSections: [ @@ -173,7 +174,10 @@ const config: ControlPanelConfig = { name: 'rowTotals', config: { type: 'CheckboxControl', - label: t('Show rows total'), + // The displayed value may be a result aggregation (Median, + // Average, ...) rather than a plain total once `aggregateFunction` + // is set below, so "summary" rather than "total". + label: () => t('Show row summaries'), default: false, renderTrigger: true, description: t('Display row level total'), @@ -197,7 +201,7 @@ const config: ControlPanelConfig = { name: 'colTotals', config: { type: 'CheckboxControl', - label: t('Show columns total'), + label: () => t('Show column summaries'), default: false, renderTrigger: true, description: t('Display column level total'), @@ -243,6 +247,33 @@ const config: ControlPanelConfig = { }, }, ], + [ + { + name: 'aggregateFunction', + config: { + type: 'SelectControl', + label: () => t('Aggregation function'), + default: 'Metric', + clearable: false, + // Not a renderTrigger: switching in or out of a result + // aggregation changes whether the query uses GROUPING SETS at + // all (see buildQuery.ts), so it needs a real requery. + choices: [ + ['Metric', t('Use metric definition')], + ...RESULT_AGGREGATIONS.map( + name => [name, t(name)] as [string, string], + ), + ], + description: t( + 'Use each metric’s own definition for cells and ' + + 'summaries (the default since SIP-216), or aggregate a ' + + 'second time over the metric’s own grouped results — ' + + 'e.g. the median of a set of per-store averages. Query ' + + 'limits apply before a result aggregation runs.', + ), + }, + }, + ], [ { name: 'showValuesAs', diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/resultAggregation.ts b/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/resultAggregation.ts new file mode 100644 index 00000000000..54d59fecf62 --- /dev/null +++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/resultAggregation.ts @@ -0,0 +1,65 @@ +/** + * 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. + */ + +/** + * 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', + 'Count Unique Values', + 'List Unique Values', + 'Sum', + 'Average', + 'Median', + 'Sample Variance', + 'Sample Standard Deviation', + 'Minimum', + 'Maximum', + 'First', + 'Last', + 'Sum as Fraction of Total', + 'Sum as Fraction of Rows', + 'Sum as Fraction of Columns', + 'Count as Fraction of Total', + 'Count as Fraction of Rows', + 'Count as Fraction of Columns', +] as const; + +export type ResultAggregation = (typeof RESULT_AGGREGATIONS)[number]; + +/** + * Absence, an unrecognized value, or the explicit `'Metric'` choice all mean + * the same thing: keep today's database-computed, metric-definition totals. + */ +export function getResultAggregation( + value: unknown, +): ResultAggregation | undefined { + return RESULT_AGGREGATIONS.find(name => name === value); +} diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts b/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts index f176ce64e21..d4f6f3083f4 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts @@ -36,6 +36,7 @@ import { getColorFormatters, } from '@superset-ui/chart-controls'; import { DateFormatter, PivotTableQueryFormData, QueryData } from '../types'; +import { getResultAggregation } from './resultAggregation'; import buildGroupbyCombinations, { additiveReducerFor, allMetricsAdditive, @@ -102,9 +103,8 @@ export default function transformProps(chartProps: ChartProps<QueryFormData>) { emitCrossFilters, theme, } = chartProps; - const groupbyCombinations = buildGroupbyCombinations( - formData as PivotTableQueryFormData, - ); + const pivotFormData = formData as PivotTableQueryFormData; + const groupbyCombinations = buildGroupbyCombinations(pivotFormData); const metricsArr = ensureIsArray(formData.metrics); let data: QueryData[]; // The rows that conditional formatting derives its color scale from. Only the @@ -112,7 +112,30 @@ export default function transformProps(chartProps: ChartProps<QueryFormData>) { // very cells being shaded, so letting them in makes the grand total the max // and leaves every detail cell nearly unshaded. let colorScaleRows: DataRecord[]; - if (allMetricsAdditive(metricsArr)) { + const resultAggregation = getResultAggregation( + pivotFormData.aggregateFunction, + ); + if (resultAggregation) { + // Result aggregation (see resultAggregation.ts): the query is always + // full-detail, and every scope -- cell, subtotal, grand total -- reduces + // its own original contributing leaf records inside PivotData + // (`processResultRecord`), not a level synthesized here. Pass the leaf + // rows through as a single, display-oriented groupby; PivotData derives + // every rollup depth from `rows`/`cols` itself. + const [rows, columns] = pivotFormData.transposePivot + ? [pivotFormData.groupbyColumns, pivotFormData.groupbyRows] + : [pivotFormData.groupbyRows, pivotFormData.groupbyColumns]; + colorScaleRows = queriesData[0].data; + data = [ + { + data: colorScaleRows, + groupby: { + rows: ensureIsArray(rows), + columns: ensureIsArray(columns), + }, + }, + ]; + } else if (allMetricsAdditive(metricsArr)) { // Additive fast-path: a single full-detail query was issued; synthesize // each rollup level by reducing the leaf rows on the client (see SIP.md). const leafRows = queriesData[0].data; @@ -188,11 +211,12 @@ export default function transformProps(chartProps: ChartProps<QueryFormData>) { dateFormat, metricsLayout, showValuesAs, + aggregateFunction, conditionalFormatting, timeGrainSqla, currencyFormat, allowRenderHtml, - } = formData; + } = pivotFormData; const { selectedFilters } = filterState; const granularity = extractTimegrain(rawFormData); @@ -274,6 +298,7 @@ export default function transformProps(chartProps: ChartProps<QueryFormData>) { currencyFormats, metricsLayout, showValuesAs, + aggregateFunction, metricColorFormatters, dateFormatters, onContextMenu, diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts b/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts index 4d351b625c3..7b30d8acc77 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts +++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts @@ -20,6 +20,8 @@ import PropTypes from 'prop-types'; import { t } from '@apache-superset/core/translation'; +import { getResultAggregation } from '../plugin/resultAggregation'; + type SortFunction = ( a: string | number | null, b: string | number | null, @@ -383,6 +385,40 @@ const fmtNonString = (x: string | number | null): string => typeof x === 'string' ? x : formatter(x as number); +/* + * Tracks which metric (via the `__metricKey`/`__rows`/`__columns` tagging + * every record carries, see PivotTableChart.tsx) a series of pushed records + * belong to. An aggregator slot opposite the Metric pseudo-dimension (the + * Total axis/corner when there's more than one metric -- see `processRecord`'s + * "Metric-collapse totals" and `processResultRecord` below) can receive one + * record per metric, e.g. MAX(sales) and MEDIAN(msrp) both landing in the + * same grand-total cell. There's no single number that means anything for + * "max of sales combined with median of msrp" regardless of which metric an + * aggregator would otherwise favor, so every aggregator that can land in one + * of those shared slots calls `sawMixedMetric` on every push and renders + * blank once it returns true, rather than silently guessing. + */ +function makeMixedMetricTracker(): { + sawMixedMetric: (record: PivotRecord) => boolean; +} { + let seenMetric: string | undefined; + let mixedMetrics = false; + return { + sawMixedMetric(record: PivotRecord): boolean { + const metricDim = record.__metricKey as unknown as string | undefined; + if (metricDim) { + const metric = String(record[metricDim]); + if (seenMetric === undefined) { + seenMetric = metric; + } else if (metric !== seenMetric) { + mixedMetrics = true; + } + } + return mixedMetrics; + }, + }; +} + /* * Passthrough "aggregator" for the rollup pivot. Because the database already * computed every rollup level (via a single GROUPING SETS query where the @@ -390,35 +426,19 @@ const fmtNonString = * cell receives exactly one record per metric, whose value we store verbatim * rather than re-aggregating. This is what makes non-additive totals correct. * See SIP.md. Currency tracking mirrors the real aggregators for AUTO-mode - * detection. - * - * The "exactly one record per metric" invariant doesn't hold for the Total - * axis/corner opposite the Metric pseudo-dimension when there's more than one - * metric (see `processRecord`'s "Metric-collapse totals"): that slot receives - * one record per metric, e.g. MAX(sales) and MEDIAN(msrp) both landing in the - * same grand-total cell. There's no single number that means anything for - * "max of sales combined with median of msrp", so once a second, different - * metric is pushed into the same cell, `value()` renders blank instead of - * silently keeping whichever metric happened to be pushed last. + * detection. See `makeMixedMetricTracker` above for the one exception (the + * Total axis/corner opposite the Metric pseudo-dimension). */ const cellValue = (formatter: Formatter = usFmt) => ([attr]: string[]) => () => ({ val: null as string | number | null, - seenMetric: undefined as string | undefined, + mixedMetricTracker: makeMixedMetricTracker(), mixedMetrics: false, currencySet: new Set<string>(), push(record: PivotRecord) { - const metricDim = record.__metricKey as unknown as string | undefined; - if (metricDim) { - const metric = String(record[metricDim]); - if (this.seenMetric === undefined) { - this.seenMetric = metric; - } else if (metric !== this.seenMetric) { - this.mixedMetrics = true; - } - } + this.mixedMetrics = this.mixedMetricTracker.sawMixedMetric(record); this.val = record[attr] as string | number | null; if ( record.__currencyColumn && @@ -1073,6 +1093,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 ( + v: string[], + ) => (...a: unknown[]) => Aggregator; + const inner = build(vals)(...args); + const innerValue = inner.value.bind(inner); + const innerPush = inner.push.bind(inner); + const tracker = makeMixedMetricTracker(); + let mixed = false; + return { + ...inner, + push(record: PivotRecord) { + mixed = tracker.sawMixedMetric(record); + innerPush(record); + }, + value() { + return mixed ? null : innerValue(); + }, + }; + } + : undefined; // Values come pre-aggregated from the database (one query per rollup level), // so the pivot stores them verbatim via `cellValue` instead of aggregating. // When "Show values as" a fraction is active, wrap that passthrough with @@ -1085,17 +1140,21 @@ class PivotData { // themselves to read 100%. This needs no new query and no per-metric // aggregator-override control (that control is gone, see SIP.md); it's a // pure display transform over values that are already DB-correct. - this.aggregator = fractionType - ? aggregatorTemplates.fractionOf( - cellValue(), - fractionType, - usFmtPct, - )(vals) - : cellValue(this.props.defaultFormatter as Formatter)(vals); + this.aggregator = + resultFactory ?? + (fractionType + ? aggregatorTemplates.fractionOf( + cellValue(), + fractionType, + usFmtPct, + )(vals) + : cellValue(this.props.defaultFormatter as Formatter)(vals)); // Percentage display always uses a fixed percent format -- a per-metric // custom formatter (currency, decimals, etc.) doesn't apply to a ratio. + // A result aggregation supplies its own formatting (and, for the " as + // Fraction of " choices, its own percentage) via `resultFactory` above. this.formattedAggregators = - !fractionType && this.props.customFormatters + !fractionType && !resultFactory && this.props.customFormatters ? Object.entries( this.props.customFormatters as Record< string, @@ -1243,7 +1302,81 @@ 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. + 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), + ]; + rowDepths.forEach(ri => + colDepths.forEach(ci => { + if (ri === 0 && ci === 0) { + this.allTotal.push(record); + return; + } + const r = rowKey.slice(0, ri); + const c = colKey.slice(0, ci); + const rk = flatKey(r); + const ck = flatKey(c); + let target: Record<string, Aggregator>; + let key: string; + if (ci === 0) { + target = this.rowTotals; + key = rk; + if (!target[key]) this.rowKeys.push(r); + } else if (ri === 0) { + target = this.colTotals; + key = ck; + if (!target[key]) this.colKeys.push(c); + } else { + this.tree[rk] ??= {}; + target = this.tree[rk]; + key = ck; + } + target[key] ??= this.getFormattedAggregator( + record, + ci === 0 ? r : ri === 0 ? c : undefined, + )(this, r, c); + target[key].push(record); + target[key].isRowSubtotal = ri > 0 && ri < rows.length; + target[key].isColSubtotal = ci > 0 && ci < cols.length; + target[key].isSubtotal = + target[key].isRowSubtotal || target[key].isColSubtotal; + }), + ); + } + processRecord(record: PivotRecord): void { + if (getResultAggregation(this.props.aggregateFunction)) { + this.processResultRecord(record); + return; + } // this code is called in a tight loop. // Each record is tagged (in PivotTableChart) with `__rows`/`__columns`: // the dimension labels of the rollup level that produced it. The database diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/src/types.ts b/superset-frontend/plugins/plugin-chart-pivot-table/src/types.ts index 87cc8f95cba..2278eac2907 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/src/types.ts +++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/types.ts @@ -112,6 +112,7 @@ interface PivotTableCustomizeProps { currencyFormats: Record<string, Currency>; metricsLayout?: MetricsLayoutEnum; showValuesAs?: ShowValuesAsEnum; + aggregateFunction?: string; metricColorFormatters: ColorFormatters; dateFormatters: Record<string, DateFormatter | undefined>; legacy_order_by: QueryFormMetric[] | QueryFormMetric | null; diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/buildQuery.test.ts b/superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/buildQuery.test.ts index bbc58996048..f8212e15392 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/buildQuery.test.ts +++ b/superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/buildQuery.test.ts @@ -178,3 +178,24 @@ test('should not omit extras.time_grain_sqla from queryContext so dashboards app const query = queryContext.queries[queryContext.queries.length - 1]; expect(query.extras?.time_grain_sqla).toEqual(TimeGranularity.QUARTER); }); + +test.each(['Average', 'Median', 'Count Unique Values'])( + 'result aggregation (%s) queries leaf detail without database rollups', + aggregateFunction => { + const queryContext = buildQuery({ ...formData, aggregateFunction }); + const query = queryContext.queries[queryContext.queries.length - 1]; + expect(query).not.toHaveProperty('grouping_sets'); + }, +); + +test('an unrecognized aggregateFunction value falls back to database rollups', () => { + // Covers both "Metric" (the control's explicit default) and any other + // value that isn't one of RESULT_AGGREGATIONS -- e.g. a stale form_data + // value from a chart saved against an older version of the control. + const queryContext = buildQuery({ + ...formData, + aggregateFunction: 'Metric', + }); + const query = queryContext.queries[queryContext.queries.length - 1]; + expect(query).toHaveProperty('grouping_sets'); +}); diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/transformProps.test.ts b/superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/transformProps.test.ts index 9adbdb4f06a..6165f38f546 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/transformProps.test.ts +++ b/superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/transformProps.test.ts @@ -601,3 +601,35 @@ test('conditional formatting on the additive path uses the raw leaf query rows', // Scale spans the leaf cells (max 40), never the client-side grand total 100. expect(getColorFromValue(40)).toEqual('#ACE1C4FF'); }); + +test('result aggregation passes the leaf query data through as a single, unsynthesized level', () => { + const resultChartProps = new ChartProps<QueryFormData>({ + ...chartProps, + formData: { ...formData, aggregateFunction: 'Median' }, + queriesData: [ + { + data: [ + { row1: 'a', row2: 'x', col1: 'p', col2: 'q', metric1: 10 }, + { row1: 'a', row2: 'y', col1: 'p', col2: 'q', metric1: 20 }, + ], + colnames: ['row1', 'row2', 'col1', 'col2', 'metric1'], + coltypes: [1, 1, 1, 1, 0], + }, + ], + }); + + const result = transformProps(resultChartProps) as ReturnType< + typeof transformProps + >; + // Passed straight through to PivotTableChart/PivotData, which does the + // actual per-scope reduction (see react-pivottable/utilities.test.ts). + expect(result.aggregateFunction).toBe('Median'); + expect(result.data).toHaveLength(1); + expect(result.data[0].data).toEqual(resultChartProps.queriesData[0].data); + // transposePivot is true in the shared fixture: rows/columns swap, same + // as buildQuery.ts's own fullGroupby. + expect(result.data[0].groupby).toEqual({ + rows: formData.groupbyColumns, + columns: formData.groupbyRows, + }); +}); diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/utilities.test.ts b/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/utilities.test.ts index 908f0b13e61..05bf894b307 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/utilities.test.ts +++ b/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/utilities.test.ts @@ -97,3 +97,58 @@ test('grand total still passes through the value for a single metric', () => { expect(pivotData.getAggregator([], []).value()).toBe(100); }); + +// Leaf records shaped like a real query result: one row per full dimension +// combination (region, store), each already carrying the metric's own +// aggregate for that group -- never raw, ungrouped source rows. +const RESULT_AGGREGATION_LEAVES: PivotRecord[] = [ + { region: 'North', store: 'A', value: 10 }, + { region: 'North', store: 'B', value: 20 }, + { region: 'South', store: 'C', value: 100 }, +] as unknown as PivotRecord[]; + +test('result aggregation reduces the grand summary from every original leaf record, not from subtotals', () => { + const pivotData = new PivotData( + { + data: RESULT_AGGREGATION_LEAVES, + rows: ['region', 'store'], + cols: [], + vals: ['value'], + aggregateFunction: 'Average', + }, + { rowEnabled: true }, + ); + + // North subtotal: average of North's own two leaves (10, 20). + expect(pivotData.getAggregator(['North'], []).value()).toBe(15); + // Grand summary: average of all three leaves (10, 20, 100) = 43.33 -- + // not the average of the two region subtotals ((15 + 100) / 2 = 57.5), + // which is exactly the pre-SIP-216 bug this restores without repeating. + expect(pivotData.getAggregator([], []).value()).toBeCloseTo(43.33, 2); +}); + +test('result aggregation blanks a shared total slot that would mix two different metrics', () => { + const mixedMetricLeaves: PivotRecord[] = [ + { + Metric: 'MAX(sales)', + value: 100, + __metricKey: 'Metric', + }, + { + Metric: 'MEDIAN(msrp)', + value: 50, + __metricKey: 'Metric', + }, + ] as unknown as PivotRecord[]; + const pivotData = new PivotData({ + data: mixedMetricLeaves, + rows: [], + cols: ['Metric'], + vals: ['value'], + aggregateFunction: 'Average', + }); + + // Not the average of a MAX(sales) value and a MEDIAN(msrp) value blended + // together -- there's no single number that means anything for that. + expect(pivotData.getAggregator([], []).value()).toBeNull(); +});
