jesperct commented on code in PR #40031:
URL: https://github.com/apache/superset/pull/40031#discussion_r3249804886
##########
superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts:
##########
@@ -141,9 +141,7 @@ function buildExistingColumnsSet(chart: ChartQueryPayload):
Set<string> {
const existingColumns = new Set<string>();
const chartType = chart.form_data?.viz_type;
- const existingGroupBy = ensureIsArray(chart.form_data?.groupby);
- extractColumnNames(existingGroupBy).forEach(col => existingColumns.add(col));
Review Comment:
That line isn't arbitrary. #39416 added it to fix a regression from #39356.
The thing is, it only ever protected one case: if the user's whole selection is
a subset of columns the chart already uses, the customization gets skipped and
the base groupby survives. The partial-overlap case is where it breaks. User
picks one overlapping column plus a new one, the overlapping one gets filtered
out as a conflict, and the chart ends up grouped by just the new column with
the base dimension silently gone. That's the bug.
What this PR does is make Dynamic Group By always replace the base groupby
with the user's selection, which is the feature's intended behavior anyway. The
safety net is still there: with no matching customization or an empty
selection, `processGroupByCustomizations` bails early and never touches the
base groupby, so charts that lean on their base dimension are fine when
nobody's using the feature. `applyChartSpecificGroupBy` also still falls back
to the base groupby for the chart types that need it, and AdhocColumn
extraction stays for box_plot/pivot.
Can add more tests if you want specific scenarios locked down.
--
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]
