aminghadersohi commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4180275313
##########
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", [])
Review Comment:
Fixed in ab73115d4e03cc3578391a9e50fc3c867b08ce04. Reproduced the TypeError
with failing saved-chart update regressions for country_map, world_map, and
deck_scatter before changing the implementation. Both the saved-binding
iteration and generated-filter helper treat null adhoc_filters as an empty
list. All six null-filter regressions pass, including explicit temporal clears.
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Scatter/transformProps.ts:
##########
@@ -38,6 +39,58 @@ 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.
+ * Other spatial formats use the native decoder after Explore format changes.
+ *
+ * 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 = [
+ [spatial?.latCol, 90],
+ [spatial?.lonCol, 180],
+ ] as const;
+ const reservedMetricLabel =
+ radiusMetricLabel !== undefined &&
+ ['position', 'weight', 'extraProps'].includes(radiusMetricLabel);
+ const drawable = records.filter(record => {
+ // Explore can change a metric label after MCP schema validation.
+ if (reservedMetricLabel) {
+ return false;
+ }
+ const validCoordinates =
+ spatial?.type !== 'latlong' ||
+ coordinateColumns.every(([column, bound]) => {
+ const value = column ? record[column] : undefined;
+ return isFiniteNumber(value) && Math.abs(value) <= bound;
+ });
+ if (!validCoordinates || !radiusMetricLabel) {
+ return validCoordinates;
+ }
+ const radius = record[radiusMetricLabel];
Review Comment:
Fixed in ab73115d4e03cc3578391a9e50fc3c867b08ce04. Transform regressions
first reproduced the loss of both valid points for legacy radii "100", "2.5",
and "0". The Scatter transform recognizes finite numeric-string radii as fixed
values and does not request a radius metric. Those points retain their fixed
radii; a separate regression confirms bare saved-metric names still work. All
47 Scatter transform tests pass.
--
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]
