From ed15be88d96668bdc0dd6a262f73bb846876fa5b Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Fri, 22 May 2026 00:47:44 +0000 Subject: [PATCH 1/6] fix(mcp): fall back to form_data query for Deck.gl charts in get_chart_data Deck.gl chart types (deck_scatter, deck_arc, deck_hex, etc.) use spatial column configs (lat/lon pairs, geohash, delimited coordinates) rather than the standard metrics/groupby structure. The get_chart_data MCP tool was returning an early MissingQueryContext error for these charts when no saved query_context was present, instead of falling back to form_data. - Add _deck_gl_spatial_cols and resolve_deck_gl_columns helpers to chart_helpers.py to extract SQL column names from spatial control configs (latlong, delimited, geohash) plus line_column, geojson, dimension, and js_columns fields. - Route deck_ viz types through the new helpers inside build_query_dicts_from_form_data, producing a raw-column or grouped-metric query matching BaseDeckGLViz.query_obj() semantics. - Remove the early-exit error block for deck_ charts in get_chart_data.py; the existing safety-net (empty columns + empty metrics) still guards charts whose spatial config is completely absent. - Add unit tests covering latlong, delimited, geohash, arc start/end, line_column, geojson, dimension, js_columns, deduplication, and the build_query_dicts_from_form_data Deck.gl branch. --- superset/mcp_service/chart/chart_helpers.py | 67 ++++++ .../mcp_service/chart/tool/get_chart_data.py | 25 +-- .../mcp_service/chart/test_chart_helpers.py | 210 ++++++++++++++++++ 3 files changed, 279 insertions(+), 23 deletions(-) diff --git a/superset/mcp_service/chart/chart_helpers.py b/superset/mcp_service/chart/chart_helpers.py index b6f2b5b30dc5..bc64157b764d 100644 --- a/superset/mcp_service/chart/chart_helpers.py +++ b/superset/mcp_service/chart/chart_helpers.py @@ -260,6 +260,52 @@ def merge_extra_form_data_filters_into_query( merge_form_data_filters_into_query(query, extra_query_form_data) +def _deck_gl_spatial_cols(spatial: dict[str, Any] | None) -> list[str]: + """Return the column names referenced by a single Deck.gl spatial control.""" + if not isinstance(spatial, dict): + return [] + spatial_type = spatial.get("type") + if spatial_type == "latlong": + return [c for c in [spatial.get("lonCol"), spatial.get("latCol")] if c] + if spatial_type == "delimited": + return [c for c in [spatial.get("lonlatCol")] if c] + if spatial_type == "geohash": + return [c for c in [spatial.get("geohashCol")] if c] + return [] + + +def resolve_deck_gl_columns(form_data: dict[str, Any]) -> list[str]: + """Extract SQL column names for Deck.gl chart types from form_data. + + Deck.gl charts use spatial controls (lat/lon pairs, geohash, etc.) + rather than the standard metrics/groupby structure. This function + maps those spatial control configs to the actual column names + needed by the SQL query. + """ + seen: set[str] = set() + columns: list[str] = [] + + def _add(col: str | None) -> None: + if col and isinstance(col, str) and col not in seen: + seen.add(col) + columns.append(col) + + # Most Deck.gl types use "spatial"; arc charts use start/end spatial + for key in ("spatial", "start_spatial", "end_spatial"): + for col in _deck_gl_spatial_cols(form_data.get(key)): + _add(col) + + # deck_path / deck_polygon use a line column; deck_geojson uses geojson + for field in ("line_column", "geojson", "dimension"): + _add(form_data.get(field)) + + for col in form_data.get("js_columns") or []: + if isinstance(col, str): + _add(col) + + return columns + + def resolve_metrics(form_data: dict[str, Any], viz_type: str) -> list[Any]: """Extract metrics from form_data, handling chart-type-specific fields.""" if viz_type == "bubble": @@ -423,6 +469,27 @@ def build_query_dicts_from_form_data( or (getattr(chart, "viz_type", "") if chart else "") or "" ) + + # Deck.gl charts use spatial column configs rather than the standard + # metrics / groupby fields. Extract columns from the spatial controls. + if viz_type.startswith("deck_"): + deck_columns = resolve_deck_gl_columns(form_data) + deck_metrics: list[Any] = [] + for field in ("size", "metric"): + m = form_data.get(field) + if m: + deck_metrics.append(m) + break + return [ + _build_single_query_dict( + form_data, + deck_columns, + deck_metrics, + row_limit=row_limit, + order_desc=order_desc, + ) + ] + is_timeseries = ( viz_type.startswith("echarts_timeseries") or viz_type == "mixed_timeseries" ) diff --git a/superset/mcp_service/chart/tool/get_chart_data.py b/superset/mcp_service/chart/tool/get_chart_data.py index 5b6c0b3a5a55..96809d9cd962 100644 --- a/superset/mcp_service/chart/tool/get_chart_data.py +++ b/superset/mcp_service/chart/tool/get_chart_data.py @@ -340,31 +340,10 @@ async def get_chart_data( # noqa: C901 # groupby-like fields (entity, series, columns): # world_map, treemap_v2, sunburst_v2, gauge_chart # Bubble charts use x/y/size as separate metric fields. + # Deck.gl charts (deck_arc, deck_scatter, etc.) use spatial + # column configs (lat/lon, geohash, etc.) instead. viz_type = chart.viz_type or "" - # Deck.gl chart types store spatial data (lat/lon) - # rather than traditional metrics/groupby. They - # require a saved query_context to retrieve data. - # Match by prefix to cover all current and future - # deck.gl viz types (deck_arc, deck_scatter, etc.). - if viz_type.startswith("deck_"): - await ctx.warning( - "Chart %s is a deck.gl visualization (%s) with no " - "saved query_context. Data retrieval requires " - "re-saving the chart in Superset." % (chart.id, viz_type) - ) - return ChartError( - error=( - f"Chart {chart.id} is a deck.gl visualization " - f"(type: {viz_type}) with no saved query_context. " - f"Deck.gl charts use spatial data (lat/lon) that " - f"cannot be reconstructed from form_data alone. " - f"Please open this chart in Superset and re-save " - f"it to generate a query_context." - ), - error_type="MissingQueryContext", - ) - fallback_queries = build_query_dicts_from_form_data( form_data, chart.datasource_id, diff --git a/tests/unit_tests/mcp_service/chart/test_chart_helpers.py b/tests/unit_tests/mcp_service/chart/test_chart_helpers.py index 964226ab0128..2be3516b5d83 100644 --- a/tests/unit_tests/mcp_service/chart/test_chart_helpers.py +++ b/tests/unit_tests/mcp_service/chart/test_chart_helpers.py @@ -26,6 +26,7 @@ merge_extra_form_data_filters_into_query, merge_form_data_filters_into_query, prepare_form_data_for_query, + resolve_deck_gl_columns, ) @@ -285,3 +286,212 @@ def test_merge_extra_form_data_filters_into_query_adds_only_extra_predicates( assert query["time_range"] == "No filter" assert query["granularity"] == "updated_at" assert query["time_grain_sqla"] == "P1D" + + +# --------------------------------------------------------------------------- +# resolve_deck_gl_columns +# --------------------------------------------------------------------------- + + +def test_resolve_deck_gl_columns_latlong(): + form_data = { + "spatial": {"type": "latlong", "lonCol": "longitude", "latCol": "latitude"}, + } + assert resolve_deck_gl_columns(form_data) == ["longitude", "latitude"] + + +def test_resolve_deck_gl_columns_delimited(): + form_data = { + "spatial": {"type": "delimited", "lonlatCol": "coords"}, + } + assert resolve_deck_gl_columns(form_data) == ["coords"] + + +def test_resolve_deck_gl_columns_geohash(): + form_data = { + "spatial": {"type": "geohash", "geohashCol": "geo"}, + } + assert resolve_deck_gl_columns(form_data) == ["geo"] + + +def test_resolve_deck_gl_columns_arc_start_end(): + form_data = { + "start_spatial": { + "type": "latlong", + "lonCol": "start_lon", + "latCol": "start_lat", + }, + "end_spatial": {"type": "latlong", "lonCol": "end_lon", "latCol": "end_lat"}, + } + cols = resolve_deck_gl_columns(form_data) + assert cols == ["start_lon", "start_lat", "end_lon", "end_lat"] + + +def test_resolve_deck_gl_columns_path_line_column(): + form_data = { + "line_column": "path_wkt", + } + assert resolve_deck_gl_columns(form_data) == ["path_wkt"] + + +def test_resolve_deck_gl_columns_geojson(): + form_data = { + "geojson": "geom_col", + } + assert resolve_deck_gl_columns(form_data) == ["geom_col"] + + +def test_resolve_deck_gl_columns_with_dimension_and_js_columns(): + form_data = { + "spatial": {"type": "latlong", "lonCol": "lon", "latCol": "lat"}, + "dimension": "category", + "js_columns": ["name", "value"], + } + cols = resolve_deck_gl_columns(form_data) + assert "lon" in cols + assert "lat" in cols + assert "category" in cols + assert "name" in cols + assert "value" in cols + + +def test_resolve_deck_gl_columns_deduplicates(): + form_data = { + "spatial": {"type": "latlong", "lonCol": "lon", "latCol": "lat"}, + "dimension": "lon", # same as lonCol — should not duplicate + } + cols = resolve_deck_gl_columns(form_data) + assert cols.count("lon") == 1 + + +def test_resolve_deck_gl_columns_empty(): + assert resolve_deck_gl_columns({}) == [] + + +def test_resolve_deck_gl_columns_ignores_non_string_js_columns(): + form_data = { + "js_columns": [42, None, "valid_col"], + } + assert resolve_deck_gl_columns(form_data) == ["valid_col"] + + +# --------------------------------------------------------------------------- +# build_query_dicts_from_form_data — Deck.gl branch +# --------------------------------------------------------------------------- + + +def test_build_query_dicts_deck_scatter_latlong(monkeypatch): + monkeypatch.setattr( + "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine", + lambda datasource_id, datasource_type: "base", + ) + form_data = { + "viz_type": "deck_scatter", + "spatial": {"type": "latlong", "lonCol": "lon", "latCol": "lat"}, + "adhoc_filters": [], + } + + queries = build_query_dicts_from_form_data(form_data, 1, "table") + + assert len(queries) == 1 + assert queries[0]["columns"] == ["lon", "lat"] + assert queries[0]["metrics"] == [] + + +def test_build_query_dicts_deck_scatter_with_size_metric(monkeypatch): + monkeypatch.setattr( + "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine", + lambda datasource_id, datasource_type: "base", + ) + metric = { + "expressionType": "SIMPLE", + "column": {"column_name": "sales"}, + "aggregate": "SUM", + } + form_data = { + "viz_type": "deck_scatter", + "spatial": {"type": "latlong", "lonCol": "lon", "latCol": "lat"}, + "size": metric, + "adhoc_filters": [], + } + + queries = build_query_dicts_from_form_data(form_data, 1, "table") + + assert len(queries) == 1 + assert queries[0]["columns"] == ["lon", "lat"] + assert queries[0]["metrics"] == [metric] + + +def test_build_query_dicts_deck_arc(monkeypatch): + monkeypatch.setattr( + "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine", + lambda datasource_id, datasource_type: "base", + ) + form_data = { + "viz_type": "deck_arc", + "start_spatial": { + "type": "latlong", + "lonCol": "origin_lon", + "latCol": "origin_lat", + }, + "end_spatial": {"type": "latlong", "lonCol": "dest_lon", "latCol": "dest_lat"}, + "adhoc_filters": [], + } + + queries = build_query_dicts_from_form_data(form_data, 1, "table") + + assert len(queries) == 1 + assert queries[0]["columns"] == ["origin_lon", "origin_lat", "dest_lon", "dest_lat"] + assert queries[0]["metrics"] == [] + + +def test_build_query_dicts_deck_geojson(monkeypatch): + monkeypatch.setattr( + "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine", + lambda datasource_id, datasource_type: "base", + ) + form_data = { + "viz_type": "deck_geojson", + "geojson": "geometry", + "adhoc_filters": [], + } + + queries = build_query_dicts_from_form_data(form_data, 1, "table") + + assert len(queries) == 1 + assert queries[0]["columns"] == ["geometry"] + assert queries[0]["metrics"] == [] + + +def test_build_query_dicts_deck_hex_geohash(monkeypatch): + monkeypatch.setattr( + "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine", + lambda datasource_id, datasource_type: "base", + ) + form_data = { + "viz_type": "deck_hex", + "spatial": {"type": "geohash", "geohashCol": "geohash"}, + "adhoc_filters": [], + } + + queries = build_query_dicts_from_form_data(form_data, 1, "table") + + assert len(queries) == 1 + assert queries[0]["columns"] == ["geohash"] + + +def test_build_query_dicts_deck_path_with_row_limit(monkeypatch): + monkeypatch.setattr( + "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine", + lambda datasource_id, datasource_type: "base", + ) + form_data = { + "viz_type": "deck_path", + "line_column": "path_col", + "adhoc_filters": [], + } + + queries = build_query_dicts_from_form_data(form_data, 1, "table", row_limit=50) + + assert queries[0]["columns"] == ["path_col"] + assert queries[0]["row_limit"] == 50 From 1d276e67b28afcba910c707c549504864f672406 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Wed, 27 May 2026 17:08:06 +0000 Subject: [PATCH 2/6] fix(mcp): include point_radius_fixed metrics, null filters, and tooltip columns in Deck.gl fallback - Extract _resolve_deck_gl_metrics to handle point_radius_fixed when type=="metric" (deck_scatter radius, deck_polygon elevation) - Add _deck_gl_null_filters to mirror BaseDeckGLViz.add_null_filters() behavior; applied by default when filter_nulls is not False - Extend resolve_deck_gl_columns to include tooltip_contents column items and cross_filter_column, matching the normal Deck.gl query - Add _deck_gl_tooltip_cols helper for tooltip column extraction - Add unit tests covering all three behaviors --- superset/mcp_service/chart/chart_helpers.py | 85 +++++-- .../mcp_service/chart/test_chart_helpers.py | 226 ++++++++++++++++++ 2 files changed, 296 insertions(+), 15 deletions(-) diff --git a/superset/mcp_service/chart/chart_helpers.py b/superset/mcp_service/chart/chart_helpers.py index bc64157b764d..a170ba0b497e 100644 --- a/superset/mcp_service/chart/chart_helpers.py +++ b/superset/mcp_service/chart/chart_helpers.py @@ -274,6 +274,58 @@ def _deck_gl_spatial_cols(spatial: dict[str, Any] | None) -> list[str]: return [] +def _deck_gl_tooltip_cols(tooltip_contents: list[Any] | None) -> list[str]: + """Return column names from Deck.gl tooltip_contents config.""" + cols: list[str] = [] + for item in tooltip_contents or []: + if isinstance(item, str): + cols.append(item) + elif isinstance(item, dict) and item.get("item_type") == "column": + col = item.get("column_name") + if isinstance(col, str) and col: + cols.append(col) + return cols + + +def _deck_gl_null_filters(form_data: dict[str, Any]) -> list[dict[str, Any]]: + """Build IS NOT NULL simple filters for Deck.gl spatial and line columns. + + Mirrors BaseDeckGLViz.add_null_filters() behavior: spatial control columns + and line_column are filtered for non-null values by default. + """ + seen: set[str] = set() + result: list[dict[str, Any]] = [] + for key in ("spatial", "start_spatial", "end_spatial"): + for col in _deck_gl_spatial_cols(form_data.get(key)): + if col not in seen: + seen.add(col) + result.append({"col": col, "op": "IS NOT NULL", "val": ""}) + line_col = form_data.get("line_column") + if isinstance(line_col, str) and line_col and line_col not in seen: + result.append({"col": line_col, "op": "IS NOT NULL", "val": ""}) + return result + + +def _resolve_deck_gl_metrics(form_data: dict[str, Any]) -> list[Any]: + """Extract metrics for Deck.gl chart types. + + deck_scatter and deck_polygon can store metric-backed values in + point_radius_fixed (radius for scatter, elevation for polygon). + deck_polygon may have both a base metric and an elevation metric. + """ + metrics: list[Any] = [] + for field in ("size", "metric"): + m = form_data.get(field) + if m: + metrics.append(m) + prf = form_data.get("point_radius_fixed") + if isinstance(prf, dict) and prf.get("type") == "metric": + value = prf.get("value") + if value: + metrics.append(value) + return metrics + + def resolve_deck_gl_columns(form_data: dict[str, Any]) -> list[str]: """Extract SQL column names for Deck.gl chart types from form_data. @@ -303,6 +355,11 @@ def _add(col: str | None) -> None: if isinstance(col, str): _add(col) + for col in _deck_gl_tooltip_cols(form_data.get("tooltip_contents")): + _add(col) + + _add(form_data.get("cross_filter_column")) + return columns @@ -474,21 +531,19 @@ def build_query_dicts_from_form_data( # metrics / groupby fields. Extract columns from the spatial controls. if viz_type.startswith("deck_"): deck_columns = resolve_deck_gl_columns(form_data) - deck_metrics: list[Any] = [] - for field in ("size", "metric"): - m = form_data.get(field) - if m: - deck_metrics.append(m) - break - return [ - _build_single_query_dict( - form_data, - deck_columns, - deck_metrics, - row_limit=row_limit, - order_desc=order_desc, - ) - ] + deck_metrics = _resolve_deck_gl_metrics(form_data) + qd = _build_single_query_dict( + form_data, + deck_columns, + deck_metrics, + row_limit=row_limit, + order_desc=order_desc, + ) + if form_data.get("filter_nulls", True): + null_filters = _deck_gl_null_filters(form_data) + if null_filters: + qd["filters"] = [*(qd.get("filters") or []), *null_filters] + return [qd] is_timeseries = ( viz_type.startswith("echarts_timeseries") or viz_type == "mixed_timeseries" diff --git a/tests/unit_tests/mcp_service/chart/test_chart_helpers.py b/tests/unit_tests/mcp_service/chart/test_chart_helpers.py index 2be3516b5d83..5c03d67ac5e9 100644 --- a/tests/unit_tests/mcp_service/chart/test_chart_helpers.py +++ b/tests/unit_tests/mcp_service/chart/test_chart_helpers.py @@ -18,6 +18,9 @@ from unittest.mock import MagicMock, patch from superset.mcp_service.chart.chart_helpers import ( + _deck_gl_null_filters, + _deck_gl_tooltip_cols, + _resolve_deck_gl_metrics, apply_form_data_filters_to_query, build_query_dicts_from_form_data, extract_form_data_key_from_url, @@ -495,3 +498,226 @@ def test_build_query_dicts_deck_path_with_row_limit(monkeypatch): assert queries[0]["columns"] == ["path_col"] assert queries[0]["row_limit"] == 50 + + +# --------------------------------------------------------------------------- +# resolve_deck_gl_columns — tooltip_contents and cross_filter_column (Fix 1) +# --------------------------------------------------------------------------- + + +def test_resolve_deck_gl_columns_with_tooltip_contents_strings(): + form_data = { + "spatial": {"type": "latlong", "lonCol": "lon", "latCol": "lat"}, + "tooltip_contents": ["name", "category"], + } + cols = resolve_deck_gl_columns(form_data) + assert "name" in cols + assert "category" in cols + + +def test_resolve_deck_gl_columns_with_tooltip_contents_dict_items(): + form_data = { + "spatial": {"type": "latlong", "lonCol": "lon", "latCol": "lat"}, + "tooltip_contents": [ + {"item_type": "column", "column_name": "city"}, + {"item_type": "metric", "key": "sum__sales"}, # metric items ignored + ], + } + cols = resolve_deck_gl_columns(form_data) + assert "city" in cols + assert "sum__sales" not in cols + + +def test_resolve_deck_gl_columns_with_cross_filter_column(): + form_data = { + "spatial": {"type": "latlong", "lonCol": "lon", "latCol": "lat"}, + "cross_filter_column": "region", + } + cols = resolve_deck_gl_columns(form_data) + assert "region" in cols + + +def test_resolve_deck_gl_columns_tooltip_deduplicates_with_spatial(): + form_data = { + "spatial": {"type": "latlong", "lonCol": "lon", "latCol": "lat"}, + "tooltip_contents": ["lon"], # already in spatial cols + } + cols = resolve_deck_gl_columns(form_data) + assert cols.count("lon") == 1 + + +# --------------------------------------------------------------------------- +# _deck_gl_tooltip_cols +# --------------------------------------------------------------------------- + + +def test_deck_gl_tooltip_cols_strings(): + assert _deck_gl_tooltip_cols(["city", "state"]) == ["city", "state"] + + +def test_deck_gl_tooltip_cols_dict_column_items(): + result = _deck_gl_tooltip_cols([{"item_type": "column", "column_name": "country"}]) + assert result == ["country"] + + +def test_deck_gl_tooltip_cols_skips_metric_items(): + result = _deck_gl_tooltip_cols([{"item_type": "metric", "key": "sum__sales"}]) + assert result == [] + + +def test_deck_gl_tooltip_cols_none(): + assert _deck_gl_tooltip_cols(None) == [] + + +# --------------------------------------------------------------------------- +# _resolve_deck_gl_metrics (Fix 2) +# --------------------------------------------------------------------------- + + +def test_resolve_deck_gl_metrics_no_metrics(): + assert _resolve_deck_gl_metrics({}) == [] + + +def test_resolve_deck_gl_metrics_size_field(): + metric = {"expressionType": "SIMPLE", "aggregate": "COUNT", "column": None} + result = _resolve_deck_gl_metrics({"size": metric}) + assert result == [metric] + + +def test_resolve_deck_gl_metrics_metric_field(): + metric = {"expressionType": "SIMPLE", "aggregate": "SUM"} + result = _resolve_deck_gl_metrics({"metric": metric}) + assert result == [metric] + + +def test_resolve_deck_gl_metrics_point_radius_fixed_metric(): + prf_metric = {"expressionType": "SIMPLE", "aggregate": "AVG"} + prf = {"type": "metric", "value": prf_metric} + result = _resolve_deck_gl_metrics({"point_radius_fixed": prf}) + assert result == [prf_metric] + + +def test_resolve_deck_gl_metrics_point_radius_fixed_not_metric(): + prf = {"type": "fix", "value": 100} + result = _resolve_deck_gl_metrics({"point_radius_fixed": prf}) + assert result == [] + + +def test_resolve_deck_gl_metrics_polygon_both_metric_and_prf(): + base_metric = {"expressionType": "SIMPLE", "aggregate": "SUM"} + elevation_metric = {"expressionType": "SIMPLE", "aggregate": "AVG"} + prf = {"type": "metric", "value": elevation_metric} + result = _resolve_deck_gl_metrics( + {"metric": base_metric, "point_radius_fixed": prf} + ) + assert result == [base_metric, elevation_metric] + + +# --------------------------------------------------------------------------- +# _deck_gl_null_filters (Fix 3) +# --------------------------------------------------------------------------- + + +def test_deck_gl_null_filters_latlong(): + form_data = { + "spatial": {"type": "latlong", "lonCol": "lon", "latCol": "lat"}, + } + result = _deck_gl_null_filters(form_data) + assert result == [ + {"col": "lon", "op": "IS NOT NULL", "val": ""}, + {"col": "lat", "op": "IS NOT NULL", "val": ""}, + ] + + +def test_deck_gl_null_filters_arc_start_end(): + form_data = { + "start_spatial": {"type": "latlong", "lonCol": "s_lon", "latCol": "s_lat"}, + "end_spatial": {"type": "latlong", "lonCol": "e_lon", "latCol": "e_lat"}, + } + result = _deck_gl_null_filters(form_data) + assert result == [ + {"col": "s_lon", "op": "IS NOT NULL", "val": ""}, + {"col": "s_lat", "op": "IS NOT NULL", "val": ""}, + {"col": "e_lon", "op": "IS NOT NULL", "val": ""}, + {"col": "e_lat", "op": "IS NOT NULL", "val": ""}, + ] + + +def test_deck_gl_null_filters_line_column(): + form_data = {"line_column": "path_col"} + result = _deck_gl_null_filters(form_data) + assert result == [{"col": "path_col", "op": "IS NOT NULL", "val": ""}] + + +def test_deck_gl_null_filters_empty(): + assert _deck_gl_null_filters({}) == [] + + +def test_deck_gl_null_filters_geojson_no_filters(): + # deck_geojson uses the geojson field which is not a spatial control + form_data = {"geojson": "geometry"} + assert _deck_gl_null_filters(form_data) == [] + + +# --------------------------------------------------------------------------- +# build_query_dicts_from_form_data — null filters behavior (Fix 3) +# --------------------------------------------------------------------------- + + +def test_build_query_dicts_deck_scatter_adds_null_filters_by_default(monkeypatch): + monkeypatch.setattr( + "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine", + lambda datasource_id, datasource_type: "base", + ) + form_data = { + "viz_type": "deck_scatter", + "spatial": {"type": "latlong", "lonCol": "lon", "latCol": "lat"}, + "adhoc_filters": [], + } + + queries = build_query_dicts_from_form_data(form_data, 1, "table") + + assert {"col": "lon", "op": "IS NOT NULL", "val": ""} in queries[0]["filters"] + assert {"col": "lat", "op": "IS NOT NULL", "val": ""} in queries[0]["filters"] + + +def test_build_query_dicts_deck_scatter_filter_nulls_false(monkeypatch): + monkeypatch.setattr( + "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine", + lambda datasource_id, datasource_type: "base", + ) + form_data = { + "viz_type": "deck_scatter", + "spatial": {"type": "latlong", "lonCol": "lon", "latCol": "lat"}, + "filter_nulls": False, + "adhoc_filters": [], + } + + queries = build_query_dicts_from_form_data(form_data, 1, "table") + + null_filters = [ + f for f in queries[0].get("filters", []) if f.get("op") == "IS NOT NULL" + ] + assert null_filters == [] + + +def test_build_query_dicts_deck_scatter_point_radius_fixed_metric(monkeypatch): + monkeypatch.setattr( + "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine", + lambda datasource_id, datasource_type: "base", + ) + radius_metric = { + "expressionType": "SIMPLE", + "aggregate": "AVG", + "column": {"column_name": "radius"}, + } + form_data = { + "viz_type": "deck_scatter", + "spatial": {"type": "latlong", "lonCol": "lon", "latCol": "lat"}, + "point_radius_fixed": {"type": "metric", "value": radius_metric}, + "adhoc_filters": [], + } + + queries = build_query_dicts_from_form_data(form_data, 1, "table") + + assert queries[0]["metrics"] == [radius_metric] From fdd97d42ad7c122cadebcfc700b5c9458f8b30d5 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Wed, 27 May 2026 19:07:00 +0000 Subject: [PATCH 3/6] fix(mcp): guard Deck.gl metric extraction against scalar size and deck_geojson _resolve_deck_gl_metrics now: - Returns [] immediately for deck_geojson, matching DeckGeoJson.query_obj() which explicitly forces metrics=[] regardless of form_data - Only includes size/metric values that are dicts (metric references); scalar values like "100" in deck_geojson/deck_path fixtures are fixed display settings and must not be passed as query metrics Adds viz_type param and passes it from build_query_dicts_from_form_data. Adds regression tests using scalar size fixtures. --- superset/mcp_service/chart/chart_helpers.py | 18 ++++-- .../mcp_service/chart/test_chart_helpers.py | 57 +++++++++++++++++++ 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/superset/mcp_service/chart/chart_helpers.py b/superset/mcp_service/chart/chart_helpers.py index a170ba0b497e..5e388e282dc8 100644 --- a/superset/mcp_service/chart/chart_helpers.py +++ b/superset/mcp_service/chart/chart_helpers.py @@ -306,17 +306,23 @@ def _deck_gl_null_filters(form_data: dict[str, Any]) -> list[dict[str, Any]]: return result -def _resolve_deck_gl_metrics(form_data: dict[str, Any]) -> list[Any]: +def _resolve_deck_gl_metrics( + form_data: dict[str, Any], viz_type: str = "" +) -> list[Any]: """Extract metrics for Deck.gl chart types. - deck_scatter and deck_polygon can store metric-backed values in - point_radius_fixed (radius for scatter, elevation for polygon). - deck_polygon may have both a base metric and an elevation metric. + deck_geojson.query_obj() forces metrics=[] regardless of form_data. + For other types, size/metric must be dict metric references — scalar + values (e.g. "100") are fixed display settings, not query metrics. + deck_scatter and deck_polygon can additionally store metric-backed + values in point_radius_fixed (radius for scatter, elevation for polygon). """ + if viz_type == "deck_geojson": + return [] metrics: list[Any] = [] for field in ("size", "metric"): m = form_data.get(field) - if m: + if isinstance(m, dict): metrics.append(m) prf = form_data.get("point_radius_fixed") if isinstance(prf, dict) and prf.get("type") == "metric": @@ -531,7 +537,7 @@ def build_query_dicts_from_form_data( # metrics / groupby fields. Extract columns from the spatial controls. if viz_type.startswith("deck_"): deck_columns = resolve_deck_gl_columns(form_data) - deck_metrics = _resolve_deck_gl_metrics(form_data) + deck_metrics = _resolve_deck_gl_metrics(form_data, viz_type) qd = _build_single_query_dict( form_data, deck_columns, diff --git a/tests/unit_tests/mcp_service/chart/test_chart_helpers.py b/tests/unit_tests/mcp_service/chart/test_chart_helpers.py index 5c03d67ac5e9..d6c43955a149 100644 --- a/tests/unit_tests/mcp_service/chart/test_chart_helpers.py +++ b/tests/unit_tests/mcp_service/chart/test_chart_helpers.py @@ -613,6 +613,26 @@ def test_resolve_deck_gl_metrics_polygon_both_metric_and_prf(): assert result == [base_metric, elevation_metric] +def test_resolve_deck_gl_metrics_geojson_returns_empty(): + # deck_geojson.query_obj() forces metrics=[] regardless of form_data + metric = {"expressionType": "SIMPLE", "aggregate": "SUM"} + result = _resolve_deck_gl_metrics( + {"size": metric, "metric": metric}, "deck_geojson" + ) + assert result == [] + + +def test_resolve_deck_gl_metrics_scalar_size_excluded(): + # Scalar size values (fixed display settings) must not be treated as metrics + result = _resolve_deck_gl_metrics({"size": "100"}, "deck_hex") + assert result == [] + + +def test_resolve_deck_gl_metrics_integer_size_excluded(): + result = _resolve_deck_gl_metrics({"size": 100}, "deck_path") + assert result == [] + + # --------------------------------------------------------------------------- # _deck_gl_null_filters (Fix 3) # --------------------------------------------------------------------------- @@ -721,3 +741,40 @@ def test_build_query_dicts_deck_scatter_point_radius_fixed_metric(monkeypatch): queries = build_query_dicts_from_form_data(form_data, 1, "table") assert queries[0]["metrics"] == [radius_metric] + + +def test_build_query_dicts_deck_geojson_scalar_size_produces_no_metrics(monkeypatch): + # Regression: deck_geojson fixture has size='100' (scalar, not a metric). + # The fallback must produce metrics=[] to match DeckGeoJson.query_obj(). + monkeypatch.setattr( + "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine", + lambda datasource_id, datasource_type: "base", + ) + form_data = { + "viz_type": "deck_geojson", + "geojson": "geometry", + "size": "100", + "adhoc_filters": [], + } + + queries = build_query_dicts_from_form_data(form_data, 1, "table") + + assert queries[0]["metrics"] == [] + + +def test_build_query_dicts_deck_path_scalar_size_produces_no_metrics(monkeypatch): + # deck_path fixture also has size='100' — scalar must not become a metric. + monkeypatch.setattr( + "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine", + lambda datasource_id, datasource_type: "base", + ) + form_data = { + "viz_type": "deck_path", + "line_column": "path_col", + "size": "100", + "adhoc_filters": [], + } + + queries = build_query_dicts_from_form_data(form_data, 1, "table") + + assert queries[0]["metrics"] == [] From 362b9faaab5a80ed7d59a01557a72d4947890b53 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Wed, 27 May 2026 20:09:08 +0000 Subject: [PATCH 4/6] fix(mcp): preserve string metric keys and add geojson null filter in Deck.gl fallback - Add _is_metric_ref() to distinguish saved metric keys (non-numeric strings like "count") from fixed display settings (numeric strings like "100"); both size and metric fields are now validated with this helper so string metric references are no longer silently dropped - Extend _deck_gl_null_filters() to add IS NOT NULL for the geojson column, mirroring the behavior of DeckGeoJson when filter_nulls is enabled - Add unit tests for _is_metric_ref and the new integration paths --- superset/mcp_service/chart/chart_helpers.py | 40 +++++++-- .../mcp_service/chart/test_chart_helpers.py | 88 ++++++++++++++++++- 2 files changed, 115 insertions(+), 13 deletions(-) diff --git a/superset/mcp_service/chart/chart_helpers.py b/superset/mcp_service/chart/chart_helpers.py index 5e388e282dc8..fb28b42e6fd7 100644 --- a/superset/mcp_service/chart/chart_helpers.py +++ b/superset/mcp_service/chart/chart_helpers.py @@ -287,11 +287,30 @@ def _deck_gl_tooltip_cols(tooltip_contents: list[Any] | None) -> list[str]: return cols +def _is_metric_ref(value: Any) -> bool: + """Return True if value is a metric reference (dict or non-numeric string). + + Deck.gl size/metric fields hold either a dict metric definition or a + simple saved-metric string key (e.g. "count"). Scalar numeric strings + like "100" are fixed display settings and must not be treated as metrics. + """ + if isinstance(value, dict): + return True + if isinstance(value, str) and value: + try: + float(value) + return False + except ValueError: + return True + return False + + def _deck_gl_null_filters(form_data: dict[str, Any]) -> list[dict[str, Any]]: - """Build IS NOT NULL simple filters for Deck.gl spatial and line columns. + """Build IS NOT NULL simple filters for Deck.gl spatial and data columns. - Mirrors BaseDeckGLViz.add_null_filters() behavior: spatial control columns - and line_column are filtered for non-null values by default. + Mirrors BaseDeckGLViz.add_null_filters() behavior: spatial control columns, + line_column, and the geojson column are filtered for non-null values by + default. """ seen: set[str] = set() result: list[dict[str, Any]] = [] @@ -300,9 +319,11 @@ def _deck_gl_null_filters(form_data: dict[str, Any]) -> list[dict[str, Any]]: if col not in seen: seen.add(col) result.append({"col": col, "op": "IS NOT NULL", "val": ""}) - line_col = form_data.get("line_column") - if isinstance(line_col, str) and line_col and line_col not in seen: - result.append({"col": line_col, "op": "IS NOT NULL", "val": ""}) + for field in ("line_column", "geojson"): + data_col = form_data.get(field) + if isinstance(data_col, str) and data_col and data_col not in seen: + seen.add(data_col) + result.append({"col": data_col, "op": "IS NOT NULL", "val": ""}) return result @@ -312,8 +333,9 @@ def _resolve_deck_gl_metrics( """Extract metrics for Deck.gl chart types. deck_geojson.query_obj() forces metrics=[] regardless of form_data. - For other types, size/metric must be dict metric references — scalar - values (e.g. "100") are fixed display settings, not query metrics. + For other types, size/metric values are included when they are metric + references (dicts or non-numeric strings); numeric scalars like "100" + are fixed display settings and are excluded. deck_scatter and deck_polygon can additionally store metric-backed values in point_radius_fixed (radius for scatter, elevation for polygon). """ @@ -322,7 +344,7 @@ def _resolve_deck_gl_metrics( metrics: list[Any] = [] for field in ("size", "metric"): m = form_data.get(field) - if isinstance(m, dict): + if _is_metric_ref(m): metrics.append(m) prf = form_data.get("point_radius_fixed") if isinstance(prf, dict) and prf.get("type") == "metric": diff --git a/tests/unit_tests/mcp_service/chart/test_chart_helpers.py b/tests/unit_tests/mcp_service/chart/test_chart_helpers.py index d6c43955a149..f8157335b088 100644 --- a/tests/unit_tests/mcp_service/chart/test_chart_helpers.py +++ b/tests/unit_tests/mcp_service/chart/test_chart_helpers.py @@ -20,6 +20,7 @@ from superset.mcp_service.chart.chart_helpers import ( _deck_gl_null_filters, _deck_gl_tooltip_cols, + _is_metric_ref, _resolve_deck_gl_metrics, apply_form_data_filters_to_query, build_query_dicts_from_form_data, @@ -569,6 +570,35 @@ def test_deck_gl_tooltip_cols_none(): assert _deck_gl_tooltip_cols(None) == [] +# --------------------------------------------------------------------------- +# _is_metric_ref +# --------------------------------------------------------------------------- + + +def test_is_metric_ref_dict(): + assert _is_metric_ref({"expressionType": "SIMPLE"}) is True + + +def test_is_metric_ref_string_key(): + assert _is_metric_ref("count") is True + assert _is_metric_ref("sum__sales") is True + + +def test_is_metric_ref_numeric_string_excluded(): + assert _is_metric_ref("100") is False + assert _is_metric_ref("3.14") is False + assert _is_metric_ref("0") is False + + +def test_is_metric_ref_integer_excluded(): + assert _is_metric_ref(100) is False + + +def test_is_metric_ref_none_and_empty(): + assert _is_metric_ref(None) is False + assert _is_metric_ref("") is False + + # --------------------------------------------------------------------------- # _resolve_deck_gl_metrics (Fix 2) # --------------------------------------------------------------------------- @@ -623,7 +653,7 @@ def test_resolve_deck_gl_metrics_geojson_returns_empty(): def test_resolve_deck_gl_metrics_scalar_size_excluded(): - # Scalar size values (fixed display settings) must not be treated as metrics + # Numeric string size values (fixed display settings) must not be metrics result = _resolve_deck_gl_metrics({"size": "100"}, "deck_hex") assert result == [] @@ -633,6 +663,17 @@ def test_resolve_deck_gl_metrics_integer_size_excluded(): assert result == [] +def test_resolve_deck_gl_metrics_string_metric_included(): + # Non-numeric string metrics (saved metric keys) must be preserved + result = _resolve_deck_gl_metrics({"size": "count"}, "deck_hex") + assert result == ["count"] + + +def test_resolve_deck_gl_metrics_string_metric_field(): + result = _resolve_deck_gl_metrics({"metric": "sum__sales"}, "deck_arc") + assert result == ["sum__sales"] + + # --------------------------------------------------------------------------- # _deck_gl_null_filters (Fix 3) # --------------------------------------------------------------------------- @@ -673,10 +714,12 @@ def test_deck_gl_null_filters_empty(): assert _deck_gl_null_filters({}) == [] -def test_deck_gl_null_filters_geojson_no_filters(): - # deck_geojson uses the geojson field which is not a spatial control +def test_deck_gl_null_filters_geojson_column(): + # geojson column gets an IS NOT NULL filter just like spatial columns form_data = {"geojson": "geometry"} - assert _deck_gl_null_filters(form_data) == [] + assert _deck_gl_null_filters(form_data) == [ + {"col": "geometry", "op": "IS NOT NULL", "val": ""} + ] # --------------------------------------------------------------------------- @@ -778,3 +821,40 @@ def test_build_query_dicts_deck_path_scalar_size_produces_no_metrics(monkeypatch queries = build_query_dicts_from_form_data(form_data, 1, "table") assert queries[0]["metrics"] == [] + + +def test_build_query_dicts_deck_geojson_adds_geojson_null_filter(monkeypatch): + # deck_geojson should add IS NOT NULL on the geojson column when filter_nulls + monkeypatch.setattr( + "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine", + lambda datasource_id, datasource_type: "base", + ) + form_data = { + "viz_type": "deck_geojson", + "geojson": "geometry_col", + "adhoc_filters": [], + } + + queries = build_query_dicts_from_form_data(form_data, 1, "table") + + assert {"col": "geometry_col", "op": "IS NOT NULL", "val": ""} in queries[0][ + "filters" + ] + + +def test_build_query_dicts_deck_hex_string_metric(monkeypatch): + # Non-numeric string size (saved metric key) must be included as a metric + monkeypatch.setattr( + "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine", + lambda datasource_id, datasource_type: "base", + ) + form_data = { + "viz_type": "deck_hex", + "spatial": {"type": "geohash", "geohashCol": "geo"}, + "size": "count", + "adhoc_filters": [], + } + + queries = build_query_dicts_from_form_data(form_data, 1, "table") + + assert queries[0]["metrics"] == ["count"] From e32a683f6f1d1115dd188175b44133c9e24be3ee Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Wed, 27 May 2026 21:06:13 +0000 Subject: [PATCH 5/6] fix(mcp): handle bare string point_radius_fixed as metric in Deck.gl fallback Legacy deck_scatter charts store point_radius_fixed as a plain metric key string (e.g. "count") rather than the dict form {"type":"metric","value":...}. The frontend isMetricValue() treats any non-numeric string as a valid metric ref, so the fallback query builder now does the same via _is_metric_ref. --- superset/mcp_service/chart/chart_helpers.py | 3 ++ .../mcp_service/chart/test_chart_helpers.py | 30 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/superset/mcp_service/chart/chart_helpers.py b/superset/mcp_service/chart/chart_helpers.py index fb28b42e6fd7..7d56b313b2b1 100644 --- a/superset/mcp_service/chart/chart_helpers.py +++ b/superset/mcp_service/chart/chart_helpers.py @@ -351,6 +351,9 @@ def _resolve_deck_gl_metrics( value = prf.get("value") if value: metrics.append(value) + elif _is_metric_ref(prf): + # Legacy deck_scatter: point_radius_fixed can be a bare metric key string + metrics.append(prf) return metrics diff --git a/tests/unit_tests/mcp_service/chart/test_chart_helpers.py b/tests/unit_tests/mcp_service/chart/test_chart_helpers.py index f8157335b088..c3563fc8e33c 100644 --- a/tests/unit_tests/mcp_service/chart/test_chart_helpers.py +++ b/tests/unit_tests/mcp_service/chart/test_chart_helpers.py @@ -674,6 +674,18 @@ def test_resolve_deck_gl_metrics_string_metric_field(): assert result == ["sum__sales"] +def test_resolve_deck_gl_metrics_string_point_radius_fixed(): + # Legacy deck_scatter: point_radius_fixed as a bare metric key string + result = _resolve_deck_gl_metrics({"point_radius_fixed": "count"}, "deck_scatter") + assert result == ["count"] + + +def test_resolve_deck_gl_metrics_numeric_point_radius_fixed_excluded(): + # Numeric string point_radius_fixed is a fixed pixel radius, not a metric + result = _resolve_deck_gl_metrics({"point_radius_fixed": "100"}, "deck_scatter") + assert result == [] + + # --------------------------------------------------------------------------- # _deck_gl_null_filters (Fix 3) # --------------------------------------------------------------------------- @@ -858,3 +870,21 @@ def test_build_query_dicts_deck_hex_string_metric(monkeypatch): queries = build_query_dicts_from_form_data(form_data, 1, "table") assert queries[0]["metrics"] == ["count"] + + +def test_build_query_dicts_deck_scatter_string_point_radius_fixed(monkeypatch): + # Legacy deck_scatter with point_radius_fixed as a bare metric key string + monkeypatch.setattr( + "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine", + lambda datasource_id, datasource_type: "base", + ) + form_data = { + "viz_type": "deck_scatter", + "spatial": {"type": "latlong", "lonCol": "lon", "latCol": "lat"}, + "point_radius_fixed": "count", + "adhoc_filters": [], + } + + queries = build_query_dicts_from_form_data(form_data, 1, "table") + + assert queries[0]["metrics"] == ["count"] From 06cea32872ee9a968d82efbe963d71c06bfb1d8c Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Wed, 27 May 2026 23:06:36 +0000 Subject: [PATCH 6/6] fix(mcp): guard point_radius_fixed string branch against non-metric dicts _is_metric_ref returns True for any dict, so the elif branch added in e32a683f would incorrectly include {"type":"fix","value":100} (a fixed pixel radius) as a SQL metric. Add isinstance(prf, str) guard so the legacy bare-string path only fires for actual string values. --- superset/mcp_service/chart/chart_helpers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/mcp_service/chart/chart_helpers.py b/superset/mcp_service/chart/chart_helpers.py index 7d56b313b2b1..66023c1d69b3 100644 --- a/superset/mcp_service/chart/chart_helpers.py +++ b/superset/mcp_service/chart/chart_helpers.py @@ -351,8 +351,8 @@ def _resolve_deck_gl_metrics( value = prf.get("value") if value: metrics.append(value) - elif _is_metric_ref(prf): - # Legacy deck_scatter: point_radius_fixed can be a bare metric key string + elif isinstance(prf, str) and _is_metric_ref(prf): + # Legacy deck_scatter: point_radius_fixed as a bare non-numeric metric key metrics.append(prf) return metrics