[SIP] Correct totals/subtotals for non-additive metrics (Table + Pivot)

superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx

@@ -41,12 +41,13 @@ 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 {
   FilterType,
   MetricsLayoutEnum,
   PivotTableProps,
   PivotTableStylesProps,
+  QueryData,
   SelectedFiltersType,
 } from './types';
 
@@ -173,51 +174,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,
-  ),
-});
-
 /* If you change this logic, please update the corresponding Python
  * function (https://github.com/apache/superset/blob/master/superset/charts/post_processing.py),
  * or reach out to @betodealmeida.
@@ -232,7 +188,6 @@ export default function PivotTableChart(props: PivotTableProps) {
     metrics,
     colOrder,
     rowOrder,
-    aggregateFunction,
     transposePivot,
     combineMetric,
     rowSubtotalPosition,
@@ -341,22 +296,43 @@ export default function PivotTableChart(props: PivotTableProps) {
 
   const unpivotedData = useMemo(
     () =>
-      data.reduce(
-        (acc: Record<string, any>[], record: Record<string, any>) => [
-          ...acc,
-          ...metricNames
+      // `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];
+        }
+        return query.data.flatMap((record: Record<string, any>) =>
+          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).
+              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,
             }))
-            .filter(record => record.value !== null),
-        ],
-        [],
-      ),
-    [data, metricNames, currencyCodeColumn],
+            .filter(r => r.value !== null),
+        );
+      }),
+    [data, metricNames, currencyCodeColumn, metricsLayout, combineMetric],
   );
   const groupbyRows = useMemo(
     () => groupbyRowsRaw.map(getColumnLabel),
@@ -698,10 +674,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}

superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/buildQuery.ts

@@ -23,29 +23,33 @@ import {
   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<QueryFormColumn>(groupbyColumns),
-      ...ensureIsArray<QueryFormColumn>(groupbyRows),
+      ...ensureIsArray<QueryFormColumn>(groupby.rows),
+      ...ensureIsArray<QueryFormColumn>(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 +58,32 @@ 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;
+
+  // Additive fast-path: when every metric is additive (SUM/COUNT/MIN/MAX), the
+  // subtotals/grand totals can be derived by reducing the leaf rows on the
+  // client, so a single full-detail query suffices and transformProps
+  // synthesizes the rollup levels. Non-additive metrics need the database to
+  // compute each rollup level, so we emit one query per level (the combination
+  // order is fixed by buildGroupbyCombinations and relied upon by
+  // transformProps to map each result back to its level). See SIP.md.
+  const additive = allMetricsAdditive(ensureIsArray(formData.metrics));
+  const groupbyCombinations: Groupby[] = additive
+    ? [
+        {
+          rows: ensureIsArray<QueryFormColumn>(formData.groupbyRows),
+          columns: ensureIsArray<QueryFormColumn>(formData.groupbyColumns),
+        },
+      ]
+    : buildGroupbyCombinations(formData);
+  const queriesColumns: QueryFormColumn[][] = groupbyCombinations.map(groupby =>
+    getQueryColumns(groupby, formData, time_grain_sqla),
+  );
 
   return buildQueryContext(formData, baseQueryObject => {
     const { series_limit_metric, metrics, order_desc } = baseQueryObject;
@@ -63,12 +93,10 @@ export default function buildQuery(formData: PivotTableQueryFormData) {
     } else if (Array.isArray(metrics) && metrics[0]) {
       orderBy = [[metrics[0], !order_desc]];
     }
-    return [
-      {
-        ...baseQueryObject,
-        orderby: orderBy,
-        columns,
-      },
-    ];
+    return queriesColumns.map(columns => ({
+      ...baseQueryObject,
+      orderby: orderBy,
+      columns,
+    }));
   });
 }

superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx

@@ -163,44 +163,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',

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,13 @@ 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,
+  synthesizeAdditiveLevels,
+} from './utilities';
 
 const { DATABASE_DATETIME } = TimeFormats;
 
@@ -88,20 +97,60 @@ export default function transformProps(chartProps: ChartProps<QueryFormData>) {
     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<string, RollupReducer> = {};
+    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: each query is one rollup level; zip results back to their
+    // combination (same order as buildQuery).
+    const queryLength = Math.min(
+      queriesData.length,
+      groupbyCombinations.length,
+    );
+    data = [];
+    for (let i = 0; i < queryLength; i += 1) {
+      data.push({
+        data: queriesData[i].data,
+        groupby: groupbyCombinations[i],
+      });
+    }
+  }
+  // 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,
     metrics,
     tableRenderer,
     colOrder,
     rowOrder,
-    aggregateFunction,
     transposePivot,
     combineMetric,
     rowSubtotalPosition,
@@ -136,7 +185,7 @@ export default function transformProps(chartProps: ChartProps<QueryFormData>) {
           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 +203,7 @@ export default function transformProps(chartProps: ChartProps<QueryFormData>) {
     );
   const metricColorFormatters = getColorFormatters(
     conditionalFormatting,
-    data,
+    mainQuery.data,
     theme,
   );
 
@@ -170,7 +219,6 @@ export default function transformProps(chartProps: ChartProps<QueryFormData>) {
     tableRenderer,
     colOrder,
     rowOrder,
-    aggregateFunction,
     transposePivot,
     combineMetric,
     rowSubtotalPosition,