fix(mcp): preserve table chart state during updates - #42655
fix(mcp): preserve table chart state during updates#42655aminghadersohi wants to merge 7 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #42655 +/- ##
==========================================
- Coverage 65.79% 65.73% -0.06%
==========================================
Files 2842 2842
Lines 162106 162165 +59
Branches 37148 37159 +11
==========================================
- Hits 106653 106606 -47
- Misses 53388 53494 +106
Partials 2065 2065
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:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
dosubot
Bot
added
api
Co-Authored-By: Claude <noreply@anthropic.com>
test_sub_day_last_normalizes asserts a premise -- that the raw value
blows up inside get_since_until() -- before checking the normalization.
That premise compares a sub-day since-expression against an until of
midnight-today, so it only holds once the clock is far enough past
midnight.
Bare-unit values ("Last hour", "Last Hour") truncate to the previous
whole unit rather than resolving relative to now, which leaves them
behind midnight until 02:00 and makes get_since_until() return a valid
range instead of raising. Runs landing in that window failed with
"DID NOT RAISE ValueError" for exactly those two parameters.
Freeze the clock at midday so the case no longer depends on when the
suite runs. Test-only: the validator's behaviour is unchanged.
Co-Authored-By: Claude <noreply@anthropic.com>
16dbdb4 to
9c962f0
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 #b295fd
Actionable Suggestions - 2
-
superset/mcp_service/chart/schemas.py - 2
- Missing test coverage for new field · Line 2350-2357
- Missing test for validator error branches · Line 2393-2398
Additional Suggestions - 3
-
superset/mcp_service/chart/tool/update_chart.py - 3
-
Missing deduplication for all_columns · Line 131-133When `add_columns` is called multiple times or includes columns already in `all_columns`, duplicates are silently appended. Superset may behave unpredictably with duplicate column entries. Consider deduplicating: `existing = set(existing_form_data.get('all_columns') or []); merged['all_columns'] = list(existing | {c.name for c in columns if c.name})`
-
Missing deduplication for groupby/metrics · Line 138-139Same pattern as `all_columns` — `groupby` and `metrics` lists are blindly extended without checking for existing entries. Duplicate metrics/groupby fields can cause Superset query failures or unexpected aggregations.
-
Incomplete doc example for add_columns · Line 441-441The docstring at line 441 mentions `add_columns` but the JSON examples only show it standalone. Users may not realize it can be combined with `chart_name` for rename + column-append operations, limiting discoverability.
-
Review Details
-
Files reviewed - 6 · Commit Range:
163a694..36c68d8- superset/mcp_service/chart/compile.py
- superset/mcp_service/chart/schemas.py
- superset/mcp_service/chart/tool/update_chart.py
- tests/unit_tests/mcp_service/chart/test_compile.py
- tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
- tests/unit_tests/mcp_service/common/test_time_range_validation.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ 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
| add_columns: List[ColumnRef] | None = Field( | ||
| None, | ||
| description=( | ||
| "Table columns or metrics to append while preserving every existing " | ||
| "column and metric. Use this instead of config.columns when adding " | ||
| "columns to an existing table chart." | ||
| ), | ||
| ) |
There was a problem hiding this comment.
The new add_columns field (lines 2350-2357) has no corresponding unit tests in test_chart_schemas.py. Per the project's testing guidelines, every new MCP mutation tool schema field requires dedicated unit test coverage including request validation and schema serialization.
Code Review Run #b295fd
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Partly. The add_columns field itself is covered — test_update_chart.py has test_add_columns_preserves_existing_columns_and_metrics, test_add_columns_rebinds_requested_dataset, and a parametrized test_add_metric_to_raw_table_returns_actionable_error. They live next to the other _build_update_payload tests rather than in test_chart_schemas.py, which is why the search missed them.
The validator was a real gap though, so I've added TestUpdateChartRequestColumnPatchValidation covering both error branches and the valid additive shape (see the reply on the validate_column_patch thread).
Writing the coverage you asked for also turned up an actual bug, so thanks — details in the other thread and in the PR comment.
There was a problem hiding this comment.
It is good to hear that the coverage gaps have been addressed and that the additional testing led to the discovery of a bug. The addition of TestUpdateChartRequestColumnPatchValidation to cover both error branches and the valid additive shape is an appropriate way to resolve the concern regarding missing test coverage for the new field.
| if self.config is not None and self.add_columns is not None: | ||
| raise ValueError( | ||
| "Use either 'config' for a full visualization replacement or " | ||
| "'add_columns' to append table columns while preserving the existing " | ||
| "configuration, not both." | ||
| ) |
There was a problem hiding this comment.
The validate_column_patch validator (lines 2393-2401) that enforces mutual exclusivity of config and add_columns has no unit tests. Both its error branches — blocking co-provision and blocking empty list — are untested.
Code Review Run #b295fd
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Valid — both branches were untested. Added in 79f2704:
class TestUpdateChartRequestColumnPatchValidation:
def test_config_and_add_columns_together_rejected(self) # "not both"
def test_empty_add_columns_rejected(self) # "at least one column"
def test_add_columns_alone_accepted(self) # the valid shapeWriting these paid for itself: the dimension-only case they exercise led me to a real bug where appending a plain column to an aggregate table silently dropped it. Fixed in the same commit — see the PR comment.
There was a problem hiding this comment.
The suggestion to add unit tests for the validator error branches is appropriate and improves the code's robustness. Implementing these tests ensures that the mutual exclusivity of 'config' and 'add_columns' is correctly enforced and helps prevent regressions, as demonstrated by the bug you identified and fixed.
Addresses review feedback on apache#42655. _append_table_columns() built its patch with map_config_to_form_data(TableChartConfig(columns=...)), which infers query_mode from the columns it is handed. A dimension-only append compiles to a raw table whose groupby is empty, so appending a plain column to an aggregate table extended groupby with nothing and the column was silently dropped. Route columns by ColumnRef.is_metric instead, so dimensions reach groupby and metrics reach metrics regardless of the mix. Appends also blindly extended the saved lists, so re-adding a column already on the chart duplicated it. Skip entries that are already present, keyed on a stable serialization -- metric lists mix saved names with adhoc dicts, so they are not reliably hashable and a set would both crash and lose the ordering that drives table layout. Cover the config/add_columns validator error branches, which had no tests, and document that add_columns combines with chart_name. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks @bito-code-review — worked through all five suggestions. All are addressed in The bug the review led to
So a dimension-only append compiled to a raw patch, Existing tests missed it because they either appended a metric ( Suggestions 1 & 2 — test coverage
Suggestions 3 & 4 — deduplicationAgreed on the substance, with one correction: the proposed >>> set(['count', {'label': 'Earliest Go Live Date', 'aggregate': 'MIN'}])
TypeError: unhashable type: 'dict'
Suggestion 5 — docsAdded an example combining Verification
|
Code Review Agent Run #089551Actionable 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 |
2 participants
SUMMARY
Adds safe patch semantics for updating existing table charts through MCP and fixes stale-filter handling during previews.
add_columnstoupdate_chart, allowing callers to append table dimensions or metrics without reconstructing or replacing the saved column list.filters: []by removing saved ad hoc filters from preview form data.No filterplaceholder as non-filtering during dataset-column validation.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable. This changes the MCP API and validation behavior without UI changes.
TESTING INSTRUCTIONS
The focused MCP suite passes 87 tests and the changed-file pre-commit gate passes. The required all-files gate was also run; unrelated repository-wide mypy errors and unavailable frontend dependencies prevent a clean all-files result in this environment.
Eval evidence: Focused regression coverage includes additive metric preservation, dataset rebinding, raw-table metric rejection, parenthesized column names, explicit empty-filter replacement, and inert stale-filter validation.
Cost & latency delta: Adds 0 LLM calls and 0 external requests. The update path adds only in-memory list/dictionary merging and a linear scan of saved filters; no model or prompt behavior changes.
ADDITIONAL INFORMATION
Blast radius: Limited to MCP
update_charttable updates and MCP chart filter validation. Existing full-config replacement behavior remains available.Risk and rollback: Low. The new request field is additive, and reverting the commits restores the prior behavior. Non-table charts and metric additions to raw tables return actionable validation errors.
Review guidance: Focus on preservation of existing
groupby/metrics/all_columns, dataset rebinding, and the distinction between omitted filters (preserve) and explicitfilters: [](clear).