EnxDev commented on code in PR #45250:
URL: https://github.com/apache/superset/pull/45250#discussion_r4264148791
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -791,11 +798,12 @@ async def _get_chart_data( # noqa: C901
if query_context_json is None and not using_unsaved_state:
Review Comment:
The `form_data_key` branch above goes through
`build_query_context_from_form_data`, which calls the same
`build_query_dicts_from_form_data` as this fallback (`chart_helpers.py:1296`).
So unsaved state skips buildQuery too, and `not using_unsaved_state` keeps the
caveat off it.
Is that deliberate? Might be missing a reason the unsaved path is trusted
more, but a big_number with `compare_lag` looks like it hits the same gap there.
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -791,11 +798,12 @@ async def _get_chart_data( # noqa: C901
if query_context_json is None and not using_unsaved_state:
# Fallback: Chart has no saved query_context
# This can happen with older charts that haven't been re-saved
- await ctx.warning(
+ query_context_accuracy_warning = (
"Chart has no saved query_context. "
"Data may not match the chart visualization exactly. "
"Consider re-saving the chart to enable full data
retrieval."
Review Comment:
This line was written for a person reading logs. In the tool result it's
read by a model whose only save tool is `update_chart`, and that writes
`query_context=None` (`update_chart.py:357`), while `generate_chart` never
stores one.
So every MCP-made chart carries the caveat, and the obvious reaction to
"re-saving" changes nothing. Could the text say the save has to happen in
Explore?
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -1049,6 +1057,8 @@ async def _get_chart_data( # noqa: C901
# Generate insights and recommendations
insights = []
+ if query_context_accuracy_warning:
Review Comment:
Nit, take it or leave it.
`ChartData.insights` is described to the client as "Key patterns discovered
in the data" (`schemas.py:6711`), and this lands next to lines like "Fresh data
retrieved from database". Widening that description to mention accuracy caveats
would help a model weigh it as a warning.
--
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]
