jesperct commented on code in PR #40031:
URL: https://github.com/apache/superset/pull/40031#discussion_r3288541431
##########
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:
#39416 bundled two things. The real crash was the null entries in
`chart_customization_config`, fixed with the `.filter(Boolean)` guards and the
type-guard null check. This PR doesn't touch any of that, it only changes the
four lines in `buildExistingColumnsSet`, so that fix stays in place.
The other half was the groupby behavior, where #39416 made an overlapping
selection keep the base groupby. That's the part I'm changing on purpose:
keeping the base meant someone who picked an existing dimension plus a new one
only got the new one back, the existing one was silently dropped.
Replace only runs when there's a non-empty selection, and when the selection
adds no new column the base groupby is left as-is, so a chart never ends up
with no dimensions. Added a regression test for the all-conflicting case
(selecting only the x_axis), on top of the existing coverage for no selection,
metric conflict, out-of-scope and dataset mismatch.
--
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]
