aminghadersohi commented on code in PR #43679:
URL: https://github.com/apache/superset/pull/43679#discussion_r3961387001
##########
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:
Addressed in 6a4de207fb. Both replacement paths now call the shared
`preserve_previous_adhoc_filters` utility (the preview implementation was moved
to `chart_utils`), so mapper-generated temporal filters are combined with
omitted saved non-temporal predicates. The new temporal regression asserts
byte-for-byte filter parity between `_build_update_payload` and
`_build_preview_form_data`; the focused preview/save suite passes (118 tests).
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -212,10 +215,29 @@ def _merge_replacement_config(
return merged
Review Comment:
Addressed in 6a4de207fb. `_merge_replacement_config` uses `model_fields_set`
to distinguish omission from explicit empty values and removes inherited
`groupby`, `groupby_b`, and `order_by_cols` for explicit clears. Regression
tests exercise the public schema aliases (`groupby=[]` and `order_by_cols=[]`)
and assert identical save/preview clearing behavior.
##########
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:
Addressed in 6a4de207fb. Dataset rebinds inspect only query state that would
actually be inherited, validate its column/metric references against the
replacement dataset, preserve it when compatible, and otherwise build cleanly
from the caller-supplied config so stale unsent fields cannot cause validation
failure. The saved `params.datasource` is stamped to the replacement dataset.
Tests cover compatible and incompatible schemas plus save/preview construction
behavior.
--
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]
