codeant-ai-for-open-source[bot] commented on code in PR #40099:
URL: https://github.com/apache/superset/pull/40099#discussion_r3238423307
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -903,18 +902,25 @@ async def _query_from_form_data(
groupby = list(form_data.get("groupby") or [])
try:
+ prepare_form_data_for_query(
+ form_data,
+ datasource_id,
+ datasource_type,
+ request.extra_form_data,
+ )
+
+ query_dict: dict[str, Any] = {
+ "columns": groupby,
+ "metrics": metrics,
+ "row_limit": row_limit,
+ "order_desc": form_data.get("order_desc", True),
+ }
+ apply_form_data_filters_to_query(query_dict, form_data)
+
factory = QueryContextFactory()
query_context = factory.create(
datasource={"id": datasource_id, "type": datasource_type},
- queries=[
- {
- "filters": form_data.get("filters", []),
- "columns": groupby,
- "metrics": metrics,
- "row_limit": row_limit,
- "order_desc": form_data.get("order_desc", True),
- }
- ],
+ queries=[query_dict],
form_data=form_data,
force=request.force_refresh,
)
Review Comment:
**Suggestion:** The unsaved-chart path now builds a single minimal query
from only `groupby` and `metrics`, then forces it into `QueryContextFactory`.
This drops chart-type-specific query structure (for example `x_axis`-driven
timeseries columns, bubble `x/y/size`, or mixed-timeseries secondary queries),
so valid cached form-data can return incorrect or empty results. Build query
objects with the same chart-type resolution logic used elsewhere (or derive
full queries from normalized form data) before passing `queries`. [incomplete
implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unsaved mixed_timeseries charts lose secondary series data.
- ⚠️ Unsaved bubble charts may fail or return empty results.
- ⚠️ LLM MCP get_chart_data consumers see inconsistent chart values.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In an MCP-enabled Superset instance, create an unsaved `mixed_timeseries`
chart in
Explore so its temporary state is cached via `GetFormDataCommand`
(superset/mcp_service/chart/chart_helpers.py:54-67) and obtain the associated
`form_data_key`.
2. From an MCP client, call `get_chart_data`
(superset/mcp_service/chart/tool/get_chart_data.py:108-183) with
`identifier=None` and
that `form_data_key`, so the unsaved-chart branch at lines 153-183 executes
and delegates
to `_query_from_form_data(form_data, request, ctx)`
(superset/mcp_service/chart/tool/get_chart_data.py:65-73, 97-105, 114-128).
3. Inside `_query_from_form_data`
(superset/mcp_service/chart/tool/get_chart_data.py:92-121, 114-120), observe
that it only
reads `metrics = form_data.get("metrics", [])` and `groupby =
list(form_data.get("groupby") or [])` and then builds a single `query_dict`
with those
minimal fields at lines 114-119, ignoring chart-type-specific fields like
`metrics_b`/`groupby_b` for `mixed_timeseries` or bubble `x`/`y`/`size`
metrics that are
handled in the saved-chart fallback path
(superset/mcp_service/chart/tool/get_chart_data.py:37-12, 37-85) and in
`_build_query_context_from_form_data` for SQL
(superset/mcp_service/chart/tool/get_chart_sql.py:233-247, 197-220).
4. When `QueryContextFactory.create(..., queries=[query_dict],
form_data=form_data, ...)`
runs (superset/mcp_service/chart/tool/get_chart_data.py:122-128,
superset/common/query_context_factory.py:45-89) and `ChartDataCommand.run()`
executes, the
resulting data only includes the primary metrics/groupby and silently omits
any secondary
mixed-timeseries series or bubble metrics, so MCP callers receive incomplete
or incorrect
data for valid unsaved 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_data.py%0A%2A%2ALine%3A%2A%2A%20912%3A926%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncomplete%20Implementation%3A%20The%20unsaved-chart%20path%20now%20builds%20a%20single%20minimal%20query%20from%20only%20%60groupby%60%20and%20%60metrics%60%2C%20then%20forces%20it%20into%20%60QueryContextFactory%60.%20This%20drops%20chart-type-specific%20query%20structure%20%28for%20example%20%60x_axis%60-driven%20timeseries%20columns%2C%20bubble%20%60x%2Fy%2Fsize%60%2C%20or%20mixed-timeseries%20secondary%20queries%29%2C%20so%20valid%20cached%20form-data%20can%20return%20incorrect%20or%20empty%20results.%20Build%20query%20objects%20with%20the%20same%20chart-type%20resolution%20logic%20used%20elsewhere%20%28or%20derive%20full%20queries%20from%20normalized%20form%20data%29%20before%20passing%20%60queries
%60.%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.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_data.py%0A%2A%2ALine%3A%2A%2A%20912%3A926%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncomplete%20Implementation%3A%20The%20unsaved-chart%20path%20now%20builds%20a%20single%20minimal%20query%20from%20only%20%60groupby%60%20and%20%60metrics%6
0%2C%20then%20forces%20it%20into%20%60QueryContextFactory%60.%20This%20drops%20chart-type-specific%20query%20structure%20%28for%20example%20%60x_axis%60-driven%20timeseries%20columns%2C%20bubble%20%60x%2Fy%2Fsize%60%2C%20or%20mixed-timeseries%20secondary%20queries%29%2C%20so%20valid%20cached%20form-data%20can%20return%20incorrect%20or%20empty%20results.%20Build%20query%20objects%20with%20the%20same%20chart-type%20resolution%20logic%20used%20elsewhere%20%28or%20derive%20full%20queries%20from%20normalized%20form%20data%29%20before%20passing%20%60queries%60.%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.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%2
0fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%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_data.py
**Line:** 912:926
**Comment:**
*Incomplete Implementation: The unsaved-chart path now builds a single
minimal query from only `groupby` and `metrics`, then forces it into
`QueryContextFactory`. This drops chart-type-specific query structure (for
example `x_axis`-driven timeseries columns, bubble `x/y/size`, or
mixed-timeseries secondary queries), so valid cached form-data can return
incorrect or empty results. Build query objects with the same chart-type
resolution logic used elsewhere (or derive full queries from normalized form
data) before passing `queries`.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40099&comment_hash=398de746dfe17a09daefae96adc09d6708a5e19d4a9488c2c64a2139c9e09954&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40099&comment_hash=398de746dfe17a09daefae96adc09d6708a5e19d4a9488c2c64a2139c9e09954&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]
