sadpandajoe commented on code in PR #44657:
URL: https://github.com/apache/superset/pull/44657#discussion_r4257446696
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/transformProps.test.ts:
##########
@@ -601,3 +601,35 @@ test('conditional formatting on the additive path uses the
raw leaf query rows',
// Scale spans the leaf cells (max 40), never the client-side grand total
100.
expect(getColorFromValue(40)).toEqual('#ACE1C4FF');
});
+
+test('result aggregation passes the leaf query data through as a single,
unsynthesized level', () => {
+ const resultChartProps = new ChartProps<QueryFormData>({
+ ...chartProps,
+ formData: { ...formData, aggregateFunction: 'Median' },
Review Comment:
Every fixture in this file uses a saved-metric string, which
`allMetricsAdditive` treats as non-additive, so nothing exercises the new
`resultAggregation` branch winning over the additive fast path. With a
`SIMPLE`/`SUM(sales)` metric, stores A = 10 and B = 30, `aggregateFunction:
'Median'` and totals on, putting the additive check ahead of this branch would
pass every current test while synthesizing an extra total row of 40, so Median
over 10/30/40 renders 30 instead of 20. Could you add a case with an additive
adhoc metric that asserts a single unsynthesized level holding just the two
leaf rows?
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/buildQuery.ts:
##########
@@ -86,17 +87,23 @@ export default function buildQuery(formData:
PivotTableQueryFormData) {
// per displayed total/subtotal) so the database computes every level; the
// backend falls back to per-level queries on engines without native
// support. transformProps splits the combined result by level.
+ // - result aggregation (see resultAggregation.ts): always a full-detail
+ // query, regardless of additivity -- every scope reduces its own
+ // original leaf records client-side, so there is nothing for the
+ // database to roll up in advance.
const additive = allMetricsAdditive(ensureIsArray(formData.metrics));
- const groupingSets = additive
- ? undefined
- : buildGroupbyCombinations(formData).map(level =>
- // A dimension placed on both axes (a valid, if unusual, config) would
- // otherwise appear twice in the same level, producing a duplicate
- // column in the GROUPING SETS tuple sent to the database.
- Array.from(
- new Set([...level.rows, ...level.columns].map(getColumnLabel)),
- ),
- );
+ const resultAggregation = getResultAggregation(formData.aggregateFunction);
+ const groupingSets =
+ additive || resultAggregation
Review Comment:
This branch skips `grouping_sets` when a result aggregation is selected, but
the Python query builder behind the MCP chart tools isn't aware of
`aggregateFunction`: `_pivot_query` in
`superset/common/form_data_query_context.py` (around line 818) still adds
`grouping_sets` whenever the metrics are non-additive. So an unsaved pivot with
rows `[store]`, an `AVG(sales)` metric, `aggregateFunction: "Sum"`, totals on
and `row_limit: 1` renders in Explore from the single leaf query (B = 40),
while `get_chart_sql` / `get_chart_data` for the same `form_data_key` build a
GROUPING SETS query that skips the row limit and returns both stores plus the
database rollup. Should that builder take the same leaf-only path when
`getResultAggregation` would be set, so the MCP output matches the chart?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
