From d2add99027104bf5c53246f00919bb3c29586a8c Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Mon, 4 May 2026 18:34:30 +0000 Subject: [PATCH 1/5] fix(mcp): get_chart_sql drops x_axis on echarts_timeseries_* and only renders one query for mixed_timeseries - Add `_extract_x_axis_col` helper to normalise the x_axis field (plain string or adhoc column dict) into a bare column name. - In `_build_query_context_from_form_data`, detect echarts_timeseries_* and mixed_timeseries viz types and prepend the x_axis column to the query's columns list so the temporal dimension appears in the generated SQL (was previously dropped because x_axis is stored separately from groupby in form_data). - For mixed_timeseries, build a second query dict from metrics_b / groupby_b and pass queries=[primary, secondary] to QueryContextFactory. Previously only the primary query was built, so the secondary series SQL was silently lost. - Add TestExtractXAxisCol and TestBuildQueryContextTimeseriesAndMixed test classes covering both bug fixes. --- .../mcp_service/chart/tool/get_chart_sql.py | 72 +++-- .../chart/tool/test_get_chart_sql.py | 259 ++++++++++++++++++ 2 files changed, 314 insertions(+), 17 deletions(-) diff --git a/superset/mcp_service/chart/tool/get_chart_sql.py b/superset/mcp_service/chart/tool/get_chart_sql.py index d586817d2612..d91a75102587 100644 --- a/superset/mcp_service/chart/tool/get_chart_sql.py +++ b/superset/mcp_service/chart/tool/get_chart_sql.py @@ -141,6 +141,21 @@ def _resolve_metrics_and_groupby( return _resolve_metrics(form_data, viz_type), _resolve_groupby(form_data) +def _extract_x_axis_col(form_data: dict[str, Any]) -> str | None: + """Return the x_axis column name from form_data, or None if not set. + + ``x_axis`` may be stored as a plain column-name string or as an adhoc + column dict (``{"column_name": "...", ...}``). + """ + x_axis = form_data.get("x_axis") + if isinstance(x_axis, str) and x_axis: + return x_axis + if isinstance(x_axis, dict): + col_name = x_axis.get("column_name") + return col_name if isinstance(col_name, str) and col_name else None + return None + + def _resolve_engine( datasource_id: Any, datasource_type: str, @@ -209,22 +224,45 @@ def _build_query_context_from_form_data( merge_extra_filters(form_data) split_adhoc_filters_into_base_filters(form_data, engine) - # Build query dict with temporal and filter fields. - # QueryObjectFactory.create() accepts time_range as a top-level kwarg - # and converts it to from_dttm/to_dttm for the QueryObject. - query_dict: dict[str, Any] = { - "columns": groupby, - "metrics": metrics, - } - - if time_range := form_data.get("time_range"): - query_dict["time_range"] = time_range - - if filters := form_data.get("filters"): - query_dict["filters"] = filters - - if (row_limit := form_data.get("row_limit")) is not None: - query_dict["row_limit"] = row_limit + viz_type: str = ( + form_data.get("viz_type") + or (getattr(chart, "viz_type", "") if chart else "") + or "" + ) + is_timeseries = viz_type.startswith("echarts_timeseries") or viz_type == "mixed_timeseries" + + # For echarts_timeseries_* and mixed_timeseries charts the temporal + # column is stored in x_axis rather than groupby. Prepend it so the + # generated SQL includes the time axis. + x_axis_col: str | None = None + if is_timeseries: + x_axis_col = _extract_x_axis_col(form_data) + if x_axis_col and x_axis_col not in groupby: + groupby = [x_axis_col] + groupby + + def _make_query_dict(cols: list[str], mets: list[Any]) -> dict[str, Any]: + """Build a single query entry for QueryContextFactory.""" + qd: dict[str, Any] = {"columns": cols, "metrics": mets} + if time_range := form_data.get("time_range"): + qd["time_range"] = time_range + if filters := form_data.get("filters"): + qd["filters"] = filters + if (row_limit := form_data.get("row_limit")) is not None: + qd["row_limit"] = row_limit + return qd + + queries: list[dict[str, Any]] = [_make_query_dict(groupby, metrics)] + + # mixed_timeseries exposes two independent query layers (primary and + # secondary). Build the second query from metrics_b / groupby_b so + # that get_chart_sql returns SQL for both and neither is silently lost. + if viz_type == "mixed_timeseries": + metrics_b: list[Any] = list(form_data.get("metrics_b") or []) + raw_b = form_data.get("groupby_b") or [] + groupby_b: list[str] = [raw_b] if isinstance(raw_b, str) else list(raw_b) + if x_axis_col and x_axis_col not in groupby_b: + groupby_b = [x_axis_col] + groupby_b + queries.append(_make_query_dict(groupby_b, metrics_b)) # Ensure datasource fields satisfy DatasourceDict typing requirements. # datasource_id must be int | str; datasource_type must be str. @@ -238,7 +276,7 @@ def _build_query_context_from_form_data( return factory.create( datasource={"id": resolved_id, "type": resolved_type_str}, - queries=[query_dict], + queries=queries, form_data=form_data, result_type=ChartDataResultType.QUERY, force=False, diff --git a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py index f752beba8b9d..177088b46cf6 100644 --- a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py +++ b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py @@ -33,6 +33,7 @@ from superset.mcp_service.chart.tool.get_chart_sql import ( _build_query_context_from_form_data, _extract_sql_from_result, + _extract_x_axis_col, _find_chart_by_identifier, _resolve_datasource_name, _resolve_effective_form_data, @@ -468,6 +469,264 @@ def test_metrics_and_groupby_in_queries(self, mock_factory_cls): assert queries[0]["columns"] == ["product"] +class TestExtractXAxisCol: + """Tests for the _extract_x_axis_col helper.""" + + def test_string_x_axis(self): + """Plain string x_axis returns the string directly.""" + assert _extract_x_axis_col({"x_axis": "order_date"}) == "order_date" + + def test_dict_x_axis(self): + """Adhoc column dict x_axis returns column_name.""" + assert _extract_x_axis_col( + {"x_axis": {"column_name": "ds", "label": "ds", "expressionType": "SIMPLE"}} + ) == "ds" + + def test_missing_x_axis_returns_none(self): + """Missing x_axis key returns None.""" + assert _extract_x_axis_col({}) is None + + def test_none_x_axis_returns_none(self): + """Explicit None x_axis returns None.""" + assert _extract_x_axis_col({"x_axis": None}) is None + + def test_empty_string_x_axis_returns_none(self): + """Empty string x_axis returns None.""" + assert _extract_x_axis_col({"x_axis": ""}) is None + + def test_dict_missing_column_name_returns_none(self): + """Adhoc column dict without column_name returns None.""" + assert _extract_x_axis_col({"x_axis": {"label": "ds"}}) is None + + +class TestBuildQueryContextTimeseriesAndMixed: + """Tests for x_axis and mixed_timeseries fixes in _build_query_context_from_form_data.""" + + @patch("superset.common.query_context_factory.QueryContextFactory") + @patch("superset.daos.datasource.DatasourceDAO.get_datasource") + def test_echarts_timeseries_x_axis_included_in_columns( + self, mock_get_ds, mock_factory_cls + ): + """x_axis column is prepended to query columns for echarts_timeseries charts.""" + mock_ds = Mock() + mock_ds.database.db_engine_spec.engine = "postgresql" + mock_get_ds.return_value = mock_ds + + mock_factory = Mock() + mock_factory.create.return_value = Mock() + mock_factory_cls.return_value = mock_factory + + form_data = { + "datasource_id": 1, + "datasource_type": "table", + "viz_type": "echarts_timeseries_line", + "x_axis": "ds", + "metrics": ["sum__sales"], + "groupby": ["region"], + } + + with patch("superset.common.chart_data.ChartDataResultType") as mock_rt: + mock_rt.QUERY = "QUERY" + _build_query_context_from_form_data(form_data, chart=None) + + queries = mock_factory.create.call_args[1]["queries"] + assert len(queries) == 1 + assert queries[0]["columns"][0] == "ds" + assert "region" in queries[0]["columns"] + + @patch("superset.common.query_context_factory.QueryContextFactory") + @patch("superset.daos.datasource.DatasourceDAO.get_datasource") + def test_echarts_timeseries_dict_x_axis_included_in_columns( + self, mock_get_ds, mock_factory_cls + ): + """Adhoc-column x_axis dict is resolved and prepended for echarts_timeseries.""" + mock_ds = Mock() + mock_ds.database.db_engine_spec.engine = "postgresql" + mock_get_ds.return_value = mock_ds + + mock_factory = Mock() + mock_factory.create.return_value = Mock() + mock_factory_cls.return_value = mock_factory + + form_data = { + "datasource_id": 1, + "datasource_type": "table", + "viz_type": "echarts_timeseries_bar", + "x_axis": {"column_name": "order_date", "expressionType": "SIMPLE"}, + "metrics": ["count"], + "groupby": [], + } + + with patch("superset.common.chart_data.ChartDataResultType") as mock_rt: + mock_rt.QUERY = "QUERY" + _build_query_context_from_form_data(form_data, chart=None) + + queries = mock_factory.create.call_args[1]["queries"] + assert queries[0]["columns"][0] == "order_date" + + @patch("superset.common.query_context_factory.QueryContextFactory") + @patch("superset.daos.datasource.DatasourceDAO.get_datasource") + def test_echarts_timeseries_x_axis_not_duplicated_if_already_in_groupby( + self, mock_get_ds, mock_factory_cls + ): + """x_axis is not duplicated if it is already in groupby.""" + mock_ds = Mock() + mock_ds.database.db_engine_spec.engine = "postgresql" + mock_get_ds.return_value = mock_ds + + mock_factory = Mock() + mock_factory.create.return_value = Mock() + mock_factory_cls.return_value = mock_factory + + form_data = { + "datasource_id": 1, + "datasource_type": "table", + "viz_type": "echarts_timeseries_line", + "x_axis": "ds", + "metrics": ["count"], + "groupby": ["ds"], # already present + } + + with patch("superset.common.chart_data.ChartDataResultType") as mock_rt: + mock_rt.QUERY = "QUERY" + _build_query_context_from_form_data(form_data, chart=None) + + queries = mock_factory.create.call_args[1]["queries"] + assert queries[0]["columns"].count("ds") == 1 + + @patch("superset.common.query_context_factory.QueryContextFactory") + @patch("superset.daos.datasource.DatasourceDAO.get_datasource") + def test_non_timeseries_x_axis_not_added(self, mock_get_ds, mock_factory_cls): + """x_axis is not added for non-timeseries chart types (e.g. table).""" + mock_ds = Mock() + mock_ds.database.db_engine_spec.engine = "postgresql" + mock_get_ds.return_value = mock_ds + + mock_factory = Mock() + mock_factory.create.return_value = Mock() + mock_factory_cls.return_value = mock_factory + + form_data = { + "datasource_id": 1, + "datasource_type": "table", + "viz_type": "table", + "x_axis": "ds", + "metrics": ["count"], + "groupby": ["region"], + } + + with patch("superset.common.chart_data.ChartDataResultType") as mock_rt: + mock_rt.QUERY = "QUERY" + _build_query_context_from_form_data(form_data, chart=None) + + queries = mock_factory.create.call_args[1]["queries"] + assert "ds" not in queries[0]["columns"] + + @patch("superset.common.query_context_factory.QueryContextFactory") + @patch("superset.daos.datasource.DatasourceDAO.get_datasource") + def test_mixed_timeseries_produces_two_queries(self, mock_get_ds, mock_factory_cls): + """mixed_timeseries builds two query dicts — one per series layer.""" + mock_ds = Mock() + mock_ds.database.db_engine_spec.engine = "postgresql" + mock_get_ds.return_value = mock_ds + + mock_factory = Mock() + mock_factory.create.return_value = Mock() + mock_factory_cls.return_value = mock_factory + + form_data = { + "datasource_id": 1, + "datasource_type": "table", + "viz_type": "mixed_timeseries", + "x_axis": "ds", + "metrics": ["sum__revenue"], + "groupby": ["country"], + "metrics_b": ["count"], + "groupby_b": ["channel"], + "time_range": "Last 30 days", + } + + with patch("superset.common.chart_data.ChartDataResultType") as mock_rt: + mock_rt.QUERY = "QUERY" + _build_query_context_from_form_data(form_data, chart=None) + + queries = mock_factory.create.call_args[1]["queries"] + assert len(queries) == 2 + + # Primary query + assert "ds" in queries[0]["columns"] + assert "country" in queries[0]["columns"] + assert queries[0]["metrics"] == ["sum__revenue"] + assert queries[0]["time_range"] == "Last 30 days" + + # Secondary query + assert "ds" in queries[1]["columns"] + assert "channel" in queries[1]["columns"] + assert queries[1]["metrics"] == ["count"] + assert queries[1]["time_range"] == "Last 30 days" + + @patch("superset.common.query_context_factory.QueryContextFactory") + @patch("superset.daos.datasource.DatasourceDAO.get_datasource") + def test_mixed_timeseries_x_axis_not_duplicated_in_secondary( + self, mock_get_ds, mock_factory_cls + ): + """x_axis is not duplicated in the secondary query if already in groupby_b.""" + mock_ds = Mock() + mock_ds.database.db_engine_spec.engine = "postgresql" + mock_get_ds.return_value = mock_ds + + mock_factory = Mock() + mock_factory.create.return_value = Mock() + mock_factory_cls.return_value = mock_factory + + form_data = { + "datasource_id": 1, + "datasource_type": "table", + "viz_type": "mixed_timeseries", + "x_axis": "ds", + "metrics": ["count"], + "groupby": [], + "metrics_b": ["sum__sales"], + "groupby_b": ["ds"], # x_axis already present + } + + with patch("superset.common.chart_data.ChartDataResultType") as mock_rt: + mock_rt.QUERY = "QUERY" + _build_query_context_from_form_data(form_data, chart=None) + + queries = mock_factory.create.call_args[1]["queries"] + assert queries[1]["columns"].count("ds") == 1 + + @patch("superset.common.query_context_factory.QueryContextFactory") + @patch("superset.daos.datasource.DatasourceDAO.get_datasource") + def test_mixed_timeseries_empty_secondary(self, mock_get_ds, mock_factory_cls): + """mixed_timeseries with no metrics_b/groupby_b still produces two queries.""" + mock_ds = Mock() + mock_ds.database.db_engine_spec.engine = "postgresql" + mock_get_ds.return_value = mock_ds + + mock_factory = Mock() + mock_factory.create.return_value = Mock() + mock_factory_cls.return_value = mock_factory + + form_data = { + "datasource_id": 1, + "datasource_type": "table", + "viz_type": "mixed_timeseries", + "x_axis": "ds", + "metrics": ["count"], + "groupby": [], + } + + with patch("superset.common.chart_data.ChartDataResultType") as mock_rt: + mock_rt.QUERY = "QUERY" + _build_query_context_from_form_data(form_data, chart=None) + + queries = mock_factory.create.call_args[1]["queries"] + assert len(queries) == 2 + assert queries[1]["metrics"] == [] + + class TestResolveDatasourceName: """Tests for _resolve_datasource_name helper.""" From 61aaed796f4607f3d84de33908608c0cd21e48e6 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Mon, 4 May 2026 18:37:16 +0000 Subject: [PATCH 2/5] chore: ruff format get_chart_sql and tests --- superset/mcp_service/chart/tool/get_chart_sql.py | 4 +++- .../mcp_service/chart/tool/test_get_chart_sql.py | 15 ++++++++++++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/superset/mcp_service/chart/tool/get_chart_sql.py b/superset/mcp_service/chart/tool/get_chart_sql.py index d91a75102587..2db5bd518512 100644 --- a/superset/mcp_service/chart/tool/get_chart_sql.py +++ b/superset/mcp_service/chart/tool/get_chart_sql.py @@ -229,7 +229,9 @@ def _build_query_context_from_form_data( or (getattr(chart, "viz_type", "") if chart else "") or "" ) - is_timeseries = viz_type.startswith("echarts_timeseries") or viz_type == "mixed_timeseries" + is_timeseries = ( + viz_type.startswith("echarts_timeseries") or viz_type == "mixed_timeseries" + ) # For echarts_timeseries_* and mixed_timeseries charts the temporal # column is stored in x_axis rather than groupby. Prepend it so the diff --git a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py index 177088b46cf6..24e38663ba45 100644 --- a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py +++ b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py @@ -478,9 +478,18 @@ def test_string_x_axis(self): def test_dict_x_axis(self): """Adhoc column dict x_axis returns column_name.""" - assert _extract_x_axis_col( - {"x_axis": {"column_name": "ds", "label": "ds", "expressionType": "SIMPLE"}} - ) == "ds" + assert ( + _extract_x_axis_col( + { + "x_axis": { + "column_name": "ds", + "label": "ds", + "expressionType": "SIMPLE", + } + } + ) + == "ds" + ) def test_missing_x_axis_returns_none(self): """Missing x_axis key returns None.""" From d099748ca69ba9de4ecf8c1eb36d2f7d13e2f013 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Wed, 6 May 2026 17:01:10 +0000 Subject: [PATCH 3/5] refactor: extract _build_single_query_dict and _build_mixed_timeseries_secondary to reduce complexity Move the nested _make_query_dict helper and the inline mixed_timeseries secondary query logic out of _build_query_context_from_form_data into module-level functions. This brings the McCabe complexity of _build_query_context_from_form_data back within the C901 limit of 10. --- .../mcp_service/chart/tool/get_chart_sql.py | 55 +++++++++++++------ .../chart/tool/test_get_chart_sql.py | 6 +- 2 files changed, 42 insertions(+), 19 deletions(-) diff --git a/superset/mcp_service/chart/tool/get_chart_sql.py b/superset/mcp_service/chart/tool/get_chart_sql.py index 2db5bd518512..369e751098be 100644 --- a/superset/mcp_service/chart/tool/get_chart_sql.py +++ b/superset/mcp_service/chart/tool/get_chart_sql.py @@ -177,6 +177,39 @@ def _resolve_engine( return "base" +def _build_single_query_dict( + form_data: dict[str, Any], + columns: list[str], + metrics: list[Any], +) -> dict[str, Any]: + """Build one query entry for QueryContextFactory from form_data fields.""" + qd: dict[str, Any] = {"columns": columns, "metrics": metrics} + if time_range := form_data.get("time_range"): + qd["time_range"] = time_range + if filters := form_data.get("filters"): + qd["filters"] = filters + if (row_limit := form_data.get("row_limit")) is not None: + qd["row_limit"] = row_limit + return qd + + +def _build_mixed_timeseries_secondary( + form_data: dict[str, Any], + x_axis_col: str | None, +) -> dict[str, Any]: + """Build the secondary query dict for the ``mixed_timeseries`` viz type. + + ``mixed_timeseries`` has two independent series layers; the secondary + layer uses ``metrics_b`` / ``groupby_b`` instead of the primary fields. + """ + metrics_b: list[Any] = list(form_data.get("metrics_b") or []) + raw_b = form_data.get("groupby_b") or [] + groupby_b: list[str] = [raw_b] if isinstance(raw_b, str) else list(raw_b) + if x_axis_col and x_axis_col not in groupby_b: + groupby_b = [x_axis_col] + groupby_b + return _build_single_query_dict(form_data, groupby_b, metrics_b) + + def _build_query_context_from_form_data( form_data: dict[str, Any], chart: "Slice | None" = None, @@ -242,29 +275,15 @@ def _build_query_context_from_form_data( if x_axis_col and x_axis_col not in groupby: groupby = [x_axis_col] + groupby - def _make_query_dict(cols: list[str], mets: list[Any]) -> dict[str, Any]: - """Build a single query entry for QueryContextFactory.""" - qd: dict[str, Any] = {"columns": cols, "metrics": mets} - if time_range := form_data.get("time_range"): - qd["time_range"] = time_range - if filters := form_data.get("filters"): - qd["filters"] = filters - if (row_limit := form_data.get("row_limit")) is not None: - qd["row_limit"] = row_limit - return qd - - queries: list[dict[str, Any]] = [_make_query_dict(groupby, metrics)] + queries: list[dict[str, Any]] = [ + _build_single_query_dict(form_data, groupby, metrics) + ] # mixed_timeseries exposes two independent query layers (primary and # secondary). Build the second query from metrics_b / groupby_b so # that get_chart_sql returns SQL for both and neither is silently lost. if viz_type == "mixed_timeseries": - metrics_b: list[Any] = list(form_data.get("metrics_b") or []) - raw_b = form_data.get("groupby_b") or [] - groupby_b: list[str] = [raw_b] if isinstance(raw_b, str) else list(raw_b) - if x_axis_col and x_axis_col not in groupby_b: - groupby_b = [x_axis_col] + groupby_b - queries.append(_make_query_dict(groupby_b, metrics_b)) + queries.append(_build_mixed_timeseries_secondary(form_data, x_axis_col)) # Ensure datasource fields satisfy DatasourceDict typing requirements. # datasource_id must be int | str; datasource_type must be str. diff --git a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py index 24e38663ba45..10da0a1f1baf 100644 --- a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py +++ b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py @@ -509,7 +509,11 @@ def test_dict_missing_column_name_returns_none(self): class TestBuildQueryContextTimeseriesAndMixed: - """Tests for x_axis and mixed_timeseries fixes in _build_query_context_from_form_data.""" + """Tests for x_axis and mixed_timeseries query-context fixes. + + Covers both bugs after #39636: x_axis dropped for echarts_timeseries_* + and only one query rendered for mixed_timeseries. + """ @patch("superset.common.query_context_factory.QueryContextFactory") @patch("superset.daos.datasource.DatasourceDAO.get_datasource") From 1507685fc5f0d96074053772b02a63e1463ee40c Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Thu, 7 May 2026 00:14:09 +0000 Subject: [PATCH 4/5] fix(mcp): handle time_range_b in mixed_timeseries secondary query, tighten types - _build_mixed_timeseries_secondary: apply time_range_b from form_data to the secondary query dict when present, so charts with an independent secondary time range produce correct SQL via the fallback path - Change groupby_b annotation from list[str] to list[Any] to reflect that _build_single_query_dict columns accept Any items (e.g. adhoc dicts) - Add test: secondary time_range overridden when time_range_b is set - Add test: SQL-expression x_axis (no column_name) returns None - Fix test class docstring: remove stale PR reference --- .../mcp_service/chart/tool/get_chart_sql.py | 11 +++- .../chart/tool/test_get_chart_sql.py | 57 ++++++++++++++++++- 2 files changed, 62 insertions(+), 6 deletions(-) diff --git a/superset/mcp_service/chart/tool/get_chart_sql.py b/superset/mcp_service/chart/tool/get_chart_sql.py index 369e751098be..b354899c057a 100644 --- a/superset/mcp_service/chart/tool/get_chart_sql.py +++ b/superset/mcp_service/chart/tool/get_chart_sql.py @@ -179,7 +179,7 @@ def _resolve_engine( def _build_single_query_dict( form_data: dict[str, Any], - columns: list[str], + columns: list[Any], metrics: list[Any], ) -> dict[str, Any]: """Build one query entry for QueryContextFactory from form_data fields.""" @@ -201,13 +201,18 @@ def _build_mixed_timeseries_secondary( ``mixed_timeseries`` has two independent series layers; the secondary layer uses ``metrics_b`` / ``groupby_b`` instead of the primary fields. + If ``time_range_b`` is set it overrides the primary ``time_range`` for + the secondary query. """ metrics_b: list[Any] = list(form_data.get("metrics_b") or []) raw_b = form_data.get("groupby_b") or [] - groupby_b: list[str] = [raw_b] if isinstance(raw_b, str) else list(raw_b) + groupby_b: list[Any] = [raw_b] if isinstance(raw_b, str) else list(raw_b) if x_axis_col and x_axis_col not in groupby_b: groupby_b = [x_axis_col] + groupby_b - return _build_single_query_dict(form_data, groupby_b, metrics_b) + qd = _build_single_query_dict(form_data, groupby_b, metrics_b) + if time_range_b := form_data.get("time_range_b"): + qd["time_range"] = time_range_b + return qd def _build_query_context_from_form_data( diff --git a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py index 10da0a1f1baf..4fcf3ef7a18f 100644 --- a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py +++ b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py @@ -507,12 +507,27 @@ def test_dict_missing_column_name_returns_none(self): """Adhoc column dict without column_name returns None.""" assert _extract_x_axis_col({"x_axis": {"label": "ds"}}) is None + def test_sql_expression_x_axis_returns_none(self): + """SQL expression adhoc columns have no column_name; returns None.""" + assert ( + _extract_x_axis_col( + { + "x_axis": { + "expressionType": "SQL", + "sqlExpression": "DATE_TRUNC('day', created_at)", + "label": "day", + } + } + ) + is None + ) + class TestBuildQueryContextTimeseriesAndMixed: - """Tests for x_axis and mixed_timeseries query-context fixes. + """Regression tests for x_axis and mixed_timeseries query-context fixes. - Covers both bugs after #39636: x_axis dropped for echarts_timeseries_* - and only one query rendered for mixed_timeseries. + Guards against two bugs: x_axis column dropped for echarts_timeseries_* + charts, and only one query rendered for mixed_timeseries charts. """ @patch("superset.common.query_context_factory.QueryContextFactory") @@ -739,6 +754,42 @@ def test_mixed_timeseries_empty_secondary(self, mock_get_ds, mock_factory_cls): assert len(queries) == 2 assert queries[1]["metrics"] == [] + @patch("superset.common.query_context_factory.QueryContextFactory") + @patch("superset.daos.datasource.DatasourceDAO.get_datasource") + def test_mixed_timeseries_time_range_b_overrides_secondary( + self, mock_get_ds, mock_factory_cls + ): + """time_range_b overrides the primary time_range for the secondary query.""" + mock_ds = Mock() + mock_ds.database.db_engine_spec.engine = "postgresql" + mock_get_ds.return_value = mock_ds + + mock_factory = Mock() + mock_factory.create.return_value = Mock() + mock_factory_cls.return_value = mock_factory + + form_data = { + "datasource_id": 1, + "datasource_type": "table", + "viz_type": "mixed_timeseries", + "x_axis": "ds", + "metrics": ["sum__revenue"], + "groupby": [], + "metrics_b": ["count"], + "groupby_b": [], + "time_range": "Last 30 days", + "time_range_b": "Last 7 days", + } + + with patch("superset.common.chart_data.ChartDataResultType") as mock_rt: + mock_rt.QUERY = "QUERY" + _build_query_context_from_form_data(form_data, chart=None) + + queries = mock_factory.create.call_args[1]["queries"] + assert len(queries) == 2 + assert queries[0]["time_range"] == "Last 30 days" + assert queries[1]["time_range"] == "Last 7 days" + class TestResolveDatasourceName: """Tests for _resolve_datasource_name helper.""" From 8b31f21e12f39e555524992d0ad5ef406784c86e Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Thu, 7 May 2026 16:45:27 +0000 Subject: [PATCH 5/5] fix(mcp): apply row_limit_b and adhoc_filters_b to mixed_timeseries secondary query The secondary series in mixed_timeseries can have independent row limit and adhoc filters configured separately from the primary series. - _build_mixed_timeseries_secondary now accepts engine and uses it to process adhoc_filters_b via split_adhoc_filters_into_base_filters, replacing the inherited primary filters on the secondary query dict - row_limit_b overrides row_limit on the secondary query dict when set - Tests: row_limit_b, adhoc_filters_b each exercised independently --- .../mcp_service/chart/tool/get_chart_sql.py | 20 ++++- .../chart/tool/test_get_chart_sql.py | 79 +++++++++++++++++++ 2 files changed, 96 insertions(+), 3 deletions(-) diff --git a/superset/mcp_service/chart/tool/get_chart_sql.py b/superset/mcp_service/chart/tool/get_chart_sql.py index b354899c057a..1792b928831b 100644 --- a/superset/mcp_service/chart/tool/get_chart_sql.py +++ b/superset/mcp_service/chart/tool/get_chart_sql.py @@ -196,13 +196,15 @@ def _build_single_query_dict( def _build_mixed_timeseries_secondary( form_data: dict[str, Any], x_axis_col: str | None, + engine: str = "base", ) -> dict[str, Any]: """Build the secondary query dict for the ``mixed_timeseries`` viz type. ``mixed_timeseries`` has two independent series layers; the secondary layer uses ``metrics_b`` / ``groupby_b`` instead of the primary fields. - If ``time_range_b`` is set it overrides the primary ``time_range`` for - the secondary query. + Secondary-specific overrides (``time_range_b``, ``row_limit_b``, + ``adhoc_filters_b``) replace the corresponding primary values so the + generated SQL accurately reflects each series' independent configuration. """ metrics_b: list[Any] = list(form_data.get("metrics_b") or []) raw_b = form_data.get("groupby_b") or [] @@ -212,6 +214,18 @@ def _build_mixed_timeseries_secondary( qd = _build_single_query_dict(form_data, groupby_b, metrics_b) if time_range_b := form_data.get("time_range_b"): qd["time_range"] = time_range_b + if (row_limit_b := form_data.get("row_limit_b")) is not None: + qd["row_limit"] = row_limit_b + # Process adhoc_filters_b into concrete filter clauses for the secondary + # query, mirroring how split_adhoc_filters_into_base_filters handles the + # primary adhoc_filters in _build_query_context_from_form_data. + if adhoc_filters_b := form_data.get("adhoc_filters_b"): + from superset.utils.core import split_adhoc_filters_into_base_filters + + secondary_fd: dict[str, Any] = {"adhoc_filters": adhoc_filters_b} + split_adhoc_filters_into_base_filters(secondary_fd, engine) + if secondary_filters := secondary_fd.get("filters"): + qd["filters"] = secondary_filters return qd @@ -288,7 +302,7 @@ def _build_query_context_from_form_data( # secondary). Build the second query from metrics_b / groupby_b so # that get_chart_sql returns SQL for both and neither is silently lost. if viz_type == "mixed_timeseries": - queries.append(_build_mixed_timeseries_secondary(form_data, x_axis_col)) + queries.append(_build_mixed_timeseries_secondary(form_data, x_axis_col, engine)) # Ensure datasource fields satisfy DatasourceDict typing requirements. # datasource_id must be int | str; datasource_type must be str. diff --git a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py index 4fcf3ef7a18f..3e6d588fa6c6 100644 --- a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py +++ b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py @@ -790,6 +790,85 @@ def test_mixed_timeseries_time_range_b_overrides_secondary( assert queries[0]["time_range"] == "Last 30 days" assert queries[1]["time_range"] == "Last 7 days" + @patch("superset.common.query_context_factory.QueryContextFactory") + @patch("superset.daos.datasource.DatasourceDAO.get_datasource") + def test_mixed_timeseries_row_limit_b_overrides_secondary( + self, mock_get_ds, mock_factory_cls + ): + """row_limit_b overrides the primary row_limit for the secondary query.""" + mock_ds = Mock() + mock_ds.database.db_engine_spec.engine = "postgresql" + mock_get_ds.return_value = mock_ds + + mock_factory = Mock() + mock_factory.create.return_value = Mock() + mock_factory_cls.return_value = mock_factory + + form_data = { + "datasource_id": 1, + "datasource_type": "table", + "viz_type": "mixed_timeseries", + "x_axis": "ds", + "metrics": ["sum__revenue"], + "groupby": [], + "metrics_b": ["count"], + "groupby_b": [], + "row_limit": 100, + "row_limit_b": 50, + } + + with patch("superset.common.chart_data.ChartDataResultType") as mock_rt: + mock_rt.QUERY = "QUERY" + _build_query_context_from_form_data(form_data, chart=None) + + queries = mock_factory.create.call_args[1]["queries"] + assert len(queries) == 2 + assert queries[0]["row_limit"] == 100 + assert queries[1]["row_limit"] == 50 + + @patch("superset.common.query_context_factory.QueryContextFactory") + @patch("superset.daos.datasource.DatasourceDAO.get_datasource") + def test_mixed_timeseries_adhoc_filters_b_applied_to_secondary( + self, mock_get_ds, mock_factory_cls + ): + """adhoc_filters_b is processed and applied to the secondary query filters.""" + mock_ds = Mock() + mock_ds.database.db_engine_spec.engine = "postgresql" + mock_get_ds.return_value = mock_ds + + mock_factory = Mock() + mock_factory.create.return_value = Mock() + mock_factory_cls.return_value = mock_factory + + form_data = { + "datasource_id": 1, + "datasource_type": "table", + "viz_type": "mixed_timeseries", + "x_axis": "ds", + "metrics": ["sum__revenue"], + "groupby": [], + "metrics_b": ["count"], + "groupby_b": [], + "adhoc_filters_b": [ + { + "clause": "WHERE", + "expressionType": "SIMPLE", + "subject": "channel", + "operator": "==", + "comparator": "organic", + } + ], + } + + with patch("superset.common.chart_data.ChartDataResultType") as mock_rt: + mock_rt.QUERY = "QUERY" + _build_query_context_from_form_data(form_data, chart=None) + + queries = mock_factory.create.call_args[1]["queries"] + assert len(queries) == 2 + secondary_filters = queries[1].get("filters", []) + assert {"col": "channel", "op": "==", "val": "organic"} in secondary_filters + class TestResolveDatasourceName: """Tests for _resolve_datasource_name helper."""