Skip to content

fix: Chatbot encoding and bytes errors#38634

Closed
kgabryje wants to merge 1 commit intoapache:masterfrom
kgabryje:fix/mcp_chatbot_encoding_bytes
Closed

fix: Chatbot encoding and bytes errors#38634
kgabryje wants to merge 1 commit intoapache:masterfrom
kgabryje:fix/mcp_chatbot_encoding_bytes

Conversation

@kgabryje
Copy link
Copy Markdown
Member

SUMMARY

Fixes two issues in the MCP chatbot/SQL execution flow:

  1. Cached query results failing with bytes/encoding errors — The result cache was storing raw pandas DataFrames (which get pickled by some cache backends into bytes). On retrieval, the code assumed thedata was always a DataFrame, causing errors when encountering bytes. This PR serializes DataFrames to a JSON-safe dict format ({"records": [...], "columns": [...]}) before caching, and adds a _deserialize_cached_data() method that gracefully handles all formats (dict, DataFrame, None, and bytes as a cache miss) — intentionally avoiding pickle deserialization to prevent code-execution attacks from a compromised cache backend.
  2. MCP middleware ordering was reversed — FastMCP 2.14+ changed how middleware is chained (uses reversed(middleware)), so the first-added middleware is now outermost. The middleware list ordering in server.py was updated to reflect this: GlobalErrorHandler → Logging → SizeGuard → Caching (outermost → innermost).

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

  1. Start Superset with MCP enabled
  2. Run a SQL query via the MCP chatbot that returns results
  3. Run the same query again — it should hit the cache and return correctly (no bytes/encoding errors)
  4. Verify DML queries (INSERT/UPDATE) that return no data also work from cache
  5. Verify middleware logging shows correct nesting order (error handler outermost)

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

@bito-code-review
Copy link
Copy Markdown
Contributor

bito-code-review Bot commented Mar 13, 2026

Code Review Agent Run #b10c04

Actionable Suggestions - 0
Review Details
  • Files reviewed - 2 · Commit Range: 521afb0..521afb0
    • superset/mcp_service/server.py
    • superset/sql/execution/executor.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

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

  • /review - 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

AI Code Review powered by Bito Logo

@dosubot dosubot Bot added the change:backend Requires changing the backend label Mar 13, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 13, 2026

Codecov Report

❌ Patch coverage is 0% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.68%. Comparing base (1867336) to head (521afb0).
⚠️ Report is 23 commits behind head on master.

Files with missing lines Patch % Lines
superset/sql/execution/executor.py 0.00% 11 Missing ⚠️
superset/mcp_service/server.py 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #38634      +/-   ##
==========================================
- Coverage   65.01%   63.68%   -1.34%     
==========================================
  Files        1817     2529     +712     
  Lines       72348   129281   +56933     
  Branches    23044    29790    +6746     
==========================================
+ Hits        47038    82334   +35296     
- Misses      25310    45484   +20174     
- Partials        0     1463    +1463     
Flag Coverage Δ
hive 40.60% <0.00%> (?)
mysql 61.63% <0.00%> (?)
postgres 61.70% <0.00%> (?)
presto 40.62% <0.00%> (?)
python 61.97% <0.00%> (?)
sqlite 61.33% <0.00%> (?)

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

☔ View full report in Codecov by Sentry.
📢 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.

@kgabryje kgabryje closed this Mar 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:backend Requires changing the backend size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants