You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org> on 2026/04/17 03:37:13 UTC
Re: [PR] feat(mcp): add get_chart_sql tool and expose chart filters in get_chart_info [superset]
codeant-ai-for-open-source[bot] commented on code in PR #38700:
URL: https://github.com/apache/superset/pull/38700#discussion_r3097702342
##########
superset/mcp_service/chart/tool/get_chart_sql.py:
##########
@@ -0,0 +1,484 @@
+# 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.
+
+"""
+MCP tool: get_chart_sql
+"""
+
+import logging
+from typing import Any, TYPE_CHECKING
+
+from fastmcp import Context
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+if TYPE_CHECKING:
+ from superset.models.slice import Slice
+
+from superset.commands.exceptions import CommandException
+from superset.commands.explore.form_data.parameters import CommandParameters
+from superset.exceptions import SupersetException
+from superset.extensions import event_logger
+from superset.mcp_service.chart.chart_utils import validate_chart_dataset
+from superset.mcp_service.chart.schemas import (
+ ChartError,
+ ChartSql,
+ GetChartSqlRequest,
+)
+from superset.mcp_service.utils.schema_utils import parse_request
+
+logger = logging.getLogger(__name__)
+
+
+def _get_cached_form_data(form_data_key: str) -> str | None:
+ """Retrieve form_data from cache using form_data_key.
+
+ Returns the JSON string of form_data if found, None otherwise.
+ """
+ from superset.commands.explore.form_data.get import GetFormDataCommand
+
+ try:
+ cmd_params = CommandParameters(key=form_data_key)
+ return GetFormDataCommand(cmd_params).run()
+ except (KeyError, ValueError, CommandException) as e:
+ logger.warning("Failed to retrieve form_data from cache: %s", e)
+ return None
+
+
+def _resolve_metrics_and_groupby(
+ form_data: dict[str, Any],
+ chart: "Slice | None",
+) -> tuple[list[Any], list[str]]:
+ """Resolve metrics and groupby columns from form_data.
+
+ Handles chart-type-specific field names: singular ``metric`` for
+ big-number variants, and fallback fields ``entity``, ``series``,
+ and ``columns`` for dimension resolution.
+ """
+ viz_type = form_data.get(
+ "viz_type", getattr(chart, "viz_type", "") if chart else ""
+ )
+
+ singular_metric_no_groupby = (
+ "big_number",
+ "big_number_total",
+ "pop_kpi",
+ )
+ if viz_type in singular_metric_no_groupby:
+ metrics: list[Any] = [metric] if (metric := form_data.get("metric")) else []
+ return metrics, []
+
+ metrics = form_data.get("metrics", [])
+ # Fallback: some chart types store the measure as singular "metric"
+ if not metrics and (metric := form_data.get("metric")):
+ metrics = [metric]
+
+ groupby = list(form_data.get("groupby") or [])
Review Comment:
**Suggestion:** Converting `groupby` directly with `list(...)` breaks when the value is a string (it becomes a list of characters), which produces invalid column names and malformed SQL. Normalize string `groupby` to a single-item list before building query columns. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ get_chart_sql mis-builds columns for migrated heatmap_v2 charts.
- ❌ Some charts yield SQL errors instead of usable query text.
- ⚠️ Inconsistent behavior versus get_chart_data string-groupby handling.
```
</details>
```suggestion
raw_groupby = form_data.get("groupby") or []
if isinstance(raw_groupby, str):
groupby = [raw_groupby]
else:
groupby = list(raw_groupby)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset with MCP enabled so that `get_chart_sql` is registered as a tool (see
`superset/mcp_service/app.py:25-39`, which imports `get_chart_sql` from
`superset.mcp_service.chart.tool`).
2. Use or migrate a legacy heatmap chart to `heatmap_v2`, which sets
`form_data["groupby"]` from the legacy `all_columns_y` field as a scalar string (see
`superset/migrations/shared/migrate_viz/processors.py:520-523`, where `"all_columns_y"` is
renamed to `"groupby"` without wrapping it in a list). This matches the known issue called
out in `superset/mcp_service/chart/tool/get_chart_data.py:95-103`, which explicitly guards
against a string `groupby`.
3. Call the MCP `get_chart_sql` tool for such a chart using its identifier (chart lookup
is handled in `_handle_chart_sql_request` at
`superset/mcp_service/chart/tool/get_chart_sql.py:378-399`, and effective form_data is
loaded from `chart.params` in `_resolve_effective_form_data` at lines 189-212).
4. When `_sql_from_form_data()` is invoked (lines 249-266), it calls
`_build_query_context_from_form_data()` (lines 106-170), which in turn calls
`_resolve_metrics_and_groupby()` at lines 61-103. There, `groupby =
list(form_data.get("groupby") or [])` at line 89 converts the string groupby (for example
`"country"`) into `["c", "o", "u", "n", "t", "r", "y"]`. These invalid column names are
passed as `"columns"` into `QueryContextFactory.create()` (lines 164-170), so
`ChartDataCommand.validate/run` (lines 256-259) generates SQL that refers to one column
per character or fails validation, causing `get_chart_sql` to return a `ChartError`
instead of correct SQL for the chart.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_sql.py%0A%2A%2ALine%3A%2A%2A%2089%3A89%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Converting%20%60groupby%60%20directly%20with%20%60list%28...%29%60%20breaks%20when%20the%20value%20is%20a%20string%20%28it%20becomes%20a%20list%20of%20characters%29%2C%20which%20produces%20invalid%20column%20names%20and%20malformed%20SQL.%20Normalize%20string%20%60groupby%60%20to%20a%20single-item%20list%20before%20building%20query%20columns.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20rev
iew.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_sql.py%0A%2A%2ALine%3A%2A%2A%2089%3A89%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Converting%20%60groupby%60%20directly%20with%20%60list%28...%29%60%20breaks%20when%20the%20value%20is%20a%20string%20%28it%20becomes%20a%20list%20of%20characters%29%2C%20which%20produces%20invalid%20column%20names%20and%20malformed%20SQL.%20Normalize%20string%20%60groupby%60%20to%20a%20single-item%20list%20before%20building%20query%20columns.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_sql.py
**Line:** 89:89
**Comment:**
*Logic Error: Converting `groupby` directly with `list(...)` breaks when the value is a string (it becomes a list of characters), which produces invalid column names and malformed SQL. Normalize string `groupby` to a single-item list before building query columns.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38700&comment_hash=70852edf274f910d42327c9cd2e1b5d99e1dd34b026f527c049ac8ef26aa5f01&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38700&comment_hash=70852edf274f910d42327c9cd2e1b5d99e1dd34b026f527c049ac8ef26aa5f01&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/tool/get_chart_sql.py:
##########
@@ -0,0 +1,484 @@
+# 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.
+
+"""
+MCP tool: get_chart_sql
+"""
+
+import logging
+from typing import Any, TYPE_CHECKING
+
+from fastmcp import Context
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+if TYPE_CHECKING:
+ from superset.models.slice import Slice
+
+from superset.commands.exceptions import CommandException
+from superset.commands.explore.form_data.parameters import CommandParameters
+from superset.exceptions import SupersetException
+from superset.extensions import event_logger
+from superset.mcp_service.chart.chart_utils import validate_chart_dataset
+from superset.mcp_service.chart.schemas import (
+ ChartError,
+ ChartSql,
+ GetChartSqlRequest,
+)
+from superset.mcp_service.utils.schema_utils import parse_request
+
+logger = logging.getLogger(__name__)
+
+
+def _get_cached_form_data(form_data_key: str) -> str | None:
+ """Retrieve form_data from cache using form_data_key.
+
+ Returns the JSON string of form_data if found, None otherwise.
+ """
+ from superset.commands.explore.form_data.get import GetFormDataCommand
+
+ try:
+ cmd_params = CommandParameters(key=form_data_key)
+ return GetFormDataCommand(cmd_params).run()
+ except (KeyError, ValueError, CommandException) as e:
+ logger.warning("Failed to retrieve form_data from cache: %s", e)
+ return None
+
+
+def _resolve_metrics_and_groupby(
+ form_data: dict[str, Any],
+ chart: "Slice | None",
+) -> tuple[list[Any], list[str]]:
+ """Resolve metrics and groupby columns from form_data.
+
+ Handles chart-type-specific field names: singular ``metric`` for
+ big-number variants, and fallback fields ``entity``, ``series``,
+ and ``columns`` for dimension resolution.
+ """
+ viz_type = form_data.get(
+ "viz_type", getattr(chart, "viz_type", "") if chart else ""
+ )
+
+ singular_metric_no_groupby = (
+ "big_number",
+ "big_number_total",
+ "pop_kpi",
+ )
+ if viz_type in singular_metric_no_groupby:
+ metrics: list[Any] = [metric] if (metric := form_data.get("metric")) else []
+ return metrics, []
+
+ metrics = form_data.get("metrics", [])
Review Comment:
**Suggestion:** Bubble charts store measures in `x`, `y`, and `size` instead of `metrics`/`metric`, so this logic currently produces empty metrics and fails SQL generation for that chart type. Add bubble-specific metric extraction before the generic metric fallback. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ get_chart_sql cannot generate SQL for bubble charts.
- ❌ Unsaved bubble charts via form_data_key also fail SQL retrieval.
- ⚠️ Agents cannot introspect bubble chart queries through MCP.
```
</details>
```suggestion
if viz_type == "bubble":
metrics = [m for field in ("x", "y", "size") if (m := form_data.get(field))]
else:
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Load the example World Bank dashboards to create the sample bubble chart `Life
Expectancy VS Rural %`, which is defined as `viz_type="bubble"` with metrics stored in
`x`, `y`, and `size` fields (see `superset/examples/world_bank.py:5-19`, where the `Slice`
is constructed with `x=...`, `y=...`, `size=...` and no `metrics` key in `params`).
2. Start Superset with MCP enabled so `get_chart_sql` is registered as a tool (tool
registration at `superset/mcp_service/app.py:25-39` imports `get_chart_sql`).
3. Invoke the MCP `get_chart_sql` tool with the identifier of this bubble chart. In
`_handle_chart_sql_request` (`superset/mcp_service/chart/tool/get_chart_sql.py:378-444`),
the chart is found via `_find_chart_by_identifier`, the dataset is validated via
`validate_chart_dataset`, and because this example chart has no saved `query_context`,
`_sql_from_saved_query_context()` at lines 215-247 returns `None`, triggering the fallback
`_sql_from_form_data()` at lines 249-266 using `chart.params` as `form_data`.
4. `_sql_from_form_data()` calls `_build_query_context_from_form_data()` (lines 106-170),
which calls `_resolve_metrics_and_groupby()` (lines 61-103). For `viz_type="bubble"`, the
current code executes `metrics = form_data.get("metrics", [])` at line 84 (empty, because
bubble stores metrics only in `x`, `y`, and `size`), then the singular-metric fallback at
lines 85-87 also finds no `metric`, leaving `metrics` empty. The resulting `QueryContext`
(lines 138-143, 164-170) has `"metrics": []`, and when `ChartDataCommand.validate()` and
`.run()` are called (lines 256-259), SQL generation fails or produces an "empty query"
style error, so `get_chart_sql` cannot return meaningful SQL for bubble charts.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_sql.py%0A%2A%2ALine%3A%2A%2A%2084%3A84%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Bubble%20charts%20store%20measures%20in%20%60x%60%2C%20%60y%60%2C%20and%20%60size%60%20instead%20of%20%60metrics%60%2F%60metric%60%2C%20so%20this%20logic%20currently%20produces%20empty%20metrics%20and%20fails%20SQL%20generation%20for%20that%20chart%20type.%20Add%20bubble-specific%20metric%20extraction%20before%20the%20generic%20metric%20fallback.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%2
0review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_sql.py%0A%2A%2ALine%3A%2A%2A%2084%3A84%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Bubble%20charts%20store%20measures%20in%20%60x%60%2C%20%60y%60%2C%20and%20%60size%60%20instead%20of%20%60metrics%60%2F%60metric%60%2C%20so%20this%20logic%20currently%20produces%20empty%20metrics%20and%20fails%20SQL%20generation%20for%20that%20chart%20type.%20Add%20bubble-specific%20metric%20extraction%20before%20the%20generic%20metric%20fallback.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_sql.py
**Line:** 84:84
**Comment:**
*Logic Error: Bubble charts store measures in `x`, `y`, and `size` instead of `metrics`/`metric`, so this logic currently produces empty metrics and fails SQL generation for that chart type. Add bubble-specific metric extraction before the generic metric fallback.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38700&comment_hash=692b94a8aac8ce1346dda2cde14007c00cc0361639790478066bf78f064cc5da&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38700&comment_hash=692b94a8aac8ce1346dda2cde14007c00cc0361639790478066bf78f064cc5da&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org