sadpandajoe commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4174758430
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -2630,3 +2645,118 @@ 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)
+ 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
+ and existing.get(MCP_DASHBOARD_TIME_FILTER_SUBJECT)
Review Comment:
After `temporal_column: null` clears a map's dashboard-time binding, a later
same-dataset update omitting that field regenerates the default `created_at`
predicate because this branch requires the now-removed marker. Could omission
preserve the cleared state too, so an unrelated update does not silently
re-enable dashboard time filtering?
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Scatter/transformProps.ts:
##########
@@ -38,6 +39,48 @@ interface ScatterPoint {
[key: string]: unknown;
}
+function isFiniteNumber(value: unknown): value is number {
+ return typeof value === 'number' && Number.isFinite(value);
+}
+
+/**
+ * Keep only typed geographic points that can be drawn: numeric, in-range
+ * latitude/longitude and, for a metric radius, a finite nonnegative radius.
+ *
+ * The MCP data and export contract rejects such rows outright; the render
+ * layer skips them so one sparse row does not blank the whole map, and logs
+ * how many points were skipped so the loss is diagnosable.
+ */
+export function filterDrawableGeographicPoints(
+ records: DataRecord[],
+ spatial: DeckScatterFormData['spatial'],
+ radiusMetricLabel?: string,
+): DataRecord[] {
+ const coordinateColumns = [
Review Comment:
Opening an MCP-generated point chart in Explore and switching its spatial
control to Geohash or delimited coordinates keeps `mcp_geographic`, but removes
`latCol`/`lonCol`, so this filter discards every row before the native spatial
decoder runs. Could validation handle the selected spatial format or stop
applying the lat/long-only guard after that switch?
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -2630,3 +2645,118 @@ 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)
+ 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
+ and existing.get(MCP_DASHBOARD_TIME_FILTER_SUBJECT)
+ ):
+ # Omission keeps the saved dashboard-time binding instead of remapping
+ # it to the dataset default the mapper generated for this update.
+ saved_binding = [
+ filter_
+ for filter_ in existing.get("adhoc_filters", [])
+ if filter_ not in
_without_generated_dashboard_time_filter(existing)
+ ]
+ patch["adhoc_filters"] = [
+ *saved_binding,
+ *_without_generated_dashboard_time_filter(patch),
+ ]
+ patch[MCP_DASHBOARD_TIME_FILTER_SUBJECT] = existing[
+ MCP_DASHBOARD_TIME_FILTER_SUBJECT
+ ]
+ # 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:
+ merged.pop(MCP_DASHBOARD_TIME_FILTER_SUBJECT, None)
+ if "time_range" in fields and getattr(config, "time_range", None) is None:
+ merged.pop("time_range", None)
+ if "filters" in fields and not getattr(config, "filters", None):
Review Comment:
A nonempty replacement `filters` list leaves inherited native `filters`,
`where`, and `having` in place; query preparation then adds those predicates
back alongside the replacements. A saved `country IN ['USA']` filter updated to
`country IN ['Canada']` therefore becomes an empty intersection—could explicit
replacement filters discard the old native predicates just as an empty list
does?
--
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]
