sadpandajoe commented on code in PR #44657:
URL: https://github.com/apache/superset/pull/44657#discussion_r4202406243
##########
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);
Review Comment:
Any recognized `aggregateFunction` now drops the GROUPING SETS query and
re-reduces the totals from leaf rows, and the MCP chart service writes
`aggregateFunction: "Sum"` on every pivot it creates
(`superset/mcp_service/chart/chart_utils.py:2027`, schema default `"Sum"`, with
no `"Metric"` choice). So an MCP-created pivot with rows `[country]` and a
`COUNT_DISTINCT(user_id)` metric, where US = {alice, bob} and FR = {alice}, now
totals 3 instead of the 2 the database computes, which brings back the
over-count this restructuring fixed. Those charts are also saved after the
migration runs, so they never get the review tag. Should the MCP path send
`"Metric"` (or omit the field) so non-additive metrics keep their
database-computed totals?
##########
superset-frontend/src/explore/actions/saveModalActions.ts:
##########
@@ -319,6 +320,26 @@ export const updateSlice =
),
);
}
+ if (formData?.viz_type === 'pivot_table_v2') {
+ // Saving is one of the two ways (alongside the
LegacyAggregationAlert's
+ // own "Accept" button) a user acknowledges a restored legacy result
+ // aggregation. Best-effort and silent: the vast majority of pivot
+ // table saves were never tagged, so a 404 here is the expected,
+ // common case, not a failure worth surfacing. Awaited (both on
+ // success and failure) before saveSliceSuccess dispatches below --
+ // that dispatch is what LegacyAggregationAlert re-fetches tags off
+ // of, so firing it before the DELETE has even been issued would
+ // race the refetch against the deletion and could leave the alert
+ // showing a tag this save already cleared.
+ await new Promise<void>(resolve => {
+ deleteTaggedObjects(
+ { objectType: 'chart', objectId: sliceId },
+ { name: LEGACY_AGGREGATION_TAG },
+ () => resolve(),
+ () => resolve(),
+ );
Review Comment:
The comment above says a 404 here is the common case (most pivots were never
tagged), but `saveModalActions.test.ts` only covers a successful DELETE; the
existing failure test fails the chart PUT and never reaches this branch. If
this error callback stops calling `resolve()`, saving an ordinary untagged
pivot would hang after the PUT succeeds and never dispatch
`SAVE_SLICE_SUCCESS`. Could you add a case where the PUT succeeds and the tag
DELETE returns 404, asserting `updateSlice` resolves with the saved chart and
dispatches success once and never `SAVE_SLICE_FAILED`?
--
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]
