fix(mcp): database filter columns, timeseries SQL, and unsaved chart datasource name by aminghadersohi · Pull Request #39636 · apache/superset

aminghadersohi · 2026-04-24T19:42:14Z

SUMMARY

Fixes three MCP tool bugs:

list_databases filter columns: DatabaseFilter schema now accepts created_by_fk and changed_by_fk as valid filter columns. Previously only database_name, expose_in_sqllab, and allow_file_upload were allowed, causing validation errors when filtering by creator.
get_chart_sql timeseries: _build_query_context_from_form_data now passes time_range, adhoc_filters, and filters from form_data into the query dict sent to QueryObjectFactory.create(). Previously the query dict only contained columns and metrics, so timeseries charts produced SQL with no temporal column or time range filtering.
get_chart_sql unsaved chart datasource_name: Added _resolve_datasource_name() helper that resolves datasource name via DatasourceDAO.get_datasource() for unsaved charts (form_data_key only, no chart object). Previously returned null because getattr(None, "datasource_name", None) was called when chart=None.

BEFORE/AFTER SCREENSHOTS OR ANIMATIONS

N/A — backend-only changes to MCP tool schemas and query building logic.

TESTING INSTRUCTIONS

Call list_databases with filters: [{"col": "created_by_fk", "opr": "eq", "value": 1}] — should succeed instead of validation error.
Call get_chart_sql for a timeseries chart — SQL should include temporal column and time range WHERE clause.
Call get_chart_sql with only form_data_key (unsaved chart) — datasource_name should be populated, not null.

ADDITIONAL INFORMATION

Added schema validation tests for created_by_fk and changed_by_fk in DatabaseFilter
Updated existing test_temporal_fields_passed_to_factory to verify time_range, adhoc_filters, and filters are in the queries dict
Added TestResolveDatasourceName test class covering: chart exists, unsaved chart DAO lookup, missing datasource_id, datasource not found, DAO error, combined datasource field

…datasource name - Add created_by_fk and changed_by_fk to DatabaseFilter schema so list_databases supports filtering by creator/modifier - Pass time_range, adhoc_filters, and filters from form_data into the query dict in get_chart_sql so timeseries charts produce temporal SQL - Resolve datasource_name from form_data via DAO lookup for unsaved charts in get_chart_sql instead of returning null

bito-code-review · 2026-04-24T19:42:25Z

Code Review Agent Run #6fca56

Actionable Suggestions - 0

Review Details

Files reviewed - 4 · Commit Range: a3e172f..a3e172f
- superset/mcp_service/chart/tool/get_chart_sql.py
- superset/mcp_service/database/schemas.py
- tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py
- tests/unit_tests/mcp_service/database/tool/test_database_tools.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-code-review · 2026-04-24T19:49:54Z

Yes, the flagged logic error is correct — adhoc_filters are added to the query payload but ignored by QueryObjectFactory.create() and QueryObject, as they don't consume the adhoc_filters kwarg. To resolve, convert adhoc_filters into query filters format using process_filters and merge with existing filters. Here's the concise fix: import process_filters, process adhoc_filters into filters, and merge before adding to query_dict.

superset/mcp_service/chart/tool/get_chart_sql.py

from superset.migrations.shared.migrate_viz.query_functions import process_filters

# ... existing code ...

# Pass simple filters
filters = form_data.get("filters", [])
if adhoc_filters := form_data.get("adhoc_filters"):
    # Convert adhoc_filters to query filters format
    processed_adhoc_filters = process_filters({"adhoc_filters": adhoc_filters})
    if processed_adhoc_filters:
        filters.extend(processed_adhoc_filters)
if filters:
    query_dict["filters"] = filters

# Remove the previous adhoc_filters addition

adhoc_filters are processed by merge_extra_filters() on form_data during query context creation. Passing them in query_dict is dead code since QueryObject.__init__ silently absorbs them via **kwargs.

codecov · 2026-04-24T19:59:02Z

Codecov Report

❌ Patch coverage is 5.00000% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.45%. Comparing base (78950fc) to head (344faee).
⚠️ Report is 37 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/mcp_service/chart/tool/get_chart_sql.py	5.00%	38 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #39636      +/-   ##
==========================================
- Coverage   64.49%   64.45%   -0.04%     
==========================================
  Files        2565     2566       +1     
  Lines      133857   133952      +95     
  Branches    31084    31097      +13     
==========================================
+ Hits        86326    86340      +14     
- Misses      46036    46117      +81     
  Partials     1495     1495

Flag	Coverage Δ
hive	`39.74% <5.00%> (-0.05%)`	⬇️
mysql	`60.18% <5.00%> (-0.08%)`	⬇️
postgres	`60.26% <5.00%> (-0.08%)`	⬇️
presto	`41.51% <5.00%> (-0.05%)`	⬇️
python	`61.82% <5.00%> (-0.08%)`	⬇️
sqlite	`59.89% <5.00%> (-0.08%)`	⬇️
unit	`100.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.

The test asserted that created_by_fk should be rejected, but it is now a valid filter column. Changed the test to verify that user-directory string fields (created_by_name) are still rejected.

bito-code-review · 2026-04-24T21:02:59Z

Code Review Agent Run #1fcc96

Actionable Suggestions - 0

Review Details

Files reviewed - 3 · Commit Range: a3e172f..c0220fc
- superset/mcp_service/chart/tool/get_chart_sql.py
- tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py
- tests/unit_tests/mcp_service/database/tool/test_database_tools.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

rebenitez1802

From my reviewer agent:

Thanks for the fix — three independent bugs squashed in one tight PR. The schema fix and time_range plumbing are clean. Two concerns on _resolve_datasource_name and one on adhoc_filters.

1. `_resolve_datasource_name` swallows the wrong exceptions (real bug)

DatasourceDAO.get_datasource() does not return None on miss — it raises:

DatasourceNotFound when the row isn't in the DB
DatasourceTypeNotSupportedError when the type isn't in cls.sources
DatasourceValueIsIncorrect when database_id_or_uuid isn't a digit/uuid

All three subclass SupersetException, not ValueError/KeyError, so except (ValueError, KeyError) won't catch them. A stale/missing datasource_id in cached form_data will bubble up an exception out of get_chart_sql instead of degrading gracefully to datasource_name=None.

The unit test test_returns_none_when_datasource_not_found patches the DAO to return None, which doesn't reflect real DAO behavior — the test passes but the code path it claims to cover doesn't exist in production.

Suggested fix:

from superset.daos.exceptions import (
    DatasourceNotFound,
    DatasourceTypeNotSupportedError,
    DatasourceValueIsIncorrect,
)

try:
    datasource = DatasourceDAO.get_datasource(...)
    return getattr(datasource, "name", None)
except (
    ValueError,
    DatasourceNotFound,
    DatasourceTypeNotSupportedError,
    DatasourceValueIsIncorrect,
):
    return None

And update the test to actually raise DatasourceNotFound rather than returning None.

2. `adhoc_filters` claim is incorrect

The new comment says:

adhoc_filters live in form_data and are processed by merge_extra_filters() during query context creation

That's not what happens on this code path. merge_extra_filters is called from view-layer code (views/core.py, commands/explore/get.py, viz.py, jinja_context.py) — not from ChartDataCommand → QueryContextFactory → QueryObjectFactory. Nothing in superset/common/ reads adhoc_filters off form_data.

Practical impact: a user calling get_chart_sql on a chart whose WHERE clause is expressed as adhoc_filters (the common case for explore-saved charts) will get SQL without that filter — same class of bug the PR is fixing for time_range. The bito-code-review bot called this out as well.

The standard approach is split_adhoc_filters_into_base_filters or merging adhoc filters into the filters list on the query dict before handing it to the factory. Either way, the test test_temporal_fields_passed_to_factory asserting "adhoc_filters" not in queries[0] is locking in the bug.

3. Minor: duplicated comment

The two-line note "QueryObjectFactory.create() accepts time_range as a top-level kwarg / and converts it to from_dttm/to_dttm for the QueryObject" appears verbatim above the query_dict definition AND immediately above if time_range := …. Drop one — the second placement (right above the assignment) is clearer.

What's solid

DatabaseFilter schema fix + tests — straightforward and the Literal expansion is the right call.
time_range plumbing into the query dict — verified QueryObjectFactory.create() accepts it as a kwarg and converts to from_dttm/to_dttm.
The combined-datasource parsing ("123__table") mirrors the existing pattern at line 149 — good consistency.
DatasourceType("table") default is safe since "table" is a valid enum value.

…filters - Catch DatasourceNotFound/DatasourceTypeNotSupportedError/ DatasourceValueIsIncorrect in _resolve_datasource_name instead of ValueError/KeyError (DAO raises these, never returns None) - Preprocess adhoc_filters via split_adhoc_filters_into_base_filters before building the query dict so WHERE clauses from explore-saved charts appear in rendered SQL - Extract _resolve_engine helper to reduce complexity of _build_query_context_from_form_data - Remove duplicate comment - Update tests to raise real DAO exceptions instead of returning None

netlify · 2026-04-27T16:36:50Z

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	`344faee`
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/69ef8fa6ee032300081b6a8a
😎 Deploy Preview	https://deploy-preview-39636--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

rebenitez1802

LGTM

bito-code-review · 2026-04-27T17:37:12Z

Code Review Agent Run #10c609

Actionable Suggestions - 0

Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

superset/mcp_service/chart/tool/get_chart_sql.py - 1
- Broad Exception Handling · Line 144-144

Review Details

Files reviewed - 2 · Commit Range: c0220fc..344faee
- superset/mcp_service/chart/tool/get_chart_sql.py
- tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.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

* fix(docs): fix 404s in documentation (apache#38974) Co-authored-by: Evan Rusackas <evan@preset.io> * chore(deps): bump baseline-browser-mapping from 2.10.21 to 2.10.23 in /docs (apache#39671) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump d3-cloud from 1.2.8 to 1.2.9 in /superset-frontend (apache#39677) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(ci): update Node.js version used in building CI image (apache#38635) * fix(explore): ensure unsaved-changes dialog renders above View SQL modal (apache#39569) Co-authored-by: yousoph <sophieyou12@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(Modal): prevent title overlapping with close button in long header titles (apache#36536) * fix(mcp): database filter columns, timeseries SQL, and unsaved chart datasource name (apache#39636) * docs(rls): adding additional rls filter documentation (apache#38829) Co-authored-by: codeant-ai-for-open-source[bot] <244253245+codeant-ai-for-open-source[bot]@users.noreply.github.com> * fix(dev): revert `react-checkbox-tree` from 2.1.0 to 1.8.0 in /superset-frontend (apache#39660) Signed-off-by: hainenber <dotronghai96@gmail.com> Co-authored-by: Evan Rusackas <evan@rusackas.com> * chore(deps-dev): bump typescript-eslint from 8.59.0 to 8.59.1 in /superset-websocket (apache#39687) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * fix(plugin-chart-handlebars): preserve template on explore open (apache#39442) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(mcp): surface structured errors for generate_chart validation failures (apache#39484) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(table chart): fix rerender bug that continuously cleared search box (apache#39707) * fix(query-history): enable sorting by Duration column (apache#39637) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(security): add generic validation for guest token API (closes #8) Co-Authored-By: Chiranjeevi Balawat <cbalawat@gmail.com> --------- Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: hainenber <dotronghai96@gmail.com> Co-authored-by: David <39565245+dmunozv04@users.noreply.github.com> Co-authored-by: Evan Rusackas <evan@preset.io> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Đỗ Trọng Hải <41283691+hainenber@users.noreply.github.com> Co-authored-by: yousoph <sophieyou12@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: innovark <eric.graham@mailfence.com> Co-authored-by: Amin Ghadersohi <amin.ghadersohi@gmail.com> Co-authored-by: SkinnyPigeon <e.blackledge@stuart.com> Co-authored-by: codeant-ai-for-open-source[bot] <244253245+codeant-ai-for-open-source[bot]@users.noreply.github.com> Co-authored-by: Evan Rusackas <evan@rusackas.com> Co-authored-by: Mehmet Salih Yavuz <salih.yavuz@proton.me> Co-authored-by: Sam Firke <sfirke@users.noreply.github.com> Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Chiranjeevi Balawat <cbalawat@gmail.com>

…mplexity - _extract_x_axis_col: also handle adhoc-SQL x_axis dicts (expressionType: "SQL") by returning the full dict so the SQL expression is preserved in the query context; column_name dicts (expressionType: "SIMPLE") still return the bare column name string - extract _build_single_query_dict and _build_mixed_timeseries_secondary as module-level helpers to reduce McCabe complexity of _build_query_context_from_form_data below the C901 limit - add tests for SQL-expression x_axis: _extract_x_axis_col unit tests and an integration test through _build_query_context_from_form_data - fix test docstring: regressions introduced after apache#38700, not apache#39636

pull-request-size Bot added the size/L label Apr 24, 2026

dosubot Bot added the change:backend Requires changing the backend label Apr 24, 2026

codeant-ai-for-open-source Bot reviewed Apr 24, 2026

View reviewed changes

Comment thread superset/mcp_service/chart/tool/get_chart_sql.py Outdated

fix(mcp): remove adhoc_filters from query_dict — processed via form_data

8bdd116

adhoc_filters are processed by merge_extra_filters() on form_data during query context creation. Passing them in query_dict is dead code since QueryObject.__init__ silently absorbs them via **kwargs.

fix(mcp): update test to expect created_by_fk as valid filter column

c0220fc

The test asserted that created_by_fk should be rejected, but it is now a valid filter column. Changed the test to verify that user-directory string fields (created_by_name) are still rejected.

aminghadersohi requested a review from richardfogaca April 24, 2026 20:59

aminghadersohi requested a review from Antonio-RiveroMartnez April 27, 2026 13:05

aminghadersohi assigned kgabryje Apr 27, 2026

aminghadersohi requested a review from rebenitez1802 April 27, 2026 13:05

rebenitez1802 reviewed Apr 27, 2026

View reviewed changes

aminghadersohi requested a review from rebenitez1802 April 27, 2026 16:57

rebenitez1802 approved these changes Apr 27, 2026

View reviewed changes

aminghadersohi merged commit 7774ec7 into apache:master Apr 27, 2026
65 checks passed

Copilot AI mentioned this pull request May 4, 2026

fix(mcp): get_chart_sql drops x_axis on echarts_timeseries_* and only renders one query for mixed_timeseries #39865

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mcp): database filter columns, timeseries SQL, and unsaved chart datasource name#39636

fix(mcp): database filter columns, timeseries SQL, and unsaved chart datasource name#39636
aminghadersohi merged 4 commits intoapache:masterfrom
aminghadersohi:aminghadersohi/fix-mcp-database-filter-timeseries-sql-datasource-name

aminghadersohi commented Apr 24, 2026

Uh oh!

bito-code-review Bot commented Apr 24, 2026 •

edited

Loading

Code Review Agent Run #6fca56

Uh oh!

Uh oh!

bito-code-review Bot commented Apr 24, 2026

Uh oh!

codecov Bot commented Apr 24, 2026 •

edited

Loading

Uh oh!

bito-code-review Bot commented Apr 24, 2026 •

edited

Loading

Code Review Agent Run #1fcc96

Uh oh!

rebenitez1802 left a comment •

edited

Loading

Uh oh!

netlify Bot commented Apr 27, 2026

Uh oh!

rebenitez1802 left a comment

Uh oh!

bito-code-review Bot commented Apr 27, 2026 •

edited

Loading

Code Review Agent Run #10c609

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

aminghadersohi commented Apr 24, 2026

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATIONS

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

bito-code-review Bot commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #6fca56

Uh oh!

Uh oh!

bito-code-review Bot commented Apr 24, 2026

Uh oh!

codecov Bot commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bito-code-review Bot commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #1fcc96

Uh oh!

rebenitez1802 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

1. _resolve_datasource_name swallows the wrong exceptions (real bug)

2. adhoc_filters claim is incorrect

3. Minor: duplicated comment

What's solid

Uh oh!

netlify Bot commented Apr 27, 2026

✅ Deploy Preview for superset-docs-preview ready!

Uh oh!

rebenitez1802 left a comment

Choose a reason for hiding this comment

Uh oh!

bito-code-review Bot commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #10c609

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bito-code-review Bot commented Apr 24, 2026 •

edited

Loading

codecov Bot commented Apr 24, 2026 •

edited

Loading

bito-code-review Bot commented Apr 24, 2026 •

edited

Loading

rebenitez1802 left a comment •

edited

Loading

1. `_resolve_datasource_name` swallows the wrong exceptions (real bug)

2. `adhoc_filters` claim is incorrect

bito-code-review Bot commented Apr 27, 2026 •

edited

Loading