aminghadersohi commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4152810431
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -2630,3 +2645,97 @@ def preserve_previous_adhoc_filters(
merged_filters.append(generated_filter)
new_form_data["adhoc_filters"] = merged_filters
+
+
+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)
+ # 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}
+ if "temporal_column" in fields and config.temporal_column is None:
Review Comment:
Fixed in ac2bee524a3b2cef862794af493753bd857ca5fa. Explicit temporal
clearing removes the newly mapped default predicate from the patch as well as
the previous generated binding and its marker. The regression maps against a
dataset with main_dttm_col and confirms that only the user-authored predicate
survives.
The regression was reproduced with a failing test before the fix.
Validation: 123 country-map/world-map Jest tests passed; 3,069 chart unit tests
passed (3 skipped); touched-file pre-commit passed.
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -2630,3 +2645,97 @@ def preserve_previous_adhoc_filters(
merged_filters.append(generated_filter)
new_form_data["adhoc_filters"] = merged_filters
+
+
+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)
+ # 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:
Review Comment:
Fixed in ac2bee524a3b2cef862794af493753bd857ca5fa. Inherited native row
limits are bounded before compilation and caching. A saved limit of 50000
becomes 10000, so the native query and geographic compile validation cover the
same rows. Regressions also verify preservation of a valid smaller limit and
normalization of malformed/nonpositive limits.
The regression was reproduced with a failing test before the fix.
Validation: 123 country-map/world-map Jest tests passed; 3,069 chart unit tests
passed (3 skipped); touched-file pre-commit passed.
--
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]
