Repository navigation
feat(mcp): support complete semantic table reselection - #45998
Open
mikebridge wants to merge 7 commits into
Open
mikebridge wants to merge 7 commits into
mikebridge wants to merge 7 commits into
Conversation
added 7 commits
October 28, 2026 14:57
Stamp only newly generated chart and Explore selections, forward their identity through query extras without certifying external filters, and apply semantic temporal metadata before sorting. Keep existing-chart updates unstamped pending a complete-reselection contract.
Require explicit fresh intent before upgrading a saved selection marker, and reject retained temporal or secondary state rather than silently resetting it. Reuse the certified candidate for preview/save and fence normal chart writes against changes to the saved basis. Stacked on the MCP marker-transport change. Local checkpoint: cross-database fence evidence and independent review remain required before publication.
PostgreSQL rejects FOR UPDATE propagated into the datasource relationship's DISTINCT subquery. Load only the fenced chart and keep normal command validation and capture afterward. Exercise the real save/audit lifecycle on disposable PostgreSQL, MySQL and SQLite, pin stale-snapshot and lock-release behavior, and cover final semantic target denial and metadata failure before publication.
Keep each contributor's source status before dashboard and request filters lose their origin. Apply the same guard when appending filters to a saved query context so missing or stale provenance cannot borrow its version. Addresses review thread 4428377061.
…tion-version' into sc-130336-mcp-full-reselection
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #45998 +/- ##
==========================================
+ Coverage 84.62% 84.88% +0.25%
==========================================
Files 3069 3073 +4
Lines 205473 209967 +4494
Branches 48295 49179 +884
==========================================
+ Hits 173875 178222 +4347
- Misses 28305 28340 +35
- Partials 3293 3405 +112
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:
|
mikebridge
marked this pull request as ready for review
November 5, 2026 02:10
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
1341
to
1358
| with event_logger.log_context(action="mcp.update_chart.db_write"): | ||
| command = UpdateChartCommand(chart.id, payload_or_error) | ||
| if certified_form_data is not None and semantic_target is not None: | ||
| assert new_form_data is not None | ||
| target_error: GenerateChartResponse | None = ( | ||
| _check_reselection_target( | ||
| semantic_target, certified_form_data, new_form_data | ||
| ) | ||
| ) | ||
| if target_error is not None: | ||
| return _finalize_response(target_error) | ||
| command: UpdateChartCommand = ( | ||
| UpdateChartCommand( | ||
| chart.id, payload_or_error, expected_basis=expected_basis | ||
| ) | ||
| if expected_basis is not None | ||
| else UpdateChartCommand(chart.id, payload_or_error) | ||
| ) | ||
| updated_chart = command.run() |
Contributor
There was a problem hiding this comment.
Suggestion: The semantic view can change after this check but before command.run(); the write fence checks only the chart, so stale selections can be saved with the certified version.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Race condition
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/update_chart.py
**Line:** 1341:1358
**Comment:**
*Race Condition: The semantic view can change after this check but before `command.run()`; the write fence checks only the chart, so stale selections can be saved with the certified version.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+431
to
+441
| SEMANTIC_SELECTION_MEMBER_KEYS: tuple[str, ...] = ( | ||
| "filters", | ||
| "adhoc_filters", | ||
| "interactive_drilldown", | ||
| "interactive_groupby", | ||
| "interactive_highlight", | ||
| "granularity_sqla", | ||
| "granularity", | ||
| "time_column", | ||
| "custom_form_data", | ||
| ) |
Contributor
There was a problem hiding this comment.
Suggestion: extra_filters can carry semantic column filters, but this allowlist omits it; source-less members merged with versioned sources are not marked unverified and may inherit the chart’s identity version.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Incorrect condition logic
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard_scope.py
**Line:** 431:441
**Comment:**
*Incorrect Condition Logic: `extra_filters` can carry semantic column filters, but this allowlist omits it; source-less members merged with versioned sources are not marked unverified and may inherit the chart’s identity version.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Contributor
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
TL;DR: Let MCP clients update an outdated semantic table chart after explicitly reselecting its query fields, without treating retained saved settings as fresh selections.
Background: semantic providers can version their member identities. A chart saved under an older identity format needs fresh selections before its query can be certified for the target view.
Problem: stamping the current version during a partial update could certify old fields or silently discard saved conditions. Concurrent edits can also invalidate the saved state used to prepare an update.
Changes: verify the final merged table/ag-grid selection, compile it, and recheck the target before save or preview. Refuse unverified retained query controls with recovery guidance. Only an explicit empty filter list (
filters: []) authorizes dropping saved WHERE/HAVING conditions; other replacements that would drop them are refused with a message. A transactional saved-state fence prevents a certified save from overwriting a competing edit. Previews retain a requested rename but are not a lease on the saved chart.This is stacked on #45828 and should be reviewed after it. The PR targets master because the prerequisite branch is in a fork. It carries these #45828 commits:
The new certification path is bounded to table/ag-grid updates requiring reselection. Ordinary current-version partial updates and physical datasets retain their existing behavior; unsupported retained state must be reselected in Explore.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable: MCP backend behavior, no UI changes.
TESTING INSTRUCTIONS
update_chartwith all columns explicitly reselected andfilters: []; verify both save and preview compile with the target version.Automated coverage includes the public-tool save/preview matrix, query transport and provenance, explicit-clear and malformed-alias cases, and real-session fencing. SQLite/tool/inventory checks passed (863 passed, 1 expected skip); native PostgreSQL and MySQL lifecycle checks each passed 10 tests. Changed-file hooks passed. CI results will be tracked on this draft.
ADDITIONAL INFORMATION