feat(mcp): add feature-gated interactive pivot support - #43480
feat(mcp): add feature-gated interactive pivot support#43480aminghadersohi wants to merge 7 commits into
Conversation
dosubot
Bot
added
change:backend
Code Review Agent Run #851990Actionable Suggestions - 0Additional Suggestions - 4
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 |
|
This recreates #43268 after its head repository was removed. The implementation and review fixes are unchanged, with the branch updated onto the latest |
✅ 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}
| if config.time_grain: | ||
| # Preset's hidden temporal_columns_lookup control identifies temporal | ||
| # dimensions in groupby, and its buildQuery applies this grain to each | ||
| # one. A single granularity_sqla is neither required nor sufficient for | ||
| # a pivot that can contain multiple temporal dimensions. | ||
| form_data["time_grain_sqla"] = config.time_grain.value |
There was a problem hiding this comment.
Suggestion: The pivot state never identifies which grouped dimensions are temporal: setting only time_grain_sqla is insufficient because the form data does not include granularity_sqla or temporal_columns_lookup. As a result, time_grain is ignored or cannot be applied to the intended row/column dimensions, so generated pivots with temporal grouping do not honor the requested grain. [logic error]
Severity Level: Major ⚠️
- ⚠️ Interactive Pivot temporal grouping can ignore requested grain.
- ⚠️ Generated pivot rows or columns may use raw timestamps.
- ❌ Time-based pivot analysis can return incorrect aggregation buckets.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/plugins/interactive_pivot.py
**Line:** 95:100
**Comment:**
*Logic Error: The pivot state never identifies which grouped dimensions are temporal: setting only `time_grain_sqla` is insufficient because the form data does not include `granularity_sqla` or `temporal_columns_lookup`. As a result, `time_grain` is ignored or cannot be applied to the intended row/column dimensions, so generated pivots with temporal grouping do not honor the requested grain.
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
|
The flagged issue is correct. In To resolve this, you should update Would you like me to implement this fix and check the rest of the PR comments for other issues? superset/mcp_service/chart/plugins/interactive_pivot.py |
| series_limit_metric: ColumnRef | None = Field( | ||
| None, | ||
| description="Metric used to rank series when a series/cell limit applies", | ||
| ) |
There was a problem hiding this comment.
Suggestion: The series_limit_metric field accepts any ColumnRef, including a plain dimension with no aggregate or saved-metric marker. map_interactive_pivot_config then passes it to create_metric_object, which defaults such a reference to SUM(column). A request such as {"name": "region"} therefore becomes a series-ranking metric of SUM(region), potentially producing an invalid SQL aggregation or ranking by unintended values. Require this reference to represent a metric (an aggregate, saved metric, or SQL expression) before accepting it. [api mismatch]
Severity Level: Major ⚠️
- ❌ Interactive Pivot generation can fail for nonnumeric ranking dimensions.
- ⚠️ Numeric dimensions may rank series by unintended SUM values.
- ⚠️ Series limiting produces incorrect user-visible pivot results.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/schemas.py
**Line:** 1147:1150
**Comment:**
*Api Mismatch: The `series_limit_metric` field accepts any `ColumnRef`, including a plain dimension with no aggregate or saved-metric marker. `map_interactive_pivot_config` then passes it to `create_metric_object`, which defaults such a reference to `SUM(column)`. A request such as `{"name": "region"}` therefore becomes a series-ranking metric of `SUM(region)`, potentially producing an invalid SQL aggregation or ranking by unintended values. Require this reference to represent a metric (an aggregate, saved metric, or SQL expression) before accepting it.
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #43480 +/- ##
==========================================
- Coverage 78.92% 78.91% -0.01%
==========================================
Files 2878 2879 +1
Lines 165088 165246 +158
Branches 38137 38168 +31
==========================================
+ Hits 130296 130411 +115
- Misses 32342 32381 +39
- Partials 2450 2454 +4
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
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks for recreating this cleanly after the fork mishap, Amin. I verified the recreation rather than assuming it: I diffed all 20 changed files at f41ad6d against the content I approved on #43268 at 895af43, and the PR delta is line for line identical at both bases; the only differences are master drift the merge brought in. The approved history is preserved in the branch, the body carries the corrected time-comparison notes, tests/unit_tests/mcp_service/chart passes locally at this head (1,311 tests), and CI is green. Same content, same verdict. LGTM.
2 participants
SUMMARY
Recreates and supersedes closed PR #43268 after its head repository was removed. This PR is explicitly based on
apache/superset:master.Adds a distinct, feature-gated MCP adapter for Preset's
ag-grid-pivot-tablevisualization. This deliberately does not alias Interactive Pivot to the OSSpivot_table_v2adapter.The new
interactive_pivotschema and plugin cover:pivot_table_statefor the Rows, Column Labels, and Values side-panel bucketsAG_GRID_PIVOT_TABLE_ENABLEDfeature flag plus the existing MCP plugin filter, so OSS-only deployments do not advertise an unavailable visualizationThis implements the supported Interactive Pivot portion of SC-113786 and records the true AG Grid pivot as distinct from the OSS Pivot Table V2 delivered under SC-100129. Time comparison remains a visualization-side prerequisite: the Preset plugin does not consume
time_compareorcomparison_type, so the MCP does not advertise those controls.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable. This changes MCP schemas and form-data generation; it does not add or modify UI components.
TESTING INSTRUCTIONS
AG_GRID_PIVOT_TABLE_ENABLEDon a host that registers Preset's Interactive Pivot plugin.get_chart_type_schema("interactive_pivot")and verify the schema and examples are returned.viz_type=ag-grid-pivot-tableand its persisted grid state populates the AG Grid side-panel buckets.ag-grid-pivot-tableand UI-managed grid state such as sizing/filter/sort state is retained.DISABLED_CHART_TYPErather than offering a broken chart.Automated validation run:
ADDITIONAL INFORMATION
AG_GRID_PIVOT_TABLE_ENABLEDAI behavior / evaluation evidence
This is a deterministic schema, validation, and form-data mapping change. Unit tests cover schema examples, exact native viz resolution, feature-gated availability, validation, omission of unsupported comparison fields, side-panel state, update/preview preservation, capabilities, and generated naming. No model prompt, routing, retrieval, or sampling behavior changes.
Cost and latency
No additional model calls or network requests are introduced. Availability checks are in-process feature-flag lookups; chart mapping remains deterministic and local.