feat(mcp): sankey chart type plugin - #43573
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43573 +/- ##
==========================================
- Coverage 79.24% 79.24% -0.01%
==========================================
Files 2888 2889 +1
Lines 166446 166529 +83
Branches 38529 38544 +15
==========================================
+ Hits 131908 131959 +51
- Misses 32045 32073 +28
- Partials 2493 2497 +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:
|
4a45a88 to
619081f
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| for col, name in ((self.source, "source"), (self.target, "target")): | ||
| _reject_sql_expression_on_dimension(col, name) | ||
| if col and col.saved_metric: | ||
| raise ValueError( | ||
| f"{name} cannot use saved_metric=True; " | ||
| "saved metrics belong in the 'metric' field" | ||
| ) |
There was a problem hiding this comment.
Suggestion: Dimension validation rejects sql_expression and saved_metric, but it does not reject aggregate on source or target. Such a configuration passes schema and dataset aggregation validation for compatible aggregates, then map_sankey_config discards the aggregate and emits only the raw column name, silently changing the requested Sankey semantics. Reject aggregated source/target references during validation. [type error]
Severity Level: Major ⚠️
- ❌ Sankey charts can show flows grouped by raw nodes despite requested source/target aggregation.
- ⚠️ `generate_chart` accepts invalid dimension configuration without feedback.
- ⚠️ Saved charts preserve misleading configuration semantics in generated form data.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/schemas.py
**Line:** 1095:1101
**Comment:**
*Type Error: Dimension validation rejects `sql_expression` and `saved_metric`, but it does not reject `aggregate` on `source` or `target`. Such a configuration passes schema and dataset aggregation validation for compatible aggregates, then `map_sankey_config` discards the aggregate and emits only the raw column name, silently changing the requested Sankey semantics. Reject aggregated source/target references during validation.
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 issue is correct. The superset/mcp_service/chart/schemas.py |
| "source": config.source.name, | ||
| "target": config.target.name, | ||
| "metric": create_metric_object(config.metric), |
There was a problem hiding this comment.
Suggestion: The MCP query builder reads grouping columns from groupby, but this mapper only emits source and target; unlike the frontend buildQuery, the backend MCP path does not derive groupby from those fields. Generated Sankey queries therefore aggregate the metric over the entire dataset instead of producing one weighted edge per source/target pair. Include both node columns in the query grouping data or add equivalent Sankey-specific handling to the MCP query builder. [api mismatch]
Severity Level: Critical 🚨
- ❌ MCP Sankey previews aggregate all edges together.
- ❌ Generated Sankey flow relationships are lost.
- ⚠️ Compile validation checks the wrong query shape.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/chart_utils.py
**Line:** 1031:1033
**Comment:**
*Api Mismatch: The MCP query builder reads grouping columns from `groupby`, but this mapper only emits `source` and `target`; unlike the frontend `buildQuery`, the backend MCP path does not derive `groupby` from those fields. Generated Sankey queries therefore aggregate the metric over the entire dataset instead of producing one weighted edge per source/target pair. Include both node columns in the query grouping data or add equivalent Sankey-specific handling to the MCP query builder.
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| "source": config.source.name, | ||
| "target": config.target.name, | ||
| "metric": create_metric_object(config.metric), | ||
| "sort_by_metric": config.sort_by_metric, |
There was a problem hiding this comment.
Suggestion: sort_by_metric is preserved in form data, but the MCP query builder does not translate it into an orderby clause; the frontend-only buildQuery implementation is not invoked by the MCP generation path. As a result, requests using the default sort_by_metric=True do not actually order edges by descending metric. Add the corresponding order-by information when constructing the MCP query. [logic error]
Severity Level: Major ⚠️
- ⚠️ MCP Sankey previews ignore default metric ordering.
- ⚠️ Largest flows may not appear first.
- ⚠️ Row-limited results can omit higher-weight edges.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/chart_utils.py
**Line:** 1034:1034
**Comment:**
*Logic Error: `sort_by_metric` is preserved in form data, but the MCP query builder does not translate it into an `orderby` clause; the frontend-only `buildQuery` implementation is not invoked by the MCP generation path. As a result, requests using the default `sort_by_metric=True` do not actually order edges by descending metric. Add the corresponding order-by information when constructing the MCP query.
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
Code Review Agent Run #8f7497Actionable Suggestions - 0Additional Suggestions - 2
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 |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Review of feat(mcp): sankey chart type plugin — first human review on this PR. This is the 6th of the MCP chart-plugin family, and it carries the most consequential defect we've seen in the set. Not requesting changes (COMMENT only), but flagging one Critical correctness bug plus the now-familiar aggregate validation gap. All claims below were executed against the PR head (619081fbad) unless marked INSPECTED.
🔴 Critical — the generated query has no GROUP BY (bot threads 2 & 3, one root cause)
map_sankey_config (chart_utils.py:1021) emits source and target as top-level form_data keys, but the MCP query builder never reads those keys. It derives grouping columns via columns_from_form_data (superset/common/form_data_query_context.py:133), which looks only at groupby / columns / x_axis. Since the mapper emits neither groupby nor orderby, the resulting query groups by nothing and orders by nothing.
Every sibling mapper emits groupby explicitly — e.g. map_pie_config does "groupby": [config.dimension.name], funnel does "groupby": [config.breakdown.name], box_plot/histogram likewise. Sankey is the only one that copied the frontend field names (source/target) verbatim instead of translating them to what the backend builder consumes. The frontend Sankey/buildQuery.ts bridges this gap (const groupby = [source, target], plus orderby from sort_by_metric), but the MCP path does not invoke buildQuery — so both transforms are silently lost.
Traced consequence (RAN):
map_sankey_config(cfg) keys = ['color_scheme','metric','row_limit','sort_by_metric','source','target','viz_type']
form_data.get('groupby') = None
form_data.get('orderby') = None
columns_from_form_data(form_data) => [] # what the query actually GROUPs BY
frontend contract groupby would be => ['from_stage','to_stage']
With columns=[] and metrics=[SUM(users)], _compile_chart builds SELECT SUM(users) FROM dataset [WHERE …] LIMIT 2 — a single aggregate row over the entire dataset, no GROUP BY. It is not a crash and nothing downstream backfills it (the only fallback in _compile_chart is granularity_sqla, which Sankey never sets). This is exactly codeant's thread-2 diagnosis, confirmed end to end.
Scope of impact (INSPECTED): generate_chart runs both _compile_chart (generate_chart.py:398,610) and generate_preview_from_form_data (:734) through this same path, and update_chart_preview too. So the compile check passes (a valid one-row query → false confidence) while the preview/data/SQL the tool returns to the agent show a single collapsed total rather than a flow diagram. A chart saved with save_chart=True still renders correctly when later opened in Explore (the frontend buildQuery runs there) — but the entire MCP-surfaced output for this tool is wrong. That is why Critical is the right severity here, distinct from the cosmetic gauge/radar issues.
Thread 3 (sort_by_metric) is the same bug: sort_by_metric is carried in form_data but never translated to orderby, because buildQuery (which would emit [metric, false]) is bypassed. form_data.get('orderby') is None above → no ordering, so row-limited results can drop the heaviest edges.
Fix — mirror the frontend contract inside map_sankey_config, matching how every other mapper already emits groupby:
form_data: Dict[str, Any] = {
"viz_type": "sankey_v2",
"groupby": [config.source.name, config.target.name],
"metric": create_metric_object(config.metric),
"sort_by_metric": config.sort_by_metric,
"row_limit": config.row_limit,
"color_scheme": config.color_scheme or "supersetColors",
}
if config.sort_by_metric:
form_data["orderby"] = [[form_data["metric"], False]](Keeping source/target too is harmless if the frontend still expects them for rendering — but groupby is what makes the backend query correct.)
🟠 Major — reject_metric_style_nodes misses aggregate (bot thread 1)
reject_metric_style_nodes (schemas.py:1093) rejects sql_expression and saved_metric on source/target, but not aggregate. A metric-shaped node passes validation and the aggregate is then silently dropped by the mapper (RAN):
source={"name":"amount","aggregate":"SUM"} accepted; source.is_metric => True
map_sankey_config(...)['source'] => 'amount' # aggregate SILENTLY DROPPED
This is byte-identical to the gap already confirmed in gauge (#43568), treemap (#43569), and radar (#43571). The canonical predicate already exists in this file: ColumnRef.is_metric at schemas.py:751 (bool(aggregate) or saved_metric or bool(sql_expression)). Gate on it:
if col.is_metric:
raise ValueError(f"{name} must be a plain node column, not a metric …")Sankey-specific severity: worse than gauge/radar (cosmetic) and on par with treemap's grain corruption. source/target are the GROUP BY dimensions of the flow. Combined with the Critical above (once groupby is fixed), an aggregated node would either be dropped to its raw name or, if it reached grouping, redefine the flow topology — so this should be closed together with the groupby fix.
Tests (Rule 26)
test_sankey_chart.py (+178, 16 tests, all RAN green). Reverting only the production files makes every test fail at import (they import map_sankey_config / SankeyChartConfig), so none is an independent regression guard. More importantly, none asserts the behavior that is actually broken:
- No test asserts
groupby/orderbyin the emitted form_data.test_basic_sankey_form_dataassertsform_data["source"] == "from_stage"/["target"] == "to_stage"— it locks in the buggy source/target-only shape as if it were the contract (the same pattern radar's tests fell into). - No negative test for
aggregateonsource/target— onlysaved_metricis covered.
A single assertion — assert columns_from_form_data(map_sankey_config(cfg)) == ["from_stage","to_stage"] — would have caught the Critical bug at authoring time. Worth adding alongside the fix, and worth back-porting the columns_from_form_data assertion to the sibling mappers.
Family synthesis
This is the 7th confirmed instance of the same shape in mcp_service: the MCP path constructs QueryContext straight from form_data and re-implements, per chart, whatever the frontend buildQuery does — so any transform that lives only in buildQuery is lost unless the mapper re-derives it. Prior instances: #43432 granularity_sqla vs granularity, SC-117852 Bugs A/B, #43225 OpenAPI/TS divergence, gauge bounds, radar arity, treemap grain. The per-PR fix here is two lines in map_sankey_config; the durable fix is architectural — have the MCP path reuse the canonical buildQuery-equivalent grouping/order derivation rather than hand-copying it into each mapper. Not a blocker for this PR, but the recurrence is the real signal.
Gates / status (re-derived at post time)
- CI: 61 unique checks (latest run per name) — 46 success, 12 skipped, 3 neutral, 0 pending, 0 failing. Green.
- Mergeability:
mergeable=MERGEABLE,mergeStateStatus=BLOCKED— blocked by branch protection (external contributor needs maintainer approval), not by CI. - External contributor; leaving this as a comment for maintainer sign-off.
Thanks for the thorough test file and the clear buildQuery-contract docstrings — the mapping bug is subtle precisely because the docstrings describe the right contract; it's just the two derived fields (groupby, orderby) that don't make it into form_data.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Round 2 — reviewed the new head cdcfaa65f0 (fix(mcp): sankey — emit orderby + reject aggregate on source/target). All three findings from my round-1 review are fixed, and each was verified by executing against the pinned head, not just reading the diff. Still a comment (external contributor) — but from a correctness standpoint this is now clean.
✅ Critical — missing GROUP BY — FIXED (verified)
map_sankey_config now emits "groupby": [config.source.name, config.target.name]. Traced end to end (RAN):
map_sankey_config(cfg)['groupby'] => ['from_stage', 'to_stage']
columns_from_form_data(map_sankey_config(cfg)) => ['from_stage', 'to_stage'] # was [] in round 1
The MCP query now groups by both node columns, so _compile_chart/preview produce one weighted edge per source→target pair instead of a single collapsed aggregate row. The added docstring accurately explains why the explicit groupby is required (backend builders resolve grouping from groupby alone and carry no source/target alias). Resolves codeant thread at chart_utils.py (grouping).
⚠️ Major — sort_by_metric → orderby — CORRECTION: this section was wrong
if config.sort_by_metric:
form_data["orderby"] = [[form_data["metric"], False]]RAN: with sort_by_metric=True, orderby == [[<metric>, False]] (descending, mirroring Sankey/buildQuery.ts); with sort_by_metric=False, the orderby key is correctly absent. Row-limited results now keep the heaviest edges. Resolves the codeant sort_by_metric thread.
Correction (2026-08-31). The claim above is inaccurate and I'm amending it rather than deleting it. Setting
form_data["orderby"]is a no-op on the MCP data path:build_query_dicts_from_form_data→_build_single_query_dictnever reads a top-levelorderby(only thedeck_*branch does). What I actually executed was the mapper populating the key — not the query dict consuming it — so "verified" overstated the evidence. The ordering was genuinely unfixed at this head. @gkneighb caught this independently and fixed it properly inb5682e87by emitting theorderbyin the shared query builder; see my round-3 review. Apologies for the noise.
✅ Major — aggregate on source/target — FIXED (verified)
reject_metric_style_nodes now gates on the canonical ColumnRef.is_metric (schemas.py:751) instead of saved_metric alone. RAN:
source={"name":"amount","aggregate":"SUM"} -> REJECTED ("source must be a plain node column, not a metric …")
target={"name":"amount","aggregate":"SUM"} -> REJECTED
source saved_metric=True -> still rejected
basic node config -> still accepted
Resolves the codeant/bito aggregate thread.
✅ Test coverage gap — CLOSED
The +62 test lines add exactly the regression guards that were missing in round 1, and they assert the real behavior (not the old buggy shape):
test_columns_from_form_data_returns_the_node_columns— end-to-end guard that the emitted form_data actually groups by[from_stage, to_stage](the assertion that would have caught the Critical at authoring time);test_resolve_groupby_returns_the_node_columns;groupby/orderbyassertions in the mapping tests, incl. thesort_by_metric=False → no orderbycase;test_sankey_source_rejects_aggregate/test_sankey_target_rejects_aggregate.
Full suite RAN green: 20 passed (was 16). Mentally reverting the two production edits now breaks these guards — they are genuine regression tests, no longer just import-coupled.
Regressions / new issues in the round-2 delta
None. The delta is +14 lines in chart_utils.py, −3/+4 in schemas.py, +62 tests. Keeping source/target in form_data alongside groupby is harmless (the frontend renderer reads source/target; the backend query reads groupby); no double-grouping since the frontend buildQuery constructs its own query object.
Status (re-derived at post time, head cdcfaa65f0)
- CI: 60 unique checks (latest run per name) — 47 success, 11 skipped, 2 neutral, 0 pending, 0 failing; netlify docs-preview StatusContext SUCCESS. Green.
- Mergeability:
mergeable=MERGEABLE,mergeStateStatus=BLOCKED— branch protection (external contributor needs maintainer approval), not CI. - The three codeant threads still show unresolved on GitHub (auto-reanchored to the new head), but all three are addressed in code as shown above — they can be marked resolved.
Nice turnaround — the fixes match the frontend buildQuery contract precisely, and this was the deepest instance of the family's buildQuery-bypass defect. Thanks for adding the end-to-end columns_from_form_data guard; that's the durable protection against this regressing.
Code Review Agent Run #b8a9f1Actionable 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 |
|
@aminghadersohi — correction on my round-2 fix: you're right that the
The GROUP BY fix from round 2 stands; this closes the ordering half. |
Code Review Agent Run #a9bf70Actionable 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.
Pull request overview
Adds MCP generate_chart support for Superset’s shipped Sankey visualization (viz_type: sankey_v2) by introducing a new chart config schema, a form-data mapper, a plugin implementation, and registry/schema/category wiring so Sankey charts can be generated via MCP with the same core query contract as the frontend.
Changes:
- Introduces
SankeyChartConfig(schema + validation) and adds it to theChartConfigdiscriminated union and chart-type schema adapters. - Implements Sankey form-data mapping (
map_sankey_config) and a newSankeyChartPlugin, then registers it in the MCP chart plugin registry. - Adds Sankey to the chart recommendation category map and adds query-dict ordering support for
sort_by_metric.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit_tests/mcp_service/chart/test_sankey_chart.py | Adds unit tests for Sankey schema validation, form_data mapping, query-dict behavior, and registry/schema/category integration. |
| superset/mcp_service/chart/tool/get_chart_type_schema.py | Registers sankey_v2 for schema discovery and provides an example payload. |
| superset/mcp_service/chart/tool/get_chart_data.py | Adds sankey_v2 to the viz-type category map used by the MCP chart tool. |
| superset/mcp_service/chart/schemas.py | Defines SankeyChartConfig and adds it into the ChartConfig union. |
| superset/mcp_service/chart/plugins/sankey.py | Adds the Sankey chart MCP plugin (validation hints, normalization, naming, form_data mapping hook). |
| superset/mcp_service/chart/plugins/init.py | Registers the new Sankey plugin. |
| superset/mcp_service/chart/chart_utils.py | Adds map_sankey_config and Sankey chart naming helper (_sankey_chart_what). |
| superset/mcp_service/chart/chart_helpers.py | Ensures sort_by_metric charts emit query-dict orderby so row limits behave as “top-N by metric”. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| suggestions=[ | ||
| "Ensure 'source' and 'target' each have a 'name'", | ||
| "Ensure 'metric' field has 'name' and 'aggregate'", | ||
| "Example: {'chart_type': 'sankey_v2', " | ||
| "'source': {'name': 'from_stage'}, " | ||
| "'target': {'name': 'to_stage'}, " | ||
| "'metric': {'name': 'users', 'aggregate': 'SUM'}}", | ||
| ], |
| # sort_by_metric charts (pie/funnel/treemap/sankey) order by the metric | ||
| # descending. buildQuery derives this on the frontend; the MCP path builds | ||
| # the query dict directly and never reads a top-level form_data['orderby'], | ||
| # so translate the flag here or a row_limit truncates an unordered result | ||
| # (dropping the heaviest rows rather than the top-N by the metric). |
| def test_sankey_form_data_with_filters_and_no_sort(self) -> None: | ||
| config = SankeyChartConfig( | ||
| chart_type="sankey_v2", | ||
| source={"name": "from_stage"}, | ||
| target={"name": "to_stage"}, | ||
| metric={"name": "users", "aggregate": "SUM"}, | ||
| sort_by_metric=False, | ||
| filters=[{"column": "year", "op": "=", "value": 2026}], | ||
| ) | ||
| form_data = map_sankey_config(config) | ||
| assert form_data["sort_by_metric"] is False | ||
| assert "orderby" not in form_data # no metric ordering when unset | ||
| assert form_data["adhoc_filters"], "filters must map to adhoc_filters" |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Reviewed at b5682e877e. All prior findings fixed and verified here:
- GROUP BY —
map_sankey_configemitsgroupby=[source, target];resolve_groupby/columns_from_form_datayield['from_stage','to_stage']. - Metric ordering — correctly moved out of
form_data(a top-levelorderbyis a no-op on the MCP data path — only thedeck_*branch reads it) into_build_single_query_dict. The built query dict now carries a descendingorderbyon the metric whensort_by_metric=True, and none whenFalse. This closes the ordering half that round-2'sform_data["orderby"]never reached. aggregateonsource/target— still rejected viaColumnRef.is_metric.
Net-new since the last round:
- Merge conflict — the branch is
CONFLICTINGagainstmasterinsuperset/mcp_service/chart/schemas.pyafter #43480 (interactive pivot) landed in the sameChartConfigunion region. A rebase is required before this can merge.
The three open Copilot threads (schema_error_hint wording, the pie/funnel/treemap/sankey comment in _build_single_query_dict — only pie and sankey set sort_by_metric on the MCP path today, and the extra sort_by_metric=False builder assertion) are non-blocking nits; no need to duplicate them here.
Adds a generate_chart plugin for the sankey viz type (viz_type 'sankey_v2'). Mirrors the frontend Sankey buildQuery contract: a source and a target column define the edges of the flow diagram (the query groups by both) and one metric weights each edge. sort_by_metric orders edges by the metric descending. - SankeyChartConfig schema (source + target + metric required; sort_by_metric, row_limit, filters, color_scheme) added to the ChartConfig discriminated union and the get_chart_type_schema adapters. source/target may not be saved_metric/sql_expression (node dimensions, not metrics). - map_sankey_config maps the config to form_data; saved metrics pass through as a bare name string, ad-hoc metrics as SIMPLE/SQL adhoc objects. - SankeyChartPlugin registered in the plugin registry; 'sankey_v2' added to the recommendation category map (it was absent). - 18 unit tests covering schema validation (three required fields, source/target not-a-metric), union dispatch, form_data mapping, and registry integration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
map_sankey_config emitted the frontend control names (source, target) but no groupby. The MCP path builds its query context from form_data instead of running the frontend buildQuery, and both backend builders derive grouping columns from groupby alone -- resolve_groupby carries aliases for entity and series but none for source/target, and columns_from_form_data has no alias at all. Both returned [], so the generated query grouped by nothing and collapsed every edge into a single aggregate row over the whole dataset. _compile_chart accepts that one-row query, so the chart saved and validated cleanly while the preview and get_chart_data output were wrong. Emit groupby explicitly, as every sibling mapper does. source and target are kept because the frontend controls read them; the added key is inert there, since buildQuery recomputes the same value when the chart opens in Explore. Tested against both consumers rather than the emitted key alone, so the assertion fails if either builder stops honoring groupby. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019GCwpgKCvmk8hFeKNfe2hD
…review) Follow-up to the groupby fix, addressing aminghadersohi's review: - map_sankey_config now emits orderby=[[metric, False]] when sort_by_metric is set. buildQuery derives this on the frontend, but the MCP path bypasses buildQuery, so without it a row-limited result can drop the heaviest edges. - reject_metric_style_nodes now gates on ColumnRef.is_metric, so an aggregate on source/target is rejected (previously only saved_metric/sql_expression were), matching the canonical is_metric predicate. - Tests assert groupby AND orderby in the emitted form_data (and that columns_from_form_data yields the node columns), plus negative aggregate tests for source/target. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rm_data (review) Correcting my earlier orderby fix, which aminghadersohi's funnel review showed was a no-op: a top-level form_data['orderby'] is never read by the MCP query builder (only the deck_* path reads one). So sankey's sort_by_metric produced no ordering, and a row_limit could drop the heaviest edges. - Remove the no-op form_data['orderby'] emission from map_sankey_config. - Emit orderby in the shared _build_single_query_dict when sort_by_metric is set (same fix as funnel), so the metric ordering actually reaches the query. - Replace the mapper-level orderby assertion with a query-context test asserting the built query GROUP BYs source+target AND ORDER BYs the metric descending. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
b5682e8 to
6a86a0a
Compare
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Approve — clean, well-tested plugin that follows the established MCP pattern (config → mapper → plugin → registration) and the security model is intact. Two non-blocking Mediums worth addressing:
🟡 Medium — sankey_v2 is wired up but never advertised to the LLM, so it's undiscoverable through the primary tool surface
The type is added to the discriminated union, registry, _CHART_TYPE_ADAPTERS, and _VIZ_CATEGORY, but every prose enumeration an LLM actually reads to pick a chart type still stops at waterfall:
generate_chart.py:88—'histogram', 'box_plot', 'waterfall'(nosankey_v2)app.py:421and the per-type description list ending atapp.py:410(DEFAULT_INSTRUCTIONS)
When waterfall was added it was threaded into all of these. Since the feature's whole purpose is MCP discoverability, this omission undercuts it — dynamic discovery (get_chart_type_schema) will surface it, but the primary generate_chart docstring won't.
Fix: add sankey_v2 to those valid-type lists and give it a one-line per-type description/example, mirroring how waterfall is documented.
🟡 Medium — the query orderby only partially reproduces the frontend buildQuery the docstrings claim it "matches"
chart_helpers.py:497-498 emits orderby = [(metric, False)], and only when sort_by_metric is truthy. But superset-frontend/plugins/plugin-chart-echarts/src/Sankey/buildQuery.ts:29-42 appends [source, true] and [target, true] unconditionally, and applies ordering whenever row_limit is nonzero:
if (sort_by_metric && metric) orderby.push([metric, false]);
[source, target].forEach(column => {
if (column) orderby.push([column, true]); // unconditional asc tiebreakers
});So with sort_by_metric=False the frontend still orders by source ASC, target ASC while the MCP path emits no ORDER BY at all. The divergence is silent and only surfaces when the distinct-edge count exceeds row_limit (default 10000): the truncation then keeps a different, nondeterministic subset of edges than the UI, and even with sort_by_metric=True the boundary ties are broken arbitrarily. That contradicts the map_sankey_config (chart_utils.py:1021) and SankeyChartConfig docstrings, which state the mapping "Matches the frontend Sankey buildQuery contract."
Fix: for sankey_v2, append (source, True) and (target, True) after the metric term (and emit them even when sort_by_metric is False), or soften the "matches the frontend contract" wording to "replicates the metric ordering only." Note the current suite wouldn't catch this — test_sankey_chart.py:227 only asserts orderby[0][1] is False.
Also spotted a few Lows (not blocking): the shared _build_single_query_dict change also alters the existing pie query path with no pie test; sort_by_metric defaults to True and is labeled a "(frontend default)" but the Sankey control sets no default (UI-created sankeys are unchecked); and several plugin methods (normalize_column_refs, generate_name, color_scheme mapping, extract_column_refs, row_limit bounds) are untested. Happy to expand on any of these if useful.
Code Review Agent Run #cfaa97Actionable Suggestions - 0Additional Suggestions - 2
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 |
|
@gkneighb Thanks for fixing the original Sankey grouping and shared-builder ordering issues. A fresh Red Hat product-completeness pass at head
I ran the 21 Sankey tests and the full MCP chart suite (1,388 passed) plus adversarial reproductions; these gaps remain despite green CI. Shared builder, normalization, and query-error work in #43737 should be reused/rebased rather than duplicated, while retaining this PR’s Sankey ordering behavior. I did not modify or push to your branch. |
SUMMARY
Adds a
generate_chartMCP plugin for the sankey chart type (viz_type: sankey_v2), so the MCPgenerate_charttool can produce sankey flow diagrams. Sankey is a shipped Superset viz type that had no MCP plugin; this closes that gap.The plugin mirrors the frontend Sankey
buildQuerycontract: asourceand atargetcolumn define the edges of the flow diagram (the query groups by both) and onemetricweights each edge. Whensort_by_metricis set (the frontend default), edges are ordered by the metric descending.Follows the established plugin pattern (
pie/funnel/gauge/treemap/heatmap/radar/bubble/waterfall) — a config schema, amap_*_configmapper, a plugin class, and registration:SankeyChartConfig—source,target, andmetricrequired; optionalsort_by_metric,row_limit,filters,color_scheme. Added to theChartConfigdiscriminated union and theget_chart_type_schemaadapters.source/targetmay not besaved_metric/sql_expression(node dimensions, not metrics).map_sankey_config— maps the config toform_data; saved metrics pass through as a bare name string, ad-hoc metrics as SIMPLE/SQL adhoc objects.SankeyChartPlugin— registered in the plugin registry.sankey_v2was absent from the recommendation category map, so this adds it (its ownsankeycategory).The field set is intentionally minimal (core query contract).
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend/MCP only, no UI change.
TESTING INSTRUCTIONS
pytest tests/unit_tests/mcp_service/chart/test_sankey_chart.py— 18 unit tests cover schema validation (three required fields,source/targetnot-a-metric, extra-field rejection),ChartConfigunion dispatch,form_datamapping (source/target/metric/sort/filters/saved-metric), and registry integration (registration,resolve_viz_type,display_name,pre_validate, recommendation category).To exercise end-to-end: call the
generate_chartMCP tool with{"chart_type": "sankey_v2", "source": {"name": "from_stage"}, "target": {"name": "to_stage"}, "metric": {"name": "users", "aggregate": "SUM"}}.ADDITIONAL INFORMATION
🤖 Generated with Claude Code