Repository navigation
feat(mcp): treemap chart type plugin - #43569
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43569 +/- ##
==========================================
- Coverage 80.13% 80.12% -0.02%
==========================================
Files 2923 2924 +1
Lines 172286 172366 +80
Branches 39995 40009 +14
==========================================
+ Hits 138063 138108 +45
- Misses 31637 31669 +32
- Partials 2586 2589 +3
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:
|
63597b1 to
260770f
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| @model_validator(mode="after") | ||
| def reject_metric_style_groupby(self) -> "TreemapChartConfig": | ||
| """groupby entries are hierarchy dimensions, not metrics.""" | ||
| for i, col in enumerate(self.groupby or []): | ||
| _reject_sql_expression_on_dimension(col, f"groupby[{i}]") | ||
| if col.saved_metric: | ||
| raise ValueError( | ||
| f"groupby[{i}] cannot use saved_metric=True; " | ||
| "saved metrics belong in the 'metric' field" | ||
| ) | ||
| return self |
There was a problem hiding this comment.
Suggestion: The validator rejects saved_metric and sql_expression, but it still accepts aggregate on a groupby entry even though aggregate also marks a ColumnRef as a metric. map_treemap_config later emits only each entry's name, silently discarding the aggregate and changing an invalid metric-style hierarchy entry into an unaggregated dimension. Reject non-null aggregate values for treemap hierarchy columns as well. [api mismatch]
Severity Level: Minor 🧹
- ⚠️ `generate_chart` accepts invalid metric-style treemap hierarchy entries.
- ⚠️ Treemap form-data silently drops requested aggregates.
- ⚠️ Generated hierarchy differs from the caller’s configuration.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/schemas.py
**Line:** 1090:1100
**Comment:**
*Api Mismatch: The validator rejects `saved_metric` and `sql_expression`, but it still accepts `aggregate` on a `groupby` entry even though `aggregate` also marks a `ColumnRef` as a metric. `map_treemap_config` later emits only each entry's name, silently discarding the aggregate and changing an invalid metric-style hierarchy entry into an unaggregated dimension. Reject non-null `aggregate` values for treemap hierarchy columns as well.
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 valid. The To resolve this, update the @model_validator(mode="after")
def reject_metric_style_groupby(self) -> "TreemapChartConfig":
"""groupby entries are hierarchy dimensions, not metrics."""
for i, col in enumerate(self.groupby or []):
_reject_sql_expression_on_dimension(col, f"groupby[{i}]")
if col.saved_metric:
raise ValueError(
f"groupby[{i}] cannot use saved_metric=True; "
"saved metrics belong in the 'metric' field"
)
if col.aggregate:
raise ValueError(
f"groupby[{i}] cannot use aggregate='{col.aggregate}'; "
"aggregates belong in the 'metric' field"
)
return selfWould you like me to check the other comments on this PR and implement fixes for them as well? superset/mcp_service/chart/schemas.py |
Code Review Agent Run #a4136bActionable 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 |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Thanks @gkneighb — this is a clean, well-documented plugin that mirrors the existing pie/pivot conventions closely (docstrings tie back to the frontend buildQuery contract, the union discriminator and get_chart_type_schema example are both wired up, and the tests are meaningful). Comments below; nothing here blocks the design, and one of them is a shared convention that will save you work on the sibling PRs. I'm leaving this as a review comment (not requesting changes) and not resolving the CodeAnt thread.
1. Confirmed: reject_metric_style_groupby misses aggregate (the live CodeAnt thread is correct)
schemas.py:1090-1099 guards groupby against two of the three ways a ColumnRef becomes a metric — sql_expression (via _reject_sql_expression_on_dimension) and saved_metric (explicit) — but not aggregate. So a hierarchy entry written metric-style passes validation, and map_treemap_config (chart_utils.py:1021) emits [g.name for g in config.groupby], dropping the aggregate.
Executed at head 260770ff (groupby=[{name:'region'}, {name:'sales', aggregate:'SUM'}]):
VALIDATION: accepted (no error raised)
groupby[1].aggregate on the model = 'SUM'
groupby[1].is_metric = True
emitted form_data groupby = ['region', 'sales']
Note groupby[1].is_metric == True — the model's own predicate already classifies this entry as a metric; the validator just doesn't consult it.
Treemap-specific consequence (traced through to form_data)
For a treemap the groupby list is the nesting hierarchy and the query's GROUP BY, so dropping the aggregate isn't cosmetic — it changes the aggregation grain:
sales(an intended-metric column) is silently promoted to the innermost nesting level. The emitted query becomesGROUP BY region, sales, so instead of one tile per region you get one leaf tile per distinct rawsalesvalue under each region.- The tile-sizing metric
SUM(revenue)is then aggregated at grain(region, sales_value)rather than(region)— every region tile is subdivided and its children no longer sum to what the caller configured. - With
row_limit=100+sort_by_metric, the backend truncates to the top 100(region, sales)rows, i.e. an arbitrary slice of a high-cardinality axis — the rendered hierarchy is non-representative.
This is a step worse than the sibling instances: on the gauge PR the dropped aggregate produced a misleading single dial and on radar a degenerate axis — both presentation-level. Here it corrupts the GROUP BY grain itself, so the aggregation is wrong, not just the picture.
2. This is a cross-family hole — one shared fix
The identical gap exists in the gauge (#43568) and radar (#43571) reject_metric_style_* validators: each rejects saved_metric/sql_expression and misses aggregate. The repo already ships the complete predicate — ColumnRef.is_metric (schemas.py:751-753 = bool(self.aggregate) or self.saved_metric or bool(self.sql_expression)), which is already used for positive metric checks (e.g. schemas.py:1517, chart_utils.py:462, update_chart.py:157). No dimension guard uses its negation, which is exactly the miss. Collapsing the hand-rolled checks to it fixes all three at once:
@model_validator(mode="after")
def reject_metric_style_groupby(self) -> "TreemapChartConfig":
"""groupby entries are hierarchy dimensions, not metrics."""
for i, col in enumerate(self.groupby or []):
if col.is_metric:
raise ValueError(
f"groupby[{i}] is a hierarchy dimension and cannot carry a "
"metric (aggregate, saved_metric, or sql_expression); "
"put metrics in the 'metric' field."
)
return self(If you want to keep the existing tailored error strings, just add an if col.aggregate is not None: branch alongside the saved_metric one — the point is closing aggregate.)
3. Test coverage — add the negative case
test_treemap_chart.py is solid (+167, and reverting the production files fails the union-dispatch, mapping, and registry tests). It has test_treemap_groupby_rejects_saved_metric but no test for aggregate on a groupby entry — the exact hole above. Worth a test_treemap_groupby_rejects_aggregate asserting ValidationError. As a family note: none of the reviewed siblings currently has a negative test for this case, so the gap has been shipping unguarded across the set.
4. Non-issues I checked (so you don't have to)
- The 2 deleted lines in
schemas.pyare benign — they're the union discriminator description string being reflowed to insert'treemap_v2'into the chart-type list. No behavior change. map_treemap_configdrops nothing else silently — every field it emits (viz_type,groupby,metric,sort_by_metric,row_limit,color_scheme, adhoc filters) has a corresponding config field.chart_utils.py(+31) is purely additive (map_treemap_config+_treemap_chart_what); no existing plugin's path changes.- Hierarchy ordering is significant and correctly handled — order is preserved end-to-end and asserted in
test_basic_treemap_form_data, and documented in the config docstring. get_chart_type_schema(+9) surfaces the treemap adapter with a usable example.
Verification
- RAN: full
test_treemap_chart.pyat head260770ff→ 15 passed. - RAN: the dropped-aggregate repro above (
superset-core/srconPYTHONPATH). - INSPECTED: cross-family
is_metricusage grep; the 2 deleted lines;map_treemap_configfield coverage. - CI at head, deduped by check-name/latest-run: 46 SUCCESS / 3 NEUTRAL / 12 SKIPPED, zero live failures (the raw rollup's 2 playwright FAILUREs are superseded re-runs).
✅ 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 — thanks for the fast turnaround, @gkneighb. The fix(mcp): treemap — reject aggregate on groupby hierarchy columns commit (8f8ef8c) resolves my round-1 finding cleanly. One heads-up below that is not a blocker for this PR (it lives in shared infra, not your plugin), plus a trivial nit.
✅ Round-1 finding fixed — verified by execution
reject_metric_style_groupby now switches the hand-rolled saved_metric-only check to the complete predicate:
if col.is_metric: # bool(aggregate) or saved_metric or bool(sql_expression)
raise ValueError(f"groupby[{i}] must be a plain column, not a metric; ...")This is exactly the ColumnRef.is_metric (schemas.py:751) reuse suggested, and it closes the aggregate hole that CodeAnt/Bito and I flagged. RAN at 8f8ef8c (superset-core/src on PYTHONPATH):
- The round-1 repro —
groupby=[{name:'region'},{name:'sales',aggregate:'SUM'}]— now raisesValidationError("groupby[1] must be a plain column, not a metric…") instead of silently emittinggroupby: ['region','sales']. Grain-corruption path closed. - New
test_treemap_groupby_rejects_aggregateis present and the file is green: 16 passed (was 15).
The CodeAnt thread is still open in the UI but is addressed by this commit — leaving it for the bot/maintainer to resolve rather than resolving someone else's thread.
🔎 Heads-up (non-blocking, shared infra, whole plugin family): sort_by_metric isn't honored on the get_chart_data path
Not introduced here and not fixable in the treemap plugin — flagging for family awareness since the config's own docstring promises "the query orders by the metric descending."
The frontend Treemap/buildQuery.ts is the only place that turns sort_by_metric into ordering:
if (sort_by_metric && metric) { orderby.push([metric, false]); }
orderby.push(...buildColumnsOrderBy(groupby));map_treemap_config correctly emits sort_by_metric into form_data. But the MCP data-retrieval path (get_chart_data → build_query_context_from_form_data → build_query_dicts_from_form_data, chart_helpers.py:546) builds the QueryObject server-side and never runs buildQuery.ts. That builder special-cases only deck_* and mixed_timeseries for orderby; treemap_v2 falls through to _build_single_query_dict, which sets columns/metrics/row_limit/filters but no orderby. Net effect: when an agent creates a treemap and then calls get_chart_data, the row_limit=100 truncation returns an arbitrary 100 rows rather than the top-100 by metric — so the returned/rendered tiles differ from a treemap saved and viewed in the browser.
This is the same buildQuery-bypass symptom already confirmed on the funnel sibling (#43567). The fix belongs in shared build_query_dicts_from_form_data (a treemap_v2 orderby branch mirroring buildQuery.ts), not in this PR — noting it so the family can address it in one place.
Nit (optional, pre-existing)
_treemap_chart_what (chart_utils.py:1555-1558): since groupby has min_length=1 and groupby entries always carry name (sql_expression is rejected there), dims is always truthy — the if dims: guard and the return f"{metric_label}" fallback are unreachable. Collapsible to a single return. Already flagged by Bito; purely cosmetic.
Verification
- RAN:
test_treemap_chart.py@8f8ef8c→ 16 passed; round-1 aggregate repro now raisesValidationError. - INSPECTED:
build_query_dicts_from_form_data/_build_single_query_dict(notreemap_v2orderby branch);generate_chartpersists form_data only;_treemap_chart_whatreachability. - CI @
8f8ef8c, deduped by check-name/latest run: 47 SUCCESS / 2 NEUTRAL / 9 SKIPPED, zero failures.mergeStateStatus: BLOCKEDis the required maintainer-review gate (reviewDecision: REVIEW_REQUIRED) for an external contributor, not a code issue.
Code Review Agent Run #784d95Actionable 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 |
8f8ef8c to
8664853
Compare
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Approve — clean, pattern-faithful addition.
map_treemap_config matches the frontend treemap_v2 buildQuery/controlPanel contract (singular metric, ordered groupby hierarchy, sort_by_metric, row_limit, color_scheme), the schema validators are sound — reject_metric_style_groupby correctly rejects metric-shaped groupby entries, mirroring the newer waterfall pattern — and the unit tests pass locally (16/16) with ruff clean.
One test-coverage gap worth addressing before/soon after merge (inline comment below). A few optional low-severity polish items, none blocking:
treemap_v2is missing from thegenerate_charttool docstring andDEFAULT_INSTRUCTIONSinapp.py(LLM discoverability), and the app.py "display name is null for all other viz_types" note is now stale since the plugin registersnative_viz_types={"treemap_v2": "Treemap"}.get_chart_previewhas notreemap_v2spec in_get_chart_spec_for_type, so previews fall through to the scatter fallback (waterfall was explicitly slotted into the "bar" bucket for this reason).TestMapTreemapConfigdoesn't assert thecolor_schemedefault,row_limitpassthrough, or thesql_expressionmetric branch.
Review assisted by automated multi-agent analysis + a local test/lint run.
| def resolve_viz_type(self, config: Any) -> str: | ||
| return "treemap_v2" | ||
|
|
||
| def normalize_column_refs(self, config: Any, dataset_context: Any) -> Any: |
There was a problem hiding this comment.
normalize_column_refs has no test coverage.
This is the most branch-heavy method in the plugin — per-column canonicalization of groupby, plus the three-way metric branch (sql_expression → passthrough, saved_metric → get_canonical_metric_name, else → get_canonical_column_name) — and it's dispatched in the live validation flow (superset/mcp_service/chart/validation/dataset_validator.py:537), yet nothing in test_treemap_chart.py exercises it.
The repo already guards exactly this saved-metric column-vs-metric casing branch for every other metric-bearing chart type via TestSavedMetricNormalizationCorrectness in tests/unit_tests/mcp_service/chart/test_column_name_normalization.py (xy / table / pie / big_number / mixed). Treemap is the only metric-bearing type missing, so a casing regression here would fail silently.
Suggested: add a treemap case to that class asserting (a) a plain groupby column is canonicalized, (b) a saved_metric metric routes through get_canonical_metric_name, (c) an adhoc-column metric routes through get_canonical_column_name, and (d) a sql_expression metric is left untouched.
Code Review Agent Run #140d04Actionable 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 |
|
@gkneighb Thanks again for the solid typed Treemap foundation. I need to elevate my earlier non-blocking note after tracing the full customer-facing MCP paths: for Treemap support, registration + mapping alone still leaves materially incorrect behavior, so could you please extend this PR before we count Treemap as complete?
At current head |
github-actions
Bot
added
the
requires:rebase
8664853 to
8d0895e
Compare
github-actions
Bot
removed
the
requires:rebase
Code Review Agent Run #7388faActionable Suggestions - 0Additional Suggestions - 3
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 |
|
Re-reviewed
Validation: all 1,403 MCP chart tests pass, as do Ruff on the six PR files and |
e0d51c4 to
8af1773
Compare
Adds a generate_chart plugin for treemap (viz_type 'treemap_v2'): one metric sizing the tiles plus an ordered groupby hierarchy; sort_by_metric orders tiles by the metric descending. Rebased onto current master (past the gauge plugin + its follow-up), and adds the generate_chart.py docstring entry (one-of list, per-type bullet, 'treemap'/'hierarchy' quick-lookup) plus the get_chart_type_schema core list. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8af1773 to
458631c
Compare
|
Re-reviewed
Validation: 1,578 chart tests passed, 1 skipped, plus eight temporary diagnostic cases confirming the remaining gaps and the fixes above; Ruff and whitespace checks passed. The Treemap test file is unchanged and still lacks the requested product-path regression coverage. CI has no failing/pending checks and mergeability is clean, but those green checks don't cover these behaviors. Please address the remaining Treemap paths before considering support complete. No contributor-branch changes made. |
Code Review Agent Run #b4f529Actionable 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 |
3 participants
SUMMARY
Adds a
generate_chartMCP plugin for the treemap chart type (viz_type: treemap_v2), so the MCPgenerate_charttool can produce treemaps. Treemap is a shipped Superset viz type that had no MCP plugin; this closes that gap.The plugin mirrors the frontend Treemap
buildQuerycontract: onemetricsizing the tiles plus an orderedgroupbyhierarchy — the first column is the outermost level and each subsequent column nests inside it. Whensort_by_metricis set (the frontend default), tiles are ordered by the metric descending.Follows the established plugin pattern (
pie/funnel/gauge/waterfall) — a config schema, amap_*_configmapper, a plugin class, and registration:TreemapChartConfig—metricrequired and a non-empty orderedgroupbylist (the hierarchy); optionalsort_by_metric,row_limit,filters,color_scheme. Added to theChartConfigdiscriminated union and theget_chart_type_schemaadapters.groupbyentries may not besaved_metric/sql_expression(dimensions, not metrics).map_treemap_config— maps the config toform_data, preservinggroupbyorder as the nesting hierarchy; saved metrics pass through as a bare name string, ad-hoc metrics as SIMPLE/SQL adhoc objects.TreemapChartPlugin— registered in the plugin registry;treemap_v2was already present in the recommendation category map.The field set is intentionally minimal (core query contract). Cosmetic controls (label type, upper labels, number/date format) 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_treemap_chart.py— 15 unit tests cover schema validation (required non-emptygroupby, hierarchy order preserved,groupby-not-a-metric, extra-field rejection),ChartConfigunion dispatch,form_datamapping (viz_type/groupby-order/metric/sort/filters/saved-metric), and registry integration (registration,resolve_viz_type,display_name,pre_validate).To exercise end-to-end: call the
generate_chartMCP tool with{"chart_type": "treemap_v2", "groupby": [{"name": "region"}, {"name": "product"}], "metric": {"name": "revenue", "aggregate": "SUM"}}.ADDITIONAL INFORMATION
🤖 Generated with Claude Code