sadpandajoe commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r3983811834
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -796,13 +796,23 @@ async def update_chart( # noqa: C901
request.dataset_id is not None
and request.dataset_id != getattr(chart, "datasource_id", None)
and request.config is None
- and getattr(chart, "viz_type", None) == "gauge_chart"
+ and getattr(chart, "viz_type", None)
+ in {"gauge_chart", "country_map", "world_map", "deck_scatter"}
Review Comment:
A geographic rebind with the now-required config still takes
`_build_replacement_form_data`'s generic non-Gauge path, which resets
`dataset_rebind` to false and can silently retain an old filter when the target
dataset has the same column name. Should geographic configs keep the strict
rebind path so moving a country map between datasets always drops the previous
dataset's query state?
##########
superset/mcp_service/chart/query_result.py:
##########
@@ -223,3 +224,153 @@ def validate_gauge_query_result(
"""Check Gauge results using the same finite-dial contract as rendering."""
normalized = normalize_gauge_query_result(result, form_data)
return normalized if isinstance(normalized, ChartError) else None
+
+
+GEOGRAPHIC_VIZ_TYPES = frozenset({"country_map", "world_map", "deck_scatter"})
+
+
+def _geographic_metric_labels(form_data: Mapping[str, Any]) -> list[str]:
+ """Resolve metrics once, including fixed versus metric point sizing."""
+ if form_data.get("viz_type") == "deck_scatter":
+ radius = form_data.get("point_radius_fixed")
+ if not isinstance(radius, Mapping) or radius.get("type") not in {
+ "fix",
+ "metric",
+ }:
+ raise ValueError("Invalid geographic point radius configuration")
+ metrics = [radius.get("value")] if radius["type"] == "metric" else []
+ else:
+ metrics = [form_data.get("metric")]
+ secondary = form_data.get("secondary_metric")
+ if form_data.get("show_bubbles") and secondary is None:
+ raise ValueError("show_bubbles requires secondary_metric")
+ if secondary is not None:
+ metrics.append(secondary)
+ labels = [metric_result_label(metric) for metric in metrics]
+ if any(label is None for label in labels):
+ raise ValueError("Geographic metric has no resolvable result label")
+ return [label for label in labels if label is not None]
+
+
+def _validate_geographic_metrics(
+ row: Mapping[str, Any], labels: list[str], form_data: Mapping[str, Any]
+) -> None:
+ """Validate every selected metric without dropping invalid rows."""
+ secondary = metric_result_label(form_data.get("secondary_metric"))
+ for label in labels:
+ value = row.get(label)
+ if (
+ isinstance(value, bool)
+ or not isinstance(value, (int, float))
+ or not math.isfinite(value)
+ ):
+ raise ValueError(f"Geographic metric {label!r} must be a finite
number")
+ if value < 0 and (
+ form_data.get("viz_type") == "deck_scatter" or label == secondary
+ ):
+ raise ValueError("Geographic size metrics must be nonnegative")
+
+
+@lru_cache(maxsize=4)
+def _world_country_entries(field: str) -> tuple[tuple[str, str], ...]:
+ """Reuse immutable country aliases for the four supported world formats."""
+ from superset.examples.countries import countries
+
+ return tuple(
+ (country[field], country["cca3"]) for country in countries if
country[field]
+ )
+
+
+def _geographic_row_identifier(
+ row: Mapping[str, Any], form_data: Mapping[str, Any]
+) -> str | None:
+ """Resolve a polygon identifier or validate numeric point coordinates."""
+ from superset.utils.geographic import resolve_geographic_value,
resolve_region
+
+ viz = form_data["viz_type"]
+ entity = form_data.get("entity")
+ if viz != "deck_scatter" and not isinstance(entity, str):
+ raise ValueError("Geographic maps require an entity column")
+ if viz == "country_map":
+ return resolve_region(
+ row.get(entity or ""),
+ form_data.get("select_country", ""),
+ form_data.get("region_format", ""),
+ )
+ if viz == "world_map":
+ field = form_data.get("country_fieldtype")
+ if field not in {"name", "cca2", "cca3", "cioc"}:
+ raise ValueError("Choose country_format name, cca2, cca3, or cioc")
+ return resolve_geographic_value(
+ row.get(entity or ""),
+ _world_country_entries(field),
+ fold_diacritics=False,
+ )
+ spatial = form_data.get("spatial")
+ if not isinstance(spatial, Mapping) or spatial.get("type") != "latlong":
+ raise ValueError("Geographic points require latlong spatial columns")
+ for role, bound in (("latCol", 90), ("lonCol", 180)):
+ column = spatial.get(role)
+ if not isinstance(column, str):
+ raise ValueError(f"{role} requires a named coordinate column")
+ value = row.get(column)
+ if (
+ isinstance(value, bool)
+ or not isinstance(value, (int, float))
Review Comment:
This rejects valid `NUMERIC` latitude/longitude values because Arrow-backed
query results preserve them as `Decimal`, and this validation runs before the
JSON converter would turn them into floats. Could the coordinate check accept
finite real/decimal numbers so a PostgreSQL `NUMERIC` point does not fail with
`INVALID_GEOGRAPHIC_RESULT`?
--
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]
