aminghadersohi commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4174882045
##########
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:
Fixed in 1f1e69a4bce0bc5d3ee8be5deeaf2276cf3966e8. Same-dataset omission
preserves both an existing dashboard-time binding and the absent-marker state
after explicit clearing, rather than restoring the dataset default. The new
clear-then-omit regression failed before the fix for country_map, world_map,
and deck_scatter and passes afterward. Chart unit suite: 3,158 passed, 3
skipped; touched-file pre-commit (including MyPy) passed.
##########
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:
Fixed in 1f1e69a4bce0bc5d3ee8be5deeaf2276cf3966e8. Any explicit filters
replacement, empty or nonempty, removes inherited native filters, where, and
having. Three fail-before/pass-after regressions cover all geographic types and
check the prepared query retains the replacement predicate without the
inherited predicates. Chart unit suite: 3,158 passed, 3 skipped; touched-file
pre-commit passed.
##########
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:
Fixed in 1f1e69a4bce0bc5d3ee8be5deeaf2276cf3966e8. The typed
latitude/longitude guard applies only to the latlong spatial format; Geohash
and delimited formats use the native spatial decoder after an Explore switch.
Radius-metric validation remains active. Both format-switch regressions failed
before the fix (zero features) and pass afterward, checking decoded positions
and radius while skipping a negative-radius row. Scatter Jest suite: 3 suites,
57 tests passed; touched-file pre-commit, including frontend type checking,
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]
