feat(mcp): add query_dataset tool to query datasets using semantic layer - #39727
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39727 +/- ##
==========================================
- Coverage 64.41% 64.36% -0.06%
==========================================
Files 2567 2568 +1
Lines 134411 134607 +196
Branches 31203 31227 +24
==========================================
+ Hits 86584 86639 +55
- Misses 46330 46471 +141
Partials 1497 1497
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new MCP tool (query_dataset) to query Superset datasets through the existing semantic-layer-backed chart data pipeline, enabling ad-hoc metric/dimension queries without creating a chart.
Changes:
- Introduces
query_datasetMCP tool usingQueryContextFactory+ChartDataCommand - Adds request/response/filter schemas for dataset querying
- Registers the tool and documents intended workflow in MCP default instructions
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py | Adds unit tests covering success, validation, time range, filters, ordering, and UUID lookup |
| superset/mcp_service/dataset/tool/query_dataset.py | Implements the query_dataset tool (dataset resolution, validation, query dict creation, execution, response shaping) |
| superset/mcp_service/dataset/tool/init.py | Exports the new tool |
| superset/mcp_service/dataset/schemas.py | Adds schemas for request/response and filter operators |
| superset/mcp_service/app.py | Registers the tool in default instructions and imports it into the MCP app |
dosubot
Bot
added
api
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #79c4b0
Actionable Suggestions - 1
-
superset/mcp_service/dataset/tool/query_dataset.py - 1
- Broad Exception Handling · Line 447-447
Review Details
-
Files reviewed - 5 · Commit Range:
45c77f0..37ab147- superset/mcp_service/app.py
- superset/mcp_service/dataset/schemas.py
- superset/mcp_service/dataset/tool/__init__.py
- superset/mcp_service/dataset/tool/query_dataset.py
- tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.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
37ab147 to
78eb406
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Posting on Richard's behalf - this is his PR reviewer agent. Forward any pushback to him and he'll loop me back in.
Left two notes below - the first one is the main functional/security thing I would look at before merge; the second is smaller hardening and coverage. All line numbers verified against HEAD 78eb406.
Functional - worth checking before merge
-
superset/mcp_service/dataset/tool/query_dataset.py:182-207This validation block builds the dataset column/metric sets and returns close-match suggestions before the query context reaches
ChartDataCommand.validate()at line 307. That means a caller who can invoke the tool but should not inspect dataset metadata can still probe names through invalidcolumns,metrics, filters, ororder_byvalues.WDYT - would it be worth moving the datasource/query access check ahead of metadata-dependent validation, or gating this tool with the same data-model metadata check used by
get_dataset_infobefore returning suggestions?
Other suggestions
-
superset/mcp_service/dataset/tool/query_dataset.py:433-443These exception paths return the raw
CommandException,SupersetException, andSQLAlchemyErrortext to the MCP client. Some of those messages can include rendered SQL, physical table names, engine details, or other backend context, which feels especially risky for a tool that may be exposed through general assistant clients.Small suggestion: could we log the full exception server-side but return a shorter sanitized client message here? Happy to keep as-is if MCP errors are intentionally treated as developer-facing diagnostics.
-
tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py:495The permission-denied test currently starts at
ChartDataCommand.validate(), so it verifies the final query denial path but not the earlier metadata disclosure path above. A regression test for an invalid metric/column on an inaccessible dataset would make the intended boundary much clearer.Could we add one test that proves an access-denied dataset cannot return column/metric suggestions before validation runs?
Praise
-
superset/mcp_service/dataset/tool/query_dataset.py:298-308Nice direction reusing
QueryContextFactoryandChartDataCommandfor the actual execution path instead of building SQL directly. That keeps the new MCP surface aligned with the existing Superset query pipeline.
|
Thanks for the review @richardfogaca (and to the agent posting on their behalf)! Addressed all three points in ffcd4b1: Functional — metadata disclosure via suggestions Exception messages to client Test for metadata disclosure boundary |
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Posting on Richard's behalf - this is his PR reviewer agent. Forward any pushback to him and he'll loop me back in.
Thanks for the follow-up fixes. The metadata-suggestion path is much better now, but I spotted two small follow-ups from the second pass. All line numbers verified against HEAD 5d9ea45.
Functional - follow-up
-
superset/mcp_service/dataset/tool/query_dataset.py:174-189The new privacy gate blocks the response before the validation/suggestion path, but this
ctx.info()still runs first and includes the dataset name plus column/metric counts. FastMCP sendsctx.info()messages to the connected MCP client, so a direct caller without data-model metadata access can still learn that the dataset exists and how many columns/metrics it has before receivingDataModelMetadataRestricted.WDYT - could we move the
user_can_view_data_model_metadata()check before this dataset-specific log, or make the pre-gate log avoid the dataset name and schema counts? -
superset/mcp_service/dataset/tool/query_dataset.py:430-443The new
effective_filterslist includes the synthesizedTEMPORAL_RANGEfilter, but the normal non-empty response still returnsapplied_filters=request.filters. That means successful time-range queries under-report the actual filters that were applied; only the empty-data branch at line 372 returnseffective_filters.Small suggestion: could the success response use
effective_filterstoo, and maybe add an assertion coveringapplied_filtersfor a non-emptytime_rangequery?
Praise
-
tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py:751-791The new regression test is a good addition for the original suggestion-disclosure path. It directly exercises the typo case that would have returned close matches before the metadata gate.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Thanks for the second pass @richardfogaca. Both caught — fixed in 3242c4c: ctx.info() metadata disclosure applied_filters under-report in success path |
|
Posting on Richard's behalf - this is his PR reviewer agent. I did one more pass against HEAD |
Add a new MCP tool that allows querying a dataset directly using its semantic layer (saved metrics, calculated columns, dimensions) without requiring a saved chart. This addresses a customer request from Alpha Tester Medialab who needed ad-hoc dataset queries via MCP. The tool accepts dataset_id + metrics/columns/filters/time_range and returns tabular data by leveraging QueryContextFactory + ChartDataCommand internally, which ensures RLS and dataset permissions are enforced. - New schemas: QueryDatasetFilter, QueryDatasetRequest, QueryDatasetResponse - Pre-flight validation of column/metric names with close-match suggestions - Time range support via TEMPORAL_RANGE filter + granularity - 10 unit tests covering happy path, validation, time range, filters, UUID
- Validate filter ops against Superset's FilterOperator enum via Literal type - Fix operator descriptions to match actual values (== not =, NOT IN not NOT_IN) - Validate filter column names against dataset columns (prevents semantic layer bypass) - Validate order_by names against columns + metrics - Validate time_column exists on dataset when explicitly provided - Pass cache_timeout through to QueryContextFactory (custom_cache_timeout) - Add comment explaining hardcoded datasource_type="table" vs semantic views
- Add warning when time_column is not marked as datetime (is_dttm)
- Add test for permission denied (SupersetSecurityException)
- Add test for order_by with valid names (passes through correctly)
- Add test for order_by with invalid names (validation error)
- Add test for explicit time_column override
- Add test for non-dttm time_column warning
- Add test for invalid filter column name
- Fix filter operator in existing test ("=" -> "==" per FilterOperator enum)
Total: 17 tests (up from 10)
- Fix filter example in schema description: "=" -> "==" to match FilterOperator - Fix unique_count to exclude nulls (None was stringified as "None") - Cap column stats computation at 5000 rows to avoid O(rows*cols) overhead - Compute null_count + unique_count in single pass per column - Return DatasetError on unexpected exceptions instead of re-raising (consistent structured response for MCP clients)
- Add type parameters to generic types (list[Any], dict[str, Any]) - Use ErrorLevel.WARNING enum instead of bare string for SupersetError - Add from __future__ import annotations
- Add @requires_data_model_metadata_access decorator and runtime privacy check before metadata-dependent validation, preventing restricted users from probing column/metric names via close-match suggestions - Fix use_cache flag being silently ignored: force=not use_cache or force_refresh - Fix applied_filters under-reporting: include synthesized TEMPORAL_RANGE filter in response when time_range is provided - Add return type annotations to test fixtures and mcp_server parameters - Add test verifying privacy gate fires before schema suggestions are returned - Add docstrings to test helper functions
All existing tests were failing with DataModelMetadataRestricted because the new privacy gate calls user_can_view_data_model_metadata() which returns False in the test environment. Add it to the autouse mock_auth fixture so tests run as a user with metadata access by default. The new privacy-denied test explicitly overrides it to False.
…ests The string path 'superset.mcp_service.dataset.tool.query_dataset.user_can_view_data_model_metadata' resolves to the query_dataset *function* (shadowed by the __init__.py import) rather than the module on some Python versions, causing AttributeError at fixture setup. Use patch.object(query_dataset_module, ...) instead, since query_dataset_module is already imported correctly via importlib.import_module.
- Move user_can_view_data_model_metadata() check before ctx.info() that reveals dataset name and column/metric counts, preventing any metadata disclosure to restricted callers before the privacy gate fires - Fix applied_filters in the non-empty success response path to use effective_filters (includes the synthesized TEMPORAL_RANGE entry) — the empty-data branch was already correct but the success branch was missed - Add assertion to test_query_dataset_with_time_range verifying that applied_filters in the response includes the TEMPORAL_RANGE filter
…tence side channel Move user_can_view_data_model_metadata() check before the DAO lookup so metadata-restricted users always receive DataModelMetadataRestricted, never NotFound. Without this ordering, a restricted caller could probe dataset existence by distinguishing the two different error types. Add test_query_dataset_metadata_access_denied_nonexistent_dataset to assert the same error is returned regardless of whether the dataset exists.
c3d8cf3 to
b091437
Compare
|
Bito Automatic Review Skipped – PR Already Merged |
Summary
Add a new MCP tool
query_datasetthat allows querying a dataset directly using its semantic layer (saved metrics, calculated columns, dimensions) without requiring a saved chart.Problem
The MCP service currently has:
execute_sql— runs raw SQL (bypasses the semantic layer entirely)get_chart_data— returns data from a saved chart (must create/save a chart first)get_dataset_info— returns dataset metadata but not actual dataThere is no way for an MCP client to query a dataset's curated metrics and dimensions ad-hoc.
Solution
New
query_datasettool that acceptsdataset_id+metrics/columns/filters/time_rangeand returns tabular data by leveraging the existingQueryContextFactory+ChartDataCommandpipeline.Changes
superset/mcp_service/dataset/tool/query_dataset.pyQueryDatasetFilter,QueryDatasetRequest,QueryDatasetResponseindataset/schemas.pyapp.pywith instructions and workflow documentationKey design decisions
QueryContextFactory+ChartDataCommand— inherits RLS enforcement, dataset permissions, and caching for freeFilterOperatorenum viaLiteraltypeTEMPORAL_RANGEfilter +granularityon the query dict (not adhoc_filters, which are a form_data concept)SqlMetric.metric_nameon the datasetScope: Superset datasets only
This tool queries Superset's built-in semantic layer (SqlaTable datasets with saved metrics and calculated columns). It does not yet support the upcoming external semantic layer integration (SemanticView /
Explorableprotocol from PRs #37815–#38611). When external semantic views land, MCP support for them would be a natural follow-up — theExplorableprotocol thatSemanticViewimplements is the same interface the chart data pipeline already uses, so the extension path is straightforward.Test plan