From c37a3ec292dc3e0152879819ea3072a1caa0c38a Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Tue, 31 Mar 2026 18:39:08 +0200 Subject: [PATCH] fix(mcp): add TEMPORAL_RANGE filter for temporal x-axis in generate_chart (#38978) --- superset/mcp_service/chart/chart_utils.py | 33 +++++ .../mcp_service/chart/test_chart_utils.py | 134 +++++++++++++++++- .../tool/test_generate_explore_link.py | 6 +- 3 files changed, 166 insertions(+), 7 deletions(-) diff --git a/superset/mcp_service/chart/chart_utils.py b/superset/mcp_service/chart/chart_utils.py index 62704a0b5ef..b4fdfd3b13e 100644 --- a/superset/mcp_service/chart/chart_utils.py +++ b/superset/mcp_service/chart/chart_utils.py @@ -26,6 +26,7 @@ import logging from dataclasses import dataclass from typing import Any, Dict +from superset.constants import NO_TIME_RANGE from superset.mcp_service.chart.schemas import ( BigNumberChartConfig, ChartCapabilities, @@ -41,6 +42,7 @@ from superset.mcp_service.chart.schemas import ( ) from superset.mcp_service.utils.url_utils import get_superset_base_url from superset.utils import json +from superset.utils.core import FilterOperator logger = logging.getLogger(__name__) @@ -545,6 +547,7 @@ def configure_temporal_handling( Stores any warnings in ``form_data["_mcp_warnings"]``. """ if x_is_temporal: + form_data["granularity_sqla"] = form_data.get("x_axis") if time_grain: form_data["time_grain_sqla"] = time_grain else: @@ -561,6 +564,33 @@ def configure_temporal_handling( ) +def _ensure_temporal_adhoc_filter(form_data: Dict[str, Any], column: str) -> None: + """Ensure a TEMPORAL_RANGE adhoc filter exists for the given column. + + Mirrors the Explore UI behavior: when a temporal column is set as + the x-axis, a TEMPORAL_RANGE filter must be present so dashboard + time-range filters can bind to it. Without this filter, Explore + shows a warning dialog asking the user to add it manually. + """ + existing = form_data.get("adhoc_filters", []) + if any( + f.get("operator") == FilterOperator.TEMPORAL_RANGE.value + and f.get("subject") == column + for f in existing + ): + return + existing.append( + { + "clause": "WHERE", + "expressionType": "SIMPLE", + "subject": column, + "operator": FilterOperator.TEMPORAL_RANGE.value, + "comparator": NO_TIME_RANGE, + } + ) + form_data["adhoc_filters"] = existing + + def map_xy_config( config: XYChartConfig, dataset_id: int | str | None = None ) -> Dict[str, Any]: @@ -618,6 +648,9 @@ def map_xy_config( _add_adhoc_filters(form_data, config.filters) + if x_is_temporal: + _ensure_temporal_adhoc_filter(form_data, config.x.name) + form_data["row_limit"] = config.row_limit # Add stacking configuration diff --git a/tests/unit_tests/mcp_service/chart/test_chart_utils.py b/tests/unit_tests/mcp_service/chart/test_chart_utils.py index 53ff6847983..293adfb09c3 100644 --- a/tests/unit_tests/mcp_service/chart/test_chart_utils.py +++ b/tests/unit_tests/mcp_service/chart/test_chart_utils.py @@ -22,8 +22,10 @@ from unittest.mock import MagicMock, patch import pytest +from superset.constants import NO_TIME_RANGE from superset.mcp_service.chart.chart_utils import ( _add_adhoc_filters, + _ensure_temporal_adhoc_filter, adhoc_filters_to_query_filters, configure_temporal_handling, create_metric_object, @@ -44,7 +46,7 @@ from superset.mcp_service.chart.schemas import ( TableChartConfig, XYChartConfig, ) -from superset.utils.core import GenericDataType +from superset.utils.core import FilterOperator, GenericDataType class TestCreateMetricObject: @@ -665,10 +667,13 @@ class TestMapXYConfig: result = map_xy_config(config) assert "adhoc_filters" in result - assert len(result["adhoc_filters"]) == 1 + # User filter + auto-added TEMPORAL_RANGE filter for temporal x-axis + assert len(result["adhoc_filters"]) == 2 assert result["adhoc_filters"][0]["subject"] == "region" assert result["adhoc_filters"][0]["operator"] == "==" assert result["adhoc_filters"][0]["comparator"] == "US" + assert result["adhoc_filters"][1]["operator"] == "TEMPORAL_RANGE" + assert result["adhoc_filters"][1]["subject"] == "date" @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal") def test_map_xy_config_row_limit(self, mock_is_temporal) -> None: @@ -1284,16 +1289,18 @@ class TestConfigureTemporalHandling: """Test configure_temporal_handling function""" def test_temporal_column_with_time_grain(self) -> None: - """Test temporal column sets time_grain_sqla""" - form_data: dict[str, Any] = {} + """Test temporal column sets time_grain_sqla and granularity_sqla""" + form_data: dict[str, Any] = {"x_axis": "order_date"} configure_temporal_handling(form_data, x_is_temporal=True, time_grain="P1M") assert form_data["time_grain_sqla"] == "P1M" + assert form_data["granularity_sqla"] == "order_date" def test_temporal_column_without_time_grain(self) -> None: - """Test temporal column without time_grain doesn't set time_grain_sqla""" - form_data: dict[str, Any] = {} + """Test temporal column sets granularity_sqla but not time_grain_sqla""" + form_data: dict[str, Any] = {"x_axis": "order_date"} configure_temporal_handling(form_data, x_is_temporal=True, time_grain=None) assert "time_grain_sqla" not in form_data + assert form_data["granularity_sqla"] == "order_date" def test_non_temporal_column_sets_categorical_config(self) -> None: """Test non-temporal column sets categorical configuration""" @@ -1355,7 +1362,16 @@ class TestMapXYConfigWithNonTemporalColumn: assert result["x_axis"] == "created_at" assert result["time_grain_sqla"] == "P1W" + assert result["granularity_sqla"] == "created_at" assert "x_axis_sort_series_type" not in result + # Temporal x-axis should have a TEMPORAL_RANGE adhoc filter + temporal_filters = [ + f + for f in result.get("adhoc_filters", []) + if f.get("operator") == "TEMPORAL_RANGE" + ] + assert len(temporal_filters) == 1 + assert temporal_filters[0]["subject"] == "created_at" @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal") def test_non_temporal_ignores_time_grain_param(self, mock_is_temporal) -> None: @@ -1377,6 +1393,112 @@ class TestMapXYConfigWithNonTemporalColumn: assert result["x_axis_sort_series_type"] == "name" +class TestEnsureTemporalAdhocFilter: + """Test _ensure_temporal_adhoc_filter helper and its integration in map_xy_config""" + + def test_adds_filter_to_empty_form_data(self) -> None: + """Test adds TEMPORAL_RANGE filter when no adhoc_filters exist""" + form_data: dict[str, Any] = {} + _ensure_temporal_adhoc_filter(form_data, "order_date") + + assert len(form_data["adhoc_filters"]) == 1 + f = form_data["adhoc_filters"][0] + assert f["operator"] == FilterOperator.TEMPORAL_RANGE.value + assert f["subject"] == "order_date" + assert f["comparator"] == NO_TIME_RANGE + assert f["expressionType"] == "SIMPLE" + assert f["clause"] == "WHERE" + + def test_appends_to_existing_filters(self) -> None: + """Test appends temporal filter after existing user filters""" + form_data: dict[str, Any] = { + "adhoc_filters": [ + {"subject": "region", "operator": "==", "comparator": "US"} + ] + } + _ensure_temporal_adhoc_filter(form_data, "order_date") + + assert len(form_data["adhoc_filters"]) == 2 + assert form_data["adhoc_filters"][0]["subject"] == "region" + assert ( + form_data["adhoc_filters"][1]["operator"] + == FilterOperator.TEMPORAL_RANGE.value + ) + + def test_does_not_duplicate_existing_temporal_filter(self) -> None: + """Test skips adding if a TEMPORAL_RANGE filter already exists for the column""" + form_data: dict[str, Any] = { + "adhoc_filters": [ + { + "subject": "order_date", + "operator": FilterOperator.TEMPORAL_RANGE.value, + "comparator": "Last 7 days", + } + ] + } + _ensure_temporal_adhoc_filter(form_data, "order_date") + + # Should still be just 1 filter (no duplicate) + assert len(form_data["adhoc_filters"]) == 1 + + def test_adds_filter_for_different_column(self) -> None: + """Test adds filter when existing temporal filter is on a different column""" + form_data: dict[str, Any] = { + "adhoc_filters": [ + { + "subject": "created_at", + "operator": FilterOperator.TEMPORAL_RANGE.value, + "comparator": NO_TIME_RANGE, + } + ] + } + _ensure_temporal_adhoc_filter(form_data, "order_date") + + assert len(form_data["adhoc_filters"]) == 2 + + @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal") + def test_temporal_x_axis_adds_filter_in_map_xy(self, mock_is_temporal) -> None: + """Test map_xy_config adds TEMPORAL_RANGE filter for temporal x-axis""" + mock_is_temporal.return_value = True + config = XYChartConfig( + chart_type="xy", + x=ColumnRef(name="order_date"), + y=[ColumnRef(name="revenue", aggregate="SUM")], + kind="bar", + ) + + result = map_xy_config(config, dataset_id=123) + + temporal_filters = [ + f + for f in result.get("adhoc_filters", []) + if f.get("operator") == FilterOperator.TEMPORAL_RANGE.value + ] + assert len(temporal_filters) == 1 + assert temporal_filters[0]["subject"] == "order_date" + assert temporal_filters[0]["comparator"] == NO_TIME_RANGE + + @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal") + def test_non_temporal_x_axis_no_temporal_filter(self, mock_is_temporal) -> None: + """Test non-temporal x-axis skips TEMPORAL_RANGE filter""" + mock_is_temporal.return_value = False + config = XYChartConfig( + chart_type="xy", + x=ColumnRef(name="year"), + y=[ColumnRef(name="sales", aggregate="SUM")], + kind="bar", + ) + + result = map_xy_config(config, dataset_id=123) + + temporal_filters = [ + f + for f in result.get("adhoc_filters", []) + if f.get("operator") == FilterOperator.TEMPORAL_RANGE.value + ] + assert len(temporal_filters) == 0 + + class TestFilterConfigValidation: """Test FilterConfig validation for new operators""" diff --git a/tests/unit_tests/mcp_service/explore/tool/test_generate_explore_link.py b/tests/unit_tests/mcp_service/explore/tool/test_generate_explore_link.py index 264868168af..fb8aee539b0 100644 --- a/tests/unit_tests/mcp_service/explore/tool/test_generate_explore_link.py +++ b/tests/unit_tests/mcp_service/explore/tool/test_generate_explore_link.py @@ -879,8 +879,12 @@ class TestGenerateExploreLinkColumnNormalization: assert form_data["x_axis"] == "OrderDate" # filter subject normalized to match x-axis adhoc_filters = form_data.get("adhoc_filters", []) - assert len(adhoc_filters) == 1 + # User filter + auto-added TEMPORAL_RANGE for temporal x-axis + assert len(adhoc_filters) == 2 assert adhoc_filters[0]["subject"] == "OrderDate" + assert adhoc_filters[0]["operator"] == ">" + assert adhoc_filters[1]["operator"] == "TEMPORAL_RANGE" + assert adhoc_filters[1]["subject"] == "OrderDate" @patch( "superset.mcp_service.chart.validation.dataset_validator.DatasetValidator._get_dataset_context"