Repository navigation
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #43570 +/- ##
===========================================
- Coverage 81.97% 66.12% -15.86%
===========================================
Files 2988 2989 +1
Lines 184142 184229 +87
Branches 42565 42581 +16
===========================================
- Hits 150952 121816 -29136
- Misses 30427 59764 +29337
+ Partials 2763 2649 -114
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:
|
6f2097f to
d255e55
Compare
Code Review Agent Run #251ea5Actionable 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 |
✅ 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.
Review — feat(mcp): heatmap chart type plugin
Thanks for closing out the MCP chart-plugin family, and for the thorough test suite. One functional issue in the query path that I confirmed by running the code, plus a secondary observation and a test-coverage note. No blocking asks from me — flagging for your call as the external-contributor gate still governs merge.
Method note: findings below are marked RAN (executed against this PR's head 55caf415 with superset-core on PYTHONPATH) or INSPECTED (read-only).
1. 🔴 x_axis is silently dropped from the query — heatmap collapses to one dimension (RAN)
map_heatmap_config (chart_utils.py) emits the X axis under the x_axis key and the Y axis under groupby. But the MCP query builder only folds x_axis into the query columns for time-series viz types:
# chart_helpers.py — build_query_dicts_from_form_data
is_timeseries = (
viz_type.startswith("echarts_timeseries") or viz_type == "mixed_timeseries"
)
x_axis_col = None
if is_timeseries: # heatmap_v2 is neither → skipped
x_axis_col = extract_x_axis_col(form_data)
if x_axis_col and x_axis_col not in groupby:
groupby = [x_axis_col] + groupbyheatmap_v2 is not a time-series type, so x_axis is never consumed. resolve_groupby returns only the groupby (Y) column, and the built query becomes columns=["hour"] — i.e. SELECT hour, COUNT(trips) … GROUP BY hour. The entire X axis (day_of_week) is gone, and the aggregation grain is wrong.
Executed repro at this PR's head:
form_data keys: ['groupby', 'metric', 'normalize_across', 'row_limit', 'viz_type', 'x_axis']
x_axis in form_data: day_of_week groupby in form_data: hour
viz_type: heatmap_v2 | is_timeseries: False
resolved groupby (GROUP BY cols): ['hour']
FINAL groupby after x_axis fold: ['hour']
>>> day_of_week (x_axis) present in GROUP BY? False
The frontend Heatmap/buildQuery.ts — which the docstrings say this mapping matches — sets both axes as query columns:
const columns = [
...ensureIsArray(getXAxisColumn(formData)), // x_axis
...ensureIsArray(groupby), // y
];This is the same class of silent drop that hit the siblings (sankey's missing GROUP BY grain, bubble's dropped metrics): the mapper emits a frontend-shaped key that the direct query-context path never reads. It manifests in get_chart_data, get_chart_preview, and get_chart_sql, all of which build the query context from this form_data rather than running the frontend buildQuery. Suggested fix: fold x_axis into the query columns for heatmap_v2 too (or, generally, whenever form_data carries an x_axis and the viz isn't handled elsewhere), so both dimensions reach GROUP BY.
2. normalize_across maps to nothing on this path (INSPECTED)
The schema documents normalize_across as driving "the server-side rank normalization", and map_heatmap_config emits it into form_data. But the frontend implements that normalization as a rankOperator post-processing step inside buildQuery, and the MCP query path adds no post_processing — normalize_across is written but never consumed (grep finds it only in the mapper/docstring, never in the query builder). Same root cause as #1: a buildQuery-only transform is lost. Lower severity — the cells still render, just un-normalized relative to the frontend — but the "server-side rank normalization" claim isn't true for MCP-executed queries.
3. Tests pin a shape the query path doesn't honor (RAN — 18/18 pass)
The suite is green even with #1 present, because it stops at the mapper output. test_basic_heatmap_form_data asserts form_data["x_axis"] == "day_of_week" and treats that as the contract, but no test builds a query context / SQL from the form_data to confirm x_axis actually reaches GROUP BY. That's the sankey pattern — a passing test cementing a shape the downstream never reads. A query-context-level test (assert both day_of_week and hour appear in the resulting columns) would have caught #1 and would guard the fix.
What's genuinely good here (called out because it breaks the family pattern)
- The
aggregatedimension gap is fixed. Every sibling'sreject_metric_style_*rejectedsaved_metric/sql_expressionbut missedaggregate. This validator usescol.is_metric(schemas.py:751→bool(aggregate) or saved_metric or bool(sql_expression)), which closes the gap, and ships a negative test (test_heatmap_axis_rejects_aggregate) for both axes. This is the only plugin in the family to get that right — nice. - The axes are unambiguous.
x_axis/y_axisareColumnRef(plain column references), notAxisConfigstyling objects, so this config does not re-create the column-vs-styling ambiguity that #42841's_route_x_axis_keyexists to disambiguate. No need to adopt that routing here.
One-line family takeaway
The single fix that would have prevented findings across all seven PRs: have the MCP query path run each viz type's real buildQuery (or a shared server-side equivalent) instead of re-deriving columns/metrics/orderby/post-processing in build_query_dicts_from_form_data. Every family bug — funnel orderby, sankey groupby, bubble metrics, and heatmap's x_axis + rank normalization — is a transform that lives only in buildQuery and is silently lost when the query context is assembled straight from form_data.
Code Review Agent Run #e91066Actionable 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 |
Code Review Agent Run #28438bActionable 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 — fixed #1 (the On #2 ( Agreed on the family synthesis — the durable fix is routing the MCP path through the real |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Re-reviewed at d2c667c8b4.
#1 (x_axis dropped from GROUP BY) — fixed, verified. Ran the query builder against the new HEAD: build_query_dicts_from_form_data now yields columns=['day_of_week', 'hour'], so both axes reach GROUP BY. The is_timeseries or viz_type == "heatmap_v2" fold is correct and the new test_x_axis_reaches_group_by guards it. Good.
#2 (normalize_across → rankOperator post-processing) — acknowledged/deferred, no objection. Agreed it's lower severity and a heavier fix; fine as a tracked follow-up.
Net-new nit — chart_type enumerations omit heatmap_v2 (discoverability). This PR registers heatmap_v2 in get_chart_type_schema.py's adapter map and examples, but the same file's tool docstring still lists the old set:
get_chart_type_schema.py (~L252): Valid chart_type values: xy, table, pie, pivot_table, mixed_timeseries, handlebars, big_number, histogram, box_plot, waterfall. — missing heatmap_v2.
The addition also makes two adjacent MCP-facing docstrings stale (not edited by this PR, but now inaccurate because heatmap is a generate_chart-supported type):
generate_chart.py(~L87):MUST include chart_type in config (one of: 'xy', ... 'waterfall')— omitsheatmap_v2.app.py(~L420): claims display_name is populated only for "the 10 chart types supported by generate_chart (…waterfall)" and lists Heatmap under "all other viz_types … it will be null" — heatmap is now supported, so both halves are wrong.
Low severity (the adapter/examples make it functional), but these strings are what an LLM reads to discover valid types. Worth updating at least the in-file docstring.
d2c667c to
3e7e2f3
Compare
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Re-reviewed at 3e7e2f369c (rebased onto latest master since round 2).
- x_axis GROUP BY fix — still correct after the rebase. Re-ran the query builder at this head:
build_query_dicts_from_form_datayieldscolumns=['day_of_week', 'hour'], both axes present. No regression from the rebase. - Docstring nit — resolved.
get_chart_type_schema's docstring now listsheatmap_v2among the core chart_type values. - normalize_across → rankOperator post-processing — still open, as agreed. Confirmed the built query still has no
post_processing; tracked as your follow-up, not a blocker here.
No new findings this round.
Code Review Agent Run #274e20Actionable 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—the prior X-axis GROUP BY and discoverability fixes remain correct at
The existing head remains green and the original focused tests pass; these are end-to-end contract gaps rather than regressions in your X-axis fix. Shared adapters, update preservation, result-envelope validation, and preview dispatch should be reused from the coordinated MCP infrastructure work rather than duplicated per chart. I did not modify or push to your branch. |
|
@aminghadersohi your call... merge, or block/wait for more updates here? |
github-actions
Bot
added
the
requires:rebase
3e7e2f3 to
e8b2701
Compare
github-actions
Bot
removed
the
requires:rebase
Code Review Agent Run #14dca6Actionable 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 |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks Greg, and thanks Amin for the earlier rounds. The GROUP BY fold fix and the is_metric axis validator are both solid, and I confirmed the suites are green at this head (1446 passed across the MCP chart directory and the form-data query-context tests).
To Evan's merge-or-wait question: wait, I am afraid. I have to request changes for one defect the earlier rounds could not have seen, because it lives in a different query builder than the one they traced: the compile check inside generate_chart crashes with an uncaught AttributeError on the scalar groupby this mapper emits, so the tool cannot create a heatmap at all. Repro and suggested fix in the inline comment; the same call with a waterfall config degrades gracefully. The columns_from_form_data coercion it needs is a couple of lines and also fixes a latent export-path crash for migrated heatmap charts.
Unlike the broader end-to-end items Amin listed on Sep 1, which read to me like fine follow-ups, this one is a hard runtime failure of the feature the PR adds, so I would fold it into this PR. The normalize_across comment is a cheap addition to the already-agreed follow-up. And a gentle nudge that two of the three doc spots from the earlier round (generate_chart.py docstring and the app.py instructions block, which still lists Heatmap as a type whose display name will be null) are still pending.
| form_data: Dict[str, Any] = { | ||
| "viz_type": "heatmap_v2", | ||
| "x_axis": config.x_axis.name, | ||
| "groupby": config.y_axis.name, |
There was a problem hiding this comment.
Hey Greg, one more query-path issue, and I am afraid this one is a blocker: generate_chart still cannot produce a heatmap on this branch. It is a different builder than the one fixed for the GROUP BY fold.
The compile check that runs on every generate_chart call (both the save path and the preview-only path) builds its columns through preview_utils._build_query_columns, which delegates to columns_from_form_data (superset/common/form_data_query_context.py:151). That helper calls .copy() on form_data["groupby"], and this mapper emits it as a bare string, so it raises AttributeError: 'str' object has no attribute 'copy'. Neither _compile_chart's except clauses nor the tool's outer handler catch AttributeError, so the whole tool call blows up.
I verified it at this head (e8b270167a), in a real app context:
fd = map_heatmap_config(HeatmapChartConfig(
chart_type="heatmap_v2",
x_axis={"name": "day_of_week"},
y_axis={"name": "hour"},
metric={"name": "trips", "aggregate": "COUNT"},
))
_compile_chart(fd, 1)
# AttributeError: 'str' object has no attribute 'copy'The same call with a waterfall config returns a structured CompileResult instead of raising.
The scalar itself is the faithful shape (the groupby control is multi: false, and MigrateHeatmapChart renames the scalar all_columns_y straight to groupby), so I would fix the shared helper rather than this mapper: coerce a string groupby into a one-element list inside columns_from_form_data, mirroring what chart_helpers.resolve_groupby already does and what your GROUP BY fold fix effectively assumes. That also fixes the same latent crash for migrated heatmap charts in the dashboard Excel export path, which reaches this helper via _columns_and_metrics.
For tests: one case in tests/unit_tests/common/test_form_data_query_context.py with {"x_axis": "day", "groupby": "hour"} expecting ["day", "hour"], plus a sibling of your test_x_axis_reaches_group_by that goes through columns_from_form_data instead of chart_helpers, since the two builders do not share code. That split is exactly why the suite stays green with this crash present. Happy to dig in with you if it does not reproduce on your side.
There was a problem hiding this comment.
Reproduced exactly — columns_from_form_data raised AttributeError: 'str' object has no attribute 'copy' on the scalar groupby, and neither _compile_chart nor the tool's outer handler caught it. Fixed in the shared helper as you suggested rather than in the mapper, since the scalar is the faithful shape (groupby is multi: false, and MigrateHeatmapChart renames all_columns_y straight to the scalar).
columns_from_form_data now runs a _as_column_list coercion on groupby, columns, and the raw-mode branch, wrapping a scalar in a one-element list, mirroring chart_helpers.resolve_groupby. That also removes the latent crash on the dashboard Excel export path for migrated heatmap charts, which reaches the same helper via _columns_and_metrics.
Tests: tests/unit_tests/common/test_form_data_query_context.py gets test_columns_scalar_groupby_is_coerced_to_list ({"x_axis": "day", "groupby": "hour"} → ["day", "hour"]) and a scalar-columns sibling; the heatmap suite gets test_x_axis_reaches_columns_from_form_data, which goes through columns_from_form_data rather than chart_helpers so the two builders are both guarded.
| description="Value metric colouring each cell (use aggregate e.g. SUM, " | ||
| "COUNT for ad-hoc, or set saved_metric=True for a saved dataset metric)", | ||
| ) | ||
| normalize_across: Literal["heatmap", "x", "y"] = Field( |
There was a problem hiding this comment.
Building on the normalize_across post-processing gap already agreed as a follow-up: there is a second half that is cheap to fix now. Even on the path where the frontend buildQuery does run (a saved chart rendered in Explore or a dashboard), the rank column is computed but never used for color, because transformProps.ts has colorColumn = normalized ? RANK_COLUMN_NAME : metricLabel and the normalized checkbox defaults to false and is not exposed here. So today a caller setting normalize_across sees no visual difference anywhere.
Exposing normalized: bool = False in this config and passing it through the mapper makes the knob real on the frontend path immediately, independent of the heavier server-side work. A mapping test asserting form_data["normalized"] would lock it in, and this field's description should mention it only takes effect with normalized=true.
There was a problem hiding this comment.
Fixed. HeatmapChartConfig now exposes normalized: bool = False, and the mapper threads it into form_data, so normalize_across is no longer inert on the frontend path — with normalized=true the rank column becomes colorColumn. Both field descriptions now state that normalize_across only takes effect when normalized=true. Mapping tests assert the default (False) and the pass-through (True).
The server-side rankOperator post-processing remains the tracked follow-up, as agreed.
| return self | ||
|
|
||
|
|
||
| class HeatmapChartConfig(BaseChartConfig): |
There was a problem hiding this comment.
Just a question, not a blocker: any reason to leave out time_grain? The heatmap control panel's Query section includes time_grain_sqla, and the waterfall config exposes time_grain with the granularity_sqla mirroring. The PR text frames the deferred fields as cosmetic, but time grain is part of the query contract; a heatmap with a temporal x_axis (say month vs region) cannot be bucketed without it. Fine as a follow-up if intentional.
There was a problem hiding this comment.
Intentional, deferred. This PR keeps the field set minimal (the stated scope), and time_grain only bites with a temporal x_axis. It is a fair query-contract gap, so I will add it in the same follow-up as the normalize_across server-side work, matching the granularity_sqla mirroring waterfall already does — unless you would prefer it folded in here.
|
@rusackas @gkneighb One independent re-review at I independently reproduced Gabo’s compile finding through the registered FastMCP tools, with authentication/dataset fixtures and no chart writes:
The September 1 product-path requests also remain open, independently of that blocker:
Checks: focused chart/query suites 1,446 passed; common/chart-data API 350 passed; 51 temporary Heatmap probes plus 9 Jinja-context checks. Broader MCP suite: 3,739 passed, 1 health-check smoke failure, independently reproduced on upstream master These are remaining contract gaps, not regressions in the original GROUP BY fix. No branch edits/pushes, approval, or thread resolutions. |
e8b2701 to
c9cc30e
Compare
Addresses gabotorresruiz's review of the heatmap plugin (apache#43570). Blocker: generate_chart could not produce a heatmap at all. The compile check derives columns via columns_from_form_data (a different builder than the GROUP BY fold fixed earlier), which called .copy() on form_data['groupby']. The heatmap Y axis is a single-select control, so the mapper emits groupby as a bare string, raising 'str' object has no attribute 'copy' — uncaught by _compile_chart, so the whole tool call blew up. Fixed in the shared helper: _as_column_list coerces a scalar groupby/columns into a one-element list, mirroring chart_helpers.resolve_groupby. This also fixes the latent crash for migrated heatmap charts (MigrateHeatmapChart renames the scalar all_columns_y straight to groupby) in the dashboard Excel export path, which reaches the same helper. Tests cover the scalar cases directly and through the heatmap mapper. normalized: expose normalized (default False) and thread it through the mapper. normalize_across has no visual effect on the frontend unless normalized is set (transformProps colorColumn = normalized ? RANK_COLUMN_NAME : metricLabel), so without this the knob was inert. Descriptions updated to say so. Docs: app.py instructions block no longer lists Heatmap among the viz_types whose display_name is null — heatmap_v2 is a registered plugin, so its display name resolves. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
the
requires:rebase
aea6824 to
1783507
Compare
Addresses gabotorresruiz's review of the heatmap plugin (apache#43570). Blocker: generate_chart could not produce a heatmap at all. The compile check derives columns via columns_from_form_data (a different builder than the GROUP BY fold fixed earlier), which called .copy() on form_data['groupby']. The heatmap Y axis is a single-select control, so the mapper emits groupby as a bare string, raising 'str' object has no attribute 'copy' — uncaught by _compile_chart, so the whole tool call blew up. Fixed in the shared helper: _as_column_list coerces a scalar groupby/columns into a one-element list, mirroring chart_helpers.resolve_groupby. This also fixes the latent crash for migrated heatmap charts (MigrateHeatmapChart renames the scalar all_columns_y straight to groupby) in the dashboard Excel export path, which reaches the same helper. Tests cover the scalar cases directly and through the heatmap mapper. normalized: expose normalized (default False) and thread it through the mapper. normalize_across has no visual effect on the frontend unless normalized is set (transformProps colorColumn = normalized ? RANK_COLUMN_NAME : metricLabel), so without this the knob was inert. Descriptions updated to say so. Docs: app.py instructions block no longer lists Heatmap among the viz_types whose display_name is null — heatmap_v2 is a registered plugin, so its display name resolves. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 #15921f
Actionable Suggestions - 5
-
tests/unit_tests/mcp_service/chart/test_heatmap_chart.py - 3
- Missing test docstrings · Line 36-36
- Untyped monkeypatch fixture · Line 212-212
- Inline imports in tests · Line 213-213
-
superset/mcp_service/chart/schemas.py - 1
- Metric role not enforced · Line 1478-1482
-
superset/mcp_service/chart/plugins/heatmap.py - 1
- Missing method docstrings (BITO 12147) · Line 45-150
Additional Suggestions - 2
-
superset/mcp_service/chart/tool/get_chart_type_schema.py - 1
-
Garbled docstring fragment · Line 352-352The docstring rewrite left a duplicated fragment on line 352 ('pivot extension also expose interactive_pivot.') that repeats lines 350-351. This is the LLM-facing contract for `get_chart_type_schema`; the garbled text can confuse clients. Remove the leftover line.
-
-
superset/mcp_service/chart/schemas.py - 1
-
Redundant always-true guard · Line 1509-1509`col` is a required, non-optional `ColumnRef` and `ColumnRef` defines no `__bool__`/`__len__` (schemas.py:715-838), so `if col and ...` is always true here; sibling `TreemapChartConfig.reject_metric_style_groupby` uses `col.is_metric` directly. Drop the redundant guard for consistency.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/mcp_service/chart/plugins/heatmap.py - 1
- Duplicated metric normalization block · Line 115-129
Review Details
-
Files reviewed - 11 · Commit Range:
6c801af..1783507- superset/common/form_data_query_context.py
- superset/mcp_service/app.py
- 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/heatmap.py
- superset/mcp_service/chart/schemas.py
- superset/mcp_service/chart/tool/generate_chart.py
- superset/mcp_service/chart/tool/get_chart_type_schema.py
- tests/unit_tests/common/test_form_data_query_context.py
- tests/unit_tests/mcp_service/chart/test_heatmap_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
| class TestHeatmapChartConfigSchema: | ||
| """HeatmapChartConfig schema validation.""" | ||
|
|
||
| def test_basic_heatmap_config(self) -> None: |
There was a problem hiding this comment.
BITO adaptive rule 12148 requires docstrings on all new test functions. 20 of 22 test methods here (e.g. test_basic_heatmap_config, test_heatmap_missing_x_axis, test_x_axis_reaches_group_by) lack them; only test_heatmap_axis_rejects_aggregate and test_groupby_alias_for_y_axis comply. Sibling test files share the gap, but the rule is org-mandated. Add one-line docstrings stating scenario and expected outcome.
Code Review Run #15921f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| heatmap_v2 needs an explicit fold or its X dimension is dropped. | ||
| """ | ||
|
|
||
| def test_x_axis_reaches_group_by(self, monkeypatch) -> None: |
There was a problem hiding this comment.
BITO rule 11810 requires fixture-injected parameters to be typed. monkeypatch is untyped at line 212; 24 of 26 monkeypatch parameters under tests/unit_tests/mcp_service annotate it as pytest.MonkeyPatch (the only other exception is pre-existing test_gauge_chart.py:319). Annotate for consistency and static type coverage.
Code Review Run #15921f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| """ | ||
|
|
||
| def test_x_axis_reaches_group_by(self, monkeypatch) -> None: | ||
| from superset.mcp_service.chart import chart_helpers |
There was a problem hiding this comment.
BITO rule 12745 requires module-level imports absent a documented circular dependency. Seven test methods import inline: chart_helpers (213), columns_from_form_data (237), registry (253, 265), display_name_for_viz_type (260), _VIZ_CATEGORY (279), _CHART_TYPE_ADAPTERS (284) — none with a justification comment. Hoist them to the top import block.
Code Review Run #15921f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| metric: ColumnRef = Field( | ||
| ..., | ||
| description="Value metric colouring each cell (use aggregate e.g. SUM, " | ||
| "COUNT for ad-hoc, or set saved_metric=True for a saved dataset metric)", | ||
| ) |
There was a problem hiding this comment.
Unlike GaugeChartConfig and BigNumberChartConfig (schemas.py:1328, schemas.py:2140), this validator never checks self.metric.is_metric, so metric={"name": "trips"} passes validation and create_metric_object silently defaults the aggregate to SUM (chart_utils.py:1004) — or surfaces a confusing DB error for non-numeric columns. Add the sibling is_metric check.
Code Review Run #15921f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| def pre_validate( | ||
| self, | ||
| config: dict[str, Any], | ||
| ) -> ChartGenerationError | None: | ||
| missing_fields = [] | ||
|
|
||
| if "x_axis" not in config: | ||
| missing_fields.append("'x_axis' (column along the X axis)") | ||
| if "y_axis" not in config and "groupby" not in config: | ||
| missing_fields.append("'y_axis' (column along the Y axis)") | ||
| if "metric" not in config: | ||
| missing_fields.append("'metric' (value colouring each cell)") | ||
|
|
||
| if missing_fields: | ||
| return ChartGenerationError( | ||
| error_type="missing_heatmap_fields", | ||
| message=( | ||
| f"Heatmap chart missing required fields: " | ||
| f"{', '.join(missing_fields)}" | ||
| ), | ||
| details=( | ||
| "Heatmaps plot a metric across two dimensions — one on the " | ||
| "x_axis and one on the y_axis — colouring each cell by the " | ||
| "metric value" | ||
| ), | ||
| suggestions=[ | ||
| "Add 'x_axis': {'name': 'day_of_week'}", | ||
| "Add 'y_axis': {'name': 'hour'}", | ||
| "Add 'metric': {'name': 'trips', 'aggregate': 'COUNT'}", | ||
| "Example: {'chart_type': 'heatmap_v2', " | ||
| "'x_axis': {'name': 'day_of_week'}, " | ||
| "'y_axis': {'name': 'hour'}, " | ||
| "'metric': {'name': 'trips', 'aggregate': 'COUNT'}}", | ||
| ], | ||
| error_code="MISSING_HEATMAP_FIELDS", | ||
| ) | ||
|
|
||
| return None | ||
|
|
||
| def extract_column_refs(self, config: Any) -> list[ColumnRef]: | ||
| if not isinstance(config, HeatmapChartConfig): | ||
| return [] | ||
| refs: list[ColumnRef] = [config.x_axis, config.y_axis, config.metric] | ||
| if config.filters: | ||
| for f in config.filters: | ||
| refs.append(ColumnRef(name=f.column)) | ||
| return refs | ||
|
|
||
| def to_form_data( | ||
| self, config: Any, dataset_id: int | str | None = None | ||
| ) -> dict[str, Any]: | ||
| return map_heatmap_config(config) | ||
|
|
||
| def generate_name(self, config: Any, dataset_name: str | None = None) -> str: | ||
| what = _heatmap_chart_what(config) | ||
| context = _summarize_filters(config.filters) | ||
| return self._with_context(what, context) | ||
|
|
||
| def resolve_viz_type(self, config: Any) -> str: | ||
| return "heatmap_v2" | ||
|
|
||
| def normalize_column_refs(self, config: Any, dataset_context: Any) -> Any: | ||
| config_dict = config.model_dump() | ||
|
|
||
| for key in ("x_axis", "y_axis"): | ||
| col = config_dict.get(key) | ||
| if col and not col.get("sql_expression") and not col.get("saved_metric"): | ||
| col["name"] = DatasetValidator.get_canonical_column_name( | ||
| col["name"], dataset_context | ||
| ) | ||
| if config_dict.get("metric"): | ||
| if config_dict["metric"].get("sql_expression"): | ||
| pass | ||
| elif config_dict["metric"].get("saved_metric"): | ||
| config_dict["metric"]["name"] = ( | ||
| DatasetValidator.get_canonical_metric_name( | ||
| config_dict["metric"]["name"], dataset_context | ||
| ) | ||
| ) | ||
| else: | ||
| config_dict["metric"]["name"] = ( | ||
| DatasetValidator.get_canonical_column_name( | ||
| config_dict["metric"]["name"], dataset_context | ||
| ) | ||
| ) | ||
| DatasetValidator.normalize_filters(config_dict, dataset_context) | ||
| return HeatmapChartConfig.model_validate(config_dict) | ||
|
|
||
| def schema_error_hint(self) -> ChartGenerationError | None: | ||
| return ChartGenerationError( | ||
| error_type="heatmap_validation_error", | ||
| message="Heatmap chart configuration validation failed", | ||
| details=( | ||
| "The heatmap chart configuration is missing required " | ||
| "fields or has invalid structure" | ||
| ), | ||
| suggestions=[ | ||
| "Ensure 'x_axis' and 'y_axis' each have a 'name'", | ||
| "Ensure 'metric' field has 'name' and 'aggregate'", | ||
| "Example: {'chart_type': 'heatmap_v2', " | ||
| "'x_axis': {'name': 'day_of_week'}, " | ||
| "'y_axis': {'name': 'hour'}, " | ||
| "'metric': {'name': 'trips', 'aggregate': 'COUNT'}}", | ||
| ], | ||
| error_code="HEATMAP_VALIDATION_ERROR", | ||
| ) |
There was a problem hiding this comment.
None of the seven overridden methods carries an inline docstring; their contracts live only on BaseChartPlugin/ChartTypePlugin. BITO adaptive rule 12147 requires a docstring on every newly introduced function. Brief per-method docstrings (even one line noting heatmap-specific behavior, e.g. the y_axis/groupby alias handling in pre_validate) keep this file self-describing.
Code Review Run #15921f
The metric-normalization block (sql_expression pass, saved_metric -> get_canonical_metric_name, else get_canonical_column_name) is copied verbatim from PieChartPlugin and TreemapChartPlugin (and near-verbatim GaugeChartPlugin). A future fix to metric normalization must be applied in four files; extracting one shared helper removes that divergence risk.
Code Review Run #c21aa0
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
|
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. |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks @gkneighb. Re-reviewed at 178350704c, since the head moved after my approval at aea6824573.
Holding for one thing only, and it is a one-line delete: the rebase left a duplicated line in the get_chart_type_schema docstring. Details inline.
Everything else is the change I already approved. I compared the full set of lines the PR adds at aea6824573 and at 178350704c, and the only differences are the chart type lists reflowing around gantt and treemap_v2. plugins/heatmap.py, test_heatmap_chart.py and the form_data_query_context.py edit are byte identical between the two.
What I ran at this head:
tests/unit_tests/mcp_service/chart/plustests/unit_tests/common/test_form_data_query_context.py: 1925 passed, 1 skipped.test_columns_scalar_groupby_is_coerced_to_listandtest_columns_scalar_columns_is_coerced_to_listagainst the parent128c0c159a: both fail there withAttributeError: 'str' object has no attribute 'copy', so they really do pin the fix.- A real create and a real preview through the tool layer against a live SQLite dataset:
generate_chartsaved aheatmap_v2chart withx_axis=day_of_week,groupby=model,metric=SUM(fare), and the executed query returned all nine cells, so both axes still reachGROUP BYafter the Gantt rewrite of that branch.
The red unit-tests check is not from this diff. All six failures are in tests/unit_tests/commands/test_base_restore_version_command.py (#44436), and master fixed them in 43fee87672, which is newer than this branch's base, so a rebase should turn it green.
Re-request me once that line is gone and I will re-approve right away.
| 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 aea6824573, the commit I approved, and absent from 128c0c159a, so it is new at this head.
| interactive_pivot. | |
| pivot extension also expose interactive_pivot. | |
| interactive_pivot. |
github-actions
Bot
added
the
requires:rebase
Addresses gabotorresruiz's review of the heatmap plugin (apache#43570). Blocker: generate_chart could not produce a heatmap at all. The compile check derives columns via columns_from_form_data (a different builder than the GROUP BY fold fixed earlier), which called .copy() on form_data['groupby']. The heatmap Y axis is a single-select control, so the mapper emits groupby as a bare string, raising 'str' object has no attribute 'copy' — uncaught by _compile_chart, so the whole tool call blew up. Fixed in the shared helper: _as_column_list coerces a scalar groupby/columns into a one-element list, mirroring chart_helpers.resolve_groupby. This also fixes the latent crash for migrated heatmap charts (MigrateHeatmapChart renames the scalar all_columns_y straight to groupby) in the dashboard Excel export path, which reaches the same helper. Tests cover the scalar cases directly and through the heatmap mapper. normalized: expose normalized (default False) and thread it through the mapper. normalize_across has no visual effect on the frontend unless normalized is set (transformProps colorColumn = normalized ? RANK_COLUMN_NAME : metricLabel), so without this the knob was inert. Descriptions updated to say so. Docs: app.py instructions block no longer lists Heatmap among the viz_types whose display_name is null — heatmap_v2 is a registered plugin, so its display name resolves. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1783507 to
8ddb7d1
Compare
|
@gabotorresruiz Fixed at The cause was the conflict region ending mid-sentence: master's tail line sat outside the conflict markers, and my replacement text re-closed the sentence, so the old tail survived underneath. Gone now, and I audited all four branches in this family for the same mistake — it was on every one of them, and every one is now clean. While I was here: bubble (#43572) merged on 2026-09-21, which re-conflicted this branch, so this head is also rebased onto current master — which picks up Re-verified at this head: |
github-actions
Bot
removed
the
requires:rebase
|
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). 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 |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks @gkneighb, and thanks for your patience on this one. Approving, which clears my earlier change request.
I did not take the fix on faith. I booted the MCP app at this head and at the merge base 298aa2f0ba and diffed the live tool surface clients actually receive. All 70 tools are present on both sides, the duplicated interactive_pivot. line is gone from the get_chart_type_schema description, and the only other deltas are the intended ones: the heatmap_v2 additions to the generate_chart description and to the server instructions block, plus the HeatmapChartConfig branch appearing in the generate_chart, update_chart, update_chart_preview and generate_explore_link input schemas. Nothing else in any description or schema moved, and the only removed line across those four schemas is the ChartConfig union description you updated on purpose.
What else I ran at 8ddb7d10e4:
tests/unit_tests/mcp_service/chart/plustests/unit_tests/common/test_form_data_query_context.py: 2206 passed, 3 skipped.test_columns_scalar_groupby_is_coerced_to_listandtest_columns_scalar_columns_is_coerced_to_listcopied onto the merge base: both fail there withAttributeError: 'str' object has no attribute 'copy', so they really do pin the fix.- A real create, read and preview through the registered MCP tools against a live SQLite dataset:
generate_chartsaved aheatmap_v2chart,get_chart_sqlreturnedSELECT day_of_week, hour, sum(fare) ... GROUP BY day_of_week, hour, andget_chart_datacame back with all nine cells.normalizedandnormalize_acrossboth thread into the savedform_data, a saved metric passes through as a bare name, and a config missingy_axisis rejected cleanly instead of crashing. _CHART_TYPE_ADAPTERSand_CHART_EXAMPLESare both 16 with no gap either way, and every one of the 16 is named in both thegenerate_chartandget_chart_type_schemadescriptions.- A line by line comparison of the patch at
178350704cand at this head:plugins/heatmap.py,test_heatmap_chart.pyand theform_data_query_context.pyedit are identical, and the only other change is the chart type list reflowing aroundbubble_v2.
Every check is green on this run, including unit-tests, so the #44436 failures really were coming from master and the rebase cleared them.
I left one non-blocking note inline about a sibling of the scalar case. Nothing there needs to happen before merge.
| if value is None: | ||
| return [] | ||
| if isinstance(value, str): | ||
| return [value] | ||
| return list(value) |
There was a problem hiding this comment.
Not a blocker, and not something this PR broke. A note on the sibling of the case you are fixing.
The same multi: false groupby control stores a bare object, not a string, when the Y axis is an adhoc or calculated column: OptionSelector.getValues() returns getColumnNameOrAdhocColumn(values[0]) when multi is false. list(value) on that object yields its keys, so the coercion succeeds and hands the query three invented column names.
I ran build_query_context_from_form_data on a heatmap form_data whose groupby is {"expressionType": "SQL", "sqlExpression": ..., "label": "hour_band"}. At this head it builds columns == ["day_of_week", "expressionType", "sqlExpression", "label"]; at the merge base 298aa2f0ba the same call raises AttributeError: 'dict' object has no attribute 'insert'. So on the dashboard Excel export path it trades a loud crash for a wrong column list. chart_helpers.resolve_groupby, the mirror this docstring cites, has the same blind spot, so this is a pre-existing family gap and not a regression.
ensureIsArray semantics cover both shapes if you want it closed here:
| if value is None: | |
| return [] | |
| if isinstance(value, str): | |
| return [value] | |
| return list(value) | |
| if value is None: | |
| return [] | |
| if isinstance(value, (list, tuple)): | |
| return list(value) | |
| return [value] |
plus a test_columns_adhoc_groupby_is_wrapped_not_expanded alongside your two scalar cases. Equally happy for you to call it out of scope, since nothing the MCP mapper emits can reach it.
|
If it's not a blocker, I'm inclined to merge. @gkneighb let us know if you want to address @gabotorresruiz's last comment first, thanks! |
8ddb7d1 to
2f6857a
Compare
|
@rusackas Thanks — nothing blocking from my side, so it's good to merge whenever you are. On @gabotorresruiz's last comment: he explicitly flagged it as "not a blocker, and not something this PR broke". It's the sibling of the case this PR fixes — a One thing I did push since your comment, at CI is green at this head. |
There was a problem hiding this comment.
Code Review Agent Run #c21aa0
Actionable Suggestions - 2
-
superset/mcp_service/chart/plugins/heatmap.py - 1
- Duplicated metric normalization · Line 45-150
-
superset/common/form_data_query_context.py - 1
- Duplicated column-list helper · Line 133-147
Additional Suggestions - 1
-
tests/unit_tests/mcp_service/chart/test_chart_utils.py - 1
-
Inline imports vs module-level rule · Line 126-127These are the only function-body `from superset` imports in this 2,600-line file; `chart_utils` and `chart.schemas` are already imported at module level (lines 26-56), so no circular-dependency rationale applies. BITO.md rule 12745 requires module-level imports unless a documented circular dependency exists. Move `map_heatmap_config` and `HeatmapChartConfig` into the existing top-level import blocks.
-
Review Details
-
Files reviewed - 12 · Commit Range:
2363c59..1147bf7- superset/common/form_data_query_context.py
- superset/mcp_service/app.py
- 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/heatmap.py
- superset/mcp_service/chart/schemas.py
- superset/mcp_service/chart/tool/generate_chart.py
- superset/mcp_service/chart/tool/get_chart_type_schema.py
- tests/unit_tests/common/test_form_data_query_context.py
- tests/unit_tests/mcp_service/chart/test_chart_utils.py
- tests/unit_tests/mcp_service/chart/test_heatmap_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 _as_column_list(value: Any) -> list[Any]: | ||
| """ | ||
| Normalize a ``groupby``/``columns`` value into a list. | ||
|
|
||
| Single-select controls (e.g. the heatmap ``groupby`` Y axis, which is | ||
| ``multi: false``, and heatmap charts migrated via ``MigrateHeatmapChart``) | ||
| store the dimension as a bare string. Wrap a scalar in a one-element list, | ||
| mirroring ``chart_helpers.resolve_groupby``, so downstream list operations | ||
| (``.copy()``, ``.insert()``) do not blow up on a ``str``. | ||
| """ | ||
| if value is None: | ||
| return [] | ||
| if isinstance(value, str): | ||
| return [value] | ||
| return list(value) |
There was a problem hiding this comment.
This new helper duplicates _as_column_list in superset/mcp_service/chart/chart_utils.py:2416 (same name, same purpose) with divergent edge semantics: that version wraps any non-list scalar, while this one calls list(value), which raises TypeError on a non-iterable scalar. as_list from superset.utils.core is already imported in this module. Consolidate in a shared util to prevent divergence. ([CWE not applicable])
Citations
- Rule Violated: dev-standard.mdc:108
Code Review Run #c21aa0
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
github-actions
Bot
added
the
requires:rebase
Adds a generate_chart plugin for heatmap (viz_type 'heatmap_v2'): an x_axis column and a single groupby (Y) column form the two axes, one metric colours the cells, and normalize_across selects the rank-normalization range. Rebased onto current master (past the gauge plugin + follow-up). Folds x_axis into the GROUP BY columns for heatmap_v2 in build_query_dicts_from_form_data (was time-series-only, so the X dimension was dropped). Adds the generate_chart.py docstring entry (one-of, per-type bullet, 'heatmap'/'density grid' quick-lookup) + the get_chart_type_schema core list. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses gabotorresruiz's review of the heatmap plugin (apache#43570). Blocker: generate_chart could not produce a heatmap at all. The compile check derives columns via columns_from_form_data (a different builder than the GROUP BY fold fixed earlier), which called .copy() on form_data['groupby']. The heatmap Y axis is a single-select control, so the mapper emits groupby as a bare string, raising 'str' object has no attribute 'copy' — uncaught by _compile_chart, so the whole tool call blew up. Fixed in the shared helper: _as_column_list coerces a scalar groupby/columns into a one-element list, mirroring chart_helpers.resolve_groupby. This also fixes the latent crash for migrated heatmap charts (MigrateHeatmapChart renames the scalar all_columns_y straight to groupby) in the dashboard Excel export path, which reaches the same helper. Tests cover the scalar cases directly and through the heatmap mapper. normalized: expose normalized (default False) and thread it through the mapper. normalize_across has no visual effect on the frontend unless normalized is set (transformProps colorColumn = normalized ? RANK_COLUMN_NAME : metricLabel), so without this the knob was inert. Descriptions updated to say so. Docs: app.py instructions block no longer lists Heatmap among the viz_types whose display_name is null — heatmap_v2 is a registered plugin, so its display name resolves. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1147bf7 to
7b09f5c
Compare
github-actions
Bot
removed
the
requires:rebase
There was a problem hiding this comment.
Code Review Agent Run #e0a661
Actionable Suggestions - 3
-
tests/unit_tests/mcp_service/chart/test_chart_utils.py - 2
- Inline duplicate imports · Line 181-182
- Weak merge assertion · Line 207-207
-
tests/unit_tests/mcp_service/chart/test_heatmap_chart.py - 1
- Repeated config arrange block · Line 37-243
Additional Suggestions - 1
-
superset/mcp_service/chart/schemas.py - 1
-
Duplicated dimension validator · Line 1777-1788`HeatmapChartConfig.reject_metric_style_dimensions` duplicates `BubbleChartConfig.reject_metric_style_dimensions` (schemas.py:1698-1712): identical loop over dimension `ColumnRef`s calling `_reject_sql_expression_on_dimension` then checking `col.is_metric`. Extract a shared helper to keep the two dimension-rejection rules in one place; divergence here would let one chart type accept a metric-style dimension the other rejects.
-
Review Details
-
Files reviewed - 12 · Commit Range:
27126b3..7b09f5c- superset/common/form_data_query_context.py
- superset/mcp_service/app.py
- 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/heatmap.py
- superset/mcp_service/chart/schemas.py
- superset/mcp_service/chart/tool/generate_chart.py
- superset/mcp_service/chart/tool/get_chart_type_schema.py
- tests/unit_tests/common/test_form_data_query_context.py
- tests/unit_tests/mcp_service/chart/test_chart_utils.py
- tests/unit_tests/mcp_service/chart/test_heatmap_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_heatmap_config | ||
| from superset.mcp_service.chart.schemas import HeatmapChartConfig |
There was a problem hiding this comment.
map_heatmap_config and HeatmapChartConfig are already imported at module level (lines 26, 47); these function-body imports duplicate them with no circular-dependency justification or explanatory comment, contrary to the repo rule requiring module-level imports. Extend the existing top-level import lists instead.
Code Review Run #e0a661
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 dict-comprehension assertion only inspects keys in expected, so a merge_chart_form_data regression that dropped the update fields (metric, groupby) would still pass. Sibling test_merge_chart_preserves_omitted_defaults also asserts merged["metric"] == new_form_data["metric"]; mirror that here so the merge overlay contract is actually validated.
Code Review Run #e0a661
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| config = HeatmapChartConfig( | ||
| chart_type="heatmap_v2", | ||
| x_axis={"name": "day_of_week"}, | ||
| y_axis={"name": "hour"}, | ||
| metric={"name": "trips", "aggregate": "COUNT"}, | ||
| ) | ||
| assert config.x_axis.name == "day_of_week" | ||
| assert config.y_axis.name == "hour" | ||
| assert config.normalize_across == "heatmap" # frontend default | ||
|
|
||
| def test_heatmap_missing_x_axis(self) -> None: | ||
| with pytest.raises(ValidationError): | ||
| HeatmapChartConfig( | ||
| chart_type="heatmap_v2", | ||
| y_axis={"name": "hour"}, | ||
| metric={"name": "trips", "aggregate": "COUNT"}, | ||
| ) | ||
|
|
||
| def test_heatmap_missing_y_axis(self) -> None: | ||
| with pytest.raises(ValidationError): | ||
| HeatmapChartConfig( | ||
| chart_type="heatmap_v2", | ||
| x_axis={"name": "day_of_week"}, | ||
| metric={"name": "trips", "aggregate": "COUNT"}, | ||
| ) | ||
|
|
||
| def test_heatmap_missing_metric(self) -> None: | ||
| with pytest.raises(ValidationError): | ||
| HeatmapChartConfig( | ||
| chart_type="heatmap_v2", | ||
| x_axis={"name": "day_of_week"}, | ||
| y_axis={"name": "hour"}, | ||
| ) | ||
|
|
||
| def test_heatmap_rejects_extra_fields(self) -> None: | ||
| with pytest.raises(ValidationError): | ||
| HeatmapChartConfig( | ||
| chart_type="heatmap_v2", | ||
| x_axis={"name": "day_of_week"}, | ||
| y_axis={"name": "hour"}, | ||
| metric={"name": "trips", "aggregate": "COUNT"}, | ||
| bogus=1, | ||
| ) | ||
|
|
||
| def test_heatmap_axis_rejects_aggregate(self) -> None: | ||
| """An aggregate makes an axis metric-like; x_axis/y_axis are dims.""" | ||
| with pytest.raises(ValidationError): | ||
| HeatmapChartConfig( | ||
| chart_type="heatmap_v2", | ||
| x_axis={"name": "day_of_week", "aggregate": "COUNT"}, | ||
| y_axis={"name": "hour"}, | ||
| metric={"name": "trips", "aggregate": "COUNT"}, | ||
| ) | ||
| with pytest.raises(ValidationError): | ||
| HeatmapChartConfig( | ||
| chart_type="heatmap_v2", | ||
| x_axis={"name": "day_of_week"}, | ||
| y_axis={"name": "hour", "aggregate": "COUNT"}, | ||
| metric={"name": "trips", "aggregate": "COUNT"}, | ||
| ) | ||
|
|
||
| def test_heatmap_y_axis_rejects_saved_metric(self) -> None: | ||
| with pytest.raises(ValidationError): | ||
| HeatmapChartConfig( | ||
| chart_type="heatmap_v2", | ||
| x_axis={"name": "day_of_week"}, | ||
| y_axis={"name": "count", "saved_metric": True}, | ||
| metric={"name": "trips", "aggregate": "COUNT"}, | ||
| ) | ||
|
|
||
| def test_heatmap_invalid_normalize_across_rejected(self) -> None: | ||
| with pytest.raises(ValidationError): | ||
| HeatmapChartConfig( | ||
| chart_type="heatmap_v2", | ||
| x_axis={"name": "day_of_week"}, | ||
| y_axis={"name": "hour"}, | ||
| metric={"name": "trips", "aggregate": "COUNT"}, | ||
| normalize_across="diagonal", | ||
| ) | ||
|
|
||
| def test_groupby_alias_for_y_axis(self) -> None: | ||
| """Superset-native 'groupby' is accepted for the Y-axis field.""" | ||
| config = HeatmapChartConfig.model_validate( | ||
| { | ||
| "chart_type": "heatmap_v2", | ||
| "x_axis": {"name": "day_of_week"}, | ||
| "groupby": {"name": "hour"}, | ||
| "metric": {"name": "trips", "aggregate": "COUNT"}, | ||
| } | ||
| ) | ||
| assert config.y_axis.name == "hour" | ||
|
|
||
| def test_chart_config_union_dispatches_heatmap(self) -> None: | ||
| config = TypeAdapter(ChartConfig).validate_python( | ||
| { | ||
| "chart_type": "heatmap_v2", | ||
| "x_axis": {"name": "day_of_week"}, | ||
| "y_axis": {"name": "hour"}, | ||
| "metric": {"name": "trips", "aggregate": "COUNT"}, | ||
| } | ||
| ) | ||
| assert isinstance(config, HeatmapChartConfig) | ||
|
|
||
|
|
||
| class TestMapHeatmapConfig: | ||
| """form_data mapping must match the frontend Heatmap buildQuery.""" | ||
|
|
||
| def test_basic_heatmap_form_data(self) -> None: | ||
| config = HeatmapChartConfig( | ||
| chart_type="heatmap_v2", | ||
| x_axis={"name": "day_of_week"}, | ||
| y_axis={"name": "hour"}, | ||
| metric={"name": "trips", "aggregate": "COUNT"}, | ||
| ) | ||
| form_data = map_heatmap_config(config) | ||
| assert form_data["viz_type"] == "heatmap_v2" | ||
| assert form_data["x_axis"] == "day_of_week" | ||
| # Y axis uses the groupby key as a single column (control is multi:false) | ||
| assert form_data["groupby"] == "hour" | ||
| assert form_data["metric"]["label"] == "COUNT(trips)" | ||
| assert form_data["normalize_across"] == "heatmap" | ||
|
|
||
| def test_heatmap_form_data_with_normalize_and_filters(self) -> None: | ||
| config = HeatmapChartConfig( | ||
| chart_type="heatmap_v2", | ||
| x_axis={"name": "day_of_week"}, | ||
| y_axis={"name": "hour"}, | ||
| metric={"name": "trips", "aggregate": "COUNT"}, | ||
| normalize_across="x", | ||
| filters=[{"column": "year", "op": "=", "value": 2026}], | ||
| ) | ||
| form_data = map_heatmap_config(config) | ||
| assert form_data["normalize_across"] == "x" | ||
| assert form_data["adhoc_filters"], "filters must map to adhoc_filters" | ||
|
|
||
| def test_normalized_defaults_false(self) -> None: | ||
| # normalize_across has no visual effect on the frontend unless the | ||
| # 'normalized' flag is also set, so it must be threaded through. | ||
| config = HeatmapChartConfig( | ||
| chart_type="heatmap_v2", | ||
| x_axis={"name": "day_of_week"}, | ||
| y_axis={"name": "hour"}, | ||
| metric={"name": "trips", "aggregate": "COUNT"}, | ||
| ) | ||
| assert map_heatmap_config(config)["normalized"] is False | ||
|
|
||
| def test_normalized_true_maps_through(self) -> None: | ||
| config = HeatmapChartConfig( | ||
| chart_type="heatmap_v2", | ||
| x_axis={"name": "day_of_week"}, | ||
| y_axis={"name": "hour"}, | ||
| metric={"name": "trips", "aggregate": "COUNT"}, | ||
| normalize_across="x", | ||
| normalized=True, | ||
| ) | ||
| assert map_heatmap_config(config)["normalized"] is True | ||
|
|
||
| def test_heatmap_saved_metric_maps_to_name_string(self) -> None: | ||
| config = HeatmapChartConfig( | ||
| chart_type="heatmap_v2", | ||
| x_axis={"name": "day_of_week"}, | ||
| y_axis={"name": "hour"}, | ||
| metric={"name": "avg_fare", "saved_metric": True}, | ||
| ) | ||
| assert map_heatmap_config(config)["metric"] == "avg_fare" | ||
|
|
||
|
|
||
| class TestHeatmapQueryContext: | ||
| """The built query must GROUP BY both axes, not just the Y (groupby) column. | ||
|
|
||
| map_heatmap_config emits X under 'x_axis' and Y under 'groupby'; the query | ||
| builder folds x_axis into the columns only for time-series viz types, so | ||
| heatmap_v2 needs an explicit fold or its X dimension is dropped. | ||
| """ | ||
|
|
||
| def test_x_axis_reaches_group_by(self, monkeypatch) -> None: | ||
| from superset.mcp_service.chart import chart_helpers | ||
|
|
||
| monkeypatch.setattr( | ||
| chart_helpers, | ||
| "resolve_datasource_engine", | ||
| lambda datasource_id, datasource_type: "base", | ||
| ) | ||
| config = HeatmapChartConfig( | ||
| chart_type="heatmap_v2", | ||
| x_axis={"name": "day_of_week"}, | ||
| y_axis={"name": "hour"}, | ||
| metric={"name": "trips", "aggregate": "COUNT"}, | ||
| ) | ||
| form_data = map_heatmap_config(config) | ||
| queries = chart_helpers.build_query_dicts_from_form_data(form_data, 1, "table") | ||
| columns = queries[0]["columns"] | ||
| assert "day_of_week" in columns, "x_axis must reach GROUP BY" | ||
| assert "hour" in columns, "y_axis must reach GROUP BY" | ||
|
|
||
| def test_x_axis_reaches_columns_from_form_data(self) -> None: | ||
| # The generate_chart compile check derives columns through a different | ||
| # builder (columns_from_form_data, used by the dashboard export path | ||
| # too), which must tolerate the scalar 'groupby' this mapper emits and | ||
| # carry both axes rather than crashing on str.copy(). | ||
| from superset.common.form_data_query_context import columns_from_form_data | ||
|
|
||
| config = HeatmapChartConfig( | ||
| chart_type="heatmap_v2", | ||
| x_axis={"name": "day_of_week"}, | ||
| y_axis={"name": "hour"}, | ||
| metric={"name": "trips", "aggregate": "COUNT"}, |
There was a problem hiding this comment.
The four-line HeatmapChartConfig(chart_type=..., x_axis=..., y_axis=..., metric=...) arrange block repeats ~18 times across the file. test_bubble_chart.py centralizes the identical pattern in a _base() helper (BubbleChartConfig(**_base())); a _heatmap_base(**overrides) helper would localize future schema changes to one site.
Code Review Run #e0a661
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 heatmap chart type (viz_type: heatmap_v2), so the MCPgenerate_charttool can produce heatmaps. Heatmap is a shipped Superset viz type that had no MCP plugin; this closes that gap.The plugin mirrors the frontend Heatmap
buildQuerycontract: anx_axiscolumn and a singlegroupbycolumn form the two axes, onemetriccolours each cell, andnormalize_acrossselects the server-side rank-normalization range (the wholeheatmap, per-xcolumn, or per-yrow).Follows the established plugin pattern (
pie/funnel/gauge/treemap/waterfall) — a config schema, amap_*_configmapper, a plugin class, and registration:HeatmapChartConfig—x_axis,y_axis, andmetricrequired; optionalnormalize_across(defaultheatmap),row_limit,filters. Added to theChartConfigdiscriminated union and theget_chart_type_schemaadapters. The Y axis accepts the nativegroupbyalias and is emitted as a single-selectgroupby(not a list), matching the frontend control (multi: false).x_axis/y_axismay not besaved_metric/sql_expression(dimensions, not metrics).map_heatmap_config— maps the config toform_data; saved metrics pass through as a bare name string, ad-hoc metrics as SIMPLE/SQL adhoc objects. The server applies the rank-normalization post-processing operator (driven bynormalize_across).HeatmapChartPlugin— registered in the plugin registry;heatmap_v2was already present in the recommendation category map.The field set is intentionally minimal (core query contract + normalization). Cosmetic controls (linear color scheme, legend/value display, sort axes, scale intervals) 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_heatmap_chart.py— 18 unit tests cover schema validation (x/y/metric required, nativegroupbyaliasing for the Y axis,x_axis/y_axis-not-a-metric,normalize_acrossenum, extra-field rejection),ChartConfigunion dispatch,form_datamapping (viz_type/x_axis/groupby-as-single/metric/normalize/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": "heatmap_v2", "x_axis": {"name": "day_of_week"}, "y_axis": {"name": "hour"}, "metric": {"name": "trips", "aggregate": "COUNT"}}.ADDITIONAL INFORMATION
🤖 Generated with Claude Code