sadpandajoe commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4180123621
##########
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:
Updating a saved Scatter chart with a legacy fixed radius such as
`point_radius_fixed: "100"` while omitting radius controls preserves that
string and adds `mcp_geographic`, but the frontend treats it as a metric name
and this filter removes every valid point. Could preserved numeric-string radii
be normalized as fixed values, with a transform regression asserting that the
points remain?
##########
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:
A saved native map with `adhoc_filters: null` is valid existing form data,
but a same-dataset update omitting `temporal_column` raises `TypeError` here
before compilation or saving. Could null filters be normalized to an empty list
before this iteration and the generated-filter helper, with a saved-chart
update regression?
--
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]
