Skip to content

fix(mcp): return all chart query results - #42824

Draft
aminghadersohi wants to merge 1 commit into
apache:masterfrom
aminghadersohi:aminghadersohi/return-all-chart-query-results
Draft

fix(mcp): return all chart query results#42824
aminghadersohi wants to merge 1 commit into
apache:masterfrom
aminghadersohi:aminghadersohi/return-all-chart-query-results

Conversation

@aminghadersohi

Copy link
Copy Markdown
Contributor

SUMMARY

get_chart_data previously selected only the first entry returned by ChartDataCommand, which dropped secondary results from charts whose query context contains multiple queries.

This change preserves the existing top-level columns, data, and row metadata as the first-query view for backward compatibility, and adds an optional query_results field containing every query result for multi-query charts. Single-query charts retain their existing behavior. Response-size enforcement applies one shared row cap across the complete multi-query response so additional results cannot bypass the configured token limit.

Mixed Timeseries is the chart type that explicitly builds two queries in the MCP chart query helper. The response handling also covers any saved query context containing multiple queries rather than coupling serialization to that visualization type.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Not applicable; this changes an MCP tool response without UI changes.

TESTING INSTRUCTIONS

  • pytest -q tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py tests/unit_tests/mcp_service/test_middleware.py (182 passed)
  • pytest -q tests/unit_tests/mcp_service (3319 passed, 1 unrelated health-check failure because the local test configuration returned a null application version)
  • uvx pre-commit run --files superset/mcp_service/chart/schemas.py superset/mcp_service/chart/tool/get_chart_data.py superset/mcp_service/utils/token_utils.py tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py tests/unit_tests/mcp_service/test_middleware.py
  • pre-commit run --all-files was also run. Python checks passed; the repository-wide frontend hooks could not complete because frontend dependencies such as glob and postcss-styled-syntax are not installed, and repository-wide Ruff reported pre-existing violations outside this change.

The regression coverage executes the unsaved Mixed Timeseries response path with two command results and asserts that both are returned. Additional coverage verifies the additive single-query behavior and shared response-size limiting.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

BACKWARD COMPATIBILITY

This is additive. Existing top-level fields continue to represent the first query, while multi-query responses additionally expose query_results. Clients that ignore unknown fields are unaffected.

EVAL EVIDENCE

Not applicable to this deterministic serialization change; no prompt, model routing, or model-generated output changed. The MCP regression suite results are listed above.

COST & LATENCY DELTA

No model or parameter changes. Multi-query responses include more data by design, bounded by the existing response token limit using response-wide truncation. No external model-cost or latency benchmark is available in the OSS worktree.

PROMPT / NON-DETERMINISM

No prompt changes and no non-deterministic behavior.

REVIEW GUIDANCE

Review the additive schema and response construction first, then the shared-budget truncation logic. Concurrent work touching get_chart_data may need to rebase carefully around the response construction paths.

@codecov

codecov Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 15.68627% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.71%. Comparing base (1de35d1) to head (4087586).
⚠️ Report is 18 commits behind head on master.

Files with missing lines Patch % Lines
superset/mcp_service/utils/token_utils.py 0.00% 38 Missing ⚠️
superset/mcp_service/chart/tool/get_chart_data.py 16.66% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #42824      +/-   ##
==========================================
- Coverage   65.73%   65.71%   -0.02%     
==========================================
  Files        2843     2843              
  Lines      162672   162727      +55     
  Branches    37255    37267      +12     
==========================================
+ Hits       106935   106941       +6     
- Misses      53645    53693      +48     
- Partials     2092     2093       +1     
Flag Coverage Δ
hive 37.94% <15.68%> (-0.02%) ⬇️
mysql 57.76% <15.68%> (-0.03%) ⬇️
postgres 57.80% <15.68%> (-0.04%) ⬇️
presto 39.85% <15.68%> (-0.02%) ⬇️
python 59.17% <15.68%> (-0.04%) ⬇️
sqlite 57.44% <15.68%> (-0.03%) ⬇️
unit 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants