aminghadersohi commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r3984444400
##########
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:
Fixed in `688aa8d`. Geographic configs now join Gauge on the strict
presentation-only rebind path in `_build_replacement_form_data`; they no longer
reset `dataset_rebind=False` through the generic inherited-state path. Generic
chart rebinding is unchanged.
Fail-before/pass-after evidence:
`test_public_geographic_rebind_drops_same_named_source_state` exercises the
actual FastMCP `update_chart` tool for all three geographic types, both saved
and unsaved. The target context deliberately contains the old filter/role
column names. All six cases failed before the fix with inherited source
filters; all pass after it. They verify removal of filters, old roles,
time/Jinja state, target datasource binding, preserved presentation, and saved
query-context clearing.
Local full MCP suite: **4,221 passed, 2 skipped**; full branch
pre-commit/MyPy/Ruff/Pylint/type checks pass. Upstream `1b03ad8` was merged
normally, retaining its Treemap support. Exact-head CI is pending; leaving the
thread open until it passes.
##########
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:
Fixed in `688aa8d`. Geographic result validation accepts finite `Decimal`
and real-number scalars before JSON conversion, without mutating query results.
The same numeric check applies to geographic metrics, since NUMERIC aggregates
encounter the same pre-serialization boundary. Booleans, strings, complex
values, NaN/signaling NaN, infinities, nonrepresentable magnitudes, and
out-of-range coordinates remain rejected. Decimal coordinate bounds are checked
at original precision rather than after float rounding.
Fail-before/pass-after evidence: eight public FastMCP
generation/Explore/update/cached-preview cases with Decimal coordinates and two
Decimal aggregate cases failed before the fix and pass afterward. Added
saved/cached JSON, CSV, and Excel export coverage, source-Decimal preservation,
native JSON numeric conversion, and invalid-number/boundary regressions
(including Decimal values just beyond ±90 at greater than float precision). MCP
JSON exports retain their existing precision-preserving Decimal-as-text
behavior; native chart serialization uses its existing numeric converter.
Local full MCP suite: **4,221 passed, 2 skipped**; full branch
pre-commit/MyPy/Ruff/Pylint/type checks pass. Exact-head CI is pending; leaving
the thread open until it passes.
--
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]
