Repository navigation
fix(mcp): isolate dashboard copies and clarify chart evidence - #45504
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
CodeAnt PR Risk: Low Risk
Assessed commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #45504 +/- ##
==========================================
+ Coverage 83.98% 84.05% +0.06%
==========================================
Files 3055 3054 -1
Lines 201017 200913 -104
Branches 46949 46950 +1
==========================================
+ Hits 168828 168875 +47
+ Misses 28943 28800 -143
+ Partials 3246 3238 -8
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:
|
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks @fitzee, this is a good set of fixes and the four behaviours all hold up when I drive them.
I ran the affected suites plus the duplication, ownership filter and histogram paths against a real instance on this branch, and reverted each behaviour one at a time to confirm the cited tests pin it. Everything in the body checks out, including the merge base count. Comments inline, none of them blocking.
Holding off on a formal approval only because unit-tests is red and the branch is conflicting. The failure is test_native_inventory_report_matches_registered_tools and it reproduces unchanged at the merge base 3cf94c87, so it is inherited rather than yours: master already refreshed those generate_chart, update_chart and get_chart_preview byte counts in #45311, which is also the conflict. A catch up to master plus a regenerated inventory should clear both.
Carry every executed query's pipeline privately through saved and rebuilt JSON results into dashboard summaries. Do not infer input coverage from post-processed output counts: affected layers return total_rows=null and truncated=true conservatively, with the input fetch limit and an explanation. Keep ordinary completeness checks for row-preserving pipelines and retain the distribution warning for legacy results without execution metadata. Post-processing registry audit: - aggregate: grouping reduces rows to group aggregates. - boxplot: grouping reduces rows to distribution statistics. - histogram: input rows become bin counts per group. - pivot: dimension values become columns and duplicate indexes aggregate. - resample: coarser time bins can reduce rows. - rolling: min_periods can slice leading rows from the result. - prophet: forecast reconstruction replaces the input time index; output counts cannot establish input coverage. - compare: adds/removes comparison columns, preserving input rows. - contribution: computes cell shares, preserving input rows. - cum: cumulative values preserve input rows. - diff: differences preserve rows, including missing leading values. - flatten: flattens column labels/resets the index without removing rows. - geodetic_parse: appends parsed coordinate columns, preserving rows. - geohash_decode: appends coordinate columns, preserving rows. - geohash_encode: appends a geohash column, preserving rows. - rank: assigns ranks without removing rows. - rename: changes column labels without removing rows. - select: selects/excludes/renames columns only, preserving rows. - sort: reorders rows without removing them. Unknown, custom and future operations are conservative by default. The escape_separator/unescape_separator helpers are not registered DataFrame operations. Regression executes a generated timeseries/groupby pivot against 500 input rows, producing 50 output rows, through saved and rebuilt query paths. It fails against 377a908 and passes after. Additional tests cover all registry classifications, custom operations, mixed-query layers, empty leading layers, and the private metadata handoff.
rebenitez1802
left a comment
There was a problem hiding this comment.
Approve: the deep-copy default, the strict self-scoped list schemas and the currency-neutral guidance all look right, and the authorization model is unchanged. Duplicating still needs Dashboard write plus editor on the source, the same capability as REST /copy/. I left two Medium inline suggestions worth taking before merge:
pivotwithout a groupby is treated as row-collapsing, so most timeseries charts now reporttruncated=true.- The compact tool description no longer mentions the new deep-copy default.
Smaller follow-ups (non-blocking):
- 🟢
fetch_row_limitreportsrequest.fetch_row_limit, not the limit that actually ran (query_context.queries[i].row_limit). They differ whenSQL_MAX_ROWis lower than the request, andtruncatedcan then be wrong too. - 🟢 The histogram/box-plot note branch only fires when operation metadata is missing. It then returns
total_rows=1, truncated=falsenext to a note saying those values prove nothing. The two tests for it buildChartDatawithout metadata, so they cover that fallback, not the production path. - 🟢 Stale descriptions on
fetch_row_limit/total_rowsindashboard/schemas.py(they describe only a "capped fetch"), plus a capital "For" after a semicolon in the completeness note. The meaning change fortruncated/total_rowsdeserves a line in UPDATING.md, since the PR calls the new fields "additive". - 🟢 A few
$,.2fexamples remain:chart/schemas.pyBig Numbery_axis_formatandd3_number_format, andupdate_dataset_metric.py(sum_revenuewith$,.2f). - 🟢 The ~370-char
completeness_noterepeats on every chart (~11 KB atmax_charts=30). It could go once onDashboardData.
Preserve ordinary completeness for ungrouped pivots and use executed query limits for each layer. Keep distribution fallback coverage conservative, share population guidance once per dashboard, and document the count semantics. Retain the isolated copy default in compact tool descriptions and use currency-neutral number-format examples.
SUMMARY
Refine MCP dashboard-copy safety, list-scope validation, and analytical guidance (implementation and tests at
831598148220a36a37c0d7a4a3890a75bff6ed59):duplicate_dashboard, including its compact tool description. Explicitduplicate_slices=falsedeliberately shares charts (test_compact_duplicate_description_keeps_default,test_duplicate_referencing_same_charts).test_list_dashboards_rejects_unknown_arguments,test_list_dashboards_self_lookup_and_aliases).total_rows=nullandtruncated=true. Each layer uses its executed query limit, and longer completeness guidance appears once per dashboard (test_pivot_completeness_respects_groupby,test_multi_query_completeness_uses_each_executed_limit,test_dashboard_completeness_guidance_is_shared).test_number_format_examples_are_currency_neutral,test_guided_chart_prompt_does_not_assume_currency).Compatibility changes are documented in
UPDATING.mdand the MCP guide. No frontend or migration changes.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable: MCP contracts, guidance, and backend tests only.
TESTING INSTRUCTIONS
Validation at
831598148220a36a37c0d7a4a3890a75bff6ed59:pytestacross the touched MCP test modules and native surface selection: 906 passed. Command:PYTHONPATH=$PWD/superset-core/src:$PWD /tmp/pr45504-pivot-venv/bin/python -m pytest <touched-test-files> -m 'not manual and not llm_compliance' -q(11 files total, including native surface).SUPERSET_MCP_UPDATE_TOOL_INVENTORY=1 pytest tests/unit_tests/mcp_service/test_native_tool_surface.py: 38 passed.pre-commit runandpre-commit run --files <changed-branch-files>.Manual checks:
duplicate_slices; expect independent chart IDs. Explicit false intentionally shares IDs.total_rows=12,truncated=false. Grouped pivots retain conservative input-coverage reporting.ADDITIONAL INFORMATION
Omitting
duplicate_slicescreates independent chart objects. Unknown list arguments fail validation. The response metadata is additive, buttotal_rowsandtruncatedhave a clarified meaning: unknown input coverage is conservative uncertainty, not observed truncation. Histogram computation and authorization are unchanged. Propagating rawsql_rowcountfor exact input-cap detection remains a separate follow-up.