aminghadersohi commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4048064834
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -177,8 +180,20 @@ def _build_candidates(
numeric = [c for c in columns if c.data_type == "numeric"]
categorical = [c for c in columns if c.data_type in ("string", "boolean")]
+ numeric_names = {c.name.lower() for c in numeric}
+ categorical_names = {
+ c.name.lower()
+ for c in categorical
+ if c.data_type == "string" and 1 < c.unique_count <= 250
+ }
if temporal and numeric:
return _candidates_temporal_numeric(numeric, row_count)
+ if {"latitude", "longitude"} <= numeric_names:
+ return ["geographic points", "table"]
Review Comment:
Confirmed and fixed in 350af643.
`_build_candidates` tested `if temporal and numeric:` and returned early, so
latitude/longitude were only reached when no temporal column existed — exactly
backwards for time-stamped spatial data. Coordinates now lead the temporal
candidate list rather than being shadowed by it:
```python
has_coordinates = {"latitude", "longitude"} <= numeric_names
if temporal and numeric:
candidates = _candidates_temporal_numeric(numeric, row_count)
return ["geographic points", *candidates] if has_coordinates else
candidates
```
Regression test `test_recommend_time_spatial_keeps_geographic_points` fails
on the pre-fix code and passes now;
`test_recommend_non_spatial_temporal_omits_geographic_points` pins that a lone
`latitude` does not trigger the suggestion.
##########
superset/mcp_service/chart/query_result.py:
##########
@@ -229,3 +232,164 @@ 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 _is_finite_geographic_number(value: object) -> TypeGuard[Real | Decimal]:
+ """Accept database NUMERIC/real scalars that remain finite in JSON.
+
+ Validation precedes JSON conversion; retain the original Decimal values for
+ data/export while rejecting booleans, complex numbers, and numeric strings.
+ """
+ if isinstance(value, bool) or not isinstance(value, (Real, Decimal)):
+ return False
+ if isinstance(value, Decimal) and not value.is_finite():
+ return False
+ try:
+ return math.isfinite(value)
+ except (OverflowError, ValueError):
+ return False
+
+
+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 not _is_finite_geographic_number(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,
Review Comment:
Confirmed and fixed in f87659b5.
Verified against the bundled list: all 247 entries are unaccented ASCII in
every supported format (`name`, `cca2`, `cca3`, `cioc`), so
`fold_diacritics=False` made `key` equal `str.lower` on the alias side while
leaving the users value accented. `Curaçao`, `Åland Islands` and `Réunion` were
therefore rejected despite matching `Curacao`, `Aland Islands` and `Reunion`
uniquely.
I also measured folding collisions across all four formats before changing
this: **zero** in every format, so folding cannot merge two distinct countries
here. The uniqueness guard in `resolve_geographic_value` is unchanged, so a
genuine collision still raises `ambiguous`.
Tests added: `test_world_map_accepts_accented_country_names` (fails
pre-fix), `test_world_map_accented_and_plain_names_resolve_alike` (proves both
spellings reach `CUW` via the duplicate-row guard), and
`test_world_map_folding_still_rejects_unknown_values` (folding does not invent
a country).
##########
tests/unit_tests/mcp_service/chart/test_geographic_chart.py:
##########
@@ -0,0 +1,1059 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Typed geographic contracts, native query semantics, and boundary parity."""
+
+from copy import deepcopy
+from decimal import Decimal
+from pathlib import Path
+from typing import Any
+from unittest.mock import Mock, patch
+
+import pytest
+from fastmcp import Client
+from pydantic import TypeAdapter, ValidationError
+from sqlalchemy.exc import SQLAlchemyError
+
+from superset.mcp_service.app import mcp
+from superset.mcp_service.chart.chart_helpers import
build_query_dicts_from_form_data
+from superset.mcp_service.chart.chart_utils import (
+ map_config_to_form_data,
+ merge_chart_form_data,
+)
+from superset.mcp_service.chart.compile import _compile_chart
+from superset.mcp_service.chart.preview_utils import (
+ _generate_ascii_preview_from_data,
+ _generate_vega_lite_preview_from_data,
+)
+from superset.mcp_service.chart.query_result import
normalize_chart_query_result
+from superset.mcp_service.chart.schemas import (
+ ChartConfig,
+ ChartError,
+ GenerateChartRequest,
+ GenerateExploreLinkRequest,
+ UpdateChartRequest,
+)
+from superset.mcp_service.chart.tool.get_chart_type_schema import (
+ _CHART_EXAMPLES,
+ _get_chart_type_schema_impl,
+)
+from superset.utils import json
+from superset.utils.geographic import resolve_geographic_value, resolve_region
+from superset.utils.geographic_regions import REGIONS
+
+KINDS = ("country_map", "world_map", "deck_scatter")
+# Reuse the compiled union schema; each validation still creates a fresh
config.
+CHART_CONFIG_ADAPTER = TypeAdapter(ChartConfig)
+
+
+def config_for(kind: str) -> Any:
+ """Parse the published example rather than duplicating a private
contract."""
+ return CHART_CONFIG_ADAPTER.validate_python(_CHART_EXAMPLES[kind][0])
+
+
+def form_for(kind: str) -> dict[str, Any]:
+ """Map the same config consumed by the three public tools."""
+ return map_config_to_form_data(config_for(kind))
+
+
+def result_for(kind: str) -> dict[str, Any]:
+ """Native query results before frontend display transforms."""
+ row = (
+ {"state": "CA", "SUM(sales)": 10}
+ if kind == "country_map"
+ else {"country": "US", "SUM(sales)": 10}
+ if kind == "world_map"
+ else {"latitude": 37.8, "longitude": -122.4}
+ )
+ return {"queries": [{"data": [row]}]}
+
+
+def invalid_result_for(kind: str) -> dict[str, Any]:
+ """Keep valid metrics while failing the actual geographic value
contract."""
+ result = result_for(kind)
+ row = result["queries"][0]["data"][0]
+ if kind == "country_map":
+ row["state"] = "BC"
+ elif kind == "world_map":
+ row["country"] = "not-a-country"
+ else:
+ row["latitude"] = 91
+ return result
+
+
[email protected]("kind", KINDS)
+def test_geographic_example_configs_are_independent(kind: str) -> None:
+ """Sharing a compiled schema must not share mutable config instances."""
+ first = config_for(kind)
+ second = config_for(kind)
+ assert first is not second
+ first.row_limit = 1
+ assert second.row_limit == 10000
+ assert config_for(kind).row_limit == 10000
+
+
[email protected]("kind", KINDS)
+def test_geographic_schema_examples_and_all_request_unions(kind: str) -> None:
+ """Each entry point uses the required, bounded shared discriminator."""
+ example = _CHART_EXAMPLES[kind][0]
+ schema = _get_chart_type_schema_impl(kind)["schema"]
+ assert "chart_type" in schema["required"]
+ assert schema["additionalProperties"] is False
+ assert schema["properties"]["row_limit"]["maximum"] == 10000
+ for model, identity in (
+ (GenerateChartRequest, {"dataset_id": 3}),
+ (GenerateExploreLinkRequest, {"dataset_id": 3}),
+ (UpdateChartRequest, {"identifier": 1}),
+ ):
+ assert (
+ model.model_validate({**identity, "config":
example}).config.chart_type
+ == kind
+ )
+ for patch_ in (
+ {"row_limit": 10001},
+ {"row_limit": True},
+ {"row_limit": "100"},
+ {"bogus": 1},
+ ):
+ with pytest.raises(ValidationError):
+ CHART_CONFIG_ADAPTER.validate_python({**example, **patch_})
+ with pytest.raises(ValidationError):
+ CHART_CONFIG_ADAPTER.validate_python(
+ {k: v for k, v in example.items() if k != "chart_type"}
+ )
+
+
[email protected](
+ "country,value,format_,expected",
+ [
+ ("usa", "CA", "abbreviation", "US-CA"),
+ ("usa", "ca", "abbreviation", "US-CA"),
+ ("usa", "California", "name", "US-CA"),
+ ("usa", "us-ca", "iso_3166_2", "US-CA"),
+ ("canada", "BC", "abbreviation", "CA-BC"),
+ ("australia", "Victoria", "name", "AU-VIC"),
+ ("australia", "NSW", "abbreviation", "AU-NSW"),
+ ("australia", "Queensland", "name", "AU-QLD"),
+ ("japan", "Tokyo", "name", "JP-13"),
+ ("japan", "Osaka", "name", "JP-27"),
+ ("uk", "Isle of Wight", "name", "GB-IOW"),
+ ],
+)
+def test_region_resolution(
+ country: str, value: str, format_: str, expected: str
+) -> None:
+ """All formats resolve only to identifiers present in the chosen
geometry."""
+ assert resolve_region(value, country, format_) == expected
+
+
[email protected](
+ "value",
+ [
+ "BC",
+ "Victoria",
+ "NSW",
+ "Queensland",
+ "Tokyo",
+ "Osaka",
+ "Isle of Wight",
+ None,
+ 1,
+ "",
+ "CA ",
+ ],
+)
+def test_us_rejects_non_us_and_malformed_values(value: object) -> None:
+ """Cross-country values never silently disappear from a map."""
+ with pytest.raises(ValueError, match="country=usa"):
+ resolve_region(value, "usa", "abbreviation")
+
+
+def test_exact_first_and_ambiguous_folded_names() -> None:
+ """Do not let a case-insensitive dictionary overwrite distinct names."""
+ pairs = [("Region", "A"), ("REGION", "B")]
+ assert resolve_geographic_value("Region", pairs) == "A"
+ with pytest.raises(ValueError, match="ambiguous"):
+ resolve_geographic_value("region", pairs)
+ with pytest.raises(ValueError, match="ambiguous"):
+ resolve_geographic_value("Region", pairs + [("Region", "C")])
+
+
[email protected]("country", REGIONS)
+def test_region_data_matches_frontend_geometry(country: str) -> None:
+ """Updating geometry requires updating its bounded backend lookup too."""
+ root = Path(__file__).resolve().parents[4]
+ path = (
+ root
+ / "superset-frontend/plugins/plugin-chart-country-map/src/countries"
+ / f"{country}.geojson"
+ )
+ expected = sorted(
+ {
+ (
+ f["properties"]["ISO"],
+ f["properties"].get("NAME_2") or f["properties"]["NAME_1"],
+ )
+ for f in json.loads(path.read_text())["features"]
+ }
+ )
+ assert REGIONS[country] == expected
Review Comment:
Not applicable — the test passes today, including for UK.
`REGIONS` is not in GeoJSON order. It is generated sorted, which is what
makes the parity assertion meaningful:
```
$ python -c "from superset.utils.geographic_regions import REGIONS;
print(REGIONS[\"uk\"] == sorted(REGIONS[\"uk\"]))"
True
```
And the test itself, all five countries:
```
$ pytest tests/unit_tests/mcp_service/chart/test_geographic_chart.py -k
region_data_matches_frontend_geometry
5 passed
```
Sorting `expected` is deliberate: it makes the comparison order-insensitive
against the GeoJSON so that reordering geometry does not break the test, while
a genuine content drift still does.
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -485,6 +485,20 @@ def resolve_metrics(form_data: dict[str, Any], viz_type:
str) -> list[Any]:
if viz_type == "bubble":
return [m for field in ("x", "y", "size") if (m :=
form_data.get(field))]
+ if viz_type in {"country_map", "world_map"}:
+ from superset.mcp_service.chart.query_result import metric_result_label
+
+ result = []
+ labels = set()
+ for field in (
+ ("metric", "secondary_metric") if viz_type == "world_map" else
("metric",)
+ ):
+ if metric := form_data.get(field):
Review Comment:
Not applicable — `field` is a string, not the tuple.
The tuple is the iterable of the `for`, so the loop binds `field` to
`"metric"` and then `"secondary_metric"` in turn:
```python
for field in (
("metric", "secondary_metric") if viz_type == "world_map" else
("metric",)
):
if metric := form_data.get(field):
```
Confirmed at runtime:
```
>>> resolve_metrics({"metric": "SUM(pop)", "secondary_metric": "AVG(gdp)"},
"world_map")
["SUM(pop)", "AVG(gdp)"]
>>> resolve_metrics({"metric": "SUM(pop)"}, "country_map")
["SUM(pop)"]
```
Both world map metrics are read, deduplicated by result label. This is also
already covered by `test_geographic_native_query_and_filters`, which asserts
the world map query metrics against the frontend `buildQuery` contract.
--
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]
