codeant-ai-for-open-source[bot] commented on code in PR #38977:
URL: https://github.com/apache/superset/pull/38977#discussion_r3016108662
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -369,6 +369,38 @@ async def get_chart_data( # noqa: C901
# Bubble charts use x/y/size as separate metric fields.
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.
+ deck_gl_types = (
+ "deck_arc",
+ "deck_geojson",
+ "deck_grid",
+ "deck_hex",
+ "deck_multi",
+ "deck_path",
+ "deck_polygon",
+ "deck_scatter",
+ "deck_screengrid",
+ )
+ if viz_type in deck_gl_types:
+ await ctx.warning(
Review Comment:
**Suggestion:** The deck.gl guard uses a hardcoded allowlist and misses
valid deck.gl viz types like `deck_heatmap` and `deck_contour`, so those charts
still fall through to the old fallback path and can trigger the same cryptic
empty-query failure this change is trying to prevent. Match all deck.gl charts
by prefix instead of enumerating a partial list. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Deck_heatmap/contour still bypass deck.gl-specific error handling.
- ⚠️ MCP get_chart_data UX inconsistent across deck.gl chart types.
- ⚠️ LLM tools get vague errors for some spatial charts.
```
</details>
```suggestion
if viz_type.startswith("deck_"):
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or locate a legacy deck.gl Heatmap or Contour chart in Superset
whose viz_type
is `"deck_heatmap"` or `"deck_contour"` as defined in
`superset/viz.py:2333-2366` and
`superset/viz.py:2368-2405` (classes `DeckHeatmap` and `DeckContour`), and
whose
`query_context` field in the `Slice` model is NULL (older charts created
before
query_context was introduced).
2. Start Superset with the MCP service enabled and invoke the MCP tool
`get_chart_data`
(entrypoint `superset/mcp_service/chart/tool/get_chart_data.py:85-190`) with
a
`GetChartDataRequest` where `identifier` is the numeric ID of this chart and
`form_data_key` is not set, so the code path uses the saved chart only.
3. In `get_chart_data`, chart lookup and dataset validation succeed
(`validate_chart_dataset` at `get_chart_data.py:202-216`), and because
`chart.query_context` is falsy and `using_unsaved_state` is False, execution
enters the
fallback block at `get_chart_data.py:343-360`, computes `viz_type =
chart.viz_type or ""`
(which is `"deck_heatmap"`/`"deck_contour"`) at `get_chart_data.py:370`, and
then
evaluates the deck.gl guard at `get_chart_data.py:372-386`.
4. Observe that the allowlist `deck_gl_types` at `get_chart_data.py:375-385`
only includes
`"deck_arc"`, `"deck_geojson"`, `"deck_grid"`, `"deck_hex"`, `"deck_multi"`,
`"deck_path"`, `"deck_polygon"`, `"deck_scatter"`, and `"deck_screengrid"`,
so the
condition `if viz_type in deck_gl_types:` at `get_chart_data.py:386` is
False for
`"deck_heatmap"` and `"deck_contour"`. As a result, these chart types bypass
the new
deck.gl-specific error branch and continue into the generic fallback query
construction
and error handling, so they still return the older generic
`MissingQueryContext`/`DataError` responses instead of the clear deck.gl
guidance this PR
intends for all deck.gl charts.
```
</details>
<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_data.py
**Line:** 376:387
**Comment:**
*Logic Error: The deck.gl guard uses a hardcoded allowlist and misses
valid deck.gl viz types like `deck_heatmap` and `deck_contour`, so those charts
still fall through to the old fallback path and can trigger the same cryptic
empty-query failure this change is trying to prevent. Match all deck.gl charts
by prefix instead of enumerating a partial list.
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%2F38977&comment_hash=c06c16fb3aff41734d6c7b6568088c09881fc046f103d76e1a4683569b299209&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38977&comment_hash=c06c16fb3aff41734d6c7b6568088c09881fc046f103d76e1a4683569b299209&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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
