gabotorresruiz commented on code in PR #43679:
URL: https://github.com/apache/superset/pull/43679#discussion_r3960785460
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -212,10 +215,29 @@ def _merge_replacement_config(
return merged
+def _build_replacement_form_data(
+ existing_form_data: dict[str, Any],
+ parsed_config: ChartConfig,
+ effective_dataset_id: int | None,
+ replacement_dataset_id: int | None = None,
+) -> dict[str, Any]:
+ """Map and merge a replacement config for both preview and save paths."""
+ new_form_data = map_config_to_form_data(
+ parsed_config, dataset_id=effective_dataset_id
+ )
+ new_form_data.pop("_mcp_warnings", None)
+ merge_table_column_config(existing_form_data, new_form_data)
+ merge_interactive_pivot_ui_config(existing_form_data, new_form_data)
+ merged = _merge_replacement_config(existing_form_data, new_form_data,
parsed_config)
Review Comment:
Not a blocker, but this worries me a bit for temporal charts:
`map_config_to_form_data` always emits `adhoc_filters` for a temporal x-axis
(the regenerated TEMPORAL_RANGE binding from `_ensure_temporal_adhoc_filter`),
so the shallow merge replaces the saved `adhoc_filters` wholesale even when
`filters` is omitted. I verified it on this branch: updating an
`echarts_timeseries_line` chart's metrics with no `filters` in the config drops
a saved `country = US` predicate from the merged params, and only the
regenerated temporal binding survives. So the omitted-filters preservation only
holds for chart types whose mapper does not emit `adhoc_filters`.
`update_chart_preview` already solves exactly this with
`_preserve_previous_adhoc_filters` (applied when `config.filters is None`).
Since this PR's goal is keeping the merge paths from drifting, would it make
sense to promote that helper and call it here too? A test like
`test_config_update_preserves_omitted_filters_on_temporal_chart` (existing
params carrying a SIMPLE predicate plus the temporal binding, config without
filters, assert the predicate survives) would lock it in.
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -212,10 +215,29 @@ def _merge_replacement_config(
return merged
Review Comment:
The explicit-empty escape hatch above covers `filters` but not its siblings.
`group_by` also defaults to `None`, so `group_by=[]` is a distinguishable
removal intent, yet the XY mapper only emits `groupby` when the list is
non-empty, and the shallow merge then resurrects the saved grouping. I verified
it on this branch: a same-type update with `group_by=[]` keeps `groupby:
["region"]` in the saved params, while the same call on master clears it since
the save path replaced form data wholesale. So an agent asking to remove a
breakdown gets a success response and an unchanged chart.
Suggest mirroring the filters check:
```python
if getattr(parsed_config, "group_by", None) == []:
merged.pop("groupby", None)
```
plus the same for `sort_by` / `order_by_cols` on table configs, with a
`test_config_update_explicit_empty_group_by_clears_grouping` to pin it.
`stacked` is a plain bool defaulting to False so it cannot signal removal this
way; `parsed_config.model_fields_set` could cover that too, but the two list
fields are the cheap wins.
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -230,12 +252,13 @@ def _build_update_payload(
)
if parsed_config is not None:
- new_form_data = map_config_to_form_data(
- parsed_config, dataset_id=effective_dataset_id
+ existing_form_data = _get_existing_form_data(chart)
+ new_form_data = _build_replacement_form_data(
+ existing_form_data,
+ parsed_config,
+ effective_dataset_id,
+ replacement_dataset_id=request.dataset_id,
Review Comment:
Question on dataset-plus-config updates: the same-type merge now carries the
old dataset's query fragments into the rebind. I verified on this branch that
rebinding a table chart to a new dataset keeps the old `groupby` and
`adhoc_filters` in the merged params, and validation then rejects the save with
"Filter references column(s) not in dataset" for a column the caller never
sent; on master the immediate save discards those keys and the rebind succeeds.
For a same-schema rebind the carryover is exactly what you want, so I am not
sure clean-replacing on dataset change is right either. Are we comfortable with
the failure mode where the rejection names carried-over columns rather than
anything in the request? If so, maybe worth a test pinning it so the behavior
is documented.
--
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]
