feat(mcp): add create_dataset tool to register physical tables as datasets - #40340
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #40340 +/- ##
==========================================
- Coverage 64.14% 64.13% -0.02%
==========================================
Files 2653 2654 +1
Lines 143494 143566 +72
Branches 33112 33122 +10
==========================================
+ Hits 92048 92073 +25
- Misses 49837 49884 +47
Partials 1609 1609
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a7720dd to
caa3d97
Compare
caa3d97 to
e220068
Compare
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Code Review Agent Run #ce1551Actionable Suggestions - 0Additional Suggestions - 5
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #25bcceActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Thanks for the bito suggestions. Addressed in the latest commit (4faa3ae):
The other notes (mock duplication with test_dataset_tools.py, function return-count) are style observations that do not affect correctness. |
Code Review Agent Run #ce0313Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Thanks for the additional suggestion. Fixed in commit f58a31c: |
f58a31c to
35c8c7c
Compare
Code Review Agent Run #069823Actionable Suggestions - 0Additional Suggestions - 3
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
b2fb339 to
f7aea7b
Compare
![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 #8742b3
Actionable Suggestions - 1
-
superset/mcp_service/dataset/tool/create_dataset.py - 1
- Missing authorization decorator · Line 53-53
Additional Suggestions - 4
-
superset/mcp_service/app.py - 3
-
Critical operational guardrails removed · Line 319-363The diff removes a 'CRITICAL RULES' section containing important operational guardrails: (1) never fabricate URLs, (2) use correct dashboard tools, (3) use update_chart for modifications, (4) use exact parameter names. Removing operational guardrails from LLM instructions increases risk of incorrect behavior.
-
Incomplete capability description · Line 143-143The simplified description 'Get a visual preview of a chart with image URL' omits that previews can also return ASCII text, Explore URLs, table data, or Vega-Lite specs. Users may not discover these capabilities without the full description.
-
Tool grouping ambiguity · Line 133-133The `list_databases` and `get_database_info` entries appear at the same indentation level as 'Dataset Management:' header, creating visual ambiguity about section boundaries. The original structure had a clear 'Database Connections' section.
-
-
superset/mcp_service/dataset/tool/create_dataset.py - 1
-
Misleading comment text · Line 84-86The comment at line 84 says 'treat blank schema as None' but this is default Python behavior when a falsy value is used in a conditional. The code correctly handles schema=None but the comment appears misleading.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
tests/unit_tests/mcp_service/dataset/tool/test_create_dataset.py - 3
- Missing return type hint · Line 34-39
- Missing return type hint · Line 76-76
- Missing return type hint · Line 81-81
-
superset/mcp_service/app.py - 4
- Resource misclassified as Prompt · Line 170-170
- Resource misclassified as Prompt · Line 171-171
- Resource URI scheme mismatch · Line 165-165
- Resource URI non-existent · Line 166-166
Review Details
-
Files reviewed - 5 · Commit Range:
3125cb0..f7aea7b- superset/mcp_service/app.py
- superset/mcp_service/dataset/schemas.py
- superset/mcp_service/dataset/tool/__init__.py
- superset/mcp_service/dataset/tool/create_dataset.py
- tests/unit_tests/mcp_service/dataset/tool/test_create_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
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 a few notes below - the main themes are startup/registration safety, physical-table access checks, and keeping the new typed error contract reachable. All line numbers verified against HEAD f7aea7b927.
Functional - worth addressing before merge
-
superset/mcp_service/app.py:540This removes the
initialize_core_mcp_dependencies()call before the file imports all tool modules. Those modules use@toolfromsuperset_core.mcp.decorators, whose default implementation raisesNotImplementedErroruntil injection runs, so direct/standalone imports ofsuperset.mcp_service.appcan fail before tools register.WDYT - could we keep the initialization before the tool imports, or otherwise prove every
app.pyentrypoint initializes the core MCP decorators first? -
superset/mcp_service/dataset/tool/create_dataset.py:112The physical-table path goes straight into
CreateDatasetCommand, but that command only callssecurity_manager.raise_for_access(...)for thesql/virtual-dataset path. For physical tables, a user withcan_writeonDatasetbut no database/catalog/schema/table access can still ask MCP to register and fetch metadata for an arbitrary table.Could we add an explicit
raise_for_access(database=database, table=Table(...))pre-check for this tool and cover the access-denied case? -
superset/mcp_service/dataset/tool/create_dataset.py:126These
DatasetExistsValidationError/TableNotFoundValidationErrorbranches look unreachable with the real command.CreateDatasetCommand.validate()collects those validation errors and raises a singleDatasetInvalidError(exceptions=...), so duplicate and missing-table cases will currently return genericValidationErrorinstead of the advertisedDatasetExistsError/TableNotFoundError.WDYT - should this inspect
exc.get_list_classnames()inside theDatasetInvalidErrorbranch instead of catching the nested validation classes directly? -
superset/mcp_service/dataset/schemas.py:336CreateDatasetRequestacceptsdatabase_id,schema,table_name, andowners, but notcatalog. That means MCP cannot register physical tables outside the default catalog even though the underlying command supportscatalogand the PR summary says the tool accepts it.Could we add
cataloghere and pass it through with the same blank-string normalization asschema?
|
Thanks @richardfogaca for the thorough review! Here is what was addressed in commit
|
Code Review Agent Run #3856eeActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Thanks for the detailed review, Richard! Addressing each point: 1.
2. Added an explicit pre-check before database = DatabaseDAO.find_by_id(request.database_id)
if database is None:
return DatasetError.create(error=..., error_type="DatabaseNotFoundError")
table_obj = Table(table_name, schema, catalog)
try:
security_manager.raise_for_access(database=database, table=table_obj)
except SupersetSecurityException as exc:
return DatasetError.create(error=str(exc), error_type="AccessDeniedError")This mirrors the check 3. The two direct 4.
|
Code Review Agent Run #2d7129Actionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
b44e9b9 to
691fd6b
Compare
Code Review Agent Run #cab258Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
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 one validation note below; otherwise the tool path looks consistent with the existing dataset command access checks.
Functional — small validation gap
-
superset/mcp_service/dataset/schemas.py:373_normalize_optional_str()returnsNonefor any non-stringschemaorcataloginput. Sincecreate_dataset()only forwards those fields when they are notNone, a malformed request like{"schema": 123}or{"catalog": {"name": "hive"}}is silently treated as if the namespace was omitted, which can register the table against the default schema/catalog instead of failing request validation.WDYT — could this return the original non-string value so Pydantic rejects it, or explicitly raise a validation error while still normalizing blank strings to
None?
Code Review Agent Run #d360cdActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…asets Adds create_dataset MCP tool that wraps POST /api/v1/dataset/ so skills and agents can register an existing physical table as a Superset dataset without manual UI interaction. Returns DatasetInfo (same shape as get_dataset_info) so the resulting dataset_id feeds directly into generate_chart. - CreateDatasetRequest schema (database_id, schema, table_name, owners?) - Tool file with typed error handling (exists/not-found/validation/internal) - Registered in dataset/tool/__init__.py and app.py - DEFAULT_INSTRUCTIONS updated to list create_dataset - Unit tests covering success, owners, error cases, and full DatasetInfo shape
- schemas.py: restore full apache/master version and add CreateDatasetRequest (previous cherry-pick used an older shorter version missing helper functions _sanitize_dataset_info_for_llm_context, _humanize_timestamp, etc.) - create_dataset.py: remove parse_request decorator (not in apache/master yet)
…and is a lazy import CreateDatasetCommand is imported inside the function body, so patching at superset.mcp_service.dataset.tool.create_dataset.CreateDatasetCommand fails with AttributeError. Patch at the source module instead. Also fix data["schema_name"] assertions: DatasetInfo.model_serializer renames the field to "schema" in the serialized output.
…ataset Restores tool imports that were accidentally dropped from app.py: create_virtual_dataset, query_dataset, get_chart_sql, get_chart_type_schema, get_database_info, list_databases, save_sql_query. Exports create_virtual_dataset and query_dataset from dataset/tool/__init__.py. Fixes KeyError in test_create_dataset by setting is_favorite=None on the mock dataset to avoid Pydantic bool|None validation errors from MagicMock auto-attributes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…decorator, normalize whitespace - CreateDatasetRequest.schema is now str | None (default None) so databases without schema namespaces (e.g. SQLite) can register tables without error - create_dataset switches from @mcp.tool/@mcp_auth_hook to the standard @tool decorator from superset_core.mcp.decorators, adding Dataset write RBAC and ToolAnnotations consistent with create_virtual_dataset - Blank/whitespace-only schema values are normalized to None before forwarding to CreateDatasetCommand, avoiding spurious table-not-found failures - Unexpected exceptions now re-raise (middleware handles them) instead of being swallowed into an InternalError response; test updated accordingly - Uses DatasetError.create() factory and event_logger/ctx instrumentation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add test_create_dataset_invalid_error to cover the DatasetInvalidError handler in create_dataset (previously untested path) - Add min_length=1 to CreateDatasetRequest.table_name to reject empty strings at the schema layer, consistent with CreateVirtualDatasetRequest Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Mirrors schema normalization — table_name is now stripped before being forwarded to CreateDatasetCommand, preventing whitespace-only strings from reaching the database layer. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…achable handlers, add catalog - Restore initialize_core_mcp_dependencies() in app.py before tool imports. The PR accidentally removed the explicit call; without it, standalone imports of app.py could fail because @tool stubs raise NotImplementedError until the host implementation is injected. - Fix unreachable DatasetExistsValidationError / TableNotFoundValidationError handlers: CreateDatasetCommand.validate() always wraps these into a single DatasetInvalidError(exceptions=[...]) and never raises them directly. Now inspects exc.get_list_classnames() inside the DatasetInvalidError branch to return the typed DatasetExistsError / TableNotFoundError responses. Update tests to use DatasetInvalidError(exceptions=[...]) matching real behavior. - Add catalog field to CreateDatasetRequest (str | None, default None) with the same blank-string normalization as schema, and pass it through to CreateDatasetCommand properties. Add test_create_dataset_with_catalog.
Before forwarding to CreateDatasetCommand, look up the database by id and call security_manager.raise_for_access(database, table) to enforce table-level access. CreateDatasetCommand only checks access for virtual (SQL) datasets, leaving physical-table registration ungated. Returns DatabaseNotFoundError when the database_id is invalid, and AccessDeniedError when the caller lacks table access. Extracts _classify_invalid_error helper to reduce cyclomatic complexity within the tool function. Adds test coverage for both new error paths.
- B1/B2: Move inline imports in create_dataset.py and tests to module top-level - H1: Replace str(exc) in AccessDenied/CreateFailed with static messages; log detail server-side - H2: Add MCP_DISABLED_TOOLS to config.py; use config["KEY"] instead of config.get() in app.py - H3: Add circular-import justification comments to inline imports in app.py - H4: Replace f-string database ID in DatabaseNotFoundError with static "Database not found" - M1: Narrow broad except Exception to except (json.JSONDecodeError, TypeError) in schemas.py - M3: Remove redundant inline datetime import inside DatasetError.create(); use top-level import - M4: Log only type(exc).__name__ to ctx.error for unexpected exceptions; use logger.exception - M5: Add -> None return type annotations to all 11 test methods - M6: Add inline comment explaining deferred sub-type classification in _classify_invalid_error - TC1: Add test for table_name whitespace normalization - TC2: Add test for blank schema normalization to None - TC3: Add test for serialize_dataset_object returning None (SerializationError path) - TC4: Assert static "Access denied" message in test_create_dataset_access_denied Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use proper type instead of object to remove type: ignore[attr-defined] comments — CommandInvalidError already defines get_list_classnames() and normalized_messages().
Four root causes:
1. config["KEY"] -> config.get("KEY", default) in app.py: tests mock
config.get() but bracket access bypassed the mock, causing KeyError
or returning MagicMock instead of string for APP_NAME/MCP_DISABLED_TOOLS.
2. Moved imports in create_dataset.py broke @patch("superset.commands...
.CreateDatasetCommand") — lazy imports (inside the function body)
pick up the active mock; top-level imports bind before the patch runs.
Fixed by patching superset.mcp_service.dataset.tool.create_dataset.*
in all relevant tests. Same fix applied to serialize_dataset_object.
3. Pydantic v2 'schema' field naming conflict: BaseModel.schema is a
deprecated classmethod; when the field is omitted from the request
Pydantic does not store None in instance.__dict__, so attribute lookup
returns the classmethod (a function) instead of None. Fixed by using
request.model_dump() to access 'schema' and 'catalog' field values.
4. DatasetError.error field_validator wrapped error strings in
UNTRUSTED-CONTENT tags, breaking assertions like == "Access denied".
DatasetError carries system-generated messages, not user-controlled
data, so the sanitization was incorrect. Validator removed.
Two test failures in CI:
1. test_create_dataset_database_not_found:
model_dump() for a Pydantic v2 model with a field named 'schema' (which
shadows the deprecated BaseModel.schema classmethod) may still return the
classmethod object via internal getattr, making the truthy check pass and
.strip() fail with AttributeError. Switch to isinstance(x, str) guard for
both schema and catalog normalization.
2. test_error_responses_sanitize_prompt_facing_error_text[DatasetError]:
DatasetError was missing the @field_validator("error") that ChartError and
DashboardError both have, so prompt-facing error text was returned unsanitized
instead of wrapped in LLM context delimiters.
…nitization DatasetError now wraps error strings in LLM context delimiters (matching DashboardError/ChartError). Update the two tests that checked exact error strings to use substring containment so they pass regardless of delimiter wrapping.
…taset Empty-string schema/catalog values (e.g. schema="") were passing isinstance(x, str) but evaluating to "" after .strip(), which is not None — so the 'if schema is not None' guard added them to the command properties dict. Use `or None` after strip so blank strings collapse to None and are excluded from the command properties entirely. Fixes test_create_dataset_blank_schema_normalized_to_none.
…k catalog test - Switch DatasetCreateFailedError handler from logger.error (loses traceback) to logger.exception so the full stack trace is captured on failure. - Add test_create_dataset_blank_catalog_normalized_to_none to cover the symmetrical case with blank catalog (parallel to blank schema test).
…rror test; rename test - Switch all @patch("superset.mcp_service.dataset.tool.create_dataset.X") to @patch.object(create_dataset_module, "X") using importlib.import_module. The tool package __init__.py exports create_dataset as a function, which shadows the module name for getattr-based resolution used by mock.patch in Python <3.12. importlib.import_module resolves via sys.modules and always returns the module object, matching the pattern used in test_query_dataset.py. - Add test_create_dataset_create_failed_error covering the DatasetCreateFailedError path that returns CreateFailedError (all other error paths were already tested). - Rename test_create_dataset_serialization_error to test_create_dataset_when_serialize_returns_none — the test mocks the function to return None rather than raising an exception, so the old name was misleading.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Pass non-string values through the optional-namespace validator unchanged so Pydantic's type validation rejects them, instead of silently coercing a malformed value (int, dict) to None and registering against the default namespace. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fc3d500 to
035d488
Compare
Code Review Agent Run #466da2Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
Summary
Adds a
create_datasetMCP tool that lets callers register an existing physicaldatabase table as a Superset dataset — the programmatic equivalent of
Data → Datasets → + Dataset in the UI.
CreateDatasetRequestschema —database_id,schema(alias forschema_nameto avoid the Pydantic v2
BaseModel.schema()clash),table_name, optionalcatalogand
owners; whitespace-only values forschema/catalogare normalised toNonebefore calling the command
create_datasettool — wrapsCreateDatasetCommand, usesDatasetInvalidError.get_list_classnames()(public API) to classify wrapped validationerrors; returns
DatasetInfo(same shape asget_dataset_info) so theidfeedsdirectly into
generate_chartorgenerate_explore_linkerror_typevalues for each failure class:DatabaseNotFoundError,AccessDeniedError,DatasetExistsError,TableNotFoundError,ValidationError,CreateFailedError,InternalErrorerrors, missing required fields, full
DatasetInfoshape, database not found,access denied, no-schema, with-catalog
DEFAULT_INSTRUCTIONSupdated to list the new toolMotivation
The MCP service already exposes
create_virtual_datasetfor SQL-based datasets. ThisPR adds the physical-table counterpart so agents can complete the full
"find DB → register table → chart it" workflow without manual UI steps.
Test plan
pytest tests/unit_tests/mcp_service/dataset/tool/test_create_dataset.py -xpre-commit run --files superset/mcp_service/dataset/tool/create_dataset.py superset/mcp_service/dataset/schemas.py