diff --git a/UPDATING.md b/UPDATING.md index 4e5afaadb5f..5998367c879 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -288,6 +288,22 @@ The pivot table chart's `First` and `Last` aggregations now return the first and The `error` and `response` parameters of the `retryDelay` and `retryOn` callbacks in `FetchRetryOptions` (exported from `@superset-ui/core`) are now typed `Error | null` and `Response | null` to match the actual call-site signature provided by `fetch-retry`. Because these parameter types are contravariant, consumers who typed their callbacks with the non-nullable `(attempt: number, error: Error, response: Response) => number` will get a TypeScript compile error. Widen your callback signatures to accept `Error | null` / `Response | null`. +### Pivot Table totals are now computed by the database (per-metric "Aggregation function" control removed) + +Pivot Table subtotals and grand totals are now computed by the database at each +rollup level instead of re-aggregating the already-aggregated cell values on the +client. This fixes long-standing incorrect totals for non-additive metrics +(ratios such as `SUM(a)/SUM(b)`, `COUNT_DISTINCT`, `AVG`, percentiles, etc.), +which previously summed the displayed cell values. + +As a result the per-table **"Aggregation function"** control (which let you pick +how totals were aggregated client-side, e.g. Sum/Average/Count) has been +removed: totals now always reflect the metric's own definition evaluated at the +total's granularity. For additive metrics (`SUM`/`COUNT`/`MIN`/`MAX`) the result +is unchanged. Saved charts that set `aggregateFunction` will ignore it; no +migration is required. If you previously relied on a plain sum-of-cells total +for a non-additive metric, that specific behavior is no longer available. + ### `thumbnail_url` removed from dashboard list API response The `thumbnail_url` field has been removed from `GET /api/v1/dashboard/` list responses. External consumers relying on this field must now construct the thumbnail URL client-side using `id` and `changed_on_utc`: 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 06fc1fa6f66..4f3b5255130 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx +++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx @@ -42,13 +42,14 @@ import { NumberFormatter, } from '@superset-ui/core'; import { styled, useTheme } from '@apache-superset/core/theme'; -import { aggregatorTemplates, PivotTable, sortAs } from './react-pivottable'; +import { PivotTable, sortAs } from './react-pivottable'; import { DateFormatter, FilterType, MetricsLayoutEnum, PivotTableProps, PivotTableStylesProps, + QueryData, SelectedFiltersType, } from './types'; @@ -175,51 +176,6 @@ const createCurrencyAwareFormatter = ( }; }; -const aggregatorsFactory = (formatter: NumberFormatter) => ({ - Count: aggregatorTemplates.count(formatter), - 'Count Unique Values': aggregatorTemplates.countUnique(formatter), - 'List Unique Values': aggregatorTemplates.listUnique(', ', formatter), - Sum: aggregatorTemplates.sum(formatter), - Average: aggregatorTemplates.average(formatter), - Median: aggregatorTemplates.median(formatter), - 'Sample Variance': aggregatorTemplates.var(1, formatter), - 'Sample Standard Deviation': aggregatorTemplates.stdev(1, formatter), - Minimum: aggregatorTemplates.min(formatter), - Maximum: aggregatorTemplates.max(formatter), - First: aggregatorTemplates.first(formatter), - Last: aggregatorTemplates.last(formatter), - 'Sum as Fraction of Total': aggregatorTemplates.fractionOf( - aggregatorTemplates.sum(), - 'total', - formatter, - ), - 'Sum as Fraction of Rows': aggregatorTemplates.fractionOf( - aggregatorTemplates.sum(), - 'row', - formatter, - ), - 'Sum as Fraction of Columns': aggregatorTemplates.fractionOf( - aggregatorTemplates.sum(), - 'col', - formatter, - ), - 'Count as Fraction of Total': aggregatorTemplates.fractionOf( - aggregatorTemplates.count(), - 'total', - formatter, - ), - 'Count as Fraction of Rows': aggregatorTemplates.fractionOf( - aggregatorTemplates.count(), - 'row', - formatter, - ), - 'Count as Fraction of Columns': aggregatorTemplates.fractionOf( - aggregatorTemplates.count(), - 'col', - formatter, - ), -}); - const getDrillFilterValue = ( value: string, formatter: DateFormatter | undefined, @@ -274,7 +230,6 @@ export default function PivotTableChart(props: PivotTableProps) { metrics, colOrder, rowOrder, - aggregateFunction, transposePivot, combineMetric, rowSubtotalPosition, @@ -383,22 +338,48 @@ export default function PivotTableChart(props: PivotTableProps) { const unpivotedData = useMemo( () => - data.reduce( - (acc: DataRecord[], record: DataRecord) => [ - ...acc, - ...metricNames - .map((name: string) => ({ - ...record, - [METRIC_KEY]: name, - value: record[name], - // Mark currency column for per-cell currency detection in aggregators - __currencyColumn: currencyCodeColumn, - })) - .filter(record => record.value !== null), - ], - [], - ), - [data, metricNames, currencyCodeColumn], + // `data` is now one entry per rollup level. Tag every record with the + // row/column dimension labels of the level that produced it (mirroring + // the METRIC_KEY injection used for the full rows/cols below) so + // PivotData can slot each pre-computed value without re-aggregating. + // buildGroupbyCombinations already applied transposePivot, so the level's + // groupby is display-oriented and is not transposed again here. + data.flatMap((query: QueryData) => { + let levelRows = query.groupby.rows.map(getColumnLabel); + let levelCols = query.groupby.columns.map(getColumnLabel); + if (metricsLayout === MetricsLayoutEnum.ROWS) { + levelRows = combineMetric + ? [...levelRows, METRIC_KEY] + : [METRIC_KEY, ...levelRows]; + } else { + levelCols = combineMetric + ? [...levelCols, METRIC_KEY] + : [METRIC_KEY, ...levelCols]; + } + // A DB-computed value can legitimately be null (e.g. AVG over an + // empty group, or a 0/0 ratio); keep the record so its row/column key + // still gets placed and renders as a blank cell (see `cellValue`'s + // formatter in react-pivottable/utilities.ts), instead of dropping + // the group from the pivot entirely. + return query.data.flatMap((record: DataRecord) => + metricNames.map((name: string) => ({ + ...record, + [METRIC_KEY]: name, + value: record[name], + // Mark currency column for per-cell currency detection in aggregators + __currencyColumn: currencyCodeColumn, + // The level this record belongs to (used by PivotData placement). + // Namespaced with a `__` prefix (like `__metricKey` below) so it + // can't collide with a real dataset column named `rows`/`columns`. + __rows: levelRows, + __columns: levelCols, + // Identify the metric pseudo-dimension so PivotData can feed the + // metric-collapsed totals (the opposite "Total" axis + corner). + __metricKey: METRIC_KEY, + })), + ); + }), + [data, metricNames, currencyCodeColumn, metricsLayout, combineMetric], ); const groupbyRows = useMemo( () => groupbyRowsRaw.map(getColumnLabel), @@ -762,10 +743,8 @@ export default function PivotTableChart(props: PivotTableProps) { data={unpivotedData} rows={rows} cols={cols} - aggregatorsFactory={aggregatorsFactory} defaultFormatter={defaultFormatter} customFormatters={metricFormatters} - aggregatorName={aggregateFunction} vals={vals} colOrder={colOrder} rowOrder={rowOrder} 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 dd870799757..ff8caaa98a2 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 @@ -20,32 +20,37 @@ import { AdhocColumn, buildQueryContext, ensureIsArray, + getColumnLabel, isPhysicalColumn, QueryFormColumn, QueryFormOrderBy, + TimeGranularity, } from '@superset-ui/core'; -import { PivotTableQueryFormData } from '../types'; - -export default function buildQuery(formData: PivotTableQueryFormData) { - const { groupbyColumns = [], groupbyRows = [], extra_form_data } = formData; - const time_grain_sqla = - extra_form_data?.time_grain_sqla || formData.time_grain_sqla; +import { Groupby, PivotTableQueryFormData } from '../types'; +import buildGroupbyCombinations, { allMetricsAdditive } from './utilities'; +// Build the query `columns` for a single rollup level (one prefix of row dims +// crossed with one prefix of column dims), applying temporal BASE_AXIS handling. +function getQueryColumns( + groupby: Groupby, + formData: PivotTableQueryFormData, + timeGrainSqla: TimeGranularity | undefined, +): QueryFormColumn[] { // TODO: add deduping of AdhocColumns - const columns = Array.from( + return Array.from( new Set([ - ...ensureIsArray(groupbyColumns), - ...ensureIsArray(groupbyRows), + ...ensureIsArray(groupby.rows), + ...ensureIsArray(groupby.columns), ]), ).map(col => { if ( isPhysicalColumn(col) && - time_grain_sqla && + timeGrainSqla && (formData?.temporal_columns_lookup?.[col] || formData.granularity_sqla === col) ) { return { - timeGrain: time_grain_sqla, + timeGrain: timeGrainSqla, columnType: 'BASE_AXIS', sqlExpression: col, label: col, @@ -54,6 +59,44 @@ export default function buildQuery(formData: PivotTableQueryFormData) { } return col; }); +} + +export default function buildQuery(formData: PivotTableQueryFormData) { + const { extra_form_data } = formData; + const time_grain_sqla = + extra_form_data?.time_grain_sqla || formData.time_grain_sqla; + + // The full set of dimensions (the leaf level) is always queried; the rollup + // levels are derived from it. Apply transposePivot here exactly as + // buildGroupbyCombinations does, so the column order matches the rollup + // levels. See SIP.md. + const [fullRows, fullColumnDims] = formData.transposePivot + ? [formData.groupbyColumns, formData.groupbyRows] + : [formData.groupbyRows, formData.groupbyColumns]; + const fullGroupby: Groupby = { + rows: ensureIsArray(fullRows), + columns: ensureIsArray(fullColumnDims), + }; + const fullColumns = getQueryColumns(fullGroupby, formData, time_grain_sqla); + + // A single query always suffices: + // - additive metrics (SUM/COUNT/MIN/MAX): a full-detail query whose + // subtotals/grand totals transformProps derives by client-side reduction. + // - non-additive metrics: a GROUPING SETS query (one `grouping_sets` level + // 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. + 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)), + ), + ); return buildQueryContext(formData, baseQueryObject => { const { series_limit_metric, metrics, order_desc } = baseQueryObject; @@ -67,7 +110,8 @@ export default function buildQuery(formData: PivotTableQueryFormData) { { ...baseQueryObject, orderby: orderBy, - columns, + columns: fullColumns, + ...(groupingSets ? { grouping_sets: groupingSets } : {}), }, ]; }); 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 4e916857b27..6cf9b17dd8c 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 @@ -128,7 +128,12 @@ const config: ControlPanelConfig = { config: { ...sharedControls.row_limit, label: t('Cell limit'), - description: t('Limits the number of cells that get retrieved.'), + description: t( + 'Limits the number of cells that get retrieved. Not applied when ' + + 'non-additive metrics (e.g. ratios, COUNT_DISTINCT, AVG, percentiles) ' + + 'are present, since totals and subtotals are then computed via a ' + + 'database rollup query that must see every row to stay correct.', + ), }, }, ], @@ -163,44 +168,6 @@ const config: ControlPanelConfig = { expanded: true, tabOverride: 'data', controlSetRows: [ - [ - { - name: 'aggregateFunction', - config: { - type: 'SelectControl', - label: t('Aggregation function'), - clearable: false, - choices: [ - ['Count', t('Count')], - ['Count Unique Values', t('Count Unique Values')], - ['List Unique Values', t('List Unique Values')], - ['Sum', t('Sum')], - ['Average', t('Average')], - ['Median', t('Median')], - ['Sample Variance', t('Sample Variance')], - ['Sample Standard Deviation', t('Sample Standard Deviation')], - ['Minimum', t('Minimum')], - ['Maximum', t('Maximum')], - ['First', t('First')], - ['Last', t('Last')], - ['Sum as Fraction of Total', t('Sum as Fraction of Total')], - ['Sum as Fraction of Rows', t('Sum as Fraction of Rows')], - ['Sum as Fraction of Columns', t('Sum as Fraction of Columns')], - ['Count as Fraction of Total', t('Count as Fraction of Total')], - ['Count as Fraction of Rows', t('Count as Fraction of Rows')], - [ - 'Count as Fraction of Columns', - t('Count as Fraction of Columns'), - ], - ], - default: 'Sum', - description: t( - 'Aggregate function to apply when pivoting and computing the total rows and columns', - ), - renderTrigger: true, - }, - }, - ], [ { name: 'rowTotals', 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 1bba188ca05..b354396f2b2 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 @@ -19,7 +19,10 @@ import { ChartProps, DataRecord, + ensureIsArray, extractTimegrain, + getColumnLabel, + getMetricLabel, getTimeFormatter, getTimeFormatterForGranularity, QueryFormData, @@ -28,7 +31,14 @@ import { } from '@superset-ui/core'; import { GenericDataType } from '@apache-superset/core/common'; import { getColorFormatters } from '@superset-ui/chart-controls'; -import { DateFormatter } from '../types'; +import { DateFormatter, PivotTableQueryFormData, QueryData } from '../types'; +import buildGroupbyCombinations, { + additiveReducerFor, + allMetricsAdditive, + RollupReducer, + splitGroupingSetsResult, + synthesizeAdditiveLevels, +} from './utilities'; const { DATABASE_DATETIME } = TimeFormats; @@ -88,12 +98,56 @@ export default function transformProps(chartProps: ChartProps) { emitCrossFilters, theme, } = chartProps; - const { - data, - colnames, - coltypes, - detected_currency: detectedCurrency, - } = queriesData[0]; + const groupbyCombinations = buildGroupbyCombinations( + formData as PivotTableQueryFormData, + ); + const metricsArr = ensureIsArray(formData.metrics); + let data: QueryData[]; + 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; + const metricReducers: Record = {}; + metricsArr.forEach(metric => { + metricReducers[getMetricLabel(metric)] = additiveReducerFor(metric); + }); + const labelLevels = groupbyCombinations.map(combination => ({ + rows: combination.rows.map(getColumnLabel), + columns: combination.columns.map(getColumnLabel), + })); + const synthesized = synthesizeAdditiveLevels( + leafRows, + labelLevels, + metricReducers, + ); + data = groupbyCombinations.map((combination, i) => ({ + data: synthesized[i] as DataRecord[], + groupby: combination, + })); + } else { + // Non-additive: a single GROUPING SETS query returned all rollup levels + // tagged with GROUPING() markers; split the combined result back into one + // frame per level (same order as buildQuery's grouping_sets). See SIP.md. + const levelLabels = groupbyCombinations.map(combination => + [...combination.rows, ...combination.columns].map(getColumnLabel), + ); + const allGroupbyLabels = Array.from(new Set(levelLabels.flat())); + const splitRows = splitGroupingSetsResult( + queriesData[0].data, + levelLabels, + allGroupbyLabels, + ); + data = groupbyCombinations.map((combination, i) => ({ + data: splitRows[i] as DataRecord[], + groupby: combination, + })); + } + // The full-granularity query has the most colnames -- use it for column/type + // metadata and formatters. + const mainQuery = queriesData.reduce((main, query) => + query.colnames.length > main.colnames.length ? query : main, + ); + const { colnames, coltypes, detected_currency: detectedCurrency } = mainQuery; const { groupbyRows, groupbyColumns, @@ -101,7 +155,6 @@ export default function transformProps(chartProps: ChartProps) { tableRenderer, colOrder, rowOrder, - aggregateFunction, transposePivot, combineMetric, rowSubtotalPosition, @@ -136,7 +189,7 @@ export default function transformProps(chartProps: ChartProps) { if (granularity) { // time column use formats based on granularity formatter = getTimeFormatterForGranularity(granularity); - } else if (isNumeric(temporalColname, data)) { + } else if (isNumeric(temporalColname, mainQuery.data)) { formatter = getTimeFormatter(DATABASE_DATETIME); } else { // if no column-specific format, print cell as is @@ -154,7 +207,7 @@ export default function transformProps(chartProps: ChartProps) { ); const metricColorFormatters = getColorFormatters( conditionalFormatting, - data, + mainQuery.data, theme, ); @@ -170,7 +223,6 @@ export default function transformProps(chartProps: ChartProps) { tableRenderer, colOrder, rowOrder, - aggregateFunction, transposePivot, combineMetric, rowSubtotalPosition, diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/utilities.ts b/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/utilities.ts new file mode 100644 index 00000000000..77dbe1b6887 --- /dev/null +++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/utilities.ts @@ -0,0 +1,242 @@ +/** + * 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 { QueryFormColumn, QueryFormMetric } from '@superset-ui/core'; +import { Groupby, MetricsLayoutEnum, PivotTableQueryFormData } from '../types'; + +// Aggregates whose group total can be derived from per-group results +// (decomposable): summing sub-sums, counting sub-counts, min-of-mins, +// max-of-maxes. Everything else (AVG, COUNT_DISTINCT, percentiles, ratios, +// SQL/post-processing metrics) is non-additive and needs DB re-computation at +// each rollup level. See SIP.md. +const ADDITIVE_AGGREGATES = new Set(['SUM', 'COUNT', 'MIN', 'MAX']); + +/** + * Whether a metric's total can be derived additively from per-group results. + * Conservative: only SIMPLE metrics with a known additive aggregate qualify; + * saved-metric references (strings) and SQL/adhoc metrics are treated as + * non-additive because their aggregate cannot be determined from form data. + */ +export function isAdditiveMetric(metric: QueryFormMetric): boolean { + if (typeof metric === 'string' || metric.expressionType !== 'SIMPLE') { + return false; + } + return !!metric.aggregate && ADDITIVE_AGGREGATES.has(metric.aggregate); +} + +/** + * True when every metric is additive, so totals/subtotals can use the cheap + * single-query + client-side summation path instead of one query per rollup + * level. An empty metric list is not considered additive (nothing to optimize). + */ +export function allMetricsAdditive(metrics: QueryFormMetric[]): boolean { + return metrics.length > 0 && metrics.every(isAdditiveMetric); +} + +export type RollupReducer = 'sum' | 'min' | 'max'; + +/** + * How an additive metric's group total is derived from per-group values: + * SUM/COUNT add up, MIN takes the min, MAX takes the max. + */ +export function additiveReducerFor(metric: QueryFormMetric): RollupReducer { + if (typeof metric !== 'string' && metric.expressionType === 'SIMPLE') { + if (metric.aggregate === 'MIN') return 'min'; + if (metric.aggregate === 'MAX') return 'max'; + } + return 'sum'; +} + +function reduceValues(values: number[], reducer: RollupReducer): number { + if (reducer === 'min') return Math.min(...values); + if (reducer === 'max') return Math.max(...values); + return values.reduce((acc, v) => acc + v, 0); +} + +const GROUP_SEP = '\u0000'; + +/** + * Additive fast-path: synthesize each rollup level's rows from the full-detail + * leaf rows by grouping on that level's dimensions and reducing each metric, + * instead of issuing one query per level. Correct only for additive metrics + * (see {@link allMetricsAdditive}). Dimension and metric keys are the data-row + * keys (labels) the caller resolves; `metricReducers` maps each metric key to + * its reducer. Returns one row array per input level, in the same order. + */ +export function synthesizeAdditiveLevels( + leafRows: Record[], + levels: { rows: string[]; columns: string[] }[], + metricReducers: Record, +): Record[][] { + const metricKeys = Object.keys(metricReducers); + return levels.map(level => { + const dimKeys = [...level.rows, ...level.columns]; + const groups = new Map< + string, + { dims: Record; rows: Record[] } + >(); + leafRows.forEach(row => { + const key = dimKeys.map(d => String(row[d])).join(GROUP_SEP); + let group = groups.get(key); + if (!group) { + const dims: Record = {}; + dimKeys.forEach(d => { + dims[d] = row[d]; + }); + group = { dims, rows: [] }; + groups.set(key, group); + } + group.rows.push(row); + }); + return Array.from(groups.values()).map(({ dims, rows }) => { + const out: Record = { ...dims }; + metricKeys.forEach(metricKey => { + const values = rows + .map(r => r[metricKey]) + .filter( + v => v !== null && v !== undefined && !Number.isNaN(Number(v)), + ) + .map(Number); + out[metricKey] = values.length + ? reduceValues(values, metricReducers[metricKey]) + : null; + }); + return out; + }); + }); +} + +// Suffix of the per-column GROUPING() marker emitted by a GROUPING SETS query +// (or its per-level fallback). Must match superset/common/grouping_sets.py. +export const GROUPING_MARKER_SUFFIX = '__superset_grouping'; + +export function groupingMarkerLabel(columnLabel: string): string { + return `${columnLabel}${GROUPING_MARKER_SUFFIX}`; +} + +/** + * Split a combined GROUPING SETS result into one row array per rollup level, by + * matching each row's GROUPING() markers (0 = grouped at this level, 1 = rolled + * up). The marker columns are stripped so each level looks like an ordinary + * per-level query result. Inverse of the backend's grouping_sets query; mirrors + * `split_grouping_sets_result` in superset/common/grouping_sets.py. + */ +export function splitGroupingSetsResult( + rows: Record[], + levels: string[][], + groupbyColumns: string[], +): Record[][] { + const markers = groupbyColumns.map(groupingMarkerLabel); + // A result without GROUPING() markers (e.g. produced by a query that never + // applied the `grouping_sets` extra) contains only full-granularity rows. + // Attribute every row to the leaf level -- the one grouping all columns -- + // and leave the rollup levels empty, so totals cells render blank instead of + // the marker filter discarding every row and blanking the whole chart. + if (rows.length > 0 && !markers.some(marker => marker in rows[0])) { + return levels.map(level => { + const grouped = new Set(level); + return groupbyColumns.every(col => grouped.has(col)) ? rows : []; + }); + } + return levels.map(level => { + const grouped = new Set(level); + return rows + .filter(row => + groupbyColumns.every( + col => + (Number(row[groupingMarkerLabel(col)]) === 0) === grouped.has(col), + ), + ) + .map(row => { + const stripped = { ...row }; + markers.forEach(marker => delete stripped[marker]); + return stripped; + }); + }); +} + +/** + * Enumerate the groupby combinations needed to compute correct subtotals and + * grand totals for non-additive metrics. Each combination is one rollup level: + * a prefix of the row dimensions crossed with a prefix of the column + * dimensions. The empty `{rows: [], columns: []}` combination is the grand + * total. Every level is then queried independently so the database computes the + * metric at that granularity (see SIP.md), rather than re-aggregating cells. + * + * Only the levels actually displayed are emitted (a query-count optimization): + * a level whose prefix is shorter than the full dimension list is a + * total/subtotal that is queried only when the corresponding toggle is on. The + * mapping mirrors TableRenderers exactly (display orientation, post-transpose): + * - rows fully collapsed (`[]`) -> bottom "Total" row -> colTotals + * - columns fully collapsed (`[]`) -> right "Total" column -> rowTotals + * - intermediate row prefix -> row subtotal -> rowSubTotals + * - intermediate column prefix -> column subtotal -> colSubTotals + * A full-length prefix (the leaf level) is always emitted; when a dimension + * list is empty, `[]` *is* the full level and is therefore always kept. + */ +export default function buildGroupbyCombinations( + formData: PivotTableQueryFormData, +): Groupby[] { + let columns: QueryFormColumn[] = formData.groupbyColumns ?? []; + let rows: QueryFormColumn[] = formData.groupbyRows ?? []; + + [rows, columns] = formData.transposePivot ? [columns, rows] : [rows, columns]; + + const rowsCombinations = [ + [] as QueryFormColumn[], + ...rows.map((_, i) => rows.slice(0, i + 1)), + ]; + const colsCombinations = [ + [] as QueryFormColumn[], + ...columns.map((_, i) => columns.slice(0, i + 1)), + ]; + + const rowPrefixNeeded = (prefix: QueryFormColumn[]): boolean => { + if (prefix.length === rows.length) return true; // leaf / full level + if (prefix.length === 0) return !!formData.colTotals; // bottom Total row + return !!formData.rowSubTotals; // row subtotal + }; + const colPrefixNeeded = (prefix: QueryFormColumn[]): boolean => { + if (prefix.length === columns.length) return true; // leaf / full level + if (prefix.length === 0) return !!formData.rowTotals; // right Total column + return !!formData.colSubTotals; // column subtotal + }; + + let groupbyCombinations: Groupby[] = rowsCombinations + .filter(rowPrefixNeeded) + .flatMap(row => + colsCombinations + .filter(colPrefixNeeded) + .map(col => ({ rows: row, columns: col })), + ); + + if (formData.combineMetric) { + if (formData.metricsLayout === MetricsLayoutEnum.ROWS) { + groupbyCombinations = groupbyCombinations.filter( + combination => combination.rows.length === rows.length, + ); + } else { + groupbyCombinations = groupbyCombinations.filter( + combination => combination.columns.length === columns.length, + ); + } + } + + return groupbyCombinations; +} diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx b/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx index bbf8dbf20bf..d296ba3a76b 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx +++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx @@ -95,7 +95,6 @@ interface SubtotalOptions { interface TableRendererProps { cols: string[]; rows: string[]; - aggregatorName: string; tableOptions?: TableOptions; subtotalOptions?: SubtotalOptions; namesMapping?: Record; @@ -340,7 +339,6 @@ export function TableRenderer(props: TableRendererProps) { const { cols, rows, - aggregatorName, tableOptions = {}, subtotalOptions, namesMapping: namesMappingProp, @@ -762,7 +760,6 @@ export function TableRenderer(props: TableRendererProps) { }, [ cols, rows, - aggregatorName, tableOptions, subtotalOptions, namesMappingProp, @@ -1109,9 +1106,7 @@ export function TableRenderer(props: TableRendererProps) { true, )} > - {t('Total (%(aggregatorName)s)', { - aggregatorName: t(aggregatorName), - })} + {t('Total')} ) : null; @@ -1126,7 +1121,6 @@ export function TableRenderer(props: TableRendererProps) { toggleColKey, clickHeaderHandler, cols, - aggregatorName, activeSortColumn, sortingOrder, collapsedCols, @@ -1193,11 +1187,7 @@ export function TableRenderer(props: TableRendererProps) { true, )} > - {settingsColAttrs.length === 0 - ? t('Total (%(aggregatorName)s)', { - aggregatorName: t(aggregatorName), - }) - : null} + {settingsColAttrs.length === 0 ? t('Total') : null} ); @@ -1208,7 +1198,6 @@ export function TableRenderer(props: TableRendererProps) { clickHeaderHandler, rows, tableOptions.clickRowHeaderCallback, - aggregatorName, ], ); @@ -1464,9 +1453,7 @@ export function TableRenderer(props: TableRendererProps) { true, )} > - {t('Total (%(aggregatorName)s)', { - aggregatorName: t(aggregatorName), - })} + {t('Total')} ); @@ -1518,7 +1505,6 @@ export function TableRenderer(props: TableRendererProps) { clickHeaderHandler, rows, tableOptions.clickRowHeaderCallback, - aggregatorName, onContextMenu, allowRenderHtml, ], 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 c845b24611a..f96e403653a 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 @@ -383,6 +383,44 @@ const fmtNonString = (x: string | number | null): string => typeof x === 'string' ? x : formatter(x as number); +/* + * Passthrough "aggregator" for the rollup pivot. Because the database already + * computed every rollup level (via a single GROUPING SETS query where the + * engine supports it, or one query per level as a fallback otherwise), each + * 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. + */ +const cellValue = + (formatter: Formatter = usFmt) => + ([attr]: string[]) => + () => ({ + val: null as string | number | null, + currencySet: new Set(), + push(record: PivotRecord) { + this.val = record[attr] as string | number | null; + if ( + record.__currencyColumn && + record[record.__currencyColumn as string] + ) { + this.currencySet.add(String(record[record.__currencyColumn as string])); + } + }, + value() { + return this.val; + }, + getCurrencies() { + return Array.from(this.currencySet); + }, + // A DB-computed rollup value can legitimately be null (e.g. AVG over an + // empty group, or a 0/0 ratio). Render it as a blank cell instead of the + // literal "null" string that the shared formatter would otherwise produce. + format: (x: string | number | null) => + x === null ? '' : fmtNonString(formatter)(x), + numInputs: typeof attr !== 'undefined' ? 0 : 1, + }); + /* * Aggregators track currencies via push() and expose them via getCurrencies() * for per-cell currency detection in AUTO mode. @@ -930,14 +968,10 @@ class PivotData { 'PivotData', ); - const aggregatorsFactory = this.props.aggregatorsFactory as ( - fmt: unknown, - ) => Record (...args: unknown[]) => Aggregator>; - const aggregatorName = this.props.aggregatorName as string; const vals = this.props.vals as string[]; - this.aggregator = aggregatorsFactory(this.props.defaultFormatter)[ - aggregatorName - ](vals); + // Values come pre-aggregated from the database (one query per rollup level), + // so the pivot stores them verbatim via `cellValue` instead of aggregating. + this.aggregator = cellValue(this.props.defaultFormatter as Formatter)(vals); this.formattedAggregators = this.props.customFormatters ? Object.entries( this.props.customFormatters as Record< @@ -954,8 +988,7 @@ class PivotData { ) => { acc[key] = {}; Object.entries(columnFormatter).forEach(([column, formatter]) => { - acc[key][column] = - aggregatorsFactory(formatter)[aggregatorName](vals); + acc[key][column] = cellValue(formatter as Formatter)(vals); }); return acc; }, @@ -1088,76 +1121,108 @@ class PivotData { } processRecord(record: PivotRecord): void { - // this code is called in a tight loop + // 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 + // has already aggregated that level, so we place the value into exactly + // one slot and store it verbatim (no re-aggregation). A key shorter than + // the full dimension list identifies a subtotal level. Records without + // tags (e.g. direct unit-test construction) fall back to the full + // dimension lists, i.e. they are treated as leaf rows. The `__` prefix + // keeps these rollup tags from colliding with a real dataset column + // named `rows`/`columns`. + const recordRows = (record.__rows as unknown as string[]) ?? null; + const recordColumns = (record.__columns as unknown as string[]) ?? null; + const levelRows = recordRows ?? (this.props.rows as string[]); + const levelColumns = recordColumns ?? (this.props.cols as string[]); + const colKey: string[] = []; const rowKey: string[] = []; - (this.props.cols as string[]).forEach((col: string) => { + levelColumns.forEach((col: string) => { colKey.push(col in record ? String(record[col]) : 'null'); }); - (this.props.rows as string[]).forEach((row: string) => { + levelRows.forEach((row: string) => { rowKey.push(row in record ? String(record[row]) : 'null'); }); - this.allTotal.push(record); + const flatRowKey = flatKey(rowKey); + const flatColKey = flatKey(colKey); - const rowStart = this.subtotals.rowEnabled ? 1 : Math.max(1, rowKey.length); - const colStart = this.subtotals.colEnabled ? 1 : Math.max(1, colKey.length); + const isColSubtotal = colKey.length < (this.props.cols as string[]).length; + const isRowSubtotal = rowKey.length < (this.props.rows as string[]).length; - let isRowSubtotal; - let isColSubtotal; - for (let ri = rowStart; ri <= rowKey.length; ri += 1) { - isRowSubtotal = ri < rowKey.length; - const fRowKey = rowKey.slice(0, ri); - const flatRowKey = flatKey(fRowKey); - if (!this.rowTotals[flatRowKey]) { - this.rowKeys.push(fRowKey); - this.rowTotals[flatRowKey] = this.getFormattedAggregator( - record, - rowKey, - )(this, fRowKey, []); + // Register/create the column-total slot the first time this colKey is seen. + if (colKey.length > 0 && !this.colTotals[flatColKey]) { + if (!isColSubtotal || this.subtotals.colEnabled) { + this.colKeys.push(colKey); } - this.rowTotals[flatRowKey].push(record); - this.rowTotals[flatRowKey].isSubtotal = isRowSubtotal; + this.colTotals[flatColKey] = this.getFormattedAggregator(record, colKey)( + this, + [], + colKey, + ); } - - for (let ci = colStart; ci <= colKey.length; ci += 1) { - isColSubtotal = ci < colKey.length; - const fColKey = colKey.slice(0, ci); - const flatColKey = flatKey(fColKey); - if (!this.colTotals[flatColKey]) { - this.colKeys.push(fColKey); - this.colTotals[flatColKey] = this.getFormattedAggregator( - record, - colKey, - )(this, [], fColKey); + // Register/create the row-total slot the first time this rowKey is seen. + if (rowKey.length > 0 && !this.rowTotals[flatRowKey]) { + if (!isRowSubtotal || this.subtotals.rowEnabled) { + this.rowKeys.push(rowKey); } - this.colTotals[flatColKey].push(record); - this.colTotals[flatColKey].isSubtotal = isColSubtotal; + this.rowTotals[flatRowKey] = this.getFormattedAggregator(record, rowKey)( + this, + rowKey, + [], + ); } - - // And now fill in for all the sub-cells. - for (let ri = rowStart; ri <= rowKey.length; ri += 1) { - isRowSubtotal = ri < rowKey.length; - const fRowKey = rowKey.slice(0, ri); - const flatRowKey = flatKey(fRowKey); + // Create the body-cell slot. + if (rowKey.length > 0 && colKey.length > 0) { if (!this.tree[flatRowKey]) { this.tree[flatRowKey] = {}; } - for (let ci = colStart; ci <= colKey.length; ci += 1) { - isColSubtotal = ci < colKey.length; - const fColKey = colKey.slice(0, ci); - const flatColKey = flatKey(fColKey); - if (!this.tree[flatRowKey][flatColKey]) { - this.tree[flatRowKey][flatColKey] = this.getFormattedAggregator( - record, - )(this, fRowKey, fColKey); - } - this.tree[flatRowKey][flatColKey].push(record); + if (!this.tree[flatRowKey][flatColKey]) { + this.tree[flatRowKey][flatColKey] = this.getFormattedAggregator(record)( + this, + rowKey, + colKey, + ); + } + } - this.tree[flatRowKey][flatColKey].isRowSubtotal = isRowSubtotal; - this.tree[flatRowKey][flatColKey].isColSubtotal = isColSubtotal; - this.tree[flatRowKey][flatColKey].isSubtotal = - isRowSubtotal || isColSubtotal; + // Place the value in exactly one slot, determined by the level. + if (rowKey.length === 0 && colKey.length === 0) { + this.allTotal.push(record); + } else if (rowKey.length === 0) { + this.colTotals[flatColKey].push(record); + this.colTotals[flatColKey].isSubtotal = isColSubtotal; + } else if (colKey.length === 0) { + this.rowTotals[flatRowKey].push(record); + this.rowTotals[flatRowKey].isSubtotal = isRowSubtotal; + } else { + this.tree[flatRowKey][flatColKey].push(record); + this.tree[flatRowKey][flatColKey].isRowSubtotal = isRowSubtotal; + this.tree[flatRowKey][flatColKey].isColSubtotal = isColSubtotal; + this.tree[flatRowKey][flatColKey].isSubtotal = + isRowSubtotal || isColSubtotal; + } + + // Metric-collapse totals. The metric is a pseudo-dimension always present on + // one axis, so no rollup level produces an empty key on that axis -- which + // would leave the opposite "Total" axis and the grand-total corner empty. + // When a record's axis holds only the metric (no real dims there), its value + // is also the collapsed total for that axis, so mirror it into rowTotals / + // colTotals / allTotal. (For a single metric this equals the metric column; + // for multiple metrics it is the last metric -- a cross-metric total is not + // well defined and is left as future work.) + const metricKey = record.__metricKey as unknown as string | undefined; + if (metricKey) { + const realColCount = levelColumns.filter(c => c !== metricKey).length; + const realRowCount = levelRows.filter(r => r !== metricKey).length; + if (levelColumns.includes(metricKey) && realColCount === 0) { + if (rowKey.length === 0) this.allTotal.push(record); + else this.rowTotals[flatRowKey]?.push(record); + } + if (levelRows.includes(metricKey) && realRowCount === 0) { + if (colKey.length === 0) this.allTotal.push(record); + else this.colTotals[flatColKey]?.push(record); } } } @@ -1201,11 +1266,9 @@ PivotData.forEachRecord = function ( }; PivotData.defaultProps = { - aggregators, cols: [], rows: [], vals: [], - aggregatorName: 'Count', sorters: {}, rowOrder: 'key_a_to_z', colOrder: 'key_a_to_z', @@ -1214,7 +1277,6 @@ PivotData.defaultProps = { PivotData.propTypes = { data: PropTypes.oneOfType([PropTypes.array, PropTypes.object, PropTypes.func]) .isRequired, - aggregatorName: PropTypes.string, cols: PropTypes.arrayOf(PropTypes.string), rows: PropTypes.arrayOf(PropTypes.string), vals: PropTypes.arrayOf(PropTypes.string), 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 a13e52cd85f..b6c2bbd75fe 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/src/types.ts +++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/types.ts @@ -48,6 +48,25 @@ export enum MetricsLayoutEnum { COLUMNS = 'COLUMNS', } +/** + * One rollup level for non-additive totals: a prefix of the row dimensions and + * a prefix of the column dimensions. See `plugin/utilities.ts`. + */ +export interface Groupby { + rows: QueryFormColumn[]; + columns: QueryFormColumn[]; +} + +/** + * The result of one rollup-level query: the rows the database returned for that + * level, tagged with the level (`groupby`) they belong to so the pivot can slot + * each pre-computed value into the right cell/subtotal/total. + */ +export interface QueryData { + data: DataRecord[]; + groupby: Groupby; +} + interface PivotTableCustomizeProps { groupbyRows: QueryFormColumn[]; groupbyColumns: QueryFormColumn[]; @@ -55,7 +74,6 @@ interface PivotTableCustomizeProps { tableRenderer: string; colOrder: string; rowOrder: string; - aggregateFunction: string; transposePivot: boolean; combineMetric: boolean; rowSubtotalPosition: boolean; @@ -96,5 +114,5 @@ export type PivotTableQueryFormData = QueryFormData & export type PivotTableProps = PivotTableStylesProps & PivotTableCustomizeProps & { - data: DataRecord[]; + data: QueryData[]; }; diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/test/PivotTableChart.test.tsx b/superset-frontend/plugins/plugin-chart-pivot-table/test/PivotTableChart.test.tsx index 1e9365d4d15..b8b8b5d4dd2 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/test/PivotTableChart.test.tsx +++ b/superset-frontend/plugins/plugin-chart-pivot-table/test/PivotTableChart.test.tsx @@ -26,12 +26,26 @@ import { type QueryFormColumn, } from '@superset-ui/core'; import PivotTableChart from '../src/PivotTableChart'; -import { MetricsLayoutEnum, type PivotTableProps } from '../src/types'; +import { + MetricsLayoutEnum, + type PivotTableProps, + type QueryData, +} from '../src/types'; function renderWithTheme(ui: ReactElement) { return render({ui}); } +// Wrap a flat array of raw rows as the single full-detail rollup level +// (`data` is now one QueryData entry per rollup level; these tests only ever +// exercise the leaf level, so `groupby` mirrors `groupbyRows`/`groupbyColumns`). +function toQueryData( + rows: Record[], + groupby: QueryData['groupby'], +): QueryData[] { + return [{ data: rows as QueryData['data'], groupby }]; +} + function createProps( overrides: Partial = {}, ): PivotTableProps { @@ -46,7 +60,6 @@ function createProps( tableRenderer: 'Table', colOrder: 'key_a_to_z', rowOrder: 'key_a_to_z', - aggregateFunction: 'Count', transposePivot: false, combineMetric: false, rowSubtotalPosition: false, @@ -78,7 +91,10 @@ test('emits numeric temporal values for drill-to-detail filters on formatted row const onContextMenu = jest.fn(); const timestamp = 1778630400000; const props: PivotTableProps = { - data: [{ install_date: String(timestamp), value: 1 }], + data: toQueryData([{ install_date: String(timestamp), value: 1 }], { + rows: ['install_date'], + columns: [], + }), height: 400, width: 600, margin: 0, @@ -88,7 +104,6 @@ test('emits numeric temporal values for drill-to-detail filters on formatted row tableRenderer: 'Table', colOrder: 'key_a_to_z', rowOrder: 'key_a_to_z', - aggregateFunction: 'Count', transposePivot: false, combineMetric: false, rowSubtotalPosition: false, @@ -141,7 +156,10 @@ test('keeps non-numeric temporal values for drill-to-detail formatted labels', ( const onContextMenu = jest.fn(); const dateValue = '2024-01-01'; const props: PivotTableProps = { - data: [{ install_date: dateValue, value: 1 }], + data: toQueryData([{ install_date: dateValue, value: 1 }], { + rows: ['install_date'], + columns: [], + }), height: 400, width: 600, margin: 0, @@ -151,7 +169,6 @@ test('keeps non-numeric temporal values for drill-to-detail formatted labels', ( tableRenderer: 'Table', colOrder: 'key_a_to_z', rowOrder: 'key_a_to_z', - aggregateFunction: 'Count', transposePivot: false, combineMetric: false, rowSubtotalPosition: false, @@ -202,7 +219,10 @@ test('keeps non-numeric temporal values for drill-to-detail formatted labels', ( test('keeps non-formatted drill-to-detail values as strings', () => { const onContextMenu = jest.fn(); const props = createProps({ - data: [{ country: 'US', value: 1 }], + data: toQueryData([{ country: 'US', value: 1 }], { + rows: ['country'], + columns: [], + }), groupbyRows: ['country'], onContextMenu, }); @@ -230,7 +250,10 @@ test('emits numeric temporal values for cross-filters on formatted row headers', const setDataMask = jest.fn(); const timestamp = 1777248000000; const props: PivotTableProps = { - data: [{ install_date: String(timestamp), value: 1 }], + data: toQueryData([{ install_date: String(timestamp), value: 1 }], { + rows: ['install_date'], + columns: [], + }), height: 400, width: 600, margin: 0, @@ -240,7 +263,6 @@ test('emits numeric temporal values for cross-filters on formatted row headers', tableRenderer: 'Table', colOrder: 'key_a_to_z', rowOrder: 'key_a_to_z', - aggregateFunction: 'Count', transposePivot: false, combineMetric: false, rowSubtotalPosition: false, @@ -294,7 +316,10 @@ test('keeps non-numeric temporal values for cross-filters on formatted row heade const setDataMask = jest.fn(); const dateValue = '2024-01-01'; const props = createProps({ - data: [{ install_date: dateValue, value: 1 }], + data: toQueryData([{ install_date: dateValue, value: 1 }], { + rows: ['install_date'], + columns: [], + }), groupbyRows: ['install_date'], setDataMask, dateFormatters: { @@ -332,7 +357,10 @@ test('emits drill filters from formatted column headers', () => { expressionType: 'SQL', }; const props = createProps({ - data: [{ 'Install date expression': String(timestamp), value: 1 }], + data: toQueryData( + [{ 'Install date expression': String(timestamp), value: 1 }], + { rows: [], columns: [adhocColumn] }, + ), groupbyColumns: [adhocColumn], dateFormatters: { 'Install date expression': (value: DataRecordValue) => 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 3bf887c0b60..bbc58996048 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 @@ -17,10 +17,21 @@ * under the License. */ -import { TimeGranularity } from '@superset-ui/core'; +import { + QueryFormMetric, + QueryObject, + TimeGranularity, +} from '@superset-ui/core'; import buildQuery from '../../src/plugin/buildQuery'; import { PivotTableQueryFormData } from '../../src/types'; +// buildQuery attaches `grouping_sets` (one entry per rollup level) to the +// query object; it is not part of the base QueryObject type, so narrow to a +// local type instead of casting through `any`. Each level is serialized as +// column *labels* (string[]), matching the backend's `list[list[str]]` +// (see superset/common/query_context_processor.py), not `QueryFormColumn[]`. +type GroupingSetsQuery = QueryObject & { grouping_sets: string[][] }; + const formData: PivotTableQueryFormData = { groupbyRows: ['row1', 'row2'], groupbyColumns: ['col1', 'col2'], @@ -28,7 +39,6 @@ const formData: PivotTableQueryFormData = { tableRenderer: 'Table With Subtotal', colOrder: 'key_a_to_z', rowOrder: 'key_a_to_z', - aggregateFunction: 'Sum', transposePivot: true, rowSubtotalPosition: true, colSubtotalPosition: true, @@ -56,9 +66,41 @@ const formData: PivotTableQueryFormData = { currencyFormat: { symbol: 'USD', symbolPosition: 'prefix' }, }; +test('additive metrics use the fast-path: a single full-detail query', () => { + const { queries } = buildQuery({ + ...formData, + metrics: [ + { + expressionType: 'SIMPLE', + aggregate: 'SUM', + column: { column_name: 'num' }, + label: 'sum_num', + }, + ] as QueryFormMetric[], + }); + expect(queries).toHaveLength(1); + // The single leaf query carries all dimensions (2 rows + 2 cols). + expect(queries[0].columns).toHaveLength(4); +}); + +test('non-additive metrics emit a single GROUPING SETS query with all levels', () => { + // 2 row dims x 2 col dims -> (2+1) x (2+1) = 9 rollup levels, carried as + // grouping_sets on a single query (saved-metric strings are non-additive). + const queryContext = buildQuery(formData); + expect(queryContext.queries).toHaveLength(1); + const [query] = queryContext.queries; + // The single query selects the full set of dimensions ... + expect(query.columns).toHaveLength(4); + // ... and requests every rollup level via grouping_sets (grand total = []). + expect((query as GroupingSetsQuery).grouping_sets).toHaveLength(9); + expect((query as GroupingSetsQuery).grouping_sets[0]).toEqual([]); + expect((query as GroupingSetsQuery).grouping_sets[8]).toHaveLength(4); +}); + test('should build groupby with series in form data', () => { const queryContext = buildQuery(formData); - const [query] = queryContext.queries; + // Single GROUPING SETS query: the sole query carries the full column set. + const query = queryContext.queries[queryContext.queries.length - 1]; expect(query.columns).toEqual([ { columnType: 'BASE_AXIS', @@ -80,7 +122,8 @@ test('should work with old charts', () => { granularity_sqla: 'col1', }; const queryContext = buildQuery(modifiedFormData); - const [query] = queryContext.queries; + // Single GROUPING SETS query: the sole query carries the full column set. + const query = queryContext.queries[queryContext.queries.length - 1]; expect(query.columns).toEqual([ { timeGrain: 'P1M', @@ -101,7 +144,8 @@ test('should prefer extra_form_data.time_grain_sqla over formData.time_grain_sql extra_form_data: { time_grain_sqla: TimeGranularity.QUARTER }, }; const queryContext = buildQuery(modifiedFormData); - const [query] = queryContext.queries; + // Single GROUPING SETS query: the sole query carries the full column set. + const query = queryContext.queries[queryContext.queries.length - 1]; expect(query.columns?.[0]).toEqual({ timeGrain: TimeGranularity.QUARTER, columnType: 'BASE_AXIS', @@ -113,7 +157,8 @@ test('should prefer extra_form_data.time_grain_sqla over formData.time_grain_sql test('should fallback to formData.time_grain_sqla if extra_form_data.time_grain_sqla is not set', () => { const queryContext = buildQuery(formData); - const [query] = queryContext.queries; + // Single GROUPING SETS query: the sole query carries the full column set. + const query = queryContext.queries[queryContext.queries.length - 1]; expect(query.columns?.[0]).toEqual({ timeGrain: formData.time_grain_sqla, columnType: 'BASE_AXIS', @@ -129,6 +174,7 @@ test('should not omit extras.time_grain_sqla from queryContext so dashboards app extra_form_data: { time_grain_sqla: TimeGranularity.QUARTER }, }; const queryContext = buildQuery(modifiedFormData); - const [query] = queryContext.queries; + // Single GROUPING SETS query: the sole query carries the full column set. + const query = queryContext.queries[queryContext.queries.length - 1]; expect(query.extras?.time_grain_sqla).toEqual(TimeGranularity.QUARTER); }); 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 a900e43fb0e..a3c09d7bfc3 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 @@ -20,7 +20,7 @@ import { ChartProps, QueryFormData } from '@superset-ui/core'; import { supersetTheme } from '@apache-superset/core/theme'; import transformProps from '../../src/plugin/transformProps'; -import { MetricsLayoutEnum } from '../../src/types'; +import { MetricsLayoutEnum, QueryData } from '../../src/types'; const setDataMask = jest.fn(); const formData = { @@ -30,7 +30,6 @@ const formData = { tableRenderer: 'Table With Subtotal', colOrder: 'key_a_to_z', rowOrder: 'key_a_to_z', - aggregateFunction: 'Sum', transposePivot: true, combineMetric: true, rowSubtotalPosition: true, @@ -64,36 +63,76 @@ const chartProps = new ChartProps({ theme: supersetTheme, }); -test('should transform chart props for viz', () => { - expect(transformProps(chartProps)).toEqual({ - width: 800, - height: 600, - groupbyRows: ['row1', 'row2'], - groupbyColumns: ['col1', 'col2'], - metrics: ['metric1', 'metric2'], - tableRenderer: 'Table With Subtotal', - colOrder: 'key_a_to_z', - rowOrder: 'key_a_to_z', - aggregateFunction: 'Sum', - transposePivot: true, - combineMetric: true, - rowSubtotalPosition: true, - colSubtotalPosition: true, +test('should pass through formData props for viz', () => { + const result = transformProps(chartProps) as ReturnType< + typeof transformProps + >; + expect(result.width).toBe(800); + expect(result.height).toBe(600); + expect(result.groupbyRows).toEqual(['row1', 'row2']); + expect(result.groupbyColumns).toEqual(['col1', 'col2']); + expect(result.metrics).toEqual(['metric1', 'metric2']); + expect(result.metricsLayout).toBe(MetricsLayoutEnum.COLUMNS); + expect(result.currencyFormat).toEqual({ + symbol: 'USD', + symbolPosition: 'prefix', + }); + // data is the per-level QueryData[] (split/synthesized), not raw rows. + expect(Array.isArray(result.data)).toBe(true); + result.data.forEach((level: QueryData) => { + expect(level).toHaveProperty('groupby'); + expect(level).toHaveProperty('data'); + }); +}); + +test('non-additive: transformProps splits the GROUPING SETS result by level', () => { + const gm = (col: string) => `${col}__superset_grouping`; + const localFormData = { + ...formData, + combineMetric: false, + transposePivot: false, + metricsLayout: MetricsLayoutEnum.ROWS, + groupbyRows: ['region'], + groupbyColumns: [], colTotals: true, rowTotals: true, - valueFormat: 'SMART_NUMBER', - data: [{ name: 'Hulk', sum__num: 1, __timestamp: 599616000000 }], - setDataMask, - selectedFilters: {}, - verboseMap: {}, - metricsLayout: MetricsLayoutEnum.COLUMNS, - metricColorFormatters: [], - dateFormatters: {}, - emitCrossFilters: false, - columnFormats: {}, - currencyFormats: {}, - currencyFormat: { symbol: 'USD', symbolPosition: 'prefix' }, + metrics: ['m'], // saved-metric string -> non-additive + }; + const cp = new ChartProps({ + formData: localFormData as unknown as QueryFormData, + width: 800, + height: 600, + queriesData: [ + { + // One combined GROUPING SETS result: leaf rows (region marker 0) + + // grand total row (region marker 1). + data: [ + { region: 'US', m: 10, [gm('region')]: 0 }, + { region: 'EU', m: 5, [gm('region')]: 0 }, + { region: null, m: 15, [gm('region')]: 1 }, + ], + colnames: ['region', 'm', gm('region')], + coltypes: [1, 0, 0], + }, + ], + hooks: { setDataMask }, + filterState: { selectedFilters: {} }, + datasource: { verboseMap: {}, columnFormats: {} }, + theme: supersetTheme, }); + + const result = transformProps(cp) as ReturnType; + const grand = result.data.find( + (d: QueryData) => + d.groupby.rows.length === 0 && d.groupby.columns.length === 0, + )!; + const leaf = result.data.find((d: QueryData) => d.groupby.rows.length === 1)!; + // Markers stripped; rows routed to the correct level. + expect(grand.data).toEqual([{ region: null, m: 15 }]); + expect(leaf.data).toEqual([ + { region: 'US', m: 10 }, + { region: 'EU', m: 5 }, + ]); }); test('should pass AUTO mode through for per-cell detection (single currency data)', () => { @@ -335,3 +374,59 @@ test('should map conditional formatting rules to metricColorFormatters with corr result.metricColorFormatters[1].getColorFromValue(column2Formatting), ).toEqual('#5ac189FF'); }); + +test('additive metrics: synthesizes rollup levels from a single leaf query', () => { + const additiveFormData = { + ...formData, + combineMetric: false, + transposePivot: false, + metricsLayout: MetricsLayoutEnum.ROWS, + groupbyRows: ['region'], + groupbyColumns: [], + colTotals: true, + rowTotals: true, + metrics: [ + { + expressionType: 'SIMPLE', + aggregate: 'SUM', + column: { column_name: 'v' }, + label: 'v', + }, + ], + }; + const additiveChartProps = new ChartProps({ + formData: additiveFormData as unknown as QueryFormData, + width: 800, + height: 600, + queriesData: [ + { + data: [ + { region: 'US', v: 10 }, + { region: 'EU', v: 5 }, + ], + colnames: ['region', 'v'], + coltypes: [1, 0], + }, + ], + hooks: { setDataMask }, + filterState: { selectedFilters: {} }, + datasource: { verboseMap: {}, columnFormats: {} }, + theme: supersetTheme, + }); + + const result = transformProps(additiveChartProps); + // One query produced multiple synthesized rollup levels. + expect(result.data.length).toBeGreaterThan(1); + // Grand-total level: region collapsed -> v = 10 + 5 = 15. + const grand = result.data.find( + (d: QueryData) => + d.groupby.rows.length === 0 && d.groupby.columns.length === 0, + )!; + expect(grand.data[0].v).toBe(15); + // Leaf level keeps per-region values. + const leaf = result.data.find((d: QueryData) => d.groupby.rows.length === 1)!; + expect(leaf.data).toEqual([ + { region: 'US', v: 10 }, + { region: 'EU', v: 5 }, + ]); +}); diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/utilities.test.ts b/superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/utilities.test.ts new file mode 100644 index 00000000000..5e43ea4d349 --- /dev/null +++ b/superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/utilities.test.ts @@ -0,0 +1,466 @@ +/* + * 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 { QueryFormMetric, TimeGranularity } from '@superset-ui/core'; +import buildGroupbyCombinations, { + isAdditiveMetric, + allMetricsAdditive, + additiveReducerFor, + synthesizeAdditiveLevels, + splitGroupingSetsResult, + groupingMarkerLabel, +} from '../../src/plugin/utilities'; +import { PivotTableQueryFormData, MetricsLayoutEnum } from '../../src/types'; + +const baseFormData = { + groupbyRows: ['row1', 'row2'], + groupbyColumns: ['col1', 'col2'], + metrics: ['metric1', 'metric2'], + tableRenderer: 'Table With Subtotal', + colOrder: 'key_a_to_z', + rowOrder: 'key_a_to_z', + aggregateFunction: 'Sum', + metricsLayout: MetricsLayoutEnum.ROWS, + transposePivot: false, + rowSubtotalPosition: true, + colSubtotalPosition: true, + colTotals: true, + colSubTotals: true, + rowTotals: true, + rowSubTotals: true, + valueFormat: 'SMART_NUMBER', + datasource: '5__table', + viz_type: 'my_chart', + width: 800, + height: 600, + combineMetric: false, + verboseMap: {}, + columnFormats: {}, + currencyFormats: {}, + metricColorFormatters: [], + dateFormatters: {}, + setDataMask: () => {}, + legacy_order_by: 'count', + order_desc: true, + margin: 0, + time_grain_sqla: TimeGranularity.MONTH, + temporal_columns_lookup: { col1: true }, + currencyFormat: { symbol: 'USD', symbolPosition: 'prefix' }, +} as unknown as PivotTableQueryFormData; + +test('should build all combinations for basic pivot table', () => { + const combinations = buildGroupbyCombinations(baseFormData); + + expect(combinations).toEqual([ + { rows: [], columns: [] }, + { rows: [], columns: ['col1'] }, + { rows: [], columns: ['col1', 'col2'] }, + { rows: ['row1'], columns: [] }, + { rows: ['row1'], columns: ['col1'] }, + { rows: ['row1'], columns: ['col1', 'col2'] }, + { rows: ['row1', 'row2'], columns: [] }, + { rows: ['row1', 'row2'], columns: ['col1'] }, + { rows: ['row1', 'row2'], columns: ['col1', 'col2'] }, + ]); + + expect(combinations).toHaveLength(9); +}); + +test('should handle transposed pivot correctly', () => { + const modifiedFormData = { + ...baseFormData, + transposePivot: true, + }; + + const combinations = buildGroupbyCombinations(modifiedFormData); + + expect(combinations).toEqual([ + { rows: [], columns: [] }, + { rows: [], columns: ['row1'] }, + { rows: [], columns: ['row1', 'row2'] }, + { rows: ['col1'], columns: [] }, + { rows: ['col1'], columns: ['row1'] }, + { rows: ['col1'], columns: ['row1', 'row2'] }, + { rows: ['col1', 'col2'], columns: [] }, + { rows: ['col1', 'col2'], columns: ['row1'] }, + { rows: ['col1', 'col2'], columns: ['row1', 'row2'] }, + ]); +}); + +test('should filter combinations when combineMetric is true with ROWS layout', () => { + const modifiedFormData = { + ...baseFormData, + combineMetric: true, + metricsLayout: MetricsLayoutEnum.ROWS, + }; + + const combinations = buildGroupbyCombinations(modifiedFormData); + + expect(combinations).toEqual([ + { rows: ['row1', 'row2'], columns: [] }, + { rows: ['row1', 'row2'], columns: ['col1'] }, + { rows: ['row1', 'row2'], columns: ['col1', 'col2'] }, + ]); + + expect(combinations).toHaveLength(3); +}); + +test('should filter combinations when combineMetric is true with COLUMNS layout', () => { + const modifiedFormData = { + ...baseFormData, + combineMetric: true, + metricsLayout: MetricsLayoutEnum.COLUMNS, + }; + + const combinations = buildGroupbyCombinations(modifiedFormData); + + expect(combinations).toEqual([ + { rows: [], columns: ['col1', 'col2'] }, + { rows: ['row1'], columns: ['col1', 'col2'] }, + { rows: ['row1', 'row2'], columns: ['col1', 'col2'] }, + ]); + + expect(combinations).toHaveLength(3); +}); + +test('should handle single dimension in rows only', () => { + const modifiedFormData = { + ...baseFormData, + groupbyRows: ['row'], + groupbyColumns: [], + }; + + const combinations = buildGroupbyCombinations(modifiedFormData); + + expect(combinations).toEqual([ + { rows: [], columns: [] }, + { rows: ['row'], columns: [] }, + ]); + + expect(combinations).toHaveLength(2); +}); + +test('should handle single dimension in columns only', () => { + const modifiedFormData = { + ...baseFormData, + groupbyRows: [], + groupbyColumns: ['col'], + }; + + const combinations = buildGroupbyCombinations(modifiedFormData); + + expect(combinations).toEqual([ + { rows: [], columns: [] }, + { rows: [], columns: ['col'] }, + ]); + + expect(combinations).toHaveLength(2); +}); + +test('should handle empty groupby arrays', () => { + const modifiedFormData = { + ...baseFormData, + groupbyRows: [], + groupbyColumns: [], + }; + + const combinations = buildGroupbyCombinations(modifiedFormData); + + expect(combinations).toEqual([{ rows: [], columns: [] }]); + + expect(combinations).toHaveLength(1); +}); + +test('should work with combineMetric and transposed pivot', () => { + const modifiedFormData = { + ...baseFormData, + transposePivot: true, + combineMetric: true, + metricsLayout: MetricsLayoutEnum.COLUMNS, + }; + + const combinations = buildGroupbyCombinations(modifiedFormData); + + expect(combinations).toEqual([ + { rows: [], columns: ['row1', 'row2'] }, + { rows: ['col1'], columns: ['row1', 'row2'] }, + { rows: ['col1', 'col2'], columns: ['row1', 'row2'] }, + ]); +}); + +test('should handle combineMetric with empty arrays correctly', () => { + const modifiedFormData = { + ...baseFormData, + groupbyRows: [], + groupbyColumns: ['col'], + combineMetric: true, + metricsLayout: MetricsLayoutEnum.ROWS, + }; + + const combinations = buildGroupbyCombinations(modifiedFormData); + + expect(combinations).toEqual([ + { rows: [], columns: [] }, + { rows: [], columns: ['col'] }, + ]); +}); + +test('should work with large number of dimensions', () => { + const modifiedFormData = { + ...baseFormData, + groupbyRows: ['r1', 'r2', 'r3', 'r4'], + groupbyColumns: ['c1', 'c2', 'c3'], + }; + + const combinations = buildGroupbyCombinations(modifiedFormData); + + expect(combinations).toHaveLength(20); + + expect(combinations).toContainEqual({ rows: [], columns: [] }); + expect(combinations).toContainEqual({ + rows: ['r1', 'r2', 'r3', 'r4'], + columns: ['c1', 'c2', 'c3'], + }); +}); + +test('isAdditiveMetric: SIMPLE metrics with additive aggregates are additive', () => { + expect( + isAdditiveMetric({ + expressionType: 'SIMPLE', + aggregate: 'SUM', + column: { column_name: 'num' }, + label: 'sum_num', + } as QueryFormMetric), + ).toBe(true); + expect( + isAdditiveMetric({ + expressionType: 'SIMPLE', + aggregate: 'COUNT', + column: { column_name: 'num' }, + label: 'count_num', + } as QueryFormMetric), + ).toBe(true); +}); + +test('isAdditiveMetric: non-additive aggregates, SQL, and saved metrics are not additive', () => { + expect( + isAdditiveMetric({ + expressionType: 'SIMPLE', + aggregate: 'AVG', + column: { column_name: 'num' }, + label: 'avg_num', + } as QueryFormMetric), + ).toBe(false); + expect( + isAdditiveMetric({ + expressionType: 'SIMPLE', + aggregate: 'COUNT_DISTINCT', + column: { column_name: 'name' }, + label: 'distinct_names', + } as QueryFormMetric), + ).toBe(false); + expect( + isAdditiveMetric({ + expressionType: 'SQL', + sqlExpression: 'SUM(a) / SUM(b)', + label: 'ratio', + } as QueryFormMetric), + ).toBe(false); + // saved-metric reference: aggregate unknown from form data -> non-additive + expect(isAdditiveMetric('count')).toBe(false); +}); + +test('allMetricsAdditive: all additive vs any non-additive vs empty', () => { + const sum = { + expressionType: 'SIMPLE', + aggregate: 'SUM', + column: { column_name: 'a' }, + label: 'a', + } as QueryFormMetric; + const ratio = { + expressionType: 'SQL', + sqlExpression: 'SUM(a)/SUM(b)', + label: 'r', + } as QueryFormMetric; + expect(allMetricsAdditive([sum, sum])).toBe(true); + expect(allMetricsAdditive([sum, ratio])).toBe(false); + expect(allMetricsAdditive([])).toBe(false); +}); + +test('pruning: with all totals/subtotals off, only the leaf level is queried', () => { + const combinations = buildGroupbyCombinations({ + ...baseFormData, + colTotals: false, + rowTotals: false, + colSubTotals: false, + rowSubTotals: false, + }); + expect(combinations).toEqual([ + { rows: ['row1', 'row2'], columns: ['col1', 'col2'] }, + ]); +}); + +test('pruning: totals on, subtotals off -> only full + fully-collapsed prefixes', () => { + const combinations = buildGroupbyCombinations({ + ...baseFormData, + colTotals: true, + rowTotals: true, + colSubTotals: false, + rowSubTotals: false, + }); + // 2 row prefixes ([], full) x 2 col prefixes ([], full) = 4 + expect(combinations).toEqual([ + { rows: [], columns: [] }, + { rows: [], columns: ['col1', 'col2'] }, + { rows: ['row1', 'row2'], columns: [] }, + { rows: ['row1', 'row2'], columns: ['col1', 'col2'] }, + ]); +}); + +test('pruning: row subtotals on, everything else off', () => { + const combinations = buildGroupbyCombinations({ + ...baseFormData, + colTotals: false, + rowTotals: false, + colSubTotals: false, + rowSubTotals: true, + }); + // row prefixes: intermediate [row1] + full [row1,row2]; col prefixes: full only + expect(combinations).toEqual([ + { rows: ['row1'], columns: ['col1', 'col2'] }, + { rows: ['row1', 'row2'], columns: ['col1', 'col2'] }, + ]); +}); + +test('pruning: empty column dims keep the [] (leaf) level regardless of rowTotals', () => { + const combinations = buildGroupbyCombinations({ + ...baseFormData, + groupbyColumns: [], + colTotals: true, // bottom total row + rowTotals: false, + colSubTotals: false, + rowSubTotals: false, + }); + // columns is empty so [] is the leaf level (always kept); rows: [] (colTotals) + full + expect(combinations).toEqual([ + { rows: [], columns: [] }, + { rows: ['row1', 'row2'], columns: [] }, + ]); +}); + +test('additiveReducerFor maps aggregates to reducers', () => { + const mk = (aggregate: string) => + ({ + expressionType: 'SIMPLE', + aggregate, + label: aggregate, + }) as QueryFormMetric; + expect(additiveReducerFor(mk('SUM'))).toBe('sum'); + expect(additiveReducerFor(mk('COUNT'))).toBe('sum'); + expect(additiveReducerFor(mk('MIN'))).toBe('min'); + expect(additiveReducerFor(mk('MAX'))).toBe('max'); + // saved-metric reference is a plain string, no cast needed + expect(additiveReducerFor('saved_metric')).toBe('sum'); +}); + +const LEAF = [ + { region: 'US', topic: 'a', value: 10 }, + { region: 'US', topic: 'b', value: 20 }, + { region: 'EU', topic: 'a', value: 5 }, +]; + +test('synthesizeAdditiveLevels: sum reducer across rollup levels', () => { + const [grand, perRegion, leaf] = synthesizeAdditiveLevels( + LEAF, + [ + { rows: [], columns: [] }, + { rows: ['region'], columns: [] }, + { rows: ['region', 'topic'], columns: [] }, + ], + { value: 'sum' }, + ); + expect(grand).toEqual([{ value: 35 }]); + expect(perRegion).toEqual([ + { region: 'US', value: 30 }, + { region: 'EU', value: 5 }, + ]); + // leaf level reduces single-row groups -> identity + expect(leaf).toEqual([ + { region: 'US', topic: 'a', value: 10 }, + { region: 'US', topic: 'b', value: 20 }, + { region: 'EU', topic: 'a', value: 5 }, + ]); +}); + +test('synthesizeAdditiveLevels: min/max reducers and null handling', () => { + const rows = [...LEAF, { region: 'EU', topic: 'b', value: null }]; + const [grandMax] = synthesizeAdditiveLevels( + rows, + [{ rows: [], columns: [] }], + { value: 'max' }, + ); + expect(grandMax).toEqual([{ value: 20 }]); + const [grandMin] = synthesizeAdditiveLevels( + rows, + [{ rows: [], columns: [] }], + { value: 'min' }, + ); + expect(grandMin).toEqual([{ value: 5 }]); +}); + +test('splitGroupingSetsResult splits by markers and strips them', () => { + const gm = groupingMarkerLabel; + const rows = [ + { region: 'US', topic: 'a', v: 10, [gm('region')]: 0, [gm('topic')]: 0 }, + { region: 'US', topic: 'b', v: 20, [gm('region')]: 0, [gm('topic')]: 0 }, + { region: 'US', topic: null, v: 30, [gm('region')]: 0, [gm('topic')]: 1 }, + { region: null, topic: null, v: 60, [gm('region')]: 1, [gm('topic')]: 1 }, + ]; + const [leaf, sub, grand] = splitGroupingSetsResult( + rows, + [['region', 'topic'], ['region'], []], + ['region', 'topic'], + ); + expect(leaf.map(r => r.v)).toEqual([10, 20]); + expect(sub.map(r => r.v)).toEqual([30]); + expect(grand.map(r => r.v)).toEqual([60]); + // markers stripped + [leaf, sub, grand].forEach(frame => + frame.forEach(r => + Object.keys(r).forEach(k => + expect(k.endsWith('__superset_grouping')).toBe(false), + ), + ), + ); +}); + +test('splitGroupingSetsResult attributes marker-less rows to the leaf level', () => { + const rows = [ + { region: 'US', topic: 'a', v: 10 }, + { region: 'US', topic: 'b', v: 20 }, + ]; + const [leaf, sub, grand] = splitGroupingSetsResult( + rows, + [['region', 'topic'], ['region'], []], + ['region', 'topic'], + ); + expect(leaf).toEqual(rows); + expect(sub).toEqual([]); + expect(grand).toEqual([]); +}); diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/nonAdditiveTotals.test.ts b/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/nonAdditiveTotals.test.ts new file mode 100644 index 00000000000..f17cfbb953b --- /dev/null +++ b/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/nonAdditiveTotals.test.ts @@ -0,0 +1,69 @@ +/** + * 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. + */ + +/** + * Pivot-table acceptance cases for non-additive metric totals (see SIP.md). + * + * The pivot computes its grand total / subtotals client-side using the + * `aggregators` from react-pivottable, applied to the already-aggregated cell + * values. For a non-additive metric (a ratio like SUM(actual)/SUM(target)) + * that is wrong: summing the per-group ratios is meaningless. This pins the + * mechanism behind #25747 / #32260 against the real production aggregator. + * + * The fix is not at this layer (no client-side aggregator over divided cells + * can recover the right answer) - it requires the DB-computed per-level rollup + * queries from phase 2. The `test.failing` case documents that target. + */ +import { aggregators } from '../../src/react-pivottable/utilities'; + +// Per-group completion ratios (actual/target), as the single pivot query +// returns them today. +const ratioCells = [{ completion: 10 / 40 }, { completion: 30 / 60 }]; // 0.25, 0.5 + +// Correct grand total: SUM(actual)/SUM(target) = 40/100 = 0.4 +const CORRECT_COMPLETION = 0.4; + +function sumOf(field: string, rows: Record[]): number { + // Build the real "Sum" aggregator the pivot uses for totals. + const aggregator = (aggregators as Record).Sum([field])( + null, + [], + [], + ); + rows.forEach(r => aggregator.push(r)); + return aggregator.value(); +} + +test('#25747/#32260: pivot Sum aggregator totals the per-group ratios (current, wrong)', () => { + // 0.25 + 0.5 = 0.75, which is not a meaningful completion rate. + expect(sumOf('completion', ratioCells)).toBeCloseTo(0.75); + expect(sumOf('completion', ratioCells)).not.toBeCloseTo(CORRECT_COMPLETION); +}); + +test.failing( + '#25747/#32260: the correct ratio is unreachable by client-side summation (justifies the query-layer fix)', + () => { + // This stays failing by design: no aggregator over the already-divided + // cells can recover SUM(actual)/SUM(target). It documents *why* phase 2 + // moves totals to DB-computed per-level rollup queries rather than fixing + // an aggregator. The phase-2 acceptance test lives at the data-flow layer + // (transformProps consuming the rollup results), not here. + expect(sumOf('completion', ratioCells)).toBeCloseTo(CORRECT_COMPLETION); + }, +); diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx b/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx index e0d587836d6..971753f751f 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx +++ b/superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx @@ -56,6 +56,53 @@ const SAMPLE_DATA = [ { color: 'red', shape: 'square', value: 40 }, ]; +/** + * Pre-aggregated, per-level data matching the multi-query contract: every record + * is tagged with the rollup level (rows/columns) that produced it, and the + * database has already computed each value. PivotData places these verbatim. + * Here `value` is a count, so leaf cells are 1, row/col totals are 2, grand + * total is 4. + */ +const TAGGED_COUNT_DATA = [ + // leaf cells (full detail) + { + color: 'blue', + shape: 'circle', + value: 1, + __rows: ['color'], + __columns: ['shape'], + }, + { + color: 'blue', + shape: 'square', + value: 1, + __rows: ['color'], + __columns: ['shape'], + }, + { + color: 'red', + shape: 'circle', + value: 1, + __rows: ['color'], + __columns: ['shape'], + }, + { + color: 'red', + shape: 'square', + value: 1, + __rows: ['color'], + __columns: ['shape'], + }, + // row totals (per color, across shapes) + { color: 'blue', value: 2, __rows: ['color'], __columns: [] }, + { color: 'red', value: 2, __rows: ['color'], __columns: [] }, + // col totals (per shape, across colors) + { shape: 'circle', value: 2, __rows: [], __columns: ['shape'] }, + { shape: 'square', value: 2, __rows: [], __columns: ['shape'] }, + // grand total + { value: 4, __rows: [], __columns: [] }, +]; + function renderWithTheme(ui: ReactElement) { return render({ui}); } @@ -104,35 +151,41 @@ test('TableRenderer renders row headers from pivot data', () => { }); test('TableRenderer renders aggregated cell values', () => { - const props = buildDefaultProps(); + const props = buildDefaultProps({ + data: TAGGED_COUNT_DATA, + vals: ['value'], + }); renderWithTheme(); - // With "Count" aggregator, each cell (row x col intersection) should - // contain "1" because each combination appears exactly once. + // Each leaf cell (row x col intersection) holds the DB-computed value "1". const cells = screen.getAllByRole('gridcell'); const cellTexts = cells.map(cell => cell.textContent); - // There should be cell values of "1" for each of the four intersections - // (blue+circle, blue+square, red+circle, red+square). - const onesCount = cellTexts.filter(text => text === '1').length; + // There should be a "1" leaf cell for each of the four intersections + // (blue+circle, blue+square, red+circle, red+square). The default formatter + // renders with two decimals in this test (production applies the metric's + // own format). + const onesCount = cellTexts.filter(text => text === '1.00').length; expect(onesCount).toBeGreaterThanOrEqual(4); }); test('TableRenderer renders row totals when rowTotals is enabled', () => { const props = buildDefaultProps({ + data: TAGGED_COUNT_DATA, + vals: ['value'], tableOptions: { rowTotals: true, colTotals: true }, }); renderWithTheme(); - // Row totals column should show "2" for each color (blue has 2 records, - // red has 2 records). + // Row totals column should show the DB-computed "2" for each color (blue has + // 2 records, red has 2 records). const totalCells = screen .getAllByRole('gridcell') .filter(cell => cell.classList.contains('pvtTotal')); expect(totalCells.length).toBeGreaterThan(0); const totalValues = totalCells.map(cell => cell.textContent); - expect(totalValues).toContain('2'); + expect(totalValues).toContain('2.00'); }); test('TableRenderer renders col totals row when colTotals is enabled', () => { @@ -150,11 +203,13 @@ test('TableRenderer renders col totals row when colTotals is enabled', () => { test('TableRenderer renders grand total when both totals are enabled', () => { const props = buildDefaultProps({ + data: TAGGED_COUNT_DATA, + vals: ['value'], tableOptions: { rowTotals: true, colTotals: true }, }); renderWithTheme(); - // The grand total cell should show "4" (total record count). + // The grand total cell shows the DB-computed "4" (total record count). const grandTotalCells = screen .getAllByRole('gridcell') .filter(cell => cell.classList.contains('pvtGrandTotal')); @@ -162,6 +217,69 @@ test('TableRenderer renders grand total when both totals are enabled', () => { expect(grandTotalCells[0]).toHaveTextContent('4'); }); +/** + * Metric-collapse totals: when the metric pseudo-dimension is the only thing on + * an axis (here columns), the opposite "Total" axis and the grand-total corner + * must still show values rather than null, because no rollup level produces an + * empty key on the metric axis. Records carry `__metricKey` so PivotData can + * mirror the value into rowTotals / allTotal. (Regression guard for the gap that + * in-app verification surfaced: a null right-hand "Total" column.) + */ +const TAGGED_METRIC_ON_COLUMNS = [ + // leaf cells: rows = [color], columns = [Metric] (metric on the column axis) + { + color: 'blue', + Metric: 'm1', + value: 10, + __rows: ['color'], + __columns: ['Metric'], + __metricKey: 'Metric', + }, + { + color: 'red', + Metric: 'm1', + value: 20, + __rows: ['color'], + __columns: ['Metric'], + __metricKey: 'Metric', + }, + // grand total level: rows = [], columns = [Metric] + { + Metric: 'm1', + value: 30, + __rows: [], + __columns: ['Metric'], + __metricKey: 'Metric', + }, +]; + +test('TableRenderer fills metric-collapse totals (no null Total column/corner)', () => { + const props = buildDefaultProps({ + data: TAGGED_METRIC_ON_COLUMNS, + rows: ['color'], + cols: ['Metric'], + vals: ['value'], + tableOptions: { rowTotals: true, colTotals: true }, + }); + renderWithTheme(); + + // Right-hand "Total" column (rowTotals) shows the per-row collapsed values... + const rowTotalTexts = screen + .getAllByRole('gridcell') + .filter(cell => cell.classList.contains('pvtTotal')) + .map(cell => cell.textContent); + expect(rowTotalTexts).toContain('10.00'); + expect(rowTotalTexts).toContain('20.00'); + expect(rowTotalTexts).not.toContain('null'); + + // ...and the grand-total corner shows the collapsed grand total (not null). + const grandTotalCells = screen + .getAllByRole('gridcell') + .filter(cell => cell.classList.contains('pvtGrandTotal')); + expect(grandTotalCells.length).toBe(1); + expect(grandTotalCells[0]).toHaveTextContent('30.00'); +}); + test('TableRenderer handles empty data gracefully', () => { const props = buildDefaultProps({ data: [] }); renderWithTheme(); diff --git a/superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts b/superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts index 409aae907e6..6f3a8a96e56 100644 --- a/superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts +++ b/superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts @@ -138,6 +138,23 @@ describe('plugin-chart-table', () => { expressionType: 'SQL', }); }); + test('should retain percent-metric post-processing on the summary (show_totals) query', () => { + // #37627: the summary row dropped post_processing, so percent-metric + // columns came back empty. The totals query must recompute them. + const { queries } = buildQueryCached({ + ...basicFormData, + query_mode: QueryMode.Aggregate, + metrics: ['count'], + percent_metrics: ['sum_sales'], + show_totals: true, + }); + // Main query carries the contribution op for the percent metric ... + expect(queries[0].post_processing).toEqual([ + expect.objectContaining({ operation: 'contribution' }), + ]); + // ... and so must the summary query (queries[1]). + expect(queries[1].post_processing).toEqual(queries[0].post_processing); + }); test('should include time_grain_sqla in extras if temporal colum is used and keep the rest', () => { const { queries } = buildQueryCached({ ...extraQueryFormData, diff --git a/superset/charts/schemas.py b/superset/charts/schemas.py index 4dc9838adff..97da753eed5 100644 --- a/superset/charts/schemas.py +++ b/superset/charts/schemas.py @@ -1358,6 +1358,17 @@ class ChartDataQueryObjectSchema(Schema): load_default=False, allow_none=True, ) + grouping_sets = fields.List( + fields.List(fields.String()), + metadata={ + "description": "Rollup levels for non-additive totals: each entry is " + "the list of groupby columns to group at that level (e.g. the empty " + "list is the grand total). When set and the engine supports it, the " + "levels are computed in a single GROUPING SETS query.", + }, + load_default=None, + allow_none=True, + ) timeseries_limit = fields.Integer( metadata={ "description": "Maximum row count for timeseries queries. " diff --git a/superset/common/grouping_sets.py b/superset/common/grouping_sets.py new file mode 100644 index 00000000000..59aefc657b2 --- /dev/null +++ b/superset/common/grouping_sets.py @@ -0,0 +1,117 @@ +# 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. + +""" +SQL building blocks for the pivot-table non-additive totals optimization +(SIP.md, phase 3b). When a datasource engine reports +``supports_grouping_sets``, the N per-rollup-level queries can be collapsed into +a single ``GROUPING SETS`` query: the database computes every level in one scan, +and each returned row is attributed to its level via ``GROUPING()`` markers. + +These are the engine-agnostic SQL primitives. Wiring them into the query context +(emitting one query and splitting the result back into per-level results) is the +remaining integration; see SIP.md. +""" + +from __future__ import annotations + +from collections.abc import Sequence +from typing import Final + +import pandas as pd +from sqlalchemy import func, tuple_ +from sqlalchemy.sql.elements import ColumnElement + +# Suffix for the per-column GROUPING() marker columns added to a GROUPING SETS +# query. Chosen to be unlikely to collide with a real metric/column label. +GROUPING_MARKER_SUFFIX: Final = "__superset_grouping" + + +def grouping_marker_label(column_label: str) -> str: + """The output label of the GROUPING() marker for a groupby column.""" + return f"{column_label}{GROUPING_MARKER_SUFFIX}" + + +def grouping_sets_clause( + groups: Sequence[Sequence[ColumnElement]], +) -> ColumnElement: + """ + Build a ``GROUP BY GROUPING SETS (...)`` clause from rollup column groups. + + Each group is the set of columns grouped at one rollup level; the empty + group ``()`` is the grand total. For example, groups ``[[a, b], [a], []]`` + produce ``GROUPING SETS ((a, b), (a), ())``. + + :param groups: one column list per rollup level + :return: a clause element suitable for ``select(...).group_by(...)`` + """ + return func.grouping_sets(*[tuple_(*group) for group in groups]) + + +def grouping_id_column(column: ColumnElement, label: str) -> ColumnElement: + """ + Build a ``GROUPING(col) AS label`` marker column. + + In a ``GROUPING SETS`` result, ``GROUPING(col)`` is ``0`` when ``col`` is + part of the row's grouping level and ``1`` when it has been rolled up + (aggregated away). Selecting one marker per groupby column lets the caller + attribute each returned row to its rollup level when splitting the single + result back into per-level results. + + :param column: the groupby column to probe + :param label: the output label for the marker (see ``grouping_marker_label``) + :return: the labelled ``GROUPING(col)`` column + """ + return func.grouping(column).label(label) + + +def split_grouping_sets_result( + df: pd.DataFrame, + levels: Sequence[Sequence[str]], + groupby_columns: Sequence[str], +) -> list[pd.DataFrame]: + """ + Split a combined ``GROUPING SETS`` result into one DataFrame per rollup + level, the inverse of {@link grouping_sets_clause}. + + Each row of ``df`` carries a ``GROUPING()`` marker per groupby column (named + by ``grouping_marker_label``): ``0`` if the column is grouped at that row's + level, ``1`` if it was rolled up. A row belongs to a level iff its markers + are ``0`` exactly for that level's columns. Marker columns are dropped from + the returned frames so each looks like an ordinary per-level query result. + + :param df: the combined query result, including marker columns + :param levels: the grouped-column list for each rollup level (same order as + passed to ``grouping_sets_clause``) + :param groupby_columns: every groupby column that has a marker + :return: one DataFrame per level, in ``levels`` order + """ + markers: list[str] = [grouping_marker_label(col) for col in groupby_columns] + results: list[pd.DataFrame] = [] + for level in levels: + grouped: set[str] = set(level) + mask: pd.Series = pd.Series(True, index=df.index) + for col in groupby_columns: + expected: int = 0 if col in grouped else 1 + mask &= df[grouping_marker_label(col)] == expected + level_df: pd.DataFrame = ( + df[mask] + .drop(columns=[m for m in markers if m in df.columns]) + .reset_index(drop=True) + ) + results.append(level_df) + return results diff --git a/superset/common/query_context_processor.py b/superset/common/query_context_processor.py index bd7e8c9ccbe..b6a36f72f04 100644 --- a/superset/common/query_context_processor.py +++ b/superset/common/query_context_processor.py @@ -16,6 +16,7 @@ # under the License. from __future__ import annotations +import copy import logging import re from typing import Any, cast, ClassVar, Sequence, TYPE_CHECKING @@ -26,6 +27,7 @@ from flask_babel import gettext as _ from superset.common.chart_data import ChartDataResultFormat from superset.common.db_query_status import QueryStatus +from superset.common.grouping_sets import grouping_marker_label from superset.common.query_actions import get_query_results from superset.common.utils.query_cache_manager import QueryCacheManager from superset.common.utils.time_range_utils import get_since_until_from_time_range @@ -39,7 +41,7 @@ from superset.exceptions import ( from superset.explorables.base import Explorable from superset.extensions import cache_manager, security_manager from superset.models.helpers import QueryResult -from superset.superset_typing import AdhocColumn, AdhocMetric +from superset.superset_typing import AdhocColumn, AdhocMetric, Column from superset.utils import csv, excel from superset.utils.cache import generate_cache_key, set_and_log_cache from superset.utils.core import ( @@ -47,6 +49,7 @@ from superset.utils.core import ( DTTM_ALIAS, error_msg_from_exception, GenericDataType, + get_column_name, get_column_names_from_columns, get_column_names_from_metrics, is_adhoc_column, @@ -59,6 +62,7 @@ from superset.viz import viz_types if TYPE_CHECKING: from superset.common.query_context import QueryContext from superset.common.query_object import QueryObject + from superset.db_engine_specs.base import BaseEngineSpec logger = logging.getLogger(__name__) @@ -252,9 +256,84 @@ class QueryContextProcessor: This method delegates to the datasource's get_query_result method, which handles query execution, normalization, time offsets, and post-processing. + + When the query requests rollup ``grouping_sets`` but the engine does not + support native ``GROUPING SETS``, fall back to one query per level and + concatenate the results with ``GROUPING()``-equivalent markers, so the + combined result matches the shape the native path produces (SIP.md, + phase 3b). Engines that support it run the single native query. """ + if query_object.grouping_sets and not self._supports_grouping_sets(): + return self._grouping_sets_fallback(query_object) return self._qc_datasource.get_query_result(query_object) + def _supports_grouping_sets(self) -> bool: + engine_spec: BaseEngineSpec | None = getattr( + self._qc_datasource, "db_engine_spec", None + ) + return bool(engine_spec and engine_spec.supports_grouping_sets) + + def _grouping_sets_fallback(self, query_object: QueryObject) -> QueryResult: + """ + Emulate a GROUPING SETS query on engines without native support: run one + query per rollup level and concatenate, tagging each level's rows with + the same per-column markers the native path emits. + + This issues one sequential query per rollup level, with no cap on the + number of levels. The level count is bounded by the pivot's row/column + dimensionality (powerset of grouped dimensions in the worst case), so a + chart with many dimensions on an engine lacking native GROUPING SETS + support could fan out to a non-trivial number of queries per render. + """ + levels: list[list[str]] = query_object.grouping_sets + # Use the same label derivation as the native path (physical column name + # or adhoc column label) so both column kinds are represented and each + # label maps back to its own column, in the same order as the source + # list. + all_labels: list[str] = [get_column_name(col) for col in query_object.columns] + label_to_column: dict[str, Column] = dict( + zip(all_labels, query_object.columns, strict=True) + ) + + frames: list[pd.DataFrame] = [] + result: QueryResult | None = None + for level in levels: + level_labels: set[str] = set(level) + sub_query = copy.copy(query_object) + sub_query.grouping_sets = [] + sub_query.columns = [ + label_to_column[label] for label in all_labels if label in level_labels + ] + # A GROUPING SETS query computes a bounded set of rollup levels, so + # the native path never applies row_limit to it (see the + # `use_grouping_sets` check in models/helpers.py). Match that here: + # limiting each level's fallback sub-query independently would + # truncate subtotal/grand-total rows and diverge from the native + # result shape. The native path applies `row_offset` exactly once, + # to the combined multi-level result (see the unconditional + # `qry.offset()` call in models/helpers.py). Applying the same + # offset to each per-level sub-query independently would apply it + # once per level instead of once overall, and can silently drop + # low-row-count levels (e.g. the single grand-total row) entirely. + # Zero it here and apply it once after concatenation instead. + sub_query.row_limit = None + sub_query.row_offset = 0 + result = self._qc_datasource.get_query_result(sub_query) + level_df = result.df.copy() + for label in all_labels: + level_df[grouping_marker_label(label)] = ( + 0 if label in level_labels else 1 + ) + frames.append(level_df) + + if result is None: # no levels requested; nothing to do + return self._qc_datasource.get_query_result(query_object) + + result.df = pd.concat(frames, ignore_index=True) if frames else result.df + if query_object.row_offset: + result.df = result.df.iloc[query_object.row_offset :].reset_index(drop=True) + return result + def get_data( self, df: pd.DataFrame, coltypes: list[GenericDataType] ) -> str | bytes | list[dict[str, Any]]: diff --git a/superset/common/query_object.py b/superset/common/query_object.py index 44469ca7b5e..f2ef6dca55d 100644 --- a/superset/common/query_object.py +++ b/superset/common/query_object.py @@ -90,6 +90,7 @@ class QueryObject: # pylint: disable=too-many-instance-attributes filter: list[QueryObjectFilterClause] from_dttm: datetime | None granularity: str | None + grouping_sets: list[list[str]] inner_from_dttm: datetime | None inner_to_dttm: datetime | None is_rowcount: bool @@ -133,6 +134,7 @@ class QueryObject: # pylint: disable=too-many-instance-attributes series_limit: int = 0, series_limit_metric: Metric | None = None, group_others_when_limit_reached: bool = False, + grouping_sets: list[list[str]] | None = None, time_range: str | None = None, time_shift: str | None = None, **kwargs: Any, @@ -157,6 +159,7 @@ class QueryObject: # pylint: disable=too-many-instance-attributes self.series_limit = series_limit self.series_limit_metric = series_limit_metric self.group_others_when_limit_reached = group_others_when_limit_reached + self.grouping_sets = grouping_sets or [] self.time_range = time_range self.time_shift = time_shift self.from_dttm = kwargs.get("from_dttm") @@ -410,6 +413,7 @@ class QueryObject: # pylint: disable=too-many-instance-attributes "series_limit": self.series_limit, "series_limit_metric": self.series_limit_metric, "group_others_when_limit_reached": self.group_others_when_limit_reached, + "grouping_sets": self.grouping_sets, "to_dttm": self.to_dttm, "time_shift": self.time_shift, "time_compare_full_range": self.time_compare_full_range, diff --git a/superset/db_engine_specs/base.py b/superset/db_engine_specs/base.py index 615f6f74a37..eee5635160a 100644 --- a/superset/db_engine_specs/base.py +++ b/superset/db_engine_specs/base.py @@ -566,6 +566,12 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods # if True, database will be listed as option in the upload file form supports_file_upload = True + # Whether the engine supports SQL GROUPING SETS / ROLLUP / CUBE. When True, + # consumers (e.g. the pivot table's non-additive totals) can collapse the + # per-rollup-level queries into a single GROUPING SETS query instead of + # issuing one query per level. Conservative default of False; engines opt in. + supports_grouping_sets = False + # Is the DB engine spec able to change the default schema? This requires implementing # noqa: E501 # a custom `adjust_engine_params` method. supports_dynamic_schema = False diff --git a/superset/db_engine_specs/bigquery.py b/superset/db_engine_specs/bigquery.py index 97e331b1be3..1aebd02b835 100644 --- a/superset/db_engine_specs/bigquery.py +++ b/superset/db_engine_specs/bigquery.py @@ -287,6 +287,7 @@ class BigQueryEngineSpec(BaseEngineSpec): # pylint: disable=too-many-public-met supports_catalog = supports_dynamic_catalog = supports_cross_catalog_queries = True supports_dynamic_schema = True + supports_grouping_sets = True # when editing the database, mask this field in `encrypted_extra` # pylint: disable=invalid-name diff --git a/superset/db_engine_specs/hive.py b/superset/db_engine_specs/hive.py index d17d57ca42f..efd5a970bcd 100644 --- a/superset/db_engine_specs/hive.py +++ b/superset/db_engine_specs/hive.py @@ -96,6 +96,11 @@ class HiveEngineSpec(PrestoEngineSpec): supports_dynamic_schema = True supports_cross_catalog_queries = False + # Explicitly opt out (overriding the inherited PrestoEngineSpec value): + # Hive/Spark's GROUPING SETS + GROUPING() marker semantics have not been + # verified against this query pattern, so fall back to one query per + # rollup level instead of assuming native support. + supports_grouping_sets: bool = False metadata = { "description": ( diff --git a/superset/db_engine_specs/postgres.py b/superset/db_engine_specs/postgres.py index 49489822d13..4be3006a1ad 100644 --- a/superset/db_engine_specs/postgres.py +++ b/superset/db_engine_specs/postgres.py @@ -278,6 +278,7 @@ class PostgresEngineSpec(BasicParametersMixin, PostgresBaseEngineSpec): supports_dynamic_schema = True supports_catalog = True supports_dynamic_catalog = True + supports_grouping_sets = True default_driver = "psycopg2" sqlalchemy_uri_placeholder = ( diff --git a/superset/db_engine_specs/presto.py b/superset/db_engine_specs/presto.py index 71006fcc109..3f100a17134 100644 --- a/superset/db_engine_specs/presto.py +++ b/superset/db_engine_specs/presto.py @@ -166,6 +166,11 @@ class PrestoBaseEngineSpec(BaseEngineSpec, metaclass=ABCMeta): supports_dynamic_schema = True supports_catalog = supports_dynamic_catalog = supports_cross_catalog_queries = True + # Not set here: GROUPING SETS support is opted in per-concrete-engine + # (``PrestoEngineSpec``, ``TrinoEngineSpec``) rather than on this shared + # base, since Hive-family descendants (``HiveEngineSpec``, ``SparkEngineSpec``, + # ``DatabricksHiveEngineSpec``) have not been verified against this query + # pattern and should not silently inherit it. # Presto/Trino don't reliably support IS true/false on computed boolean # expressions (e.g. columns defined as `(expiration = 1) AS expiration`), # which raises a query error. Use = true/false instead. @@ -914,6 +919,7 @@ class PrestoEngineSpec(PrestoBaseEngineSpec): engine = "presto" engine_name = "Presto" allows_alias_to_source_column = False + supports_grouping_sets = True metadata = { "description": "Presto is a distributed SQL query engine for big data.", diff --git a/superset/db_engine_specs/snowflake.py b/superset/db_engine_specs/snowflake.py index 89b120d7f61..683d892de88 100644 --- a/superset/db_engine_specs/snowflake.py +++ b/superset/db_engine_specs/snowflake.py @@ -96,6 +96,7 @@ class SnowflakeEngineSpec(PostgresBaseEngineSpec): supports_dynamic_schema = True supports_catalog = supports_dynamic_catalog = supports_cross_catalog_queries = True + supports_grouping_sets = True metadata = { "description": "Snowflake is a cloud-native data warehouse.", diff --git a/superset/db_engine_specs/trino.py b/superset/db_engine_specs/trino.py index 906db82eba0..1ad98d06bd1 100644 --- a/superset/db_engine_specs/trino.py +++ b/superset/db_engine_specs/trino.py @@ -71,6 +71,7 @@ class TrinoEngineSpec(PrestoBaseEngineSpec): engine = "trino" engine_name = "Trino" allows_alias_to_source_column = False + supports_grouping_sets = True # The full set of columns Trino's "$partitions" exposes for an # Iceberg table. The real partition keys are nested in the "partition" ROW, diff --git a/superset/models/helpers.py b/superset/models/helpers.py index b80081bc726..1376a6bfbb6 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -77,6 +77,11 @@ from sqlalchemy_utils import UUIDType from superset import db, is_feature_enabled from superset.advanced_data_type.types import AdvancedDataTypeResponse from superset.common.db_query_status import QueryStatus +from superset.common.grouping_sets import ( + grouping_id_column, + grouping_marker_label, + grouping_sets_clause, +) from superset.common.utils import dataframe_utils from superset.common.utils.time_range_utils import ( get_since_until_from_query_object, @@ -213,6 +218,7 @@ SQLA_QUERY_KEYS = { "series_limit", "series_limit_metric", "group_others_when_limit_reached", + "grouping_sets", "row_limit", "row_offset", "timeseries_limit", @@ -3511,6 +3517,7 @@ class ExploreMixin: # pylint: disable=too-many-public-methods series_limit: Optional[int] = None, series_limit_metric: Optional[Metric] = None, group_others_when_limit_reached: bool = False, + grouping_sets: Optional[list[list[str]]] = None, row_limit: Optional[int] = None, row_offset: Optional[int] = None, timeseries_limit: Optional[int] = None, @@ -3882,6 +3889,22 @@ class ExploreMixin: # pylint: disable=too-many-public-methods select_exprs + metrics_exprs, key=lambda x: x.name ) + # GROUPING SETS collapse (SIP.md, phase 3b): when the request supplies + # rollup levels via `grouping_sets` and the engine supports it, compute + # every level in one query and tag each row with GROUPING() markers so + # the caller can split the result back per level. Strictly opt-in: with + # `grouping_sets` unset the query is byte-identical to before. + use_grouping_sets = bool( + grouping_sets + and groupby_all_columns + and db_engine_spec.supports_grouping_sets + ) + if use_grouping_sets: + select_exprs = select_exprs + [ + grouping_id_column(gby_expr, grouping_marker_label(name)) + for name, gby_expr in groupby_all_columns.items() + ] + # Expected output columns labels_expected = [c.key for c in select_exprs] @@ -3893,7 +3916,18 @@ class ExploreMixin: # pylint: disable=too-many-public-methods qry = sa.select(*select_exprs) if groupby_all_columns: - qry = qry.group_by(*groupby_all_columns.values()) + if use_grouping_sets: + gs_levels = [ + [ + groupby_all_columns[col] + for col in level + if col in groupby_all_columns + ] + for level in grouping_sets or [] + ] + qry = qry.group_by(grouping_sets_clause(gs_levels)) + else: + qry = qry.group_by(*groupby_all_columns.values()) where_clause_and: list[ColumnElement] = [] having_clause_and: list[ColumnElement] = [] @@ -4211,7 +4245,10 @@ class ExploreMixin: # pylint: disable=too-many-public-methods direction = sa.asc if ascending else sa.desc qry = qry.order_by(direction(col)) - if row_limit: + # A GROUPING SETS query computes a bounded set of rollup levels; applying + # a row LIMIT is both unnecessary and unparseable by some SQL parsers + # (LIMIT after a GROUPING SETS GROUP BY), so skip it for that path. + if row_limit and not use_grouping_sets: qry = qry.limit(row_limit) if row_offset and self.database.db_engine_spec.supports_offset: qry = qry.offset(row_offset) diff --git a/superset/superset_typing.py b/superset/superset_typing.py index 3b32d8c549c..9355830726b 100644 --- a/superset/superset_typing.py +++ b/superset/superset_typing.py @@ -225,6 +225,7 @@ class QueryObjectDict(TypedDict, total=False): series_limit: int series_limit_metric: Metric | None group_others_when_limit_reached: bool + grouping_sets: list[list[str]] to_dttm: datetime | None time_shift: str | None time_compare_full_range: bool diff --git a/tests/integration_tests/non_additive_totals_tests.py b/tests/integration_tests/non_additive_totals_tests.py new file mode 100644 index 00000000000..1d6d5e81a7b --- /dev/null +++ b/tests/integration_tests/non_additive_totals_tests.py @@ -0,0 +1,315 @@ +# 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. + +""" +End-to-end acceptance tests for non-additive metric totals (Table chart). + +Companion to tests/unit_tests/charts/test_non_additive_totals.py and the +root SIP.md. These exercise the real ``api/v1/chart/data`` query path against +the example ``birth_names`` table. + +Findings these tests pin down: + +* **Bucket A (SQL-aggregate grand total)** is already correct for the Table + chart, because the "Show summary" row is produced by a *separate query with + no GROUP BY* (``columns=[]``). The database evaluates the metric expression + over all rows, so a ratio / distinct count is right even though summing the + per-group cells would be wrong. These are asserted as regression guards. +* **Bucket B (post-processing % columns)** was broken in the *frontend*: + ``plugin-chart-table/buildQuery.ts`` built the totals query with + ``post_processing=[]``, so a contribution / percent column never appeared in + the summary row (#37627, #34350). The fix retains post-processing on that + query; the guard below pins the backend capability the fix relies on (a + no-GROUP-BY summary query computes the percent column = 100%). +""" + +from __future__ import annotations + +from typing import Any + +import numpy as np +import pytest + +from superset.charts.schemas import ChartDataQueryContextSchema +from superset.common.grouping_sets import split_grouping_sets_result +from superset.common.query_context import QueryContext +from superset.utils.core import QueryStatus +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.fixtures.birth_names_dashboard import ( + load_birth_names_dashboard_with_slices, # noqa: F401 + load_birth_names_data, # noqa: F401 +) +from tests.integration_tests.fixtures.query_context import get_query_context + +# A non-additive ratio metric: fraction of births in California. +# Inlined CASE over physical columns (num, state); * 1.0 forces float division +# so SQLite doesn't truncate the ratio to integer 0. +RATIO_METRIC = { + "expressionType": "SQL", + "sqlExpression": "SUM(CASE WHEN state = 'CA' THEN num ELSE 0 END) * 1.0 / SUM(num)", + "label": "ca_share", +} + +# COUNT_DISTINCT over a low-cardinality column (gender) that recurs across the +# groupby (state). The per-group distinct counts therefore overlap heavily, so +# summing them double-counts -- which is exactly what the grand total must avoid. +DISTINCT_METRIC = { + "expressionType": "SIMPLE", + "column": {"column_name": "gender"}, + "aggregate": "COUNT_DISTINCT", + "label": "distinct_genders", +} + + +def _result_df(payload: dict[str, Any]): + # Use get_query_result (not get_payload) to avoid the flask-caching/Redis + # layer, so these tests run against just the metadata + results DB. + query_context: QueryContext = ChartDataQueryContextSchema().load(payload) + query_object = query_context.queries[0] + result = query_context.get_query_result(query_object) + assert result.status == QueryStatus.SUCCESS, result.errors + return result.df + + +def _base_payload( + metric: dict[str, Any], columns: list[str], clear_filters: bool = False +): + payload = get_query_context("birth_names") + query = payload["queries"][0] + query["metrics"] = [metric] + query["columns"] = columns + query["groupby"] = columns + query["orderby"] = [] + query["post_processing"] = [] + query["is_timeseries"] = False + query["row_limit"] = None + if clear_filters: + # Drop the default gender='boy' filter so both genders are present and + # genuinely recur across states. + query["filters"] = [] + return payload + + +@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") +class TestNonAdditiveTotalsTable(SupersetTestCase): + def test_ratio_grand_total_is_db_computed_not_summed(self): + """ + Bucket A regression guard: the grand total of a ratio metric is the + ratio of the summed parts (DB-computed at no-GROUP-BY), and is NOT the + sum of the per-group ratios. + """ + self.login("admin") + + # Per-group ratios (grouped by state). + per_group = _result_df(_base_payload(RATIO_METRIC, ["state"])) + summed_ratios = per_group["ca_share"].sum() + + # Grand total: no GROUP BY -> the summary-row query the table chart runs. + grand_total_df = _result_df(_base_payload(RATIO_METRIC, [])) + grand_total = grand_total_df["ca_share"].iloc[0] + + # The grand total is a genuine ratio in [0, 1] ... + assert 0.0 <= grand_total <= 1.0 + # ... and summing the per-group ratios is the wrong answer the SIP fixes + # for pivot subtotals; the table grand-total path already avoids it. + assert not np.isclose(grand_total, summed_ratios), ( + f"grand total {grand_total} should differ from summed per-group " + f"ratios {summed_ratios}" + ) + + def test_distinct_count_grand_total_is_db_computed(self): + """ + Bucket A regression guard: COUNT_DISTINCT grand total is computed over + all rows, so it equals the true distinct count and is strictly less than + the sum of per-group distinct counts when members recur across groups + (gender recurs across every state, so summing double-counts). + """ + self.login("admin") + + per_group = _result_df( + _base_payload(DISTINCT_METRIC, ["state"], clear_filters=True) + ) + summed = per_group["distinct_genders"].sum() + + grand_total_df = _result_df( + _base_payload(DISTINCT_METRIC, [], clear_filters=True) + ) + grand_total = grand_total_df["distinct_genders"].iloc[0] + + # Grand total is the true number of distinct genders (boy + girl) ... + assert grand_total == 2 + # ... and summing per-state distinct counts double-counts (2 per state + # across many states), so the naive sum is strictly larger. + assert grand_total < summed + + def test_backend_computes_percent_column_for_summary_query(self): + """ + Bucket B backend-capability guard. + + The #37627 bug is in the frontend (``plugin-chart-table/buildQuery.ts`` + built the totals query with ``post_processing=[]``). The backend itself + can compute the contribution/percent column on a no-GROUP-BY summary + query: the total's contribution to itself is 100%. This guard pins that + capability so the frontend fix (retaining post_processing on the totals + query) produces a correct summary % rather than an empty one. + """ + self.login("admin") + + # Mirror the *fixed* frontend totals query: no GROUP BY, but with the + # percent-metric contribution op retained. + totals_payload = _base_payload({"label": "sum__num"}, []) + totals_payload["queries"][0]["post_processing"] = [ + { + "operation": "contribution", + "options": { + "columns": ["sum__num"], + "orientation": "column", + "rename_columns": ["sum__num_pct"], + }, + } + ] + totals_df = _result_df(totals_payload) + + assert "sum__num_pct" in totals_df.columns + # Single total row -> its contribution to itself is 100%. + assert totals_df["sum__num_pct"].iloc[0] == pytest.approx(1.0) + + +@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") +class TestTablePercentMetricSummary(SupersetTestCase): + """ + #37627 / #34350: a Table "Percentage metrics" column must appear in the + summary row when "Show summary" is on. The fix retains the contribution + post-processing on the totals query (plugin-chart-table buildQuery), so the + summary row recomputes the percent column instead of omitting it. The main + column is unaffected (verified: identical with/without the fix). + """ + + def test_percent_metric_present_in_summary_and_nonzero_in_body(self): + self.login("admin") + pct = { + "expressionType": "SIMPLE", + "column": {"column_name": "num"}, + "aggregate": "SUM", + "label": "sum__num", + } + contribution = { + "operation": "contribution", + "options": { + "columns": ["sum__num"], + "rename_columns": ["%sum__num"], + }, + } + main_query = { + "columns": ["state"], + "metrics": [pct], + "post_processing": [contribution], + "orderby": [], + "row_limit": 100, + "is_timeseries": False, + } + # Mirrors the fixed frontend buildQuery: the show_totals query retains + # the contribution op (was post_processing=[]). + totals_query = { + "columns": [], + "metrics": [pct], + "post_processing": [contribution], + "orderby": [], + "row_limit": 0, + "is_timeseries": False, + } + payload = { + "datasource": {"id": self.get_birth_names_dataset().id, "type": "table"}, + "queries": [main_query, totals_query], + "result_type": "full", + } + query_context: QueryContext = ChartDataQueryContextSchema().load(payload) + response = query_context.get_payload() + + body = response["queries"][0]["data"] + summary = response["queries"][1]["data"] + # Body percent column is computed (non-zero for rows with data). + assert any(row.get("%sum__num") for row in body) + # The fix: the percent column is present in the summary row too. + assert summary + assert "%sum__num" in summary[0] + + def get_birth_names_dataset(self): + from superset import db + from superset.connectors.sqla.models import SqlaTable + + return db.session.query(SqlaTable).filter_by(table_name="birth_names").one() + + +@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") +class TestGroupingSetsRollup(SupersetTestCase): + """ + Phase 3b: a `grouping_sets` query returns every rollup level tagged with + GROUPING()-equivalent markers, regardless of engine. Engines that support + GROUPING SETS (Postgres) compute it in one native query + (get_sqla_query); engines that don't (SQLite) use the per-level fallback in + QueryContextProcessor. Either way the combined result is identical in shape, + so this test exercises both paths. + """ + + def test_grouping_sets_returns_all_levels(self): + self.login("admin") + ratio = { + "expressionType": "SQL", + "sqlExpression": "SUM(CASE WHEN state = 'CA' THEN num ELSE 0 END) " + "* 1.0 / SUM(num)", + "label": "ca_share", + } + levels = [["gender", "state"], ["gender"], []] + payload = { + "datasource": {"id": 1, "type": "table"}, + "queries": [ + { + "columns": ["gender", "state"], + "metrics": [ratio], + "grouping_sets": levels, + "orderby": [], + "row_limit": 50000, + "is_timeseries": False, + } + ], + "result_type": "full", + } + # birth_names dataset id is environment-specific; resolve it. + from superset import db + from superset.connectors.sqla.models import SqlaTable + + table = db.session.query(SqlaTable).filter_by(table_name="birth_names").one() + payload["datasource"]["id"] = table.id + + query_context: QueryContext = ChartDataQueryContextSchema().load(payload) + df = query_context.get_query_result(query_context.queries[0]).df + + # One query produced the GROUPING() markers for both groupby columns. + assert "gender__superset_grouping" in df.columns + assert "state__superset_grouping" in df.columns + + leaf, gender_sub, grand = split_grouping_sets_result( + df, levels, ["gender", "state"] + ) + # Markers are stripped from the split frames. + assert "gender__superset_grouping" not in leaf.columns + # Grand total is a single, valid ratio (DB-computed, not summed cells). + assert len(grand) == 1 + assert 0.0 <= grand["ca_share"].iloc[0] <= 1.0 + # One subtotal row per gender. + assert len(gender_sub) == 2 diff --git a/tests/unit_tests/charts/test_non_additive_totals.py b/tests/unit_tests/charts/test_non_additive_totals.py new file mode 100644 index 00000000000..5c40c366464 --- /dev/null +++ b/tests/unit_tests/charts/test_non_additive_totals.py @@ -0,0 +1,136 @@ +# 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. + +""" +TDD acceptance matrix for non-additive metric totals/subtotals. + +See SIP.md (root) for the full proposal. These tests encode the known reported +cases as the spec the POC must satisfy. Cases the current code already gets +right are asserted as regression guards; cases it gets wrong are marked +``xfail(strict=True)`` so they flip to a hard failure the moment the POC makes +them pass (at which point the marker is removed). + +Principle under test: a total/subtotal for a non-additive metric must be +computed at the relevant grouping granularity, never by summing the +already-aggregated per-row cells. +""" + +from __future__ import annotations + +import pytest +from pandas import DataFrame + +from superset.utils.core import PostProcessingContributionOrientation +from superset.utils.pandas_postprocessing import contribution + +# --------------------------------------------------------------------------- +# Bucket B — post-processing (percentage / contribution) columns +# --------------------------------------------------------------------------- + + +def test_contribution_zeroes_when_total_missing() -> None: + """ + Characterization of the #37627 / #34350 root cause. + + ``contribution`` zeroes an entire percentage column whenever that column's + total is absent from ``contribution_totals`` (or is 0). The table chart's + "Show summary" extra query is built with ``post_processing: []`` and so does + not surface these totals, which is exactly how the percentage column ends up + displaying only zeros in the summary row. + + This documents *current* behavior (passes today); the POC must stop relying + on a totals dict that can silently miss columns. + """ + df = DataFrame({"actual": [10.0, 30.0], "target": [40.0, 60.0]}) + result = contribution( + df.copy(), + orientation=PostProcessingContributionOrientation.COLUMN, + columns=["actual"], + rename_columns=["pct_actual"], + # "actual" deliberately missing -> the bug surface + contribution_totals={"target": 100.0}, + ) + assert result["pct_actual"].tolist() == [0, 0] + + +# --------------------------------------------------------------------------- +# Bucket A — SQL-aggregate metrics (ratios / distinct counts) +# --------------------------------------------------------------------------- + + +@pytest.mark.xfail( + strict=True, + reason="POC not implemented: ratio totals are summed cell-wise, not " + "recomputed as SUM(num)/SUM(den) at the total grouping level (#25747).", +) +def test_ratio_total_is_recomputed_not_summed() -> None: + """ + #25747 / #32260 / #38674: a completion-rate metric SUM(actual)/SUM(target). + + Per-group rows already hold the divided ratio. The correct grand total is + SUM(actual)/SUM(target) = 40/100 = 0.4, NOT the sum (0.25 + 0.5 = 0.75) or + mean (0.375) of the per-group ratios. + + Stand-in for the totals computation the POC introduces; importing the real + helper here will replace ``_total_ratio`` once it exists. + """ + per_group = DataFrame( + { + "actual": [10.0, 30.0], + "target": [40.0, 60.0], + "completion": [10.0 / 40.0, 30.0 / 60.0], # 0.25, 0.5 + } + ) + # Current (broken) derivation sums the cells: + summed = per_group["completion"].sum() + assert summed == pytest.approx(0.4), ( + f"ratio total should be SUM(actual)/SUM(target)=0.4, got {summed}" + ) + + +@pytest.mark.xfail( + strict=True, + reason="POC not implemented: COUNT_DISTINCT subtotals are summed across " + "groups, double-counting overlapping members (#36165).", +) +def test_distinct_count_total_is_recomputed_not_summed() -> None: + """ + #36165: COUNT(DISTINCT user) per group cannot be summed for the total. + + Groups A and B each have 2 distinct users but share one user, so the true + distinct total is 3, not 2 + 2 = 4. + """ + per_group_distinct = DataFrame({"group": ["A", "B"], "distinct_users": [2, 2]}) + true_total = 3 # |{u1,u2} ∪ {u2,u3}| + summed = per_group_distinct["distinct_users"].sum() + assert summed == true_total, ( + f"distinct total should be recomputed (=3), got summed value {summed}" + ) + + +# --------------------------------------------------------------------------- +# Regression guard — additive metrics must stay on the cheap path +# --------------------------------------------------------------------------- + + +def test_additive_total_is_plain_sum() -> None: + """ + Additive metrics (SUM/COUNT/MIN/MAX) are correct via the existing cheap + summation and must remain unchanged (no extra queries, same numbers). + """ + per_group = DataFrame({"revenue": [10.0, 30.0, 60.0]}) + assert per_group["revenue"].sum() == pytest.approx(100.0) diff --git a/tests/unit_tests/common/test_grouping_sets.py b/tests/unit_tests/common/test_grouping_sets.py new file mode 100644 index 00000000000..9a4d1900632 --- /dev/null +++ b/tests/unit_tests/common/test_grouping_sets.py @@ -0,0 +1,112 @@ +# 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 pandas as pd +from sqlalchemy import column, select +from sqlalchemy.dialects import postgresql + +from superset.common.grouping_sets import ( + grouping_id_column, + grouping_marker_label, + grouping_sets_clause, + split_grouping_sets_result, +) + + +def _compile(stmt) -> str: + return str( + stmt.compile( + dialect=postgresql.dialect(), + compile_kwargs={"literal_binds": True}, + ) + ).replace("\n", " ") + + +def test_grouping_sets_clause_emits_rollup_levels() -> None: + a, b = column("a"), column("b") + # rollup hierarchy: leaf (a, b), row subtotal (a), grand total () + stmt = select(a, b).group_by(grouping_sets_clause([[a, b], [a], []])) + sql = _compile(stmt) + assert "GROUPING SETS((a, b), (a), ())" in sql + + +def test_grouping_sets_single_level() -> None: + a = column("a") + stmt = select(a).group_by(grouping_sets_clause([[a]])) + assert "GROUPING SETS((a))" in _compile(stmt) + + +def test_grouping_id_marker_column() -> None: + a = column("a") + stmt = select(a, grouping_id_column(a, "a__grouping")) + sql = _compile(stmt).lower() + assert "grouping(a) as a__grouping" in sql + + +def _gm(col: str) -> str: + return grouping_marker_label(col) + + +def test_split_grouping_sets_result_by_level() -> None: + # Combined GROUPING SETS result for groupby [region, topic] with levels + # leaf [region, topic], row subtotal [region], grand total []. + df = pd.DataFrame( + [ + { + "region": "US", + "topic": "a", + "value": 10, + _gm("region"): 0, + _gm("topic"): 0, + }, + { + "region": "US", + "topic": "b", + "value": 20, + _gm("region"): 0, + _gm("topic"): 0, + }, + { + "region": "US", + "topic": None, + "value": 30, + _gm("region"): 0, + _gm("topic"): 1, + }, + { + "region": None, + "topic": None, + "value": 60, + _gm("region"): 1, + _gm("topic"): 1, + }, + ] + ) + leaf, subtotal, grand = split_grouping_sets_result( + df, + [["region", "topic"], ["region"], []], + ["region", "topic"], + ) + + # Marker columns are stripped from every level. + for frame in (leaf, subtotal, grand): + assert not any(col.endswith("__superset_grouping") for col in frame.columns) + + assert leaf["value"].tolist() == [10, 20] + assert subtotal["value"].tolist() == [30] + assert subtotal["region"].tolist() == ["US"] + assert grand["value"].tolist() == [60] diff --git a/tests/unit_tests/common/test_query_context_processor.py b/tests/unit_tests/common/test_query_context_processor.py index a60228cd24f..a2f259dd627 100644 --- a/tests/unit_tests/common/test_query_context_processor.py +++ b/tests/unit_tests/common/test_query_context_processor.py @@ -2151,3 +2151,108 @@ def test_raise_for_access_evaluates_access_before_validate(): processor.raise_for_access() query.validate.assert_not_called() + + +def test_grouping_sets_fallback_handles_adhoc_and_physical_columns() -> None: + """ + The fallback used on engines without native GROUPING SETS support must + build ``label_to_column`` from the same labels used to run each level's + subquery, for both physical (string) and adhoc (dict) columns, and in the + original column order. Otherwise an adhoc column ahead of a physical one + in ``query_object.columns`` misaligns the mapping, and adhoc groupby + columns referenced in ``grouping_sets`` are silently dropped. + """ + import copy + from datetime import timedelta + + from superset.common.query_object import QueryObject + from superset.models.helpers import QueryResult + from superset.superset_typing import AdhocColumn + + adhoc_col: AdhocColumn = { + "sqlExpression": "DATE_TRUNC('month', dttm)", + "label": "month", + } + mock_datasource = MagicMock() + + query_obj = QueryObject( + datasource=mock_datasource, + columns=[adhoc_col, "state"], + grouping_sets=[["month", "state"], ["month"], []], + ) + + processor = QueryContextProcessor(MagicMock()) + processor._qc_datasource = mock_datasource + + captured_columns: list[list[Any]] = [] + + def fake_get_query_result(sub_query: QueryObject) -> QueryResult: + captured_columns.append(copy.copy(sub_query.columns)) + return QueryResult( + df=pd.DataFrame({"metric": [1]}), + query="SELECT 1", + duration=timedelta(seconds=0), + ) + + mock_datasource.get_query_result.side_effect = fake_get_query_result + + processor._grouping_sets_fallback(query_obj) + + # Level ["month", "state"] must include both the adhoc and physical column, + # each mapped to its own definition (not swapped). + assert captured_columns[0] == [adhoc_col, "state"] + # Level ["month"] must still include the adhoc column, not drop it. + assert captured_columns[1] == [adhoc_col] + # Grand total level has no groupby columns. + assert captured_columns[2] == [] + + +def test_grouping_sets_fallback_applies_row_offset_once_globally() -> None: + """ + The native GROUPING SETS path applies `row_offset` exactly once, to the + combined multi-level result (see the unconditional `qry.offset()` call in + `models/helpers.py`). The fallback must match that: it must not apply the + same offset independently to every per-level subquery, since that would + apply it once per level (and can drop an entire low-row-count level, e.g. + a single grand-total row, outright). + """ + from datetime import timedelta + + from superset.common.query_object import QueryObject + from superset.models.helpers import QueryResult + + mock_datasource = MagicMock() + + query_obj = QueryObject( + datasource=mock_datasource, + columns=["state"], + grouping_sets=[["state"], []], + row_offset=1, + ) + + processor = QueryContextProcessor(MagicMock()) + processor._qc_datasource = mock_datasource + + captured_offsets: list[int] = [] + + # Each level returns 2 rows regardless of the (should-be-ignored) offset, + # emulating a real datasource that would otherwise apply row_offset itself. + def fake_get_query_result(sub_query: QueryObject) -> QueryResult: + captured_offsets.append(sub_query.row_offset) + return QueryResult( + df=pd.DataFrame({"state": ["CA", "NY"]}) + if sub_query.columns + else pd.DataFrame({"state": ["total"]}), + query="SELECT 1", + duration=timedelta(seconds=0), + ) + + mock_datasource.get_query_result.side_effect = fake_get_query_result + + result = processor._grouping_sets_fallback(query_obj) + + # Each per-level subquery must run unoffset... + assert captured_offsets == [0, 0] + # ...and the requested offset is applied exactly once, to the combined + # result: 2 + 1 = 3 total rows in, minus an offset of 1 = 2 rows out. + assert len(result.df) == 2 diff --git a/tests/unit_tests/db_engine_specs/test_grouping_sets_capability.py b/tests/unit_tests/db_engine_specs/test_grouping_sets_capability.py new file mode 100644 index 00000000000..6723ee1207d --- /dev/null +++ b/tests/unit_tests/db_engine_specs/test_grouping_sets_capability.py @@ -0,0 +1,59 @@ +# 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. + +""" +The ``supports_grouping_sets`` engine capability gates the single-query +GROUPING SETS collapse of the pivot table's per-rollup-level queries (SIP.md, +phase 3). It defaults to False and is opted into by engines with native support. +""" + +from superset.db_engine_specs.base import BaseEngineSpec +from superset.db_engine_specs.bigquery import BigQueryEngineSpec +from superset.db_engine_specs.databricks import DatabricksHiveEngineSpec +from superset.db_engine_specs.hive import HiveEngineSpec +from superset.db_engine_specs.postgres import PostgresEngineSpec +from superset.db_engine_specs.presto import PrestoBaseEngineSpec, PrestoEngineSpec +from superset.db_engine_specs.snowflake import SnowflakeEngineSpec +from superset.db_engine_specs.spark import SparkEngineSpec +from superset.db_engine_specs.sqlite import SqliteEngineSpec +from superset.db_engine_specs.trino import TrinoEngineSpec + + +def test_base_default_is_false() -> None: + assert BaseEngineSpec.supports_grouping_sets is False + # SQLite has no GROUPING SETS support and must keep the conservative default. + assert SqliteEngineSpec.supports_grouping_sets is False + # The shared Presto/Trino base doesn't opt in itself; each concrete engine + # (or an explicit override, for the Hive family below) decides. + assert PrestoBaseEngineSpec.supports_grouping_sets is False + + +def test_supporting_engines_opt_in() -> None: + assert PostgresEngineSpec.supports_grouping_sets is True + assert BigQueryEngineSpec.supports_grouping_sets is True + assert SnowflakeEngineSpec.supports_grouping_sets is True + assert PrestoEngineSpec.supports_grouping_sets is True + assert TrinoEngineSpec.supports_grouping_sets is True + + +def test_hive_family_explicitly_opts_out() -> None: + # Hive/Spark/Databricks-Hive extend PrestoEngineSpec but their GROUPING + # SETS + GROUPING() marker semantics haven't been verified against this + # query pattern, so they must not silently inherit native support. + assert HiveEngineSpec.supports_grouping_sets is False + assert SparkEngineSpec.supports_grouping_sets is False + assert DatabricksHiveEngineSpec.supports_grouping_sets is False diff --git a/tests/unit_tests/queries/query_object_test.py b/tests/unit_tests/queries/query_object_test.py index 0d299e4722e..15fbc13c3eb 100644 --- a/tests/unit_tests/queries/query_object_test.py +++ b/tests/unit_tests/queries/query_object_test.py @@ -45,6 +45,7 @@ def test_default_query_object_to_dict(): "from_dttm": None, "granularity": None, "group_others_when_limit_reached": False, + "grouping_sets": [], "inner_from_dttm": None, "inner_to_dttm": None, "is_rowcount": False,