Repository navigation
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #43571 +/- ##
===========================================
- Coverage 81.97% 66.11% -15.86%
===========================================
Files 2988 2989 +1
Lines 184142 184223 +81
Branches 42565 42579 +14
===========================================
- Hits 150952 121808 -29144
- Misses 30427 59765 +29338
+ Partials 2763 2650 -113
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:
|
1dd87ab to
28a7652
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| metrics: List[ColumnRef] = Field( | ||
| ..., | ||
| min_length=1, |
There was a problem hiding this comment.
Suggestion: The schema accepts a single metric, even though the radar plugin describes radar charts as requiring two or more axes. This allows a one-axis configuration to pass validation and reach chart generation as a degenerate radar; enforce the same minimum metric count in the schema and plugin contract. [api mismatch]
Severity Level: Major ⚠️
- ❌ MCP can generate degenerate one-axis radar charts.
- ⚠️ Radar requests contradict the plugin’s documented two-axis contract.
- ⚠️ Users receive an unusable visualization instead of validation guidance.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/schemas.py
**Line:** 1060:1062
**Comment:**
*Api Mismatch: The schema accepts a single metric, even though the radar plugin describes radar charts as requiring two or more axes. This allows a one-axis configuration to pass validation and reach chart generation as a degenerate radar; enforce the same minimum metric count in the schema and plugin contract.
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 fixThere was a problem hiding this comment.
I looked into this one and I think min_length=1 is correct as-is, so I would resolve this without the arity change. The Explore control panel for Radar uses the shared metrics control, whose only validator is validateNonEmpty (superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/dndControls.tsx), so the UI itself accepts a single-metric radar. Bumping the MCP schema to min_length=2 would make the API stricter than the frontend it mirrors. I also generated a single-metric radar on this branch over MCP and it validates and maps fine.
@gkneighb the part of this thread worth acting on is the wording drift: pre_validate says "Add two or more 'metrics'" and schema_error_hint says "at least one", while the schema enforces one. Aligning all three to "at least one metric; radars read best with 3 or more" keeps the guidance consistent with the contract and with Explore. Not a blocker.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
For radar chart validation, allow at least one metric and keep the schema, pre-validation message, and schema error hint consistent; describe multiple metrics as a readability recommendation rather than a requirement.
Applied to:
superset/mcp_service/chart/**
💡 To manage or update this instruction, visit: CodeAnt AI Settings
|
The flagged issue is correct. The Would you like me to implement these changes and check the rest of the PR comments for similar issues? superset/mcp_service/chart/schemas.py superset/mcp_service/chart/plugins/radar.py |
Code Review Agent Run #dc4bdfActionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
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.
Thanks for this — the radar plugin follows the established MCP chart-plugin template cleanly and is a pleasure to review. RadarChartConfig, map_radar_config, the _radar_chart_what namer, the discriminated-union wiring, the TypeAdapter registration in get_chart_type_schema.py, and the plugin registration are all consistent with the pie/funnel/gauge siblings, and the field descriptions surface well to an agent. Registry integration and form_data mapping are correct, and the tests exercise the happy paths thoroughly. This is a first human review; nothing below blocks the design, and most of it is a shared convention worth applying across all seven sibling PRs (funnel/gauge/treemap/heatmap/radar/bubble/sankey) at once.
I ran the unit tests (15 passed) and constructed configs directly to verify each claim below. RAN = executed, INSPECTED = read-only.
1. Radar-specific — the schema permits a 1-metric radar its own docs call invalid (confirms codeant)
RAN. A single-metric radar passes every gate and reaches form_data generation:
RadarChartConfig(chart_type='radar', metrics=[{'name':'speed','aggregate':'AVG'}]) # -> valid, len 1
plugin.pre_validate({'chart_type':'radar','metrics':[{'name':'speed','aggregate':'AVG'}]}) # -> None (accepted)
map_radar_config(...) # -> {'viz_type':'radar','metrics':['AVG(speed)'], ...} (degenerate single-spoke radar)
The codeant finding is confirmed, and it is sharper than "seems wrong" because the plugin contradicts its own documented contract:
pre_validatedetails (radar.py): "Radar charts plot one axis per metric. Add two or more 'metrics'"- schema field (
schemas.py:1060): "radars read best with 3 or more" - but the enforcement is
metrics: min_length=1, andpre_validateonly rejects empty metrics (if not config.get("metrics")). - and
schema_error_hintthen says the opposite: "Ensure 'metrics' has at least one metric".
So three doc strings say ≥2/≥3 and one says ≥1, while the code enforces ≥1. Downstream this does not throw — form_data generates fine and ECharts renders a degenerate one-axis radar (a spoke, not a polygon). So this is a quality/UX issue (an unusable chart instead of validation guidance), not a crash or a security issue — I'd call it Minor–Major rather than the bot's flat Major. The fix is one line in the existing @model_validator(mode="after"):
if len(self.metrics) < 2:
raise ValueError("radar charts need at least 2 metrics (axes); a single axis is degenerate")and align the min_length=1 / schema_error_hint wording to match.
2. Template-level — "schemas encode field types but not semantic constraints" is confirmed across the family
I read the schema diffs of all six siblings. Every config class already carries a @model_validator(mode="after") — but in every case it does exactly one job: reject a metric-shaped entry sitting in a dimension slot (reject_metric_style_groupby / reject_metric_style_dimensions / reject_metric_style_nodes). None of them encode the chart's semantic constraint:
| PR | documented constraint | enforced? |
|---|---|---|
| #43568 gauge | min_val < max_val |
no (ordering unchecked) |
| #43569 treemap | aggregate marks a metric, not a groupby |
flagged by codeant |
| #43571 radar | ≥2 metric axes | no (this PR) |
So the synthesis holds: the validators check what kind of field each ref is, never the chart-level invariant (arity, ordering). The good news is the fix is cheap and uniform — the mode="after" validator hook is already present in all seven classes; each just needs its one semantic assertion added. A shared convention here saves this author six more rounds. The radar arity check is radar-specific; the pattern of adding it is template-level.
3. Template-level — the seven PRs will merge-conflict on shared files
INSPECTED (gh api .../files patches). Every sibling inserts at the identical anchors:
chart_utils.py:map_<type>_configright aftermap_pie_config(@@ -1017,6), and_<type>_chart_whatafter_pie_chart_what(@@ -1527,6) — all seven target the same two hunks.schemas.py: a new class afterPieChartConfig, plus edits to the sameChartConfigunion list and the same discriminator description string.get_chart_type_schema.py: same_CHART_TYPE_ADAPTERS/_CHART_EXAMPLESblocks.
These are not the "same helper duplicated" — each function is genuinely distinct (per-chart mapping). But because they land on the same lines, whichever merges first will force mechanical rebases on the other six. Worth sequencing the merges, or landing the shared scaffolding once.
4. Radar-specific — the get_chart_data.py +1 line is benign (and effectively a no-op)
INSPECTED. The one line adds "radar": "radar" to _VIZ_CATEGORY, used only by _recommend_visualizations via current_category = _VIZ_CATEGORY.get(viz_type, viz_type) to avoid recommending a chart type the user already has. Two notes:
- It's why radar alone touches this file: the siblings' viz_types (
gauge_chart,funnel,treemap_v2,heatmap_v2) were already in the map;radarwas not. - Because the lookup already defaults to
viz_typeitself,.get("radar", "radar")returns"radar"without this entry — so the line is functionally a no-op. It's harmless and matches the file's self-documenting convention (funnel,waterfall,box_plotalso self-map), so no change needed; just noting it affects no other chart type and is unrelated to the separateget_chart_dataguard being fixed in #43598.
5. Rule 26 — tests are happy-path only; no arity negative test
RAN / INSPECTED. Reverting only the production files, the whole test_radar_chart.py fails to import (RadarChartConfig, map_radar_config don't exist), so the tests are genuinely coupled to the production code. But there is no negative test for the core defect — and worse, several tests positively assert that a 1-metric radar is valid (test_chart_config_union_dispatches_radar, test_radar_form_data_with_series_and_filters, test_radar_metric_accepts_saved_metric all build single-metric configs and expect success). So the suite currently locks in the behavior its own plugin docs call invalid. Once the ≥2 rule lands, add test_radar_single_metric_rejected and flip those fixtures to 2+ metrics. Bad-column handling is inherited from the shared DatasetValidator.get_canonical_column_name, which returns the original name on no match (silent pass-through) — same as the other plugins, not introduced here.
Summary
- The synthesis ("types enforced, semantics not") is confirmed with code across all seven.
- Radar-specific: add a ≥2-metric check to the existing
model_validator; align the contradictory doc strings; add a negative arity test and fix the single-metric happy-path fixtures. - Template-level: apply the same "one semantic assertion in the existing
mode=aftervalidator" convention to the family; expect (mechanical) merge conflicts onchart_utils.py/schemas.py/get_chart_type_schema.pyacross the seven. - The
get_chart_data.pyline is a benign self-map (no-op given the identity default); the single deleted line inschemas.pyis just the discriminator description string gaining'radar'.
The template is sound and worth the other six following — the main gap is that the semantic invariant belongs in the validator, not just the docstring. Nice work; happy to re-look once the arity check lands.
CI at 28a7652d, deduped by latest run per check name: 47 SUCCESS (incl. netlify preview) / 3 NEUTRAL / 12 SKIPPED, no failures or pending.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Round 2 — re-reviewed at head 5160b0e5. Verified by pinning the worktree to that SHA (clean tree) and running the unit tests (16 passed); RAN = executed, INSPECTED = read-only.
The new commit is a good fix ✅
fix(mcp): radar — reject aggregate on groupby series columns does exactly the right thing:
- RAN.
groupby=[{'name':'model','aggregate':'SUM'}]now raisesValidationError(before this commit it validated and theaggregatewas silently dropped downstream). The switch fromif col.saved_metric:toif col.is_metric:is the correct fix — it reuses the existingColumnRef.is_metricproperty (schemas.py:751), which unifies all three metric markers (aggregate/saved_metric/sql_expression) instead of checking them one at a time. That's the same class of gap worth closing in the sibling PRs'reject_metric_style_*validators —is_metricis the shared, already-in-repo primitive to standardize on across the family. - The error message now names both
aggregateandsaved_metric, and the newtest_radar_groupby_rejects_aggregatelocks the behavior in. Clean, minimal, well-tested.
One item from round 1 is still open — the single-metric / doc-string arity gap
This is codeant's original thread on schemas.py:1062 (still unresolved, not outdated), and it's a distinct issue from the aggregate fix above — so the new commit doesn't cover it. Re-confirmed RAN at 5160b0e5:
RadarChartConfig(chart_type='radar', metrics=[{'name':'speed','aggregate':'AVG'}]) # -> valid, len 1
plugin.pre_validate({'chart_type':'radar','metrics':[{'name':'speed','aggregate':'AVG'}]}) # -> None (accepted)
The plugin's own three contract strings still disagree on the minimum, and the code enforces the loosest of them:
pre_validatedetails (radar.py): "Add two or more 'metrics'"metricsfield description (schemas.py:1062): "radars read best with 3 or more"schema_error_hint(radar.py): "Ensure 'metrics' has at least one metric"- enforced:
min_length=1+pre_validaterejects only empty → 1 is accepted.
A one-axis radar is geometrically degenerate (a spoke, not a polygon); it doesn't throw, it just produces an unusable chart instead of validation guidance. This is a Minor–Major quality issue, not a blocker. The same mode="after" validator you just edited is the natural home for the fix:
if len(self.metrics) < 2:
raise ValueError("radar needs at least 2 metrics (axes); one axis is degenerate")and align the min_length and the three doc strings to one agreed minimum. A negative test (test_radar_single_metric_rejected) plus flipping the current single-metric happy-path fixtures to 2+ metrics would round it out.
Nothing else regressed — radar.py, chart_utils.py, get_chart_data.py, get_chart_type_schema.py, and plugins/__init__.py are unchanged since round 1, and my earlier notes on those still hold (the get_chart_data.py _VIZ_CATEGORY self-map is a benign no-op; the family will still merge-conflict mechanically on the shared insertion anchors). Thanks for the quick turnaround on the aggregate fix.
CI at 5160b0e5, deduped to the latest run per check name: 48 SUCCESS (incl. netlify preview) / 2 NEUTRAL / 9 SKIPPED, no failures or pending.
Code Review Agent Run #759fb1Actionable 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 |
6fadbfc to
e168736
Compare
Code Review Agent Run #8d76d0Actionable 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 |
|
Hi again... I'm being a broken record, but not sure if we should merge this or wait for updates. LMK, @aminghadersohi / @gkneighb |
github-actions
Bot
added
the
requires:rebase
e168736 to
29f350b
Compare
github-actions
Bot
removed
the
requires:rebase
Code Review Agent Run #27b1abActionable 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 |
|
Rechecked 29f350b against the refreshed base. The radar implementation/tests are unchanged from e168736; material integration gaps remain:
Focused tests: 250 existing tests passed, including all 16 radar tests, plus 7 temporary probes confirming the behaviors above. Please add radar product-path regressions for query generation, native request/update preservation, previews, and effective-aggregate validation; mapper/registry tests alone miss these. No live-server test was possible (localhost:8088 unavailable). |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Hey @gkneighb, re-checked at the current head after the rebase: the groupby is_metric fix and its regression-valid test still hold, tests/unit_tests/mcp_service/chart/ passes in full locally (1403 passed), and CI is green.
One remaining ask before merging, because for an MCP plugin the tool descriptions are the interface: radar is still missing from the generate_chart docstring, and after the gauge plugin merged this gap is now visible side by side. In superset/mcp_service/chart/tool/generate_chart.py:
- the "one of:" list (lines 88-91) includes
'gauge_chart'but not'radar', so a client following the tool description will conclude radar is invalid and never send it, even though the union accepts it - gauge and waterfall each have a "Use chart_type=... for ..." bullet; radar needs one
- the quick-lookup block (around line 152) maps "gauge" / "dial" / "speedometer" but has no "radar chart" / "spider chart" line
On superset/mcp_service/app.py: the "Chart Types You Can CREATE" list and the registry-membership sentence (lines 422-427) omit radar too, but I noticed the merged gauge PR did not update them either, so that block is already drifted on master (line 427 says a Gauge chart's display name will be null, and it is not anymore). Happy either way: add radar's entries here, or leave that block for one sweep across the plugin family in a follow-up. Would a small consistency test that iterates VALID_CHART_TYPES and asserts each appears in the generate_chart docstring be worth adding while you are in there? It would have caught this for gauge and radar both.
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Round 4 at 29f350b (unchanged since my last pass; nothing has been pushed).
Answering @rusackas directly: merge, don't wait — after one small docs commit. In my judgement the only genuinely merge-blocking item on this PR is @gabotorresruiz's generate_chart.py docstring gap, which is documentation-only and unaddressed. Of the four integration findings I raised earlier, three are pre-existing, family-wide MCP characteristics that the already-merged gauge plugin shares — they are follow-ups, not radar defects, and holding this PR for them would be holding it for architecture it didn't introduce. One (query ordering) is radar-specific and genuinely wrong, but it is a one-line fix that I have tested; I'd take it here rather than defer it. The plugin itself is idiomatic, mirrors the gauge plugin structurally, CI is green on both surfaces (50 SUCCESS / 0 failure / 0 pending, deduped latest-per-name across check-runs and StatusContexts), and tests/unit_tests/mcp_service/chart/ is 1403 passed locally (RAN), matching @gabotorresruiz's number exactly.
Note the branch is 39 commits behind master (3 ahead) — worth a rebase before merge, though nothing in the drift affects the findings below.
Status of my four earlier findings — already published, not new
These were posted in #43571 (comment) at this same SHA. @gkneighb, this is a status re-test, not a second bill — no new work is being asked of you beyond what that comment already said. All four still hold; here is each one re-tested with its severity re-scored now that I can compare against merged siblings.
1. Query ordering — STILL HOLDS (RAN). The one I would fix before merge.
map_radar_config (chart_utils.py:1078-1086) emits no orderby, no sort_by_metric, no series_limit_metric. Feeding its output to _build_single_query_dict produces row_limit: 10 with orderby: None, so a radar with more than 10 categories keeps an arbitrary 10 polygons rather than the top 10.
What sharpens this since last round: the PR's own docstrings claim the opposite. chart_utils.py:1076 and schemas.py:1130 both say "The query orders by the first metric descending" — the code does not. That is an internal contradiction, not just a divergence from Radar/buildQuery.ts:33-39.
The mechanism already exists — chart_helpers.py:508-514 translates sort_by_metric into orderby, and its comment enumerates "pie/funnel/treemap/sankey/gauge", with radar absent. I RAN the candidate fix: adding "sort_by_metric": True to map_radar_config yields orderby = [('AVG(speed)', False)] — first metric descending, byte-for-byte the frontend's buildQuery.ts:38 behaviour. One line plus a regression test, and it makes the two docstrings true.
2. Native round-trip / update preservation — STILL HOLDS (RAN + INSPECTED), but downgrade to follow-up.
RAN: mapper output fed back into GenerateChartRequest fails with 2 validation errors, and a saved-metric string (metrics: ["count"]) is rejected. INSPECTED: update_chart.py:249 writes "params": json.dumps(new_form_data) wholesale; the only preservation hooks are merge_table_column_config and merge_interactive_pivot_ui_config (:237-238).
I want to correct the emphasis I gave this last round. Those two helpers are the only merge helpers in the MCP update path, so every plugin — gauge, pie, waterfall, funnel — drops native-only keys on update, not just radar. This is generic MCP update behaviour that shipped with the merged gauge plugin. It is worth an issue for the family, but it is not a reason to block radar. The round-trip half I'd weight lower still: the MCP config schema is a deliberately simplified agent-facing contract, and round-tripping native form_data back through it isn't a supported operation.
3. Preview fidelity — STILL HOLDS (RAN). Follow-up.
preview_utils.py:479-490's viz_to_mark has no radar key, so :492 falls through to mark = "bar". RAN on a two-metric radar: the spec comes back 'mark': 'bar' with encoding containing only y: AVG(speed) — the second axis AVG(power) is absent entirely. There are zero occurrences of radar in that file. Same shape as other unmapped types, so again family-wide; a faithful spec or an explicit "preview unsupported" would both be improvements.
4. Validation / recommendation coverage — STILL HOLDS (RAN + INSPECTED). Low.
RAN: metrics: [{"name": "model"}] maps to {'aggregate': 'SUM', 'label': 'SUM(model)', ...} — a bare column name silently acquires SUM, which on a VARCHAR column is meaningless. INSPECTED: the _VIZ_CATEGORY["radar"] entry at get_chart_data.py:158 feeds only the suppression path at :201; _build_candidates and its four helpers never emit "radar", so the entry is currently inert. That said, it is consistent with the map's stated purpose ("avoid recommending a chart type the user already has") and harmless — _candidates_multi_numeric is where radar would naturally belong if you ever want it recommended.
@gabotorresruiz's docstring gap — confirmed, and the fix is in a different file than the last commit touched
Every line reference in your review checks out at this SHA, and I want to flag a mismatch that I think explains the confusion:
The most recent commit on this branch is docs(mcp): list radar in get_chart_type_schema docstring (review) — it added radar to get_chart_type_schema.py:314. Your ask is about generate_chart.py, which the changed-file list confirms this PR does not touch at all. So the docs fix landed in a neighbouring docstring, not the one you flagged. Concretely (INSPECTED):
generate_chart.py:88-91— the "one of:" list has'gauge_chart', no'radar'generate_chart.py:127(gauge) and:139(waterfall) havechart_type=...bullets; radar has nonegenerate_chart.py:153—"gauge" / "dial" / "speedometer"; no"radar chart"/"spider chart"linegrep -ci radar generate_chart.py→ 0, andgrep -ci radar app.py→ 0
Your app.py drift claim also checks out. app.py:424-425's registry list omits both radar and gauge_chart, and :427 still names Gauge as a null-display-name type — but plugins/gauge.py:39-42 sets chart_type = "gauge_chart", display_name = "Gauge Chart", so that sentence is factually wrong on master today. Agreed this is pre-existing drift from the gauge merge and belongs in a family-wide sweep, not here.
On your consistency-test question — yes, and I prototyped it (RAN). Six lines comparing VALID_CHART_TYPES against generate_chart.__doc__ produces, right now:
VALID_CHART_TYPES: ['big_number', 'box_plot', 'gauge_chart', 'handlebars', 'histogram',
'interactive_pivot', 'mixed_timeseries', 'pie', 'pivot_table',
'radar', 'table', 'waterfall', 'xy']
MISSING: ['radar']
Exactly one entry, no false positives — interactive_pivot is already mentioned so the host-gated type doesn't trip it. It would have caught gauge too. Strongly worth adding.
The codeant arity thread — I agree with resolving as-is, on a corrected citation
@gabotorresruiz, I checked dndControls.tsx rather than taking the conclusion on faith, and your substance is right but the line pointer is off: :145 is dndEntityControl. The control Radar actually uses is dndAdhocMetricsControl at dndControls.tsx:172-190, bound as the shared metrics control at sharedControls.tsx:471 and pulled in by Radar/controlPanel.tsx:99. Its only validator is validateNonEmpty (:178) and its own description reads "Select one or many metrics". So Explore does accept a single-metric radar, and your conclusion stands on the correct line.
I'll add the reason I find this decisive rather than merely a parity preference. A one-metric radar is visually degenerate — one axis is one spoke — so @codeant-ai-for-open-source isn't wrong about the outcome. But raising the MCP floor to min_length=2 would mean an agent cannot reproduce a chart a human can build in Explore, which is a worse failure than an ugly chart. And for an agent specifically, the schema field description is the better lever: an LLM reads schemas.py:1139-1140 at planning time, whereas a hard 422 only teaches it after a failed call. Soft guidance in the description beats a hard floor here. Resolve without the arity change.
The wording drift is real and is the actionable residue (INSPECTED). Three strings, and it turns out only one needs editing:
schemas.py:1138—min_length=1, with:1140already reading "one axis per metric; radars read best with 3 or more" — this is already exactly the phrasing you proposedplugins/radar.py:124— "Ensure 'metrics' has at least one metric" — consistentplugins/radar.py:54-55— "Add two or more 'metrics'" — the sole outlier
Worth noting the impact is copy-only, not behavioural: pre_validate guards if not config.get("metrics"), so that "two or more" message only ever renders when zero metrics were supplied. Aligning radar.py:54-55 to the schema's existing wording is a one-line change and closes the thread.
Test quality — the fix is genuinely pinned (RAN)
I ran the revert experiment rather than trusting the green suite. Reverting only RadarChartConfig.reject_metric_style_groupby (schemas.py:1162-1173) while keeping all tests fails exactly two: test_radar_groupby_rejects_aggregate and test_radar_groupby_rejects_saved_metric. The regression is real, not incidentally-green. The remaining 14 are mapper/registry coverage — which is why none of findings 1-4 above are caught by this suite, and why product-path regressions for query generation and previews would be the highest-value additions.
I also confirmed all three commits on this branch are authored by @gkneighb.
Summary for merge: blocking = the generate_chart.py docstring entries. Strongly recommended in the same commit = sort_by_metric for ordering, and the radar.py:54-55 wording. Everything else is a family-wide follow-up that shouldn't hold this PR. I can't approve here — flagging for @aminghadersohi / @gabotorresruiz, and noting my earlier approval predates the rebase and no longer points at a commit on this branch.
|
Heads-up for anyone reviewing the red check here: the three failing checks are not from this diff. All three come from the same six tests in Everything else on this run is green (61 passed, 3 failed, 0 cancelled), and the branch is rebased onto master and mergeable. |
41d0a91 to
ba60bc8
Compare
Code Review Agent Run #61bb1aActionable Suggestions - 0Additional Suggestions - 2
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
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 |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks @gkneighb. Re-reviewed at ba60bc8c28, since the head moved after my approval at e08d75c5d5.
Holding for two lines the rebase brought in, both of them deletes, both inline. Nothing about the radar plugin itself changed.
The delta is a clean replay of the same single commit from daf41bdf04 onto c9fd9bf94f. I compared the full set of lines the PR adds on both sides, and the only differences are the chart type lists and the sort_by_metric comment reflowing around gantt and treemap_v2. plugins/radar.py and test_radar_chart.py are byte identical between the two.
What I ran at this head:
tests/unit_tests/mcp_service/chart/: 1878 passed, 1 skipped, and CI is green.- The ordering fix survives the Gantt rewrite of
_build_single_query_dict: the mapper'sform_datastill yieldsorderby=[('AVG(speed)', descending)], and an explicitorderbyinform_datastill wins oversort_by_metric, which is what the new guard is there for. - A real create and a real preview through the tool layer against a live SQLite dataset:
generate_chartsaved aradarchart with three metric axes andgroupby=['model'], and the executed query returned one row per model.
Re-request me once those two lines are gone and I will re-approve right away.
| - "treemap" / "hierarchy" -> chart_type='treemap_v2' | ||
| - "gauge" / "dial" / "speedometer" -> chart_type='gauge_chart' |
There was a problem hiding this comment.
This one bothers me a bit more than a stray duplicate. The rebase resurrected the pre-rename gauge line, so the quick lookup now maps the same ask twice: to chart_type='gauge' on line 166 and to chart_type='gauge_chart' here. gauge_chart survives only as an input alias (schemas.py:1196), and resources/chart_configs.py:329 tells clients "Use the public chart_type='gauge'; gauge_chart is the native viz_type", so this line points them at the one we do not want them to send.
Read off the live tool description, not the diff: at this head generate_chart carries both lines, c9fd9bf94f carries only the gauge one, and e08d75c5d5 carried only the gauge_chart one, which was correct for that older base. Nothing radar specific, just rebase fallout.
| - "treemap" / "hierarchy" -> chart_type='treemap_v2' | |
| - "gauge" / "dial" / "speedometer" -> chart_type='gauge_chart' | |
| - "treemap" / "hierarchy" -> chart_type='treemap_v2' |
| interactive_pivot. | ||
| pivot extension also expose interactive_pivot. |
There was a problem hiding this comment.
The rebase left master's old tail behind here: the new wording already closes the sentence with interactive_pivot. on the line above, so it is now printed twice.
I checked what clients actually receive rather than reading the diff. Booting the MCP app and reading the live description for get_chart_type_schema shows both lines. It is absent from e08d75c5d5, the commit I approved, and absent from c9fd9bf94f, so it is new at this head.
| interactive_pivot. | |
| pivot extension also expose interactive_pivot. | |
| interactive_pivot. |
github-actions
Bot
added
the
requires:rebase
ba60bc8 to
79bde84
Compare
|
@gabotorresruiz Both lines gone at The The duplicated docstring tail had the same root cause as on #43570: the conflict region ended mid-sentence, master's tail line sat outside the markers, and my replacement re-closed the sentence. I audited all four branches in this family; the duplicate was on every one, and all are now clean. I built on Re-verified at this head: only one |
github-actions
Bot
removed
the
requires:rebase
![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 #5ce0b2
Actionable Suggestions - 1
-
superset/mcp_service/chart/plugins/radar.py - 1
- Inconsistent model_dump usage · Line 93-93
Additional Suggestions - 3
-
superset/mcp_service/chart/plugins/radar.py - 2
-
Missing method docstrings · Line 45-48New methods `pre_validate`, `extract_column_refs`, `to_form_data`, `generate_name`, `resolve_viz_type`, `normalize_column_refs`, and `schema_error_hint` lack docstrings, unlike the documented protocol in `plugin.py`. BITO.md adaptive rule 12147 requires docstrings on all new Python functions. Add brief docstrings describing each override's behavior.
-
Replace Any with specific type · Line 69-69Replace `typing.Any` with specific types in method signatures. Affects `extract_column_refs`, `to_form_data`, `generate_name`, `resolve_viz_type`, and `normalize_column_refs`. This is the first occurrence.
-
-
superset/mcp_service/chart/schemas.py - 1
-
Duplicated groupby validator logic · Line 1714-1725This validator's loop body is the third near-verbatim copy of the same check in this file: `GaugeChartConfig.reject_metric_style_groupby` (lines 1390-1408) and `TreemapChartUpdateConfig.reject_metric_style_groupby` (lines 1555-1576) already run the identical `_reject_sql_expression_on_dimension(col, f"groupby[{i}]")` + `col.is_metric` rejection, differing only in the metric field name quoted in the message. The copies have already diverged once (Gauge additionally checks name-uniqueness), which is the classic cost of this duplication. Extracting a helper parameterized on the metric field label keeps the four chart configs from drifting further.
-
Review Details
-
Files reviewed - 9 · Commit Range:
79bde84..79bde84- superset/mcp_service/chart/chart_helpers.py
- superset/mcp_service/chart/chart_utils.py
- superset/mcp_service/chart/plugins/__init__.py
- superset/mcp_service/chart/plugins/radar.py
- superset/mcp_service/chart/schemas.py
- superset/mcp_service/chart/tool/generate_chart.py
- superset/mcp_service/chart/tool/get_chart_data.py
- superset/mcp_service/chart/tool/get_chart_type_schema.py
- tests/unit_tests/mcp_service/chart/test_radar_chart.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 an incremental AI Review. -
/review full- 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
| def resolve_viz_type(self, config: Any) -> str: | ||
| return "radar" | ||
|
|
||
| def normalize_column_refs(self, config: Any, dataset_context: Any) -> Any: |
There was a problem hiding this comment.
normalize_column_refs uses config.model_dump() while 12 of 15 sibling plugins (e.g. PieChartPlugin.normalize_column_refs, pie.py:99) use model_dump(exclude_unset=True). Dumping defaults materializes them before RadarChartConfig.model_validate, so the round-tripped config can diverge from user input if a field gains a non-round-trippable default. Match the sibling pattern.
Code Review Run #5ce0b2
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Hey @gkneighb, thanks for turning the rebase around so quickly.
Both lines from my last review are gone, and I checked it the same way they were found in the first place rather than by reading the diff. I booted the MCP app at this head and at the commit this branch sits on (298aa2f0ba) and diffed the description plus the input schema of all 72 registered tools. The only differences are the intended radar additions: generate_chart now carries exactly one gauge quick lookup line, and the get_chart_type_schema description closes on interactive_pivot. exactly once. That change request is cleared.
I am holding on two new things, both inline and both small. Apologies for arriving with these after a rebase round; neither is visible in the diff or in the unit suite, they only showed up once I drove the tools against a live dataset. The plugin itself reads well and the sort_by_metric ordering fix you added still holds.
What I ran at 79bde847e4: tests/unit_tests/mcp_service/chart/ in full (2159 passed, 3 skipped); test_radar_chart.py against the parent commit, where it fails to collect, as expected for a brand new plugin; a mutation run that deletes the sort_by_metric line from map_radar_config, which does fail TestRadarQueryContext, so that pin is real and survived the rebase; and a throwaway SQLite dataset driven through generate_chart, get_chart_data, update_chart and both preview formats, with groupby and without, with row_limit=2, and with a saved metric as an axis. The row_limit=2 case does come back ordered by the first metric descending.
CI is green at this head, 64 passing and 0 failing.
| "'aggregate'/'saved_metric' (metrics belong in the 'metrics' " | ||
| "field)" | ||
| ) | ||
| return self |
There was a problem hiding this comment.
Separate from the min_length thread above, and I think the more consequential of the two things I am holding for.
metrics is a metric slot, but nothing requires an entry to be metric shaped. A bare {"name": "..."} reaches create_metric_object and picks up an implicit SUM (chart_utils.py:1146). The ColumnRef itself still has aggregate=None, so is_metric stays False and _validate_aggregations skips the ref, and the non-numeric check never runs.
Verified on this branch against a live SQLite dataset where model is a VARCHAR column:
radar metrics=[{"name": "model"}] -> validate_against_dataset True
chart saves, get_chart_data returns SUM(model) = [0.0, 0.0, 0.0]
bubble x={"name": "model"} -> validate_against_dataset False
"Cannot apply SUM to non-numeric column 'model' (type: VARCHAR)"
So radar renders a polygon of zeros where bubble refuses with a clear message. BubbleChartConfig.record_implicit_metric_aggregate sits directly above this class (schemas.py:1660-1673) and its docstring describes exactly this case, so radar wants the same validator. I ran this version: validate_against_dataset then returns the same invalid_aggregation error bubble gives, and all 17 radar tests plus test_chart_schemas.py, test_chart_utils.py and tool/test_get_chart_type_schema.py still pass (306 passed).
| return self | |
| return self | |
| @model_validator(mode="after") | |
| def record_implicit_metric_aggregate(self) -> "RadarChartConfig": | |
| """Each axis is a metric slot, so a bare column is summed.""" | |
| self.metrics = [ | |
| col if col.is_metric else col.model_copy(update={"aggregate": "SUM"}) | |
| for col in self.metrics | |
| ] | |
| return self |
A test_radar_bare_axis_records_sum next to test_radar_groupby_rejects_aggregate would lock it in.
| return "radar" | ||
|
|
||
| def normalize_column_refs(self, config: Any, dataset_context: Any) -> Any: | ||
| config_dict = config.model_dump() |
There was a problem hiding this comment.
This one is worth fixing before merge.
normalize_column_refs dumps without exclude_unset=True, so the config it hands back has every field in model_fields_set, including the ones the caller never sent. update_chart then swaps parsed_config for that normalized config (tool/update_chart.py:904-908), and merge_chart_form_data preserves color_scheme and row_limit only when they are absent from model_fields_set (chart_utils.py:1011-1019). So for radar the preservation never fires.
I verified it on this branch against a live SQLite dataset: saved a radar chart with row_limit=3 and color_scheme='googleCategory10c', then called update_chart changing only the metric aggregate.
radar BEFORE row_limit=3 color_scheme=googleCategory10c
radar AFTER row_limit=10 color_scheme=supersetColors
pie BEFORE row_limit=3 color_scheme=googleCategory10c
pie AFTER row_limit=3 color_scheme=googleCategory10c
Pie keeps both because PieChartPlugin.normalize_column_refs (pie.py:99) passes exclude_unset=True. With the one word added here radar keeps them too, canonical name resolution still works (SPEED still resolves to speed), and all 17 radar tests still pass.
| config_dict = config.model_dump() | |
| config_dict = config.model_dump(exclude_unset=True) |
For the test, test_merge_chart_preserves_omitted_defaults in tests/unit_tests/mcp_service/chart/test_chart_utils.py already pins this invariant for pie and runs normalize_column_names first, so a radar twin belongs right next to it. I wrote one and it fails on this head for the omitted case and passes with the word added.
Two things so this does not read as a moving target. bubble.py:106 has the identical form and the identical behaviour on master today, so you did not introduce the shape, and I am happy to send the bubble fix myself. And bito flagged this line for a different, hypothetical reason, so this is the concrete consequence rather than a second copy of that point.
79bde84 to
05e2455
Compare
05e2455 to
30cde3a
Compare
|
@gabotorresruiz Both fixed at Implicit aggregate on the axes. Fixed with the same validator bubble carries; you were right that its docstring already described this case. Reproduced first through the plugin's own
On the bubble twin — I have taken that one rather than leaving it with you: #44618, same one-word fix plus a bubble version of the same test. Thanks for the offer, and for separating the concrete consequence from the bot's hypothetical version of the same line; that distinction is what made it obvious which change was actually needed. Verified at this head: 17 radar tests passing plus the 2 new ones, the |
Code Review Agent Run #b74f78Actionable Suggestions - 0Additional Suggestions - 10
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
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 looks like the implicit-aggregate and exclude_unset fixes from @gabotorresruiz's review landed in the last push, nice. The two rebase leftovers he flagged are still there though: the duplicate |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Hey @gkneighb, both of these are fixed, and the way you wrote them up made re-verification quick. Approving, which clears my change request.
I re-ran the same probes that produced the two findings rather than reading the diff. On the implicit aggregate: a bare {"name": "model"} axis over a VARCHAR column now comes back with success: false and invalid_aggregation, "Cannot apply SUM to non-numeric column 'model' (type: VARCHAR)", identical to what bubble returns for the same input, where the previous head saved the chart and served [0.0, 0.0, 0.0]. On exclude_unset: a saved radar chart with row_limit=3 and color_scheme='googleCategory10c', put through a real persisted update_chart that mentions neither, keeps both and still applies the metric change, matching pie. I also checked the two fixes compose rather than quietly undoing each other: model_fields_set still holds only the fields the caller actually sent (['groupby', 'metrics'] in my probe) both before and after normalize_column_names, in the explicit and the bare axis case alike, and the injected SUM survives the exclude_unset round trip.
I looked for over-correction too, since a validator in the wrong place can start refusing valid input. Explicit aggregates, saved metrics, adhoc sql_expression metrics, bare numeric columns, a mixed list of all three, and COUNT on a text column all still validate, save and return rows. A nameless metric entry never reaches that loop at all, ColumnRef already rejects it with "requires either 'name' or 'sql_expression'".
@rusackas one correction so this is not held up on it: those two rebase leftovers are already gone. They were fixed in the 79bde847 push, one before this one. At 30cde3abad the only gauge quick-lookup line in generate_chart.py is line 169 pointing at chart_type='gauge', with no gauge_chart anywhere in that file, and the get_chart_type_schema docstring mentions interactive_pivot exactly once, on line 384. I confirmed it at runtime as well as in source: booting the MCP app at this head and at the commit the branch sits on (298aa2f0ba) and diffing the description plus input schema of all 72 registered tools shows only the intended radar additions. My old inline comments are still rendered on the page against the commit they were written for, which I suspect is what it looks like from the timeline.
Also ran at this head: tests/unit_tests/mcp_service/chart/ in full, 2163 passed and 3 skipped; and a mutation pass on all three pins, each of which fails when its fix is removed. Reverting exclude_unset=True fails test_merge_radar_preserves_omitted_defaults, deleting record_implicit_metric_aggregate fails both new bare axis tests, and deleting the sort_by_metric line still fails TestRadarQueryContext.
CI is green at this head, 63 passing and 0 failing.
| ), | ||
| ], | ||
| ) | ||
| def test_merge_radar_preserves_omitted_defaults( |
There was a problem hiding this comment.
Not a blocker, purely a merge-order heads up since you own both PRs.
This test and test_merge_bubble_preserves_omitted_defaults in #44618 are inserted at the same point in this file, both directly after test_merge_chart_preserves_omitted_defaults, and both branches start from the same blob of it. I ran git merge-tree across the two heads and they do conflict, in this file only:
CONFLICT (content): Merge conflict in tests/unit_tests/mcp_service/chart/test_chart_utils.py
The plugin changes are in different files, so whichever lands second just needs a keep both resolution here. Nothing to do now, just so it is not a surprise.
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
LGTM, and I re-checked the three things that tripped up the other MCP chart plugins (bubble, sankey) against the current head rather than taking the green reviewDecision on faith: exclude_unset=True is there, record_implicit_metric_aggregate is there, and the shared _build_single_query_dict comment already lists radar for sort_by_metric. @gabotorresruiz's two rebase-leftover duplicate-text catches (the stray gauge_chart line, the doubled interactive_pivot. tail) are gone too.
|
Heads up, this now has a merge conflict with |
github-actions
Bot
added
the
requires:rebase
Adds a generate_chart plugin for the radar viz type (viz_type 'radar'). Multiple metrics form the radar axes; an optional groupby draws one polygon per category. Rebased onto master after the gauge plugin (apache#43568) merged, and addresses the review on the branch: - Query ordering (aminghadersohi): map_radar_config now emits sort_by_metric, so _build_single_query_dict orders by the first metric descending — matching Radar/buildQuery.ts and making the config/mapper docstrings true (they already claimed it). Without it a radar with >10 categories kept an arbitrary 10 polygons. Added a query-context regression test. - Docstring discoverability (gabotorresruiz): radar added to the generate_chart one-of list, a per-type bullet, and a 'radar'/'spider' quick-lookup, plus the get_chart_type_schema core-type list. - groupby entries reject aggregate/saved_metric/sql_expression via is_metric. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
30cde3a to
b90991f
Compare
github-actions
Bot
removed
the
requires:rebase
There was a problem hiding this comment.
Code Review Agent Run #722d04
Actionable Suggestions - 3
-
tests/unit_tests/mcp_service/chart/test_chart_utils.py - 2
- Inline imports violate rule 12745 · Line 183-184
- Under-asserted merge result · Line 214-214
-
tests/unit_tests/mcp_service/chart/test_radar_chart.py - 1
- Untyped monkeypatch fixture param · Line 191-191
Additional Suggestions - 2
-
tests/unit_tests/mcp_service/chart/test_radar_chart.py - 1
-
Function-local imports undocumented · Line 92-96BITO.md rule 12745 requires module-level imports unless a documented circular dependency exists. These function-local import blocks (also lines 192, 219, 226, 231, 244, 249) carry no justification comment, and sibling test_chart_helpers.py imports `chart_helpers` at module level successfully. Hoist them to the module top.
-
-
superset/mcp_service/chart/chart_helpers.py - 1
-
Misleading radar comment · Line 748-749Radar/buildQuery.ts never reads `sort_by_metric` — it orders by `series_limit_metric` or `metrics[0]` descending unconditionally (lines 33-39). Grouping radar under "sort_by_metric charts ... buildQuery derives this on the frontend" misstates the mechanism; `map_radar_config` hardcodes `sort_by_metric: True` (chart_utils.py:1711) to trigger the shared translation. Reword so radar's unconditional frontend ordering is described accurately.
-
Review Details
-
Files reviewed - 10 · Commit Range:
b90991f..b90991f- superset/mcp_service/chart/chart_helpers.py
- superset/mcp_service/chart/chart_utils.py
- superset/mcp_service/chart/plugins/__init__.py
- superset/mcp_service/chart/plugins/radar.py
- superset/mcp_service/chart/schemas.py
- superset/mcp_service/chart/tool/generate_chart.py
- superset/mcp_service/chart/tool/get_chart_data.py
- superset/mcp_service/chart/tool/get_chart_type_schema.py
- tests/unit_tests/mcp_service/chart/test_chart_utils.py
- tests/unit_tests/mcp_service/chart/test_radar_chart.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 an incremental AI Review. -
/review full- 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
| from superset.mcp_service.chart.chart_utils import map_radar_config | ||
| from superset.mcp_service.chart.schemas import RadarChartConfig |
There was a problem hiding this comment.
Inline imports of map_radar_config and RadarChartConfig inside the test body have no circular-dependency justification: this module already imports merge_chart_form_data from chart_utils and ColumnRef from schemas at top level, so the same modules are safely importable at module scope. Repo rule [12745] requires module-level imports unless a documented cycle exists. Please hoist these two imports into the existing top-level import blocks. (BITO.md rule 12745)
Code Review Run #722d04
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
|
|
||
| merged = merge_chart_form_data(existing, new_form_data, config) | ||
|
|
||
| assert {key: merged[key] for key in expected} == expected |
There was a problem hiding this comment.
The final assertion inspects only the color_scheme/row_limit keys named in expected, so a regression in merge_chart_form_data/_merge_shared_form_data affecting any other key (e.g. dropping groupby or corrupting metrics) would pass silently. The sibling test_merge_chart_preserves_omitted_defaults also asserts merged["metric"] == new_form_data["metric"]; mirror that here for the radar payload.
Code Review Run #722d04
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| the top-N polygons deterministic under a row_limit. | ||
| """ | ||
|
|
||
| def test_orders_by_first_metric_descending(self, monkeypatch) -> None: |
There was a problem hiding this comment.
BITO.md rule 11810 requires annotating fixture-injected parameters. Annotate as monkeypatch: pytest.MonkeyPatch; 32 of 43 sibling monkeypatch parameters in tests/unit_tests/mcp_service/chart/ are already annotated, so this matches house style and lets static typing cover the stubbed resolve_datasource_engine call.
Code Review Run #722d04
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
|
Rebased onto current master. CI here is down to one failing test, The same single test fails on all four chart-plugin PRs (#43567, #43570, #43571, #43573) for the same structural reason: that guard flags dispatcher branches keyed on registered chart types, so registering a new type is enough to turn pre-existing code into a violation. It needs the per-type behavior moved into the new plugin hooks rather than a line edit. I've written the full analysis and two questions for @rusackas in one place to avoid four parallel threads: #43573 (comment) Otherwise this branch is rebased, mergeable, and green. |
github-actions
Bot
added
the
requires:rebase
This branch has not been deployed
4 participants
SUMMARY
Adds a
generate_chartMCP plugin for the radar chart type (viz_type: radar), so the MCPgenerate_charttool can produce radar/spider charts. Radar is a shipped Superset viz type that had no MCP plugin; this closes that gap.The plugin mirrors the frontend Radar
buildQuerycontract: multiplemetricsbecome the radar axes (indicators), and an optionalgroupbysplits the data into one polygon per category (with no groupby the whole dataset is a single polygon). The query orders by the first metric descending.Follows the established plugin pattern (
pie/funnel/gauge/treemap/heatmap/waterfall) — a config schema, amap_*_configmapper, a plugin class, and registration:RadarChartConfig— a non-emptymetricslist required (each metric is one axis); optionalgroupby(series polygons),row_limit(default 10),filters,color_scheme. Added to theChartConfigdiscriminated union and theget_chart_type_schemaadapters.groupbyentries may not besaved_metric/sql_expression(dimensions, not metrics).map_radar_config— maps the config toform_data; each metric is emitted as a metric object (saved metrics as bare name strings, ad-hoc metrics as SIMPLE/SQL adhoc objects).RadarChartPlugin— registered in the plugin registry.radarwas absent from the recommendation category map, so this adds it (its ownradarcategory, consistent with how funnel/gauge/waterfall each carry a distinct category).The field set is intentionally minimal (core query contract). Cosmetic controls (per-metric min/max bounds via
column_config, label type/position, circular vs polygon shape) are left for a follow-up.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend/MCP only, no UI change.
TESTING INSTRUCTIONS
pytest tests/unit_tests/mcp_service/chart/test_radar_chart.py— 17 unit tests cover schema validation (non-emptymetrics,groupbyoptional and not-a-metric, saved-metric axis accepted, extra-field rejection),ChartConfigunion dispatch,form_datamapping (multi-metric labels, series groupby, filters, saved-metric passthrough), 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": "radar", "metrics": [{"name": "speed", "aggregate": "AVG"}, {"name": "power", "aggregate": "AVG"}, {"name": "range", "aggregate": "AVG"}]}.ADDITIONAL INFORMATION
🤖 Generated with Claude Code