aminghadersohi commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4191748297
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -2630,3 +2648,128 @@ def preserve_previous_adhoc_filters(
merged_filters.append(generated_filter)
new_form_data["adhoc_filters"] = merged_filters
+
+
+def _apply_geographic_temporal_update(
+ form_data: dict[str, Any], config: ChartConfig
+) -> None:
+ """Apply explicit time changes after native controls have been merged."""
+ if "temporal_column" not in config.model_fields_set:
+ return
+ # Native granularity takes precedence over the requested adhoc binding.
+ form_data.pop("granularity_sqla", None)
+ if config.temporal_column is None:
+ form_data["adhoc_filters"] =
_without_generated_dashboard_time_filter(form_data)
+ form_data.pop(MCP_DASHBOARD_TIME_FILTER_SUBJECT, None)
+
+
+def merge_geographic_update_form_data( # noqa: C901
+ existing: dict[str, Any],
+ mapped: dict[str, Any],
+ config: ChartConfig,
+ *,
+ dataset_rebind: bool,
+) -> dict[str, Any]:
+ """Preserve omitted native controls; clear explicit nullable roles and
filters.
+
+ Rebinding drops old dataset references and Jinja/query state. All optional
+ roles are remapped from the complete target-dataset config.
+ """
+ if dataset_rebind:
+ existing = {
+ key: value
+ for key, value in existing.items()
+ if key
+ in {
+ "linear_color_scheme",
+ "number_format",
+ "max_bubble_size",
+ "point_unit",
+ "color_by",
+ "color_picker",
+ "color_scheme",
+ "y_axis_format",
+ "map_renderer",
+ "maplibre_style",
+ "mapbox_style",
+ "min_radius",
+ "max_radius",
+ "multiplier",
+ }
+ }
+ field_map = {
+ "country": "select_country",
+ "region_format": "region_format",
+ "country_format": "country_fieldtype",
+ "entity": "entity",
+ "metric": "metric",
+ "secondary_metric": "secondary_metric",
+ "dimension": "dimension",
+ "show_bubbles": "show_bubbles",
+ "max_bubble_size": "max_bubble_size",
+ "sort_by_metric": "sort_by_metric",
+ "linear_color_scheme": "linear_color_scheme",
+ "number_format": "number_format",
+ "row_limit": "row_limit",
+ "time_range": "time_range",
+ "point_unit": "point_unit",
+ }
+ fields = config.model_fields_set
+ patch = dict(mapped)
+ if "temporal_column" in fields and config.temporal_column is None:
+ patch["adhoc_filters"] =
_without_generated_dashboard_time_filter(patch)
+ elif "temporal_column" not in fields and not dataset_rebind:
+ # Omission keeps the saved dashboard-time binding instead of remapping
+ # it to the dataset default the mapper generated for this update.
+ # An absent marker also preserves an explicitly cleared binding.
+ saved_binding = [
+ filter_
+ for filter_ in existing.get("adhoc_filters") or []
+ if filter_ not in
_without_generated_dashboard_time_filter(existing)
+ ]
+ patch["adhoc_filters"] = [
+ *saved_binding,
+ *_without_generated_dashboard_time_filter(patch),
+ ]
+ if binding := existing.get(MCP_DASHBOARD_TIME_FILTER_SUBJECT):
+ patch[MCP_DASHBOARD_TIME_FILTER_SUBJECT] = binding
+ else:
+ patch.pop(MCP_DASHBOARD_TIME_FILTER_SUBJECT, None)
+ # Native UI-only presentation controls are not part of this public config.
+ for key in (
+ "color_by",
+ "color_picker",
+ "color_scheme",
+ "y_axis_format",
+ "map_renderer",
+ "maplibre_style",
+ "mapbox_style",
+ "viewport",
+ "autozoom",
+ "min_radius",
+ "max_radius",
+ "multiplier",
+ ):
+ if key in existing:
+ patch.pop(key, None)
+ for source, target in field_map.items():
+ if source not in fields and target in existing:
+ patch.pop(target, None)
+ if not {"radius", "radius_metric"} & fields and "point_radius_fixed" in
existing:
+ patch.pop("point_radius_fixed", None)
+ if "filters" not in fields:
+ source_form = dict(existing)
+ if "temporal_column" in fields:
+ source_form["adhoc_filters"] =
_without_generated_dashboard_time_filter(
+ existing
+ )
+ preserve_previous_adhoc_filters(patch, source_form)
+ merged = {**existing, **patch}
Review Comment:
Confirmed and fixed in e97884ace97ecf8302e657abdf4f395022fb866f.
Scatter's merged-form validation checks the effective dimension against both
coordinates, including a saved dimension omitted from the request. Saved
updates, unsaved previews, and cached-preview updates reject the collision
before querying or persisting. Explicit clearing/replacement and dataset
rebinding remain valid.
All six latitude/longitude collision cases failed before the fix because the
public tools accepted the update; all pass afterward. Eight additional cases
cover valid preservation, clearing, replacement, and rebinding. The chart unit
suite passed: **3,349 passed, 3 skipped**. Pre-commit passed on the staged
changes and all 92 branch-changed files, including MyPy and frontend checks.
Resolving this addressed thread.
--
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]
