Repository navigation
fix(mcp): complete Gauge chart support follow-up - #43943
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #43943 +/- ##
==========================================
+ Coverage 79.85% 79.90% +0.04%
==========================================
Files 2916 2916
Lines 170703 171199 +496
Branches 39553 39672 +119
==========================================
+ Hits 136319 136797 +478
+ Misses 31867 31819 -48
- Partials 2517 2583 +66
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:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Code Review Agent Run #2e1438Actionable Suggestions - 0Additional Suggestions - 22
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 |
|
Finite maintainer pass on the 22 additional suggestions in Bito's comment, starting from
The four already-resolved inline fixes are retained and covered by the broad regression run. Validation and exact-head CI are recorded in the PR body. |
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Two correctness findings worth addressing on the Gauge follow-up. Flagging the mediums here; I have a separate batch of low/nit items (native "No filter" sentinel treated as a temporal conflict, the config-less rebind guard, a self-healing duplicate NO_TIME_RANGE filter, dead _preserve_previous_adhoc_filters, and a couple of test-assertion gaps) I can share if useful.
🟡 A single NULL/NaN dial rejects the entire Gauge result — superset/mcp_service/chart/query_result.py:180,189
validate_gauge_query_result returns on the first row whose metric value isn't finite, so one empty-aggregate group discards all dials of a multi-dial gauge — the "up to 10 dials" feature this series adds. A SQL NULL aggregate reaches the validator as None/NaN (QueryContextProcessor.get_data returns df.to_dict(orient="records") with NaN verbatim on the JSON path, no verbose_map), and the ECharts gauge (transformProps.ts maps data_point[metricLabel] with no null filtering) renders such rows fine — so MCP is strictly stricter than the viz it fronts. It gates every path (get_chart_data both query paths, all three get_chart_preview strategies, generate_preview_from_form_data, and the generate/update_chart compile check), so it also regresses reads of already-saved gauges whose current data has a null group. Fix: skip non-finite dial values (still render the valid dials) and only return an error when no finite value exists, matching the frontend.
🟡 Grouped (faceted) Gauge preview draws the needle at the cell corner — superset/mcp_service/chart/preview_utils.py:845
The pointer rule encodes x/y as value-exprs "width / 2" / "height / 2". The single-dial path is correct, but when groupby wraps the spec in facet/spec, the facet child scope has no width/height signals (only child_width/child_height), so both resolve to 0 — the needle is drawn from (0, 0) while the arc auto-centers at (child_width/2, child_height/2), detaching the pointer from the dial in every grouped preview. Reproduced by compiling the generated spec through vega-lite@5 + vega@5 (scenegraph: rule at x=0,y=0; arc centered at 100,150). Preview-only — the saved chart, its form_data, and query results are unaffected. Fix: reference child_width/child_height in the pointer value-exprs, or move the pointer into the unit spec so width/height resolve to the cell.
|
Addressed this review in
The offered low/nit batch was not posted separately; all named areas above were inspected and addressed directly. No additional review is requested. The four existing resolved threads remain resolved; this review has no new inline thread to resolve. Evidence: 20 fail-before cases against starting head |
|
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 |
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Re-reviewed f17d7f9e81 — the two rendering issues from the last pass are genuinely fixed (faceted needle centering and finite-dial preservation), and I found no regressions. Requesting changes on one correctness item; the rest are non-blocking follow-ups.
🔴 Blocking — get_chart_data silently drops null gauge groups and misreports counts — superset/mcp_service/chart/tool/get_chart_data.py:772,1140
normalize_gauge_query_result is the right behavior for the preview/compile renderability checks, but it's also wired into get_chart_data, a raw data-inspection tool. For a saved gauge with groupby where one group's aggregate is NULL/NaN, get_chart_data now returns fewer rows than the query produced and rewrites rowcount / row_count / data_completeness to match, so the dropped group vanishes with no signal; if all groups are non-finite it returns a hard NonFiniteGaugeMetric error. Meanwhile the ECharts gauge (Gauge/transformProps.ts maps data_point[metricLabel] with no null filtering) still renders a dial per group, and every non-gauge chart type returns all rows — so the agent's view of the data disagrees with the chart it's inspecting, which for an agent-facing tool is a silent-correctness failure. Fix: restrict the drop/hard-error normalization to the preview/compile paths; in get_chart_data, return all rows (at most annotate non-finite groups) so the data tool stays faithful to the query and consistent with other chart types.
The following are non-blocking — fine as follow-ups:
🟢 Low — Fix for native "No filter" misses the empty/whitespace time_range sentinel — superset/mcp_service/chart/schemas.py:1254
adapt_native_form_data now treats None and NO_TIME_RANGE ("No filter") as neutral, but not "" / " " — which time_range_validation.py:155-162 documents as the same "no filter applied" sentinel. So time_range="" plus a real adhoc TEMPORAL_RANGE is still rejected as a bogus conflict. Fix: normalize empty/whitespace to unset alongside NO_TIME_RANGE on both operands before the conflict check.
🟢 Low — Facet-centering fix has no enforced test in default CI — tests/unit_tests/mcp_service/chart/test_gauge_render.py:31
The only assertion that the needle origin equals its per-cell arc center (rule.x==arc.x, rule.y==arc.y) is guarded by @pytest.mark.skipif(not os.environ.get("NODE_PATH")), and NODE_PATH is set nowhere in CI/conftest/pyproject — so it's skipped in every default run. The one non-skipped pointer test asserts only the encoding key set and sin/cos substrings, never the x/y origin exprs, so reverting the faceted center_x/center_y to width/2 would reintroduce the off-center needle with the suite still green. Fix: set NODE_PATH in the unit-test job, or add a non-skipped assertion that encoding['x']/['y'] resolve to the cell center for the faceted spec.
🟢 Low — test_gauge_all_zero_auto_range doesn't pin the 0–1 range — tests/unit_tests/mcp_service/chart/test_gauge_chart.py:879
It asserts only "no ChartError" (renderable) and that explicit degenerate bounds reject; it never checks the derived range is [0, 1]. A regression changing the omitted-bounds fallback to any other nondegenerate range (e.g. 0–100) would pass. The ascii preview already emits Range: 0 to 1, so it's trivially assertable.
🟢 Low — Needle length (52px) still stops short of the dial band (innerRadius 55px) — superset/mcp_service/chart/preview_utils.py:854
Unchanged: the pointer reaches radius 52 while the arc band starts at 55, so the tip stops ~3px inside the hollow center and never overlaps the colored dial (unlike the ECharts pointer). Cosmetic, preview-only. Fix: derive the needle length from the arc innerRadius/outerRadius.
|
Addressed all five items in review 5140601116 in
No new items were rejected. Earlier 22-suggestion dispositions and human-review follow-ups remain applicable except the explicitly superseded raw-data behavior and needle length. All four earlier inline threads remain resolved; this review has no new inline thread to resolve. Validation: 1,810 affected Python tests, 21 frontend Gauge tests, 12 rendered specs, full branch-applicable/staged pre-commit (including mypy), compileall, and optimized-mode renderability checks passed. Recorded fail-before cases cover raw-data/export and sentinel/needle fixes. All 13 required checks passed on this exact head, including unit tests, PostgreSQL integration, Playwright, pre-commit. The unit job initially exceeded its existing 30-minute limit after all 14,776 main-suite tests passed; one rerun completed successfully, including the remaining coverage batches. No tests, coverage thresholds, or timeouts were changed. The prior non-required dependency-check failure occurred while setup-uv fetched Astral's version manifest, before dependency checks. One failed-job rerun succeeded; the new head's check-python-deps also passed. No code failure was masked. PR testing/coordination sections are updated. No additional review is requested. |
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Re-reviewed 307f84d5bc — the blocking get_chart_data raw-rows issue is resolved (renderability normalization removed from both data query paths, so raw null/NaN/inf groups are now preserved in JSON/CSV/XLSX, and row_count is correctly distinct from source rowcount), and I confirmed it did not regress the preview/compile filtering — the gauge renderability normalizer and all-invalid error are still enforced in compile.py, preview_utils.py, and get_chart_preview.py (all three strategies).
All four follow-ups from the last pass are addressed and now have non-skipped test coverage:
- Native
""/whitespace/"No filter"temporal sentinels normalized on both operands; genuine conflicts and missing/non-string comparators still reject. - Faceted needle centering pinned by a default-CI test (
x/yorigin == cell center), not just theNODE_PATH-gated render test. - All-zero auto-range pins
Range: 0 to 1(ASCII + Vegausermeta); explicit degenerate bounds still reject. - Needle length derived from the dial band midpoint (68.5 of the 55–82 band) so the tip reaches the band.
Approving.
One optional, non-blocking follow-up: on the default openpyxl Excel path, non-finite metric cells serialize as blank, so a NULL group and a NaN/inf group are indistinguishable in .xlsx (CSV renders nan/inf and the xlsxwriter fallback renders #NUM!/#DIV/0!). Consider stringifying non-finite floats in _write_excel_data for cross-format consistency — largely an inherent xlsx limitation, fine to defer.
|
Addressed the optional XLSX item in the approving review in
Validation: 1,816 affected Python tests, 21 frontend Gauge tests, 12 rendered specs, full branch-applicable/staged pre-commit including mypy, compileall, and optimized-mode renderability checks passed. All 13 required checks passed on this exact head, including unit tests, PostgreSQL integration, Playwright, pre-commit. PR body testing/coordination sections are updated. No additional review is requested. |
Code Review Agent Run #8b4b0aActionable Suggestions - 0Additional Suggestions - 7
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 |
SUMMARY
Completes Gauge MCP support introduced by #43568, retaining the public
gaugeidentity and nativegauge_chartcompatibility, complete controls, shared query construction, and saved/unsaved update and preview paths.The latest follow-up also addresses the optional XLSX note in approving review 5146726270: openpyxl exports preserve non-finite floats as
nan/inf/-inftext, distinct from blank NULLs and consistent with CSV. Mastere6d088dwas merged normally to resolve the shared rejected-filter helper conflict while preserving both query-error and rejected-filter checks.This finite follow-up addresses review 5140601116:
get_chart_datapreserves every source group, including NULL/NaN/non-finite aggregates, without Gauge-specific filtering, count rewriting, or all-invalid errorsEarlier fixes remain: datasource overrides, dataset-rebind filter isolation, metric-label resolution, Gauge dispatch, ambiguity errors, config-less same-dataset updates, duplicate neutral-binding self-healing, and dead-helper removal.
Original merge: #43568
Completeness request: #43568 (comment)
Previous 22-suggestion dispositions: #43943 (comment)
Previous human-review dispositions: #43943 (comment)
Coordination: One finite pass over all live reviews, issue comments, and inline threads. This correction supersedes the earlier get-data normalization disposition: raw inspection is not a renderability check. All five preceding items and the optional item in the approving review are addressed; four earlier resolved inline threads remain covered. The new review is body-only, with no new inline thread to resolve. Ready for review; no additional review request, draft conversion, history rewrite, or PR merge.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No live UI screenshot: the local application health endpoint is unavailable.
TESTING INSTRUCTIONS
Validation for head
dfeb3922517376e51e7ccacecfefab6965beda48:Six XLSX fail-before cases cover saved, saved-with-form-data-key, and unsaved public FastMCP paths with mixed/all-non-finite groups. Loading the exported workbooks asserts NULL remains blank, NaN/Infinity remain distinguishable text, and finite values remain numeric. All six pass after the fix.
Earlier fail-before evidence against
f17d7f9e81: 19 raw-data/export cases and 19 sentinel/needle cases failed before the fixes. Reverting facet centers to outer signals separately makes the default-CI geometry test fail withoutNODE_PATH.1,816 affected Python tests passed, covering chart/explore/common MCP paths, query-context JSON/CSV/XLSX serialization, and SQLAlchemy label resolution:
NODE_PATH=/path/to/vega-node-modules PYTHONPATH=$PWD:$PWD/superset-core/src pytest -q tests/unit_tests/mcp_service/chart tests/unit_tests/mcp_service/explore tests/unit_tests/mcp_service/common tests/unit_tests/common/test_query_context_processor.py tests/unit_tests/connectors/sqla/models_test.pyPublic FastMCP regressions cover saved, saved-with-form-data-key, and unsaved requests; mixed and all-non-finite groups; JSON/CSV/XLSX rows; source
rowcountdistinct from returnedrow_count; completeness; and both XLSX engines. Preview and real generate/update compile regressions remain in the broad suite.21 frontend Gauge tests passed: buildQuery and transformProps.
Actual Vega-Lite 5/Vega 5 compilation, SVG rendering, and scenegraph assertions passed across 12 product-generated specs, checking per-cell centers, endpoints, band-derived length, angles, and visibility. The optional renderer requires
vega@5/vega-lite@5throughNODE_PATH; facet-center and exact 0–1 range assertions also run in default Python CI without those dependencies.Full branch-applicable and staged pre-commit passed, including mypy, Ruff/format, Pylint, and repository checks. Compileall, optimized-mode renderability checks, and diff whitespace checks passed.
All 13 required checks passed on this exact head, including unit tests, PostgreSQL integration, Playwright, pre-commit.
The exact-head dependency check passed. No checks, coverage thresholds, or timeouts were weakened.
Manual verification:
nan/inf/-inftext. Query saved and unsaved grouped Gauges containing finite, NULL, NaN, and infinite aggregates in JSON/CSV/XLSX: all groups remain. All-invalid raw data is not a renderability error.No filtertemporal sentinels with a real native range: no false conflict. Actual conflicting restrictions still reject.No live UI smoke test is claimed.
ADDITIONAL INFORMATION