fix(mcp): avoid duplicate SQL execution results

[aminghadersohi]

TL;DR

• Return SQL result rows once instead of repeating the final result in statement metadata.
• Preserve top-level results and earlier multi-statement results.
• Avoid repeating unchanged SQL; retain transformed SQL when it differs.

SUMMARY

execute_sql included the last data-bearing result both in top-level rows/columns and in statements[].data, increasing response size without adding information.

The top-level result stays unchanged. The corresponding statement's data is null; earlier data-bearing statements retain their nested rows and columns. This also handles trailing DML, empty results, and cached row lists. Per-statement counts, truncation flags, and timing remain available. executed_sql is null when it equals original_sql. Schema descriptions, the multi-statement warning, and user documentation explain where each result lives.

Scope: response conversion and schema documentation only. Query execution, access checks, and caching behavior are unchanged.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Not applicable; no UI changes.

Before a single SELECT 1 AS x: both rows and statements[0].data.rows contained [{"x": 1}], and both SQL fields contained the same string.

After: rows contains [{"x": 1}]; statements[0].data and unchanged executed_sql are null.

TESTING INSTRUCTIONS

PYTHONPATH="$PWD/superset-core/src:$PWD" pytest -q \
  tests/unit_tests/mcp_service/sql_lab/ \
  tests/unit_tests/mcp_service/test_response_serialization.py

118 tests passed. The eight added regression cases cover single SELECTs, empty/cached results, multiple identical SELECTs, trailing DML, and unchanged versus transformed SQL. Before the fix, seven regression cases failed and the transformed-SQL case passed. After the fix, all eight pass. Existing MCP client tests verify the serialized single- and multi-statement response shape.

All changed-file pre-commit hooks passed, including mypy; explicit pylint checks passed.

Manual verification: call execute_sql with SELECT 1 AS x, then with SELECT 1 AS x; SELECT 2 AS y. Confirm the final result is only at the top level, earlier results remain in statements[].data, and unchanged SQL is not repeated.

Eval evidence

Local deterministic regression cases: 1/8 before → 8/8 after; affected SQL Lab and serialization suite: 118/118 pass. End-to-end evaluations against a deployed server were not run.

Cost & latency delta

Local conversion + JSON serialization benchmark, 100 iterations per fixture (not an end-to-end latency measurement):

Fixture	JSON bytes before → after	p50 ms before → after	p95 ms before → after
1 row	456 → 372	0.207 → 0.198	0.350 → 0.287
1,000 rows	16,446 → 8,370	3.070 → 3.020	4.155 → 3.364

The 1,000-row payload is 49.1% smaller. Model tokens, billing, and time-to-first-token were not measured. No model or prompt changes.

ADDITIONAL INFORMATION

Risk: clients reading the final result from statements[].data must use the existing top-level rows/columns instead, and clients reading unchanged executed_sql should fall back to original_sql. Earlier results and transformed SQL remain available. Rollback is a revert; no migration or configuration changes.

Review guidance: focus on selecting the last data-bearing statement rather than the last executed statement, and retaining earlier results even when their rows equal the final result.

• 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

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	b7c7b90
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6ac113e14d23ca0009ca052a
😎 Deploy Preview	https://deploy-preview-44949--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.28%. Comparing base (549508b) to head (b7c7b90).
⚠️ Report is 9 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #44949   +/-   ##
=======================================
  Coverage   82.28%   82.28%           
=======================================
  Files        2996     2996           
  Lines      185109   185110    +1     
  Branches    42834    42834           
=======================================
+ Hits       152316   152317    +1     
  Misses      30031    30031           
  Partials     2762     2762           

Flag	Coverage Δ
hive	36.30% <50.00%> (-0.01%)	⬇️
mysql	55.32% <50.00%> (-0.01%)	⬇️
postgres	55.33% <50.00%> (-0.01%)	⬇️
presto	38.17% <50.00%> (-0.01%)	⬇️
python	86.23% <100.00%> (+<0.01%)	⬆️
sqlite	55.05% <50.00%> (-0.01%)	⬇️
unit	79.23% <100.00%> (+<0.01%)	⬆️

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.

[bito-code-review]

Code Review Agent Run #f5e179

Actionable Suggestions - 0

Additional Suggestions - 8

• tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py - 4

  • Duplicated promotion-policy assertion · Line 202-203
    The new assertions at lines 202-203 duplicate coverage already guaranteed by `_convert_to_response` (`execute_sql.py:374-378` nulls `executed_sql` when equal to `original_sql`; line 404 nulls the promoted statement's `data`) and by the existing `executed_sql`-focused tests. They only pin the current implementation, so any intentional schema evolution must rewrite them. Consider dropping them or asserting the documented contract once.
  • Triplicated nulling assertions · Line 779-781
    These assertions hard-code the promotion policy from `_convert_to_response` (`execute_sql.py:399-404`): the last data-bearing statement's `data` is nulled and its rows surface only at top level. They duplicate the schema contract in `StatementInfo.data` and will break on any intentional serialization change. Prefer asserting the documented contract (earlier results nested, top level holds the last result) once, not per test.
  • Redundant nulling assertion copy · Line 867-867
    Line 867 asserts `statements[1]['data'] is None`, duplicating the promotion policy pinned at lines 779-781 and 1006 against `_convert_to_response` (`execute_sql.py:399-404`). This test's unique value is the SET-then-SELECT flow (rows, `affected_rows` is None, no warning); the repeated null check adds a third copy that must be updated on any intentional change. Consider dropping it here.
  • Fourth copy of nulling assertion · Line 1006-1006
    Line 1006 repeats the same promotion-policy assertion as lines 779-781 and 867, hard-coding `_convert_to_response`'s nulling of the last data-bearing statement's `data` (`execute_sql.py:399-404`). The unique value here is `truncated` plus top-level columns; the duplicated null check is a third copy to maintain on schema evolution. Consider dropping it.
• tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql_response.py - 2

  • Missing local type annotation · Line 33-41
    Local `rows` is untyped while the sibling `response` at line 121 is explicitly annotated `dict[str, Any]`. BITO adaptive rule 13153 requires explicit annotations for all local variables in test files, even when inferable. Adding `list[dict[str, Any]]` keeps the file consistent with its own convention.
  • Guard duplicates fixture branch logic · Line 54-55
    The guard `if not cached or not empty` encodes the DataFrame-vs-list branch of the `data=` fixture expression at line 40 rather than the property under test. If `_data_to_statement_data` changes how columns are derived, this conditional must be re-derived by hand. Deriving the assertion from the same `rows`/`cached` expression (or asserting columns only when `rows` is non-empty) would keep the test self-consistent.
• superset/mcp_service/sql_lab/tool/execute_sql.py - 2

  • SQL dedup misses whitespace variants · Line 374-378
    The dedup guard `stmt.executed_sql != stmt.original_sql` compares raw strings, so whitespace-only differences (e.g. a trailing `;` or newline added by the executor) keep `executed_sql` populated even when the executed SQL is semantically identical to `original_sql` — the exact duplication the schema says to avoid. `StatementResult.executed_sql` is typed non-optional (`superset_core/queries/types.py`), so the None branch is unreachable; a stripped comparison would also cover that case defensively.
  • Mutation breaks cache contract · Line 404-404
    Setting `last_data_stmt.data = None` mutates the `StatementInfo` in `statements`, which is fine locally, but the `StatementData` object it held is the same object built from `stmt.data` in `result.statements` — the caller's `QueryResult` is silently stripped of the last statement's rows. The sibling test `test_single_select_returns_data_once` explicitly asserts `result.statements[0].data is not None` after conversion, so this violates the documented no-side-effect contract and will fail that test. Build the response without mutating shared inputs.

Review Details

• Files reviewed - 5 · Commit Range: b7c7b90..b7c7b90

  • docs/docs/using-superset/using-ai-with-superset.mdx
  • superset/mcp_service/sql_lab/schemas.py
  • superset/mcp_service/sql_lab/tool/execute_sql.py
  • tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py
  • tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql_response.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[rebenitez1802]

Approve: no blocking issues found. CI unit tests and pre-commit pass; tests were not run locally.

[gabotorresruiz]

Hi @aminghadersohi, LGTM, thanks for this one. I verified on this branch that SELECT 1 AS x; SELECT 2 AS y still runs each statement exactly once on both master and this head (the duplication was only in the payload, never in execution), that the promoted result is dropped from the new StatementInfo and not from the QueryResult, and that the published execute_sql schema only changes executed_sql to nullable plus the descriptions. Ran tests/unit_tests/mcp_service/sql_lab/ and test_response_serialization.py locally (114 passed); the new and updated tests fail on the merge base as expected (11 of 73). CI was green.

[gabotorresruiz]

Just a small NIT, not a blocker: after this a promoted statement and a no-data statement (SET, DML) both come back as data: null, so a client has to infer from original_sql which entry the top-level rows belong to. If that ever becomes a problem, a tiny data_at_top_level: bool on StatementInfo would make it explicit. Fine to leave as is.

[bito-code-review]

The suggestion to add a data_at_top_level: bool flag to StatementInfo is a reasonable way to make the distinction between statements explicit, especially since the current implementation relies on the client inferring the relationship from original_sql. Given that the current design already handles this by setting data to null for the last data-bearing statement and providing the results at the top level, adding this flag would improve clarity for clients. Since you mentioned it is fine to leave as is, no immediate action is required, but this remains a valid enhancement for future consideration.