Skip to content

feat(mcp): add typed Sunburst chart support - #43771

Open
aminghadersohi wants to merge 53 commits into
apache:masterfrom
aminghadersohi:sc-119162-mcp-sunburst
Open

feat(mcp): add typed Sunburst chart support#43771
aminghadersohi wants to merge 53 commits into
apache:masterfrom
aminghadersohi:sc-119162-mcp-sunburst

Conversation

@aminghadersohi

@aminghadersohi aminghadersohi commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

SUMMARY

Adds complete typed MCP support for the frontend ECharts Sunburst plugin using the exact sunburst_v2 viz tag and the typed chart_type="sunburst" discriminator.

  • Registers ordered hierarchy, saved/SIMPLE/SQL primary and secondary metrics, filters/time controls, ordering/limits, presentation controls, schema discovery, previews, and recommendation metadata.
  • Preserves native/plugin presentation controls on same-viz updates while replacing every target-owned query-role alias from the chart plugin registry before overlay. This removes stale singular/plural, raw/aggregate, primary/secondary, pivot, series, and x-axis roles across every enabled registered native viz alias and table/Handlebars query mode.
  • Treats an explicit null temporal range, column, or grain as an atomic clear across top-level range/granularity/grain keys, adhoc filters, top-level legacy filters/extra_filters, and nested extra_form_data filters while retaining unrelated predicates.
  • Seeds g.form_data with set_query_context_form_data immediately before ChartDataCommand.run() for saved ASCII, table, and Vega-Lite previews, so virtual-dataset url_param, filter_values, and get_filters Jinja macros receive the saved query fields.
  • Retains strict first-query result handling, operation-owned chart/datasource identity, exact/unique-casefold namespace resolution, native WHERE semantics, finite numeric Sunburst result validation, and programmatic query-context seeding for all other compile/preview paths.

Coordination (read-only inspection; only Amin's Sunburst branch and this PR body were changed):

  • This post-coordination pass started from clean exact local/fork/PR head 923d925437f6317ba461154a102e8aa9eb902329 after inspecting the Agor session/zone state. The completed reviewer was archived; no competing Sunburst reviewer/fixer was created.

  • Upstream master was integrated only through normal merge commits, never rebase/force-push: bcf11aed7381c02064aba68d0d44c13f02a90466 has parents 923d925437f6317ba461154a102e8aa9eb902329 and 46c8c67a400f57031e3ec61298279470971071ce; after master advanced during validation, ce32695a047c2635e604815b30cf39a0cb80158a integrated 3bd437869a68814af19408de349567702ebe740a with parents 3a2b9ba2630a9e4ff22deefb0fc7fb2799cf2cb5 and 3bd437869a68814af19408de349567702ebe740a.

  • The semantic get_table.py conflict was resolved in favor of upstream's shared build_query_dict / validate_query_names / execute_tabular_query flow, preserving datasource name validation, time filters/grain/order construction, cache/force handling, standard execution, access validation, and Jinja seeding. Sunburst's strict bounded normalized envelope, coltype-aware metadata, structured errors, and final-response projection preflight are layered after that shared execution. query_dataset was re-inspected for the same semantic overlap and retains the shared upstream tabular-query path plus the same bounded response contract.

  • feat(mcp): add typed Gantt chart support #43737 was re-inspected at 6fd8ab5a13fa7481fd5b083544f74abf90e08ddc: it overlaps registry/query-result/compile/preview/update surfaces but remains Gantt-specific. If it lands first, reconcile its registered-chart additions with Sunburst while preserving this branch's shared exact-container/result budgets.

  • fix(mcp): preserve chart form data on update #43679 was re-inspected at 364f479c810f815bac51a8bee0a33a53c880bf54: it overlaps update_chart form-data preservation. If it lands first, retain its same-viz overlay behavior and Sunburst's complete target-owned query-role replacement, explicit clears, provenance, and rebind fail-closed rules.

  • fix(mcp): render virtual-dataset Jinja by seeding g.form_data in chart compile + preview (#40570) #43711 was re-inspected at e06be4f694a091bbc93af23fad69fd2e23c587e9: it overlaps compile/preview Jinja seeding. If it lands first, keep seeding before command construction/validation/run in every saved and unsaved compile/preview path without duplicating the upstream shared tabular-query seeding.

  • feat(mcp): add typed Bullet chart support #43770/Bullet and its active ROUND-19 fixer were inspected read-only at bc2ce63e549d4368858b5d06981cdbcec742c950; no Bullet file/ref/commit was touched or cherry-picked. Whichever shared-contract PR lands second must manually reconcile dataframe.py, query_result.py, SQL/data consumers, response metadata, and tests, preserving exact-type/no-hook projection, Period/Interval support, source/metadata/response budgets, strict infinity semantics, real envelopes, Jinja/cache/rebind ordering, and each chart type's discriminator/plugin behavior.

  • Fresh findings were implemented independently in 3a2b9ba2630a9e4ff22deefb0fc7fb2799cf2cb5: final dataset/semantic response projection bounds, exact trusted pandas Period/Interval normalization, hostile-producer-safe bounded get_chart_sql, and hook-free post-materialization nonfinite/missing projection in dataframe.py.

  • The branch was normal-pushed from 923d925437f6317ba461154a102e8aa9eb902329 to ce32695a047c2635e604815b30cf39a0cb80158a. No GitHub comments/reviews, Shortcut changes, Agor mutations, contributor/community branches, or other PR bodies were modified. A fresh independent review follows; this update does not claim review-clean status.

  • The fresh post-merge fixes started from exact clean head ce32695a047c2635e604815b30cf39a0cb80158a and were delivered in child commit 781698ba7c660d561759aff6a22495cc55c3b89e by a normal fast-forward push. Exact Decimal values now retain trusted numeric identity through result validation and are serialized by Pydantic as precision-preserving JSON strings; Sunburst accepts only exact finite int/float/Decimal values, so arbitrary numeric strings and nonfinite values remain rejected. Every public get_chart_sql success/error result crosses the complete-response preflight; the error-only extractor also preflights its QueryGenerationFailed result.

  • The active feat(mcp): add typed Bullet chart support #43770/Bullet Round-19 follow-up was compared read-only at 276f6511a6f4d0da7f4c620ee378053026c2be36. Agor branch 01a05dad-4b31-7188-b2ac-217d4b294374 remains in mcp chart types, with existing task 01a06379-4cc1-7008-8f56-775d914c4fa9 handling the same SQL-error final-preflight finding. Its shared result code independently retains exact Decimal values, and its SQL error-only branch still had the same direct return at the inspected head. No Bullet worktree, ref, file, commit, session, or PR body was changed.

  • During exact-head CI, upstream master advanced from the already-integrated 3bd437869a68814af19408de349567702ebe740a to 0ea234e9ae1bb8e0f8f93aafa3232de87085a5d3 through chore(deps-dev): bump @typescript-eslint/parser from 8.67.0 to 8.68.0 in /superset-websocket #43789 and chore(deps): bump github/codeql-action/analyze from 4.37.8 to 4.37.9 #43791; final delivery verification observed e7c42dd8ed8053a80b031fa8f6e84df8b4105b0d after chore(deps-dev): bump fast-uri from 3.1.5 to 3.1.7 in /superset-embedded-sdk #43796. The branch was intentionally not merged again, preserving the requested upstream merge and its 13 green required checks. A read-only git merge-tree --write-tree 781698ba... e7c42dd8... completed without conflicts (4dc295e384993058d1341fd0574813ca5791d1e9). GitHub reports MERGEABLE; BLOCKED is only REVIEW_REQUIRED, while its cached base/merge ref still names the integrated 3bd43786... base.

  • Local, Amin-fork, and live PR heads all match 781698ba7c660d561759aff6a22495cc55c3b89e, and the worktree is clean. Only Amin's Sunburst branch plus this testing/coordination body text changed. No GitHub comments/reviews, Shortcut/Agor mutations, contributor/community branches, or other PR bodies were touched. A fresh independent review follows; this update does not claim review-clean status.

  • Fresh strict get-data and exact-response follow-up started from the requested clean exact head 781698ba7c660d561759aff6a22495cc55c3b89e and was delivered by normal fast-forward pushes in eac84f7fb8331482ed1e3bac7df19130cdad7fa0 and be228ec3f7aeed11756be345124eaffd6c47839b. Saved, cached-update, and unsaved/form-data-key Sunburst data now resolve the form data used by the executed query, run strict Sunburst result validation exactly once immediately after generic envelope validation, and reject malformed hierarchy/aliases plus string/boolean metrics before profiling, metadata, exports, or response construction. Exact producer Decimal values remain numeric through validation and preserve their wire precision; finite builtin integers/floats retain the same contract.

  • Complete-response preflight now walks Pydantic's JSON-mode projection rather than its Python projection. This accounts UTC Z, naive and offset/fold timestamps, computed message/error duplication, Decimal/date/time/timedelta/UUID/bool/int values, and pydantic-core's shortest float exponent exactly. A 16,777,216-byte ChartError passes and the one-byte-larger response fails, while source-cell and metadata budgets are unchanged and projection failures return a bounded fixed error.

  • feat(mcp): add typed Bullet chart support #43770/Bullet was compared read-only at exact head 8a053c8ff905f3ed6997f0fd1873c2863532352d, including its completed independent review session 01a063b5-4215-73dc-9229-63ba43e107d1. Its shared response_json_failure still used Python-mode datetime projection at that inspected head, so the UTC exact-boundary correction in this Sunburst follow-up must be retained when the shared branches are reconciled. No Bullet worktree, local branch, file, commit, session, ref, PR body, comment, or review was changed.

  • Current upstream master was inspected at e8540b7c49583a8ca89cfbcfd95a6a6e6a66a6ed; git merge-tree --write-tree be228ec3... e8540b7c... completed conflict-free with tree ce7024af07b9d3f94e09de97ccbf393bf5259732. GitHub reports the exact PR head MERGEABLE; BLOCKED remains review policy only. Local, tracking, fork, and live PR heads match, the worktree is clean, and no GitHub comments/reviews, Shortcut/Agor mutations, contributor/community branches, or other PR bodies were touched. A fresh independent review follows; this update does not claim review-clean status.

  • Round 18 started from the requested exact clean local/fork/PR head 84dbac6dbea4387bd49045279d01b2ba99c8a2aa after read-only inspection of the mcp chart types Agor zone and confirmation that the independent review sessions were idle/completed, with no parallel same-branch fixer. Exact trusted Decimal NaN, sNaN, Infinity, and -Infinity are canonicalized to None immediately after real DataFrame record materialization; finite exact Decimal identity and precision are preserved. The exact-type check invokes Decimal.is_finite only for type(value) is Decimal, so hostile subclasses remain opaque and are rejected later by the strict result-envelope boundary without executing their numeric or equality hooks.

  • The fix and producer/consumer regressions were committed as 0ae605f5ce6df8a9bff0cee13aa0d42c4b7a70a5. Coverage spans real DataFrame and QueryContextProcessor materialization, actual saved/unsaved/cached Sunburst Decimal entry paths, an unsaved FastMCP ChartData wire path for all four non-finite Decimal variants, strict hostile-subclass rejection, synchronous SQL Lab serialization, asynchronous result-backend serialization, and the SQL Lab view consumer.

  • Upstream master d921131c5d95ccc3d442d4389d5e42acb234f017 was merged normally and conflict-free as delivered head 28bbacacd0421f79913761c922b0428a9be52f23, with parents 0ae605f5ce6df8a9bff0cee13aa0d42c4b7a70a5 and d921131c5d95ccc3d442d4389d5e42acb234f017; the final merge-tree is 3677c1507113f0c141790d7c30a7035ff4a6d9bc. After exact-head validation began, upstream advanced to unrelated OpenAPI regeneration b457280b57ac79be13877fb6ab1132cad75d1118; read-only reconciliation remains conflict-free (1d3cd322135696c3bd544ccfc9a73a64d036a01e) and no further merge was needed for this finding. Bullet feat(mcp): add typed Bullet chart support #43770 was compared read-only at 50106d7b81b342271a60fe6df09f1be17b6a5abf; no Bullet branch/ref/file/commit or PR body was changed or cherry-picked.

  • The two commits were normal-pushed only to Amin's sc-119162-mcp-sunburst. Only that branch and this PR's coordination/testing body sections were changed. No GitHub comments/reviews, Shortcut/Agor mutations, contributor/community branches, or other PR bodies were touched. A fresh independent review follows; this update does not claim review-clean status.

  • Round 19 started from the requested exact clean local/fork/PR head 28bbacacd0421f79913761c922b0428a9be52f23 after read-only inspection of the mcp chart types zone and confirmation that the Round 17–19 independent review sessions were idle, with no parallel same-branch fixer. The fix was committed as d8bff70c06616ffe44534c2f10bfb858ca1ab914.

  • get_chart_data, query_dataset, and semantic get_table now each expose one public wrapper that finalizes the complete ChartData | ChartError, QueryDatasetResponse | DatasetError, or GetTableResponse | SemanticLayerError result exactly once. Saved, cached-unsaved, validation, query/command, OAuth, export, empty-data, and dynamic exception returns all cross the same exact Pydantic JSON-wire gate. Oversized or uninspectable results map directly to fixed schema-specific errors under 1 KiB; no dynamic failure text is copied into the fallback and the fallback is not recursively preflighted.

  • The response-boundary matrix uses monkeypatched 4 KiB limits for route/schema/timestamp/fold/alias/amplification/exact/+1 cases while retaining one real 16,777,216-byte ChartError wire-parity allocation. The Python-unit timeout was changed from 30 to 40 minutes using the exact independently inspected Bullet workflow diff so the main coverage run plus SQL and semantic 100% gates can finish under runner variance. Bullet feat(mcp): add typed Bullet chart support #43770 was compared read-only at 6d0041f459aa319dad115d033fbf7b4b284ff338; feat(mcp): add typed Gantt chart support #43737, fix(mcp): preserve chart form data on update #43679, and fix(mcp): render virtual-dataset Jinja by seeding g.form_data in chart compile + preview (#40570) #43711 were rechecked read-only at 6fd8ab5a..., 364f479c..., and e06be4f6.... No related branch/ref/file/commit or PR body was modified or cherry-picked.

  • Upstream master b457280b57ac79be13877fb6ab1132cad75d1118 was merged normally and conflict-free as delivered head a84a870101d09e32117185803e9b004f20974fcb, with parents d8bff70c06616ffe44534c2f10bfb858ca1ab914 and b457280b57ac79be13877fb6ab1132cad75d1118. After merged-head validation, upstream advanced to unrelated report-screenshot fix 4a18556fdc0badac4a82857c06c153623b776480; read-only reconciliation remains conflict-free (37236a5cac1923856c79c07e067097245eb7f827) and no additional merge was needed for these findings.

  • The commits were normal-pushed only to Amin's sc-119162-mcp-sunburst. Only that branch and this PR's coordination/testing body sections were changed. No GitHub comments/reviews, Shortcut/Agor mutations, contributor/community branches, or other PR bodies were touched. A fresh independent review follows; this update does not claim review-clean status.

  • Round 20 started from the requested exact clean local/fork/PR head a84a870101d09e32117185803e9b004f20974fcb after read-only inspection of the mcp chart types zone and confirmation that the completed independent review was idle and no parallel same-branch fixer was running. The wrapper fix is commit 615b784f5ca0669bb64f1d8951c7ffd466fbb4b9.

  • Each public get_chart_data, query_dataset, and semantic get_table wrapper now catches exceptions raised while evaluating the producer call or during its await, records only a fixed server-side logger.exception message, constructs the declared union's fixed InternalError, and invokes the schema-specific finalizer exactly once. The fixed fallbacks remain below 1 KiB and do not contain, duplicate, or recursively amplify dynamic exception text. Real FastMCP tests exercise startup logging, cached-unsaved force/auth helpers, saved-chart selection, semantic metadata permission/selection, and synchronous-before-await producer failures.

  • Current upstream master 4a18556fdc0badac4a82857c06c153623b776480 was merged normally without conflict as delivered head 7d28f3a1c342140716a57407b685e23d115f6813. feat(mcp): add typed Bullet chart support #43770/Bullet was compared read-only at 6d0041f459aa319dad115d033fbf7b4b284ff338; overlapping feat(mcp): add typed Gantt chart support #43737, fix(mcp): preserve chart form data on update #43679, and fix(mcp): render virtual-dataset Jinja by seeding g.form_data in chart compile + preview (#40570) #43711 heads were compared read-only at 6fd8ab5a13fa7481fd5b083544f74abf90e08ddc, 364f479c810f815bac51a8bee0a33a53c880bf54, and e06be4f694a091bbc93af23fad69fd2e23c587e9. No code/ref was copied or modified on those branches.

  • The commits were normal-pushed only to Amin's sc-119162-mcp-sunburst. Only that branch and this PR's coordination/testing body sections were changed. No GitHub comments/reviews, Shortcut or Agor mutations, contributor/community branches, or other PR bodies were touched. A fresh independent review follows; this update does not claim review-clean status.

  • Round 22 started from the requested exact clean local/fork/PR head 414c7a6e74832fde0e33ccd7972c99e3c9528afe after read-only inspection of the Agor zones and confirmation that the completed reviewer was idle/archived and no same-branch fixer was running. The fix was committed as cc77c468e94940d8711bdb2d2650e2eb475da8db and normal-pushed only to Amin's sc-119162-mcp-sunburst branch.

  • Native Sunburst SIMPLE metrics now treat the nested frontend ColumnMeta as intentionally extensible: the complete object crosses exact-type, depth, container, key, per-string, aggregate-string, value-count, finite-number, and integer bounds without invoking arbitrary mapping/key/value hooks, then only required exact column_name (including columnName) and optional exact type are projected. Missing/malformed owned fields fail closed, while top-level metric typo rejection and the existing closed native XY contract remain unchanged. Public request, native round-trip, preview, save, update, and initialized FastMCP tests use the repository TestDataset shape and bounded forward-compatible fields.

  • Upstream master 18465bad51fae607d29b30f2c73d87fac14a920e was merged normally and without conflict as 5601e3f59714ba9ec9eb78b1b1cf2fd70e08d1fa, with parents 414c7a6e... and 18465bad.... During exact-head CI, upstream advanced to unrelated chart-list sorting bf12a21367ca10f6f29e553b3245f7d888d543f5; read-only reconciliation remains conflict-free with merge tree 2242e04ac2d4a3782591b1a01d0a1af07eb5f239, so no additional merge was needed and the green exact-head CI was preserved. Clean Bullet feat(mcp): add typed Bullet chart support #43770 was compared read-only at 6d0041f459aa319dad115d033fbf7b4b284ff338; feat(mcp): add typed Gantt chart support #43737, fix(mcp): preserve chart form data on update #43679, and fix(mcp): render virtual-dataset Jinja by seeding g.form_data in chart compile + preview (#40570) #43711 were rechecked read-only at 6fd8ab5a..., 364f479c..., and e06be4f6.... No related branch/ref/file/commit was modified or cherry-picked. No comments/reviews, Shortcut/Agor mutations, contributor/community branches, or other PR bodies were touched. A fresh independent review follows; this update does not claim review-clean status.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Not applicable; this is MCP backend/schema/query support with no frontend UI change.

TESTING INSTRUCTIONS

Validated with SQLAlchemy 2.0.52 at exact delivered head ce32695a047c2635e604815b30cf39a0cb80158a.

PYTHONPATH=./superset-core/src pytest -q \
  tests/unit_tests/dataframe_test.py \
  tests/unit_tests/mcp_service/chart/test_query_result.py \
  tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py \
  tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py \
  tests/unit_tests/mcp_service/semantic_layer/tool/test_get_table.py \
  tests/unit_tests/common/test_tabular_query.py
# 378 passed on exact head

PYTHONPATH=./superset-core/src pytest -q \
  tests/unit_tests/mcp_service \
  tests/unit_tests/dataframe_test.py \
  tests/unit_tests/common/test_query_context_processor.py \
  tests/unit_tests/common/test_query_context_processor_timing.py \
  tests/unit_tests/common/test_query_actions.py \
  tests/unit_tests/common/test_query_cache_manager.py \
  tests/unit_tests/common/test_tabular_query.py
# 4,437 passed on exact head

# Staged branch-owned files: auto-walrus, MyPy, Ruff format/check, Pylint,
# license/large-file/debug/whitespace and remaining applicable hooks.
pre-commit run
# passed

python -m compileall -q \
  superset/dataframe.py superset/common/tabular_query.py \
  superset/mcp_service/chart/query_result.py \
  superset/mcp_service/chart/tool/get_chart_sql.py \
  superset/mcp_service/dataset/tool/query_dataset.py \
  superset/mcp_service/semantic_layer/tool/get_table.py
git diff --check origin/master...HEAD
# passed

Adversarial and boundary coverage includes actual FastMCP dataset/semantic entry points where a real DataFrame → ChartDataCommand source envelope fits under 16 MiB but duplicated column samples push the projected response above 16 MiB; both return structured InvalidQueryResult before transport. Real producer envelopes normalize exact pandas Period and Interval to bounded canonical strings across the shared chart consumer plus dataset and semantic tools. SQL tests cover exact dict/list/string access, hostile mapping/list/string/error/exception subclasses, malformed entries, the shared query-count limit, aggregate source bytes, >64 KiB real SQL, multi-query prefixes, and the near-16 MiB response boundary. DataFrame tests prove object-column hostile equality and Enum equality hooks are not invoked while trusted builtin/NumPy nonfinites and pandas missing values retain their null contract.

The full shared suite re-covers the exact 65,537-byte source-cell failure; SQL/query metadata above 64 KiB with independent 1 MiB metadata and 16 MiB response budgets; [True, 1, False, 0] -> unique_count=4; date/timedelta/UUID accounting and exports; cycles; completeness; Jinja/cache/rebind order; strict none_as_empty=False; finite Sunburst metrics; real envelopes; and chart/query parity. No branch-owned frontend files differ from master, so frontend-specific local tests are not applicable. The development health endpoint was unavailable; this is an environment limitation.

Exact-head GitHub CI completed on ce32695a047c2635e604815b30cf39a0cb80158a: all 13 required checks passed, including frontend build, pre-commit, lint, current unit tests, MySQL/PostgreSQL/SQLite integration, Presto/Hive, Cypress, Playwright, dependency review, and migration-head enforcement. The complete rollup was 62 successful, 9 skipped, 3 neutral, 2 failed, 0 pending. The only failures were the non-required Python and JavaScript CodeQL analysis jobs; both failed before analysis with the upstream workflow/version mismatch Loaded a configuration file for version '4.37.9', but running version '4.37.8' because merged master pins codeql-action/init 4.37.9 while analyze remains 4.37.8. This is CI infrastructure/configuration, not a code/test failure in this PR.

Fresh post-merge Decimal and SQL-error validation

Validated with Python 3.11 and SQLAlchemy 2.0.52 at exact delivered head 781698ba7c660d561759aff6a22495cc55c3b89e:

export PYTHONPATH="$PWD/superset-core/src${PYTHONPATH:+:$PYTHONPATH}"
PY=/home/agorpg/tmp/sc119154-review-venv/bin/python

$PY -m pytest -q \
  tests/unit_tests/mcp_service/chart/test_query_result.py \
  tests/unit_tests/mcp_service/chart/test_sunburst_chart.py \
  tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py \
  tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py \
  tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py
# 681 passed

$PY -m pytest -q tests/unit_tests/mcp_service
# 4,334 passed

$PY -m pytest -q \
  tests/unit_tests/dataframe_test.py \
  tests/unit_tests/common/test_dataframe_utils.py \
  tests/unit_tests/common/test_query_context_factory.py \
  tests/unit_tests/common/test_query_context_processor.py \
  tests/unit_tests/common/test_query_context_processor_timing.py \
  tests/unit_tests/common/test_query_object_factory.py \
  tests/unit_tests/common/test_form_data_query_context.py \
  tests/unit_tests/queries/query_object_test.py \
  tests/unit_tests/charts/data/form_data_test.py \
  tests/unit_tests/charts/data/test_empty_query_context.py
# 222 passed

ruff format --check <7 changed Python files>
ruff check <7 changed Python files>
pre-commit run mypy --files <7 changed Python files>
$PY -m pylint --rcfile=.pylintrc <3 changed production modules>
$PY -m py_compile <7 changed Python files>
pre-commit run
git diff --check
git diff --cached --check
# all passed; Pylint 10.00/10

The producer-to-consumer coverage uses real DataFrames through QueryContextProcessor.get_data() and real ChartDataCommand.run() envelopes. It covers saved, SIMPLE, and SQL metric aliases; primary and secondary metrics; compile; saved/unsaved ASCII and table previews; saved/unsaved get-data; exact precision-preserving JSON projection; Decimal digit and positive/negative exponent boundaries; nonfinite/oversized Decimals; and strict rejection of numeric strings. SQL coverage includes a 16 MiB error-only source that would otherwise duplicate to about 32 MiB, 16 MiB + 1, realistic bounded and hostile oversized errors, every public final response branch, bounded InvalidQueryResult fallback, and a one-pass/no-recursion assertion. The full suites retain the prior dataset/semantic final preflight, Period/Interval, hostile SQL extraction, hook-free DataFrame projection, upstream tabular-query reconciliation, source/metadata/final budgets, bool/int/timedelta/scalar/timezone/cycle/profile/UUID/Jinja/cache/rebind/query parity, and strict Sunburst none_as_empty=False coverage.

No branch-owned frontend file differs from the integrated upstream base. A local Sunburst Jest invocation could not start because this worktree has no node_modules/cross-env; the exact-head required frontend-build GitHub job passed. The localhost health endpoint was also unavailable, so live/manual UI validation was not possible.

Exact-head GitHub CI is complete: all 13 required checks passed. The full rollup is 58 successful, 9 skipped, 4 neutral, 2 failed, 0 pending. The two failures are non-required CodeQL Python/JavaScript jobs that exited before analysis with Loaded a configuration file for version '4.37.9', but running version '4.37.8'. Upstream #43791 aligned the analyze action to 4.37.9 after this exact-head run; these failures are CI workflow infrastructure, not code-analysis findings from this change.

Fresh strict get-data and exact-response validation

Validated with Python 3.11 and SQLAlchemy 2.0.52 at exact delivered head be228ec3f7aeed11756be345124eaffd6c47839b:

export PYTHONPATH="$PWD/superset-core/src${PYTHONPATH:+:$PYTHONPATH}"

uv run --frozen --no-sync pytest -q \
  tests/unit_tests/mcp_service/chart/test_sunburst_chart.py \
  tests/unit_tests/mcp_service/chart/test_compile.py \
  tests/unit_tests/mcp_service/chart/test_preview_utils.py \
  tests/unit_tests/mcp_service/chart/test_query_result.py \
  tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py \
  tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py \
  tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py \
  tests/unit_tests/mcp_service/chart/tool/test_update_chart.py \
  tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py
# 876 passed

uv run --frozen --no-sync pytest -q tests/unit_tests/mcp_service
# 4,348 passed

uv run --frozen --no-sync pytest -q \
  tests/unit_tests/common/test_form_data_query_context.py \
  tests/unit_tests/common/test_query_context_factory.py \
  tests/unit_tests/common/test_query_context_processor.py \
  tests/unit_tests/common/test_query_context_processor_timing.py \
  tests/unit_tests/common/test_tabular_query.py \
  tests/unit_tests/dataframe_test.py
# 207 passed

pre-commit run
python -m compileall -q \
  superset/mcp_service/chart/query_result.py \
  superset/mcp_service/chart/tool/get_chart_data.py \
  tests/unit_tests/mcp_service/chart/test_query_result.py \
  tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
git diff --check
git diff --cached --check
# passed: staged MyPy, Ruff/format, Pylint, remaining applicable hooks,
# compileall, and diff checks

Actual FastMCP entry coverage spans saved JSON, unsaved JSON/CSV, and identifier-plus-cached-update CSV/Excel paths. It proves numeric strings, booleans, malformed hierarchy values, and ambiguous aliases fail before export, while producer Decimal primary/secondary metrics reach the strict validator once with identity and precision intact. Response tests exercise exact 16 MiB and +1 ChartError wires for UTC Z, naive timestamps, and both ZoneInfo fold offsets; timestamped ChartData; supported scalar projections; computed error duplication; and bounded serialization fallback. The full suites retain SQL error preflight, dataset/semantic response bounds, Period/Interval, hostile extraction, hook-free DataFrame projection, tabular-query semantics, scalar/timezone/timedelta/date/UUID/bool-int/cycle/profile/export/cache/Jinja/update/rebind/query parity, and strict Sunburst none_as_empty=False.

The local frontend dependency install could not start tests because this environment has Node 22/npm 10 while the checkout requires Node 24/npm 11, and the checked-in lock is missing @noble/hashes@2.4.0; no tracked frontend file was modified. The exact-head frontend-build CI job passed. The configured localhost health endpoint was unavailable, so live UI smoke testing was not possible.

Exact-head GitHub CI completed with all 13 required checks passing. At final verification, every completed exact-head check run was successful, skipped, or neutral, with 0 failed and 0 pending; frontend build, pre-commit/lint, current unit tests, MySQL/PostgreSQL/SQLite, Presto/Hive, Cypress/Playwright, dependency review, migration enforcement, Codecov, and both Python/JavaScript CodeQL analyses passed. The prior exact-head CodeQL 4.37.9/4.37.8 mismatch was separately confirmed as workflow infrastructure; upstream #43791 aligned the actions, and CodeQL is green on this delivered head.

Round 17 response preflight and Decimal cardinality

Round 17 started from the requested exact clean local/live head be228ec3f7aeed11756be345124eaffd6c47839b, after confirming the independent review session was idle/completed and no same-branch fixer was running. The fixes were committed as 68d77b4d575b3933e409a288074d979e7c25d143. Upstream master e8540b7c49583a8ca89cfbcfd95a6a6e6a66a6ed was merged normally without conflict as 8feda98d4760af0e4cd7a7baea13165193c6e352; after upstream advanced during validation, master a81c0f677393656e61e436f5bd51d675b757df58 was also merged normally without conflict as 8b204b5cb5b0b7fca2db837eaec0a2cec4fe5aaf. A final upstream advance to 52a4cb6314aea332f8061a7b3babd814cdebc33b was reconciled in the same way, producing delivered head 84dbac6dbea4387bd49045279d01b2ba99c8a2aa.

A shared schema-preserving response finalizer now applies the exact 16 MiB Pydantic JSON-wire gate to every public success and error return from generate_chart, update_chart, get_chart_preview, and update_chart_preview. GenerateChartResponse failures remain in that schema, ChartPreview failures become bounded ChartError values, and the legacy dict-shaped update-preview result is measured through an explicit Pydantic RootModel. Coverage includes exact-limit acceptance and +1 rejection through actual FastMCP entry calls, serialization aliases, saved/unsaved and cached preview routing, nested previews, complete form_data, repeated/amplified payloads, and bounded structured fallbacks. Bullet PR #43770 was inspected read-only at c3c07fda; its response_preflight.py approach informed comparison only, with semantics independently reconciled and no Bullet code or refs modified.

The shared metadata identity builder now reduces trusted finite exact Decimal values to bounded canonical rational tokens. Repeated and scale-equivalent Decimals coalesce correctly; exact integer and exactly equal binary-float values intentionally share the same numeric identity, while unequal exact values such as Decimal("0.1") and binary float 0.1 remain distinct. Digit/exponent work is capped and hostile Decimal subclasses remain opaque without invoking their hooks. Actual chart-data, dataset-query, and semantic-table producer/consumer tests assert the same cardinality behavior.

Merged-head validation:

  • Focused chart producer/consumer and actual generate/update/preview/data/SQL entry matrix: 1,041 passed.
  • Full MCP service: 4,364 passed.
  • Common DataFrame, QueryContext, QueryObject, and tabular-query suite under SQLAlchemy 2.0.52: 217 passed.
  • Python compilation, Ruff format/check, MyPy, Pylint, staged and merged-head pre-commit checks, and diff checks passed.
  • Local frontend dependencies are absent and the checkout requires newer Node/npm than the available Node 22/npm 10, so frontend execution remained an environment limitation; exact-head frontend build and required E2E CI passed.
  • Exact-head GitHub CI completed with every check successful, skipped, or neutral and no failures or pending jobs. All 13 current required checks passed, including frontend build, pre-commit, current unit tests, MySQL/PostgreSQL/SQLite, Presto/Hive, Cypress/Playwright, dependency review, and migration enforcement. Python and JavaScript CodeQL analyses and the CodeQL aggregate passed.

Local, tracking, Amin fork, and live PR heads were verified at 84dbac6dbea4387bd49045279d01b2ba99c8a2aa; GitHub reports the PR mergeable, with the review/protection gate as the only blocked state. The worktree is clean. No PR comments/reviews or other branch, Shortcut, or Agor mutations were made. A fresh independent review follows; this section does not claim review-clean.

Round 18 Decimal non-finite producer validation

Validated with Python 3.11 and SQLAlchemy 2.0.52 at exact delivered head 28bbacacd0421f79913761c922b0428a9be52f23:

python -m pytest -q \
  tests/unit_tests/dataframe_test.py \
  tests/unit_tests/common/test_query_context_processor.py \
  tests/unit_tests/mcp_service/chart/test_query_result.py \
  tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py \
  tests/unit_tests/sql/execution/test_celery_task.py \
  tests/unit_tests/sql_lab_test.py
# 446 passed

python -m pytest -q \
  tests/unit_tests/common \
  tests/unit_tests/dataframe_test.py \
  tests/unit_tests/jinja_context_test.py
# 446 passed

python -m pytest -q tests/unit_tests/mcp_service
# 4,365 passed

pre-commit run --files \
  superset/dataframe.py \
  tests/unit_tests/dataframe_test.py \
  tests/unit_tests/mcp_service/chart/test_query_result.py \
  tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py \
  tests/unit_tests/sql/execution/test_celery_task.py \
  tests/unit_tests/sql_lab_test.py
python -m compileall -q <the 6 changed Python files>
git diff --check origin/master...HEAD
git show --check HEAD
# passed: MyPy, Ruff format/check, Pylint 10.00/10, all applicable
# pre-commit hooks, compilation, and diff checks

The real producer/consumer paths prove quiet/signaling NaN and positive/negative Infinity become JSON null before strict serialization; high-precision finite Decimal identity survives producer validation and reaches the Pydantic wire unchanged; and a hostile Decimal subclass is neither classified nor coerced by DataFrame projection and fails closed at the bounded result-envelope boundary without hooks. The full suites retain exact final-response gates, Decimal value-semantic cardinality, strict Sunburst result validation, Pydantic-wire budgets, source/metadata bounds, saved/unsaved/cache/update/export/query/Jinja contracts, QueryContext/QueryObject/tabular behavior, and SQL Lab synchronous/asynchronous/view serialization.

Exact-head GitHub CI has 12 of 13 required checks passing. The sole non-passing required check is the current Python unit-test aggregate: its test job was canceled by the workflow hard 30-minute limit on six attempts at roughly 90–91% completion, without a reported test failure, so the aggregate reports failure from RESULT: cancelled. The full local MCP service suite passed. Every other required check passed—frontend build, pre-commit/lint, MySQL/PostgreSQL/SQLite, Presto/Hive, Cypress/Playwright, dependency review, and migration enforcement—and both Python/JavaScript CodeQL analyses passed. This timeout is reported explicitly rather than represented as green.

Round 19 complete-return preflight and runtime validation

Validated with Python 3.11 and SQLAlchemy 2.0.52 at exact delivered head a84a870101d09e32117185803e9b004f20974fcb:

PYTHONPATH="$PWD/superset-core/src" python -m pytest -q \
  tests/unit_tests/mcp_service/chart/test_response_preflight.py \
  tests/unit_tests/mcp_service/chart/test_query_result.py \
  tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py \
  tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py \
  tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py \
  tests/unit_tests/mcp_service/semantic_layer/tool/test_get_table.py
# 518 passed in 33.49s

PYTHONPATH="$PWD/superset-core/src" python -m pytest -q \
  tests/unit_tests/mcp_service/chart/test_response_preflight.py \
  tests/unit_tests/mcp_service/chart/test_query_result.py::test_response_json_failure_matches_exact_chart_error_wire_boundary \
  tests/unit_tests/mcp_service/chart/test_query_result.py::test_response_json_failure_real_16_mib_wire_boundary
# 17 passed in 5.25s

PYTHONPATH="$PWD/superset-core/src" python -m pytest -q tests/unit_tests/mcp_service
# 4,375 passed in 5m14s

PYTHONPATH="$PWD/superset-core/src" python -m pytest -q \
  tests/unit_tests/common \
  tests/unit_tests/queries/query_object_test.py \
  tests/unit_tests/jinja_context_test.py \
  tests/unit_tests/dataframe_test.py \
  tests/unit_tests/sql \
  tests/unit_tests/sql_lab_test.py
# 1,981 passed, 1 xfailed in 40.68s

pre-commit run --files <11 Round-19 files>
python -m compileall -q <10 changed Python files>
git diff --check origin/master...HEAD
git show --check HEAD
# passed: MyPy, Ruff format/check, Pylint, YAML/zizmor, all applicable
# pre-commit hooks, compilation, and diff checks

Actual FastMCP Client.call_tool tests cover exact-limit preservation, +1 rejection, and dynamically amplified exception errors for all three public tools, including duplicated message/error schemas and timestamp projection. Existing success, cached-unsaved, dataset/semantic source validation, export, strict Sunburst, Decimal normalization/cardinality, query-result/source/metadata budgets, Jinja/tabular/cache/update/rebind/profiling contracts, SQL Lab serialization, and cross-chart suites remain covered.

Exact-head GitHub CI completed with all 13 required checks passing. The current Python unit job finished in 24m19s: the main suite completed with 14,639 passed, 4 skipped, and 3 xfailed in 19m44s; the SQL gate completed with 1,487 passed, 1 xfailed and 100% coverage; the semantic-layer gate completed with 415 passed and 100% coverage; Codecov upload succeeded; and the required aggregate passed. Frontend build, pre-commit/lint, MySQL/PostgreSQL/SQLite, Presto/Hive, Cypress/Playwright, dependency review, migration enforcement, Codecov project/patch, and both Python/JavaScript CodeQL analyses also passed, with no failed or pending exact-head checks.

Round 20 uncaught producer exception boundary

Validated with Python 3.11 and SQLAlchemy 2.0.52 at exact delivered head 7d28f3a1c342140716a57407b685e23d115f6813:

python -m pytest -q \
  tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py::test_get_chart_data_mcp_entry_bounds_uncaught_dynamic_exception \
  tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py::test_query_dataset_mcp_entry_bounds_uncaught_dynamic_exception \
  tests/unit_tests/mcp_service/semantic_layer/tool/test_get_table.py::test_get_table_mcp_entry_bounds_uncaught_dynamic_exception
# 9 passed in 5.32s

python -m pytest -q \
  tests/unit_tests/mcp_service/chart/test_response_preflight.py \
  tests/unit_tests/mcp_service/chart/test_query_result.py \
  tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py \
  tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py \
  tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py \
  tests/unit_tests/mcp_service/semantic_layer/tool/test_get_table.py
# 527 passed in 33.79s

python -m pytest -q tests/unit_tests/mcp_service
# 4,384 passed in 5m01s

python -m pytest -q \
  tests/unit_tests/common \
  tests/unit_tests/dataframe_test.py \
  tests/unit_tests/queries/query_object_test.py \
  tests/unit_tests/jinja_context_test.py \
  tests/unit_tests/sql_lab_test.py \
  tests/unit_tests/semantic_layers \
  tests/unit_tests/commands/semantic_layer
# 970 passed in 44.44s

python -m pytest -q tests/unit_tests/sql
# 1,487 passed, 1 xfailed in 19.22s

python -m pytest -q \
  tests/unit_tests/connectors/sqla \
  tests/unit_tests/commands/sql_lab \
  tests/unit_tests/models/sql_lab_test.py \
  tests/unit_tests/views/test_sqla.py \
  tests/unit_tests/views/test_sql_lab_tab_state_views.py
# 173 passed in 10.63s

pre-commit run
python -m compileall -q <MCP/common/DataFrame and changed test paths>
ruff format --check <6 Round-20 files>
ruff check <6 Round-20 files>
git diff --check origin/master...HEAD
git show --check 615b784f5ca0669bb64f1d8951c7ffd466fbb4b9
# passed: MyPy, Ruff format/check, Pylint, all applicable pre-commit hooks,
# compilation, and diff checks

The initialized FastMCP Client.call_tool cases inject oversized RuntimeError text both before an awaitable is returned and from real helpers during producer execution. They validate ChartError, DatasetError, and SemanticLayerError, their InternalError discriminator, structured content and compatibility aliases, sub-1-KiB Pydantic wire size, absence of dynamic/secret text from both wire output and the fixed log record, and exactly one finalizer call. Existing exact-limit/+1, success, known-error, cached/unsaved/export, strict Sunburst, Decimal/provenance/cardinality, DataFrame/timezone, query-result/source/metadata budgets, preview/generate/update, SQL/Jinja/tabular, cache/update/rebind, profiling, and cross-chart paths remain covered by the focused and full suites.

Exact-head GitHub CI completed with all 13 required checks passing. The current Python unit job completed in 29m58s under the coordinated 40-minute workflow budget: the main suite completed with 14,648 passed, 4 skipped, and 3 xfailed in 24m55s; the SQL gate completed with 1,487 passed, 1 xfailed and 100% coverage; the semantic-layer gate completed with 415 passed and 100% coverage; Codecov upload and the required aggregate passed. Frontend build, pre-commit/lint, MySQL/PostgreSQL/SQLite, Presto/Hive, Cypress/Playwright, dependency review, migration enforcement, Codecov, and both Python/JavaScript CodeQL analyses passed; the exact-head rollup has no failed checks.

The configured localhost health endpoint was unavailable, so live UI smoke testing was not possible; no branch-owned frontend file changed, and exact-head frontend CI passed. Local, tracking, Amin-fork, and live PR heads match 7d28f3a1c342140716a57407b685e23d115f6813; GitHub reports MERGEABLE, with BLOCKED reflecting review/protection policy. The worktree is clean. A fresh independent review follows; this section does not claim review-clean.

Round 21 hostile failure containment and dateutil fidelity

Round 21 started from the requested exact clean local/live head 7d28f3a1c342140716a57407b685e23d115f6813, after inspecting the Agor zones and confirming the completed independent reviewer was idle/archived and no same-branch fixer was running. The fix was committed and normally pushed as 414c7a6e74832fde0e33ccd7972c99e3c9528afe.

get_chart_data, query_dataset, and semantic get_table now delegate to one schema-specific containment/finalization boundary. Inner handling is limited to known domain exceptions and uses bounded exact primitive arguments rather than exception formatting. Unexpected Exception values reach the outer boundary, which emits a fixed, non-dynamic, traceback-free best-effort log record and a declared ChartError, DatasetError, or SemanticLayerError with InternalError. The finalizer is invoked exactly once inside its own guard; logger or finalizer failure returns the static sub-1-KiB declared fallback directly without recursive finalization. CancelledError, KeyboardInterrupt, SystemExit, and other BaseException values continue to propagate. Initialized real FastMCP tests cover hostile ValueError/RuntimeError, context-manager enter/exit failures, logger/finalizer failures, cached/unsaved and resolution stages, structured aliases, fixed logs, absence of secret text, zero hostile __str__/__repr__ calls, and unchanged success/known-error behavior.

Exact dateutil tzfile values are no longer reinterpreted through the host ZoneInfo database. A shared bounded, hook-free transition-state reader validates exact transition containers, transition-record identities and fields, standard/before selection, UTC-to-wall consistency, ordering, offsets, and table limits, then mirrors dateutil's own fold/gap/post-final-transition selection while replacing the source timezone only with its selected fixed offset. Coverage includes Dublin negative-DST fold, New York spring gap, the 2040 post-final-transition case, historical transition boundaries, equivalent instants with distinct source offsets, system and packaged transitionless zones, invalid/mutated tables, and hostile timezone methods through the shared helper and real chart/dataset/semantic FastMCP producers. Clean Bullet PR #43770 was compared read-only at 6d0041f459aa319dad115d033fbf7b4b284ff338; overlap PRs #43737, #43679, and #43711 were also compared at their live heads. Nothing was cherry-picked or modified outside Amin's branch.

Exact delivered-head validation used Python 3.11 and SQLAlchemy 2.0.52:

python -m pytest -q \
  tests/unit_tests/mcp_service/chart/test_response_preflight.py \
  tests/unit_tests/mcp_service/chart/test_query_result.py \
  tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py \
  tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py \
  tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py \
  tests/unit_tests/mcp_service/semantic_layer/tool/test_get_table.py
# 573 passed in 37.06s

python -m pytest -q tests/unit_tests/mcp_service
# 4,431 passed in 5m01s

python -m pytest -q \
  tests/unit_tests/common tests/unit_tests/dataframe_test.py \
  tests/unit_tests/queries/query_object_test.py \
  tests/unit_tests/jinja_context_test.py tests/unit_tests/sql_lab_test.py \
  tests/unit_tests/semantic_layers tests/unit_tests/commands/semantic_layer \
  tests/unit_tests/sql tests/unit_tests/connectors/sqla \
  tests/unit_tests/commands/sql_lab tests/unit_tests/models/sql_lab_test.py \
  tests/unit_tests/views/test_sqla.py \
  tests/unit_tests/views/test_sql_lab_tab_state_views.py
# 2,630 passed, 1 xfailed in 74.42s

pre-commit run --from-ref HEAD^ --to-ref HEAD
python -m compileall -q superset/mcp_service tests/unit_tests/mcp_service
git diff --check origin/master...HEAD
git show --check HEAD
# passed: MyPy, Ruff format/check, Pylint, all applicable pre-commit hooks,
# compilation, and diff checks

Exact-head GitHub CI completed with all 13 required checks passing and no failed or pending checks. The current Python unit job completed in 24m25s under the coordinated 40-minute budget: the main suite completed with 14,695 passed, 4 skipped, and 3 xfailed in 19m52s; the SQL gate completed with 1,487 passed, 1 xfailed and 100% coverage; the semantic-layer gate completed with 415 passed and 100% coverage; Codecov upload and the required aggregate passed. Frontend build, pre-commit/lint, MySQL/PostgreSQL/SQLite, Presto/Hive, Cypress/Playwright, dependency review, migration enforcement, and Python/JavaScript CodeQL passed.

Current upstream master 4a18556fdc0badac4a82857c06c153623b776480 is already an ancestor of the delivered head; the read-only merge-tree completed without conflict and GitHub reports MERGEABLE (BLOCKED only by review/protection policy). Local, tracking, Amin-fork, and live PR heads match 414c7a6e74832fde0e33ccd7972c99e3c9528afe, and the worktree is clean. No PR comments/reviews or other branch, Shortcut, or Agor mutations were made. A fresh independent review follows; this section does not claim review-clean.

Round 22 frontend ColumnMeta compatibility

Validated with Python 3.11 and SQLAlchemy 2.0.52 at exact delivered head cc77c468e94940d8711bdb2d2650e2eb475da8db:

PYTHONPATH="$PWD/superset-core/src" python -m pytest -q \
  tests/unit_tests/mcp_service/chart/test_sunburst_chart.py \
  tests/unit_tests/mcp_service/explore/tool/test_generate_explore_link.py
# 287 passed in 12.10s

PYTHONPATH="$PWD/superset-core/src" python -m pytest -q tests/unit_tests/mcp_service
# 4,456 passed in 4m58s

PYTHONPATH="$PWD/superset-core/src" python -m pytest -q \
  tests/unit_tests/common tests/unit_tests/dataframe_test.py \
  tests/unit_tests/queries/query_object_test.py \
  tests/unit_tests/jinja_context_test.py tests/unit_tests/sql_lab_test.py \
  tests/unit_tests/semantic_layers tests/unit_tests/commands/semantic_layer \
  tests/unit_tests/sql tests/unit_tests/connectors/sqla \
  tests/unit_tests/commands/sql_lab tests/unit_tests/models/sql_lab_test.py \
  tests/unit_tests/views/test_sqla.py \
  tests/unit_tests/views/test_sql_lab_tab_state_views.py
# 2,630 passed, 1 xfailed in 75.24s

pre-commit run
python -m compileall -q superset/mcp_service tests/unit_tests/mcp_service
ruff format --check <3 Round-22 files>
ruff check <3 Round-22 files>
git diff --check origin/master...HEAD
git show --check HEAD
# passed: MyPy, Ruff format/check, Pylint, all applicable pre-commit hooks,
# compilation, and diff checks

Coverage includes complete and reduced frontend metric shapes; column_name/columnName and optional type; unknown forward-compatible nested metadata; top-level and owned-field typos; oversized/deep mappings, lists, keys, strings, aggregate values, and integers; non-finite floats; hostile dict/string/value subclasses with zero hook calls; saved and SQL metric behavior; native round-trip; direct preview/save/update product paths; and a real FastMCP Client.call_tool Explore-link path. The full suites retain the Round-21 containment/finalizer/dateutil contract, strict Sunburst result and Decimal behavior, exact response/source/metadata budgets, DataFrame, SQL/Jinja/tabular, cache, provenance, rebind/update, and cross-chart behavior.

No branch-owned frontend file changed. Local frontend dependencies are absent, and the checkout requires Node 24/npm 11 while the environment provides Node 22/npm 10, so frontend execution is an environment limitation; exact-head frontend CI is used for that validation. Exact-head GitHub CI completed with all 13 required checks passing and no failed or pending checks. The current Python unit job completed in 30m20s under the coordinated 40-minute budget: the main suite completed with 14,726 passed, 4 skipped, and 3 xfailed in 25m23s; the SQL gate completed with 1,487 passed, 1 xfailed and 100% coverage; the semantic-layer gate completed with 415 passed and 100% coverage; Codecov upload and the required aggregator passed. Frontend build, pre-commit/lint, MySQL/PostgreSQL/SQLite, Presto/Hive, Cypress/Playwright, dependency review, migration enforcement, and Codecov project/patch checks passed. Python and JavaScript CodeQL analyses passed separately.

Round 23 native metric wrapper hardening

Round 23 started from the requested exact clean local/tracking/fork/PR head cc77c468e94940d8711bdb2d2650e2eb475da8db. Read-only Agor session/board/zone inspection confirmed the completed independent reviewer was idle/archived and no same-PR/head fixer or reviewer was modifying the branch. Delivery consists of normal-pushed commits c14684d8ece58f110c22092cfc3188264788090c, 4a6990264829e2e468eaf7d4a2ea9f3101b57887, f4635a6309f6de297a6fc2672d50ccdf9804beb6, and final head 506c5cb2f1af779250a71ed002882d0a5c334c94.

The Sunburst native-metric discriminator now inspects only exact built-in dictionaries through built-in operations, bounds the wrapper before scanning, requires exact bounded string keys, and requires an exact built-in expressionType before comparing it against the closed SIMPLE/SQL set. Closed top-level metric fields and bounded values remain strict, while canonical typed ColumnRef input remains accepted. Hostile dictionary/string subclasses are removed from both working and original Pydantic inputs before a validation error can render them, so registered FastMCP validation and version-specific error wrapping cannot execute __hash__, __eq__, __str__, __repr__, mapping, or iteration hooks. SunburstNativeMetricColumn still accepts its bounded open-ended frontend metadata, but when both owned aliases are present it requires exact strings with identical values; either alias alone remains valid.

Validated with Python 3.11, SQLAlchemy 2.0.52, and the CI-minimum FastMCP 3.4.7 at exact delivered head:

PYTHONPATH="$PWD/superset-core/src" python -m pytest -q \
  tests/unit_tests/mcp_service/chart/test_sunburst_chart.py \
  tests/unit_tests/mcp_service/explore/tool/test_generate_explore_link.py
# 320 passed in 13.07s

PYTHONPATH="$PWD/superset-core/src" python -m pytest -q tests/unit_tests/mcp_service
# 4,490 passed in 4m57s

PYTHONPATH="$PWD/superset-core/src" python -m pytest -q \
  tests/unit_tests/common tests/unit_tests/dataframe_test.py \
  tests/unit_tests/queries/query_object_test.py \
  tests/unit_tests/jinja_context_test.py tests/unit_tests/sql_lab_test.py \
  tests/unit_tests/semantic_layers tests/unit_tests/commands/semantic_layer \
  tests/unit_tests/sql tests/unit_tests/connectors/sqla \
  tests/unit_tests/commands/sql_lab tests/unit_tests/models/sql_lab_test.py \
  tests/unit_tests/views/test_sqla.py \
  tests/unit_tests/views/test_sql_lab_tab_state_views.py
# 2,630 passed, 1 xfailed in 73.62s

pre-commit run
python -m compileall -q superset/mcp_service tests/unit_tests/mcp_service
ruff format --check <3 Round-23 files>
ruff check <3 Round-23 files>
python -m pylint --rcfile=.pylintrc superset/mcp_service/chart/schemas.py
git diff --check origin/master...HEAD
git show --check HEAD
# passed: MyPy, Ruff format/check, Pylint, all applicable pre-commit hooks,
# compilation, and diff checks

Coverage includes direct models, model_validate_json, Generate/Update/Explore-link schemas, native preview/save/update round-trips inherited from the focused matrix, and real registered FastMCP calls for complete frontend metadata, equal/conflicting/malformed dual aliases, missing/malformed expression types, valid SIMPLE/SQL forms, hostile keys and values, rendered validation errors, and zero hostile hook calls. The full MCP and broad producer/consumer suites retain the strict Sunburst hierarchy/finite/Decimal contracts, exact response finalization and budgets, DataFrame/pandas/NumPy/dateutil handling, saved/unsaved/cache/update-preview/export paths, SQL/Jinja/tabular semantics, provenance, rebind/update, and cross-chart behavior.

Exact-head CI completed with all 13 required checks passing. The Python unit workflow completed successfully in 34m02s under the 40-minute budget: the main suite reported 14,765 passed, 4 skipped, 3 xfailed in 26m49s; the SQL gate reported 1,487 passed, 1 xfailed with 100% coverage; the semantic-layer gate reported 415 passed with 100% coverage; Codecov upload and the required aggregator passed. Frontend build, pre-commit/lint, MySQL/PostgreSQL/SQLite, Presto/Hive, Cypress/Playwright, dependency review, and migration enforcement passed. Python and JavaScript CodeQL analyses and Codecov project/patch checks passed separately. Two superseded intermediate heads exposed FastMCP 3.4.2/3.4.7 exception-wrapping and error-rendering differences; the delivered exact head includes the bounded, hook-free reconciliation and its CI is green.

Current master bf12a21367ca10f6f29e553b3245f7d888d543f5 is one unrelated chart-list sorting commit ahead of the merge base; a read-only merge-tree is conflict-free, so no upstream merge was needed. #43770 at 6d0041f459aa319dad115d033fbf7b4b284ff338, #43737 at 6fd8ab5a13fa7481fd5b083544f74abf90e08ddc, #43679 at 364f479c810f815bac51a8bee0a33a53c880bf54, and #43711 at e06be4f694a091bbc93af23fad69fd2e23c587e9 were compared read-only; no ref, branch, file, commit, comment, review, or metadata for them was changed. GitHub reports PR #43771 MERGEABLE (BLOCKED only by review/protection policy). Local, tracking, Amin-fork, and PR heads match 506c5cb2f1af779250a71ed002882d0a5c334c94, and the worktree is clean. No Shortcut or Agor mutation was made. A fresh independent review follows; this section does not claim review-clean.

Round 24 retained-input sanitization

Round 24 started from the requested exact clean local/tracking/fork/PR head 506c5cb2f1af779250a71ed002882d0a5c334c94. Read-only Agor session, branch, board, and zone inspection confirmed the completed independent reviewer was idle/archived and no same-PR/head fixer or reviewer was modifying the branch. Delivery consists of normal-pushed commits 4bed954a5a14a2e66f69813090d24312d20a0973, 003f18d5e634921cde5b3a875ec4e2c641c8b4ab, and final head 05c9abcaa8a91c636b309f85bd0db29f4a7a0188.

Rejected Sunburst native-metric subtrees are now recursively projected through exact built-in dict/list/scalar operations before any schema error is raised. Hook-bearing keys, string/scalar subclasses, mapping/list/dict subclasses, malformed expression discriminators and boolean controls, and hostile nested open ColumnMeta values are replaced in both the working input and the original input Pydantic retains. The traversal is depth/count/container/string/key/integer bounded, cycle-safe, and never consumes or renders non-exact containers. Normal JSON objects, exact SIMPLE/SQL wrappers, typed/saved metrics, all metric aliases, full forward-compatible frontend ColumnMeta, and the exact accepted key/string boundaries remain unchanged.

Validated with Python 3.11, SQLAlchemy 2.0.52, and the supported FastMCP validation variants at exact delivered head:

PYTHONPATH="$PWD/superset-core/src" python -m pytest -q \
  tests/unit_tests/mcp_service/chart/test_sunburst_chart.py \
  tests/unit_tests/mcp_service/explore/tool/test_generate_explore_link.py
# 333 passed in 13.93s

PYTHONPATH="$PWD/superset-core/src" python -m pytest -q tests/unit_tests/mcp_service
# 4,503 passed in 5m30s

PYTHONPATH="$PWD/superset-core/src" python -m pytest -q \
  tests/unit_tests/common tests/unit_tests/dataframe_test.py \
  tests/unit_tests/queries/query_object_test.py \
  tests/unit_tests/jinja_context_test.py tests/unit_tests/sql_lab_test.py \
  tests/unit_tests/semantic_layers tests/unit_tests/commands/semantic_layer \
  tests/unit_tests/sql tests/unit_tests/connectors/sqla \
  tests/unit_tests/commands/sql_lab tests/unit_tests/models/sql_lab_test.py \
  tests/unit_tests/views/test_sqla.py \
  tests/unit_tests/views/test_sql_lab_tab_state_views.py
# 2,630 passed, 1 xfailed in 80.98s

pre-commit run --from-ref 506c5cb2f1af779250a71ed002882d0a5c334c94 --to-ref HEAD
python -m compileall -q superset/mcp_service tests/unit_tests/mcp_service
ruff format --check <3 Round-24 files>
ruff check <3 Round-24 files>
python -m pylint superset/mcp_service/chart/schemas.py
python -m pylint <all 3 Round-24 files>
git diff --check origin/master...HEAD
git show --check HEAD
# passed: MyPy, Ruff format/check, Pylint, all applicable pre-commit hooks,
# compilation, and diff checks

Adversarial coverage exercises direct Pydantic model_validate/model_validate_json, Generate/Update/Explore-link request schemas, every native metric alias/path, registered FastMCP dispatch, and str, repr, and errors() rendering where the validation implementation exposes that method. It covers hostile subclasses in label, aggregate, sqlExpression, optionName, expressionType, custom boolean fields, column wrappers, nested ColumnMeta containers/scalars/keys, arbitrary mappings/lists/dicts, cycles and bounds, with zero hostile hook calls and bounded rendered errors. The complete suites retain all earlier strict Sunburst hierarchy/finite/Decimal, response budget/finalizer, exception containment, DataFrame/pandas/NumPy/dateutil, cache/update-preview/export, SQL/Jinja/tabular, provenance/rebind/update, and cross-chart contracts.

Exact-head GitHub CI completed with all 13 required checks passing and no failed or pending checks. The Python unit workflow completed successfully in 26m55s under the 40-minute budget: the main suite reported 14,778 passed, 4 skipped, and 3 xfailed in 22m21s; the SQL gate reported 1,487 passed, 1 xfailed with 100% coverage; the semantic-layer gate reported 415 passed with 100% coverage; Codecov upload and the required aggregator passed. Frontend build, pre-commit/lint, MySQL/PostgreSQL/SQLite, Presto/Hive, Cypress/Playwright, dependency review, and migration enforcement passed. Python and JavaScript CodeQL analyses and Codecov project/patch checks passed separately. A superseded intermediate head exposed a FastMCP-version difference where the wrapped validation exception lacks Pydantic's errors() method; the final compatibility assertion preserves full Pydantic errors() coverage and validates bounded str/repr rendering for either registered FastMCP exception form.

Current upstream master a95c40aa1e4d46d558c69160255350f4aa04578e is nine unrelated commits ahead of the merge base; the read-only merge-tree is conflict-free, so no upstream merge was needed. #43770 at 6d0041f459aa319dad115d033fbf7b4b284ff338, #43737 at 6fd8ab5a13fa7481fd5b083544f74abf90e08ddc, #43679 at 364f479c810f815bac51a8bee0a33a53c880bf54, and #43711 at e06be4f694a091bbc93af23fad69fd2e23c587e9 were inspected read-only; no ref, branch, file, commit, comment, review, or metadata for them was changed. GitHub reports PR #43771 MERGEABLE (BLOCKED only by review/protection policy). Local, tracking, Amin-fork, and PR heads match 05c9abcaa8a91c636b309f85bd0db29f4a7a0188, and the worktree is clean. No Shortcut or Agor mutation was made. A fresh independent review follows; this section does not claim review-clean.

Finite reviewer-comment resolution pass

This pass started from the requested exact clean local/tracking/fork/PR head 05c9abcaa8a91c636b309f85bd0db29f4a7a0188. Read-only Agor session/branch/board/zone inspection found no other active same-branch writer. Delivery uses normal-pushed fix commits 27f6a119 and 1990cb97 plus conflict-free upstream merge commit 5306ee42; current master d2d965e0 is incorporated. Bullet PR #43770 was inspected read-only at 6d0041f459aa319dad115d033fbf7b4b284ff338; it was not modified or cherry-picked. Final head is 1990cb976da6c1029645713f874184499f9be5c3.

The human Sunburst feedback is implemented: a semantically identical primary/secondary metric pair is accepted for saved, SIMPLE, and SQL metrics, while hierarchy collisions and different metrics sharing an output alias remain rejected. Result-role validation collapses that repeated physical output to the frontend's categorical-color fallback without weakening strict finite numeric validation. The pass also fixes exact ordinary simplejson.JSONDecodeError fallback without accepting hostile exception subclasses; canonical metadata identity for bool/int, reordered JSON objects, and bounded Decimal/int equivalence; generic data: null compatibility while retaining Sunburst none_as_empty=False; duplicate frontend column labels; stale malformed ordering, legacy WHERE normalization, Deck numeric fixed-radius handling, inherited Table offsets, preview/result depth parity, requested Sunburst ASCII dimensions, post-merge Sunburst filter validation, response-alias boundary test parity, and the SQL response-boundary test. Existing current-code regressions cover SQL hasCustomLabel=false, cross-viz cached role scrubbing, temporal-control/filter clearing, full open frontend ColumnMeta, and hook-free hostile input rendering.

Exact-head local validation:

.venv/bin/python -m pytest -q tests/unit_tests/mcp_service \
  tests/unit_tests/common/test_form_data_query_context.py \
  tests/unit_tests/common/test_query_context_factory.py \
  tests/unit_tests/common/test_query_context_processor.py \
  tests/unit_tests/common/test_query_object_factory.py \
  tests/unit_tests/common/test_tabular_query.py \
  tests/unit_tests/dataframe_test.py tests/unit_tests/queries/query_object_test.py \
  tests/unit_tests/sqllab
# 4,779 passed in 6m15s

pre-commit run
python -m compileall -q superset/common/form_data_query_context.py \
  superset/mcp_service/chart tests/unit_tests/common/test_form_data_query_context.py \
  tests/unit_tests/mcp_service/chart tests/unit_tests/mcp_service/utils/test_response_utils.py
git diff --check
# passed: auto-walrus, MyPy, Ruff format/check, Pylint, compilation, and diff checks

Focused registered FastMCP/model tests cover repeated-metric fallback and invalid cached JSON. The full matrix also covers exact response budgets/finalizers, fixed exception containment, hostile native-metric sanitization, strict Sunburst saved/unsaved/cache/preview/export data, Decimal/DataFrame/pandas/NumPy/dateutil behavior, SQL/Jinja/tabular semantics, provenance, rebind, and update flows. The repository's frontend dependencies are not materialized in this worktree (cross-env: not found), so the local Sunburst Jest command could not start; exact-head CI frontend build, Cypress, and Playwright jobs passed.

Every one of the seven live unresolved review threads received an individual evidence reply and was resolved; the older already-resolved ColumnMeta thread remains resolved. No blanket resolve, approval, review submission, or new reviewer loop was used. Exact-head CI completed with all 13 required checks passing, 0 failed named checks, and no pending named checks. The Python workflow completed in 25m07s: the main suite reported 14,801 passed, 4 skipped, 3 xfailed in 20m28s; SQL reported 1,487 passed, 1 xfailed with 100% coverage; semantic layers reported 415 passed with 100% coverage; Codecov upload and required aggregator passed. Frontend build, pre-commit/lint, MySQL/PostgreSQL/SQLite, Presto/Hive, Cypress/Playwright, dependency review, migration enforcement, CodeQL, and Codecov project/patch checks passed. GitHub reports MERGEABLE (BLOCKED only by normal review/protection policy). Local, tracking, Amin-fork, and live PR heads match the final head, and the worktree is clean.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@bito-code-review

bito-code-review Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #eec59f

Actionable Suggestions - 0
Additional Suggestions - 6
  • superset/mcp_service/chart/chart_utils.py - 2
    • Broad merge changes all chart updates · Line 1124-1128
      `merge_form_data_for_update` is called for every chart type in `update_chart_preview.py`, but the non-Sunburst branch returns `{**existing_form_data, **new_form_data}`, retaining all prior keys. Previously the preview only preserved specific keys via `merge_table_column_config`/`merge_interactive_pivot_ui_config`. This now keeps stale keys (e.g. removed `groupby`/metrics) on any non-Sunburst update. Consider returning `new_form_data` for non-Sunburst configs.
    • Missing tests for new branch · Line 1882-1886
      The new `SunburstChartConfig` branch in `analyze_chart_semantics` has no test coverage, unlike the big_number/handlebars/pivot branches which have dedicated tests. Per BITO.md rule [11730], add unit tests asserting `primary_insight`, the `data_story` including hierarchy names/metric, and the >3-column ellipsis path.
  • superset/mcp_service/chart/tool/update_chart_preview.py - 1
    • Unconditional merge changes all charts · Line 248-250
      `merge_form_data_for_update` is called for every chart type, but its non-Sunburst path (chart_utils.py:1125-1128) returns a blanket `{**existing, **new}` merge. This now retains stale keys from the previous cached form_data (e.g. `time_range`, `granularity_sqla`) that the mapper omits when the new config leaves them unset (chart_utils.py:1076-1078). Previously only `column_config`/`pivot_table_state`/`adhoc_filters` were preserved. Consider restricting the merge to Sunburst same-viz updates.
  • superset/mcp_service/chart/schemas.py - 1
    • SQL metric round-trip fails · Line 1178-1183
      In `_coerce_native_metric`, a native SQL adhoc metric with `hasCustomLabel: false` yields `label=None`, but `ColumnRef` raises "sql_expression requires a 'label'" (line 786). Round-tripping such a saved metric therefore fails validation. Fall back to `sqlExpression` when `label` is None.
  • superset/mcp_service/chart/plugins/sunburst.py - 2
    • Unconditional canonicalization crash · Line 106-109
      `hierarchy` is `List[ColumnRef]`, and `ColumnRef` permits `sql_expression` with `name=None`. Calling `get_canonical_column_name(dimension["name"], ...)` unconditionally would pass `None` into `.lower()` and raise `AttributeError`. The sibling `pie.py` guards with `if not dim.get("sql_expression") and not dim.get("saved_metric")`; mirror that guard here for consistency.
    • Dynamically typed Any disallowed · Line 73-73
      `Any` is used in `config` parameters across `extract_column_refs`, `to_form_data`, `generate_name`, `resolve_viz_type`, and `normalize_column_refs` (ANN401). Replace with specific types like `SunburstChartConfig` or `object` where narrowing applies.
Review Details
  • Files reviewed - 17 · Commit Range: fb0a0e3..fb0a0e3
    • 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/compile.py
    • superset/mcp_service/chart/plugins/__init__.py
    • superset/mcp_service/chart/plugins/sunburst.py
    • superset/mcp_service/chart/preview_utils.py
    • superset/mcp_service/chart/resources/chart_configs.py
    • superset/mcp_service/chart/schemas.py
    • superset/mcp_service/chart/tool/generate_chart.py
    • superset/mcp_service/chart/tool/get_chart_data.py
    • superset/mcp_service/chart/tool/get_chart_type_schema.py
    • superset/mcp_service/chart/tool/update_chart.py
    • superset/mcp_service/chart/tool/update_chart_preview.py
    • superset/mcp_service/chart/validation/runtime/chart_type_suggester.py
    • tests/unit_tests/mcp_service/chart/test_sunburst_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 Bito Logo

Comment thread superset/mcp_service/chart/schemas.py
Comment thread superset/mcp_service/chart/tool/update_chart_preview.py
Comment thread superset/mcp_service/chart/chart_utils.py
@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 78.84814% with 819 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.54%. Comparing base (d2d965e) to head (1990cb9).

Files with missing lines Patch % Lines
superset/mcp_service/chart/query_result.py 72.17% 169 Missing and 102 partials ⚠️
superset/mcp_service/chart/schemas.py 83.28% 57 Missing and 48 partials ⚠️
superset/common/form_data_query_context.py 82.30% 66 Missing and 37 partials ⚠️
superset/mcp_service/chart/sunburst.py 63.52% 43 Missing and 19 partials ⚠️
superset/mcp_service/chart/preview_utils.py 60.74% 29 Missing and 13 partials ⚠️
superset/mcp_service/chart/chart_helpers.py 82.98% 12 Missing and 21 partials ⚠️
superset/mcp_service/chart/tool/get_chart_data.py 73.55% 27 Missing and 5 partials ⚠️
superset/mcp_service/chart/tool/get_chart_sql.py 63.95% 24 Missing and 7 partials ⚠️
.../mcp_service/chart/validation/dataset_validator.py 77.31% 14 Missing and 8 partials ⚠️
superset/mcp_service/utils/response_utils.py 88.41% 10 Missing and 9 partials ⚠️
... and 14 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #43771      +/-   ##
==========================================
+ Coverage   79.41%   79.54%   +0.13%     
==========================================
  Files        2894     2898       +4     
  Lines      167820   171230    +3410     
  Branches    38863    39751     +888     
==========================================
+ Hits       133269   136207    +2938     
- Misses      32051    32240     +189     
- Partials     2500     2783     +283     
Flag Coverage Δ
hive 36.90% <14.43%> (-0.89%) ⬇️
mysql 55.85% <14.56%> (-1.66%) ⬇️
postgres 55.88% <14.56%> (-1.66%) ⬇️
presto 38.72% <14.46%> (-0.96%) ⬇️
python 83.95% <78.84%> (+0.09%) ⬆️
sqlite 55.58% <14.51%> (-1.65%) ⬇️
unit 74.83% <78.84%> (+0.49%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@netlify

netlify Bot commented Sep 1, 2026

Copy link
Copy Markdown

Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit 5306ee4
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6a99b2194212de00072bb0f4
😎 Deploy Preview https://deploy-preview-43771--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@bito-code-review

bito-code-review Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #d98b39

Actionable Suggestions - 0
Additional Suggestions - 9
  • superset/mcp_service/chart/chart_utils.py - 1
    • Orphan grain not rejected · Line 1352-1355
      The comment says the final-form-data validator rejects orphan grains, but `_validate_sunburst_temporal_subject` in `compile.py` returns early for any `viz_type != "sunburst_v2"`. Non-sunburst configs (`BigNumberChartConfig`, `interactive_pivot`, `waterfall`) also carry `time_grain`, so an orphan `time_grain_sqla` set at lines 1346-1350 is now neither popped nor rejected and reaches the query builder. Consider extending the validator or keeping the pop for non-sunburst types.
  • superset/mcp_service/chart/schemas.py - 1
    • Strict model rejects saved columns · Line 1088-1122
      `SunburstNativeMetricColumn` inherits `UnknownFieldCheckMixin`, whose `check_unknown_fields` validator raises `ValueError` on any undeclared field, defeating the `extra="ignore"` config. It is used in `_coerce_native_metric` (line 1355) while reading *saved* Sunburst form_data, where real frontend `ColumnMeta` objects carry many fields not declared here (`is_certified`, `certified_by`, `extra`, `warnings`, `is_physical`, ...). Such saved charts would fail to load. Consider a plain `BaseModel` with `extra="ignore"` for this saved-data path.
  • superset/mcp_service/chart/sunburst.py - 1
    • Unhandled exception on ambiguity · Line 246-246
      `DatasetValidator.get_canonical_column_name` raises on ambiguous (case-folded) column matches, but `normalize_sunburst_form_data_references` calls it (and `get_canonical_metric_name`) without try/except. The caller `update_chart_preview` also has no handler, so an ambiguous reference in form_data would propagate as an unhandled exception (500) instead of a clean `ChartError`. Consider catching and returning a validation error.
  • superset/mcp_service/chart/preview_utils.py - 1
    • Datasource precedence override · Line 94-94
      `resolve_form_data_datasource` (chart_helpers.py:686) prefers `form_data.get("datasource_id")` over the `datasource` string set here. If `form_data` already carries a `datasource_id` (e.g. merged from a saved chart's params in `update_chart_preview.py`), the preview queries that datasource instead of the validated `dataset_id` param, silently ignoring the explicit dataset. Consider clearing `datasource_id`/`datasource_type` before setting `datasource`.
  • superset/mcp_service/chart/tool/get_chart_preview.py - 1
    • Sunburst ignores ascii dimensions · Line 277-277
      For `sunburst_v2`, `_generate_ascii_preview_from_data` hardcodes `width=80, height=20` (preview_utils.py:153), so the returned `ASCIIPreview` metadata ignores the caller's `self.request.ascii_width`/`ascii_height`. The non-sunburst branch (lines 288-289) honors them. Consider threading the requested dimensions through so both paths behave consistently.
  • superset/mcp_service/chart/chart_helpers.py - 1
    • Validation coverage gap · Line 608-608
      Move the `_validate_sunburst_simple_filter_clauses(form_data, viz_type)` call to immediately after `prepare_form_data_for_query(...)` so that legacy `having`/`filters` and `extra_form_data` are also validated.
  • superset/mcp_service/chart/tool/generate_chart.py - 1
    • Misleading cache comment · Line 672-674
      The comment claims "Cache only a chart state that passed its compile checks," but the compile check is gated on `if numeric_dataset_id is not None:` (line 625). When the dataset is missing or the user lacks access, `_compile_chart` is skipped yet `generate_explore_link` still caches `form_data`. Consider gating the cache on compile success or adjusting the comment to match the conditional behavior.
  • superset/mcp_service/chart/query_result.py - 1
    • Unreachable defensive branches · Line 112-116
      Both defensive branches are unreachable: `query_result_failure()` (line 110) already returns a `ChartError` for a non-`Mapping` result and for any non-`Mapping` query in the `queries` list. Lines 112-116 and 124-128 can never execute. Consider removing them to avoid dead code; the empty-`queries` check at line 118 remains reachable and should be kept.
  • superset/common/form_data_query_context.py - 1
    • Clause validation regression · Line 78-80
      The old code used `(flt.get("clause") or "WHERE").upper()`, tolerating lowercase and `None` clause values. The new strict `isinstance`/set check now raises `ValueError` for a lowercase `"where"` or a `None` clause in stored form_data on the export path (`where_only=True`). Consider normalizing with `.upper()` before validating to avoid a regression for legacy charts.
Review Details
  • Files reviewed - 23 · Commit Range: fb0a0e3..a609fb8
    • superset/common/form_data_query_context.py
    • superset/mcp_service/chart/chart_utils.py
    • superset/mcp_service/chart/compile.py
    • superset/mcp_service/chart/plugins/sunburst.py
    • superset/mcp_service/chart/preview_utils.py
    • superset/mcp_service/chart/query_result.py
    • superset/mcp_service/chart/schemas.py
    • superset/mcp_service/chart/sunburst.py
    • superset/mcp_service/chart/tool/generate_chart.py
    • superset/mcp_service/chart/tool/get_chart_preview.py
    • superset/mcp_service/chart/tool/update_chart.py
    • superset/mcp_service/chart/tool/update_chart_preview.py
    • superset/mcp_service/chart/validation/dataset_validator.py
    • tests/unit_tests/mcp_service/chart/test_query_result.py
    • tests/unit_tests/mcp_service/chart/test_sunburst_chart.py
    • tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
    • tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py
    • tests/unit_tests/mcp_service/chart/validation/test_dataset_validator.py
    • superset/mcp_service/chart/chart_helpers.py
    • superset/mcp_service/chart/plugins/histogram.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_compile.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 Bito Logo

@bito-code-review

bito-code-review Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #3f2309

Actionable Suggestions - 0
Additional Suggestions - 15
  • superset/mcp_service/chart/tool/update_chart.py - 1
    • Stale column config on rebind · Line 275-278
      On a dataset rebind, `merge_table_column_config`/`merge_interactive_pivot_ui_config` (lines 265-266) copy the old dataset's `column_config`/`pivot_table_state` into `new_form_data` before `_merge_replacement_config` triggers `scrub_dataset_bound_form_data`. Since both keys are in `_DATASET_BOUND_FORM_DATA_KEYS`, the scrub is meant to drop them, but `merged.update(new_form_data)` re-adds the stale values. Guard the merge helpers with `if not dataset_rebind` (same applies to the preview path at lines 374-375).
  • superset/mcp_service/chart/tool/update_chart_preview.py - 1
    • Stale dataset config on rebind · Line 251-265
      When `dataset_rebind` is True, `merge_form_data_for_update` scrubs dataset-bound keys via `scrub_dataset_bound_form_data`, but `merge_table_column_config`/`merge_interactive_pivot_ui_config` run first and re-inject `column_config`/`pivot_table_state` from the previous dataset into `new_form_data`, which then overrides the scrubbed base. Stale column config leaks into the rebound preview. Compute `dataset_rebind` first and skip these merges when rebinding.
  • superset/common/form_data_query_context.py - 2
    • Orderby parse raises on discarded result · Line 144-145
      `query_fields_from_form_data` raises `ValueError` on malformed orderby JSON, but `columns_from_form_data` (line 252) and `_columns_and_metrics` (line 354) call it and discard the orderby result. A saved chart carrying a stale/malformed `order_by_cols` would now crash where it previously ran. `orderby_from_form_data` already drops such entries leniently; mirror that with `continue`.
    • Duplicated x_axis logic · Line 362-369
      The new `x_axis` block duplicates the identical logic already in `columns_from_form_data` (lines 254-260). Both are live query-building paths: `columns_from_form_data` feeds the MCP preview (`preview_utils.py`) and `_columns_and_metrics` feeds dashboard export. A future fix to one handler would silently diverge the other. Consider calling `columns_from_form_data` here to keep a single source of truth.
  • superset/mcp_service/chart/chart_utils.py - 1
    • Rebind leaks old-dataset columns · Line 1559-1560
      `scrub_dataset_bound_form_data` only cleans `existing_form_data`, but in `update_chart.py` the callers `merge_table_column_config`/`merge_interactive_pivot_ui_config` already copy dataset-bound keys (`column_config`, `conditional_formatting`, `pivot_table_state`) from the old dataset into `new_form_data` before this runs. On rebind those stale column references survive into the new dataset's form data, defeating the scrub. Guard the merge helpers with the same `dataset_rebind` flag or scrub `new_form_data` too.
  • superset/mcp_service/chart/schemas.py - 1
    • Temporal binding lost on None granularity · Line 1442-1443
      When native form data has `granularity_sqla: None` (set by `chart_utils.py` for non-temporal columns) plus a `TEMPORAL_RANGE` filter, the new key-presence guard blocks the filter-subject fallback, and line 1510 then sets `temporal_column = None`. The old `data.setdefault("temporal_column", native_filter.get("subject"))` preserved the binding. Use `not data.get("granularity_sqla")` so a None value still falls back to the filter subject.
  • superset/mcp_service/chart/plugin.py - 1
    • Dropped legacy query role key · Line 52-63
      The refactor derives QUERY_ROLE_KEYS from SHARED_FORM_DATA_QUERY_ROLE_KEYS plus registered roles, which drops the legacy "entity" key the old frozenset contained. `resolve_groupby()` (chart_helpers.py:495) still reads `entity` as a fallback groupby, and `_DATASET_BOUND_FORM_DATA_KEYS` doesn't cover it, so a stale `entity` from a prior viz survives `merge_form_data_for_update` and can leak into the query. Consider restoring it.
  • superset/mcp_service/chart/chart_helpers.py - 1
    • Duplicated pop-None loop · Line 90-92
      The 3-line pop-None loop is duplicated in `canonicalize_operation_form_data` (90-92) and `prepare_form_data_for_query` (199-201), and an identical loop already exists in `chart_utils.py:1649`. The key set is centralized in `MAPPING_ENVELOPE_KEYS`, but the loop body is repeated, so a future change to sentinel-removal semantics must be applied in three places. Consider a shared helper.
  • superset/mcp_service/chart/plugins/table.py - 1
    • Redundant no-op union · Line 45-45
      `percent_metrics` is already in `BaseChartPlugin.query_role_keys` (it is part of `_REGISTERED_PRIMARY_QUERY_ROLE_KEYS` in plugin.py:53, which feeds `QUERY_ROLE_KEYS`). This union is a no-op and misleadingly implies the base set lacks it. Assign `BaseChartPlugin.query_role_keys` directly or remove the override.
  • superset/mcp_service/chart/plugins/xy.py - 1
    • Redundant no-op union · Line 56-56
      This union is a no-op: `x_axis` is already in `BaseChartPlugin.query_role_keys` via `_REGISTERED_PRIMARY_QUERY_ROLE_KEYS` (plugin.py:53), so `query_role_keys` never changes. It misleads readers into thinking the XY plugin contributes a role that is already globally registered. Remove the line, or add a comment if documenting ownership is the goal.
  • tests/unit_tests/mcp_service/chart/test_sunburst_chart.py - 1
    • Dead dispatch branch · Line 609-613
      The new `isinstance(config, InteractivePivotChartConfig)` branch is unreachable: `InteractivePivotChartConfig` is not in this test's parametrize list (lines 581-601), so `map_interactive_pivot_config` is never called here. The interactive-pivot path of `merge_form_data_for_update` goes untested. Add an `InteractivePivotChartConfig` case (as in `test_explicit_sparse_controls_replace_same_viz_preservation`) to cover it.
  • superset/mcp_service/chart/plugins/mixed_timeseries.py - 1
    • Redundant no-op assignment · Line 44-44
      This assignment is a no-op: `MixedTimeseriesChartPlugin` already inherits `query_role_keys` from `BaseChartPlugin` (which equals `QUERY_ROLE_KEYS`), and unlike sibling plugins (`table`, `waterfall`, `xy`, `histogram`) it adds no extra keys via `| {...}`. The line is misleading and can be dropped.
  • superset/mcp_service/chart/plugins/waterfall.py - 1
    • Redundant query role union · Line 44-44
      `x_axis` is already in `BaseChartPlugin.query_role_keys` (plugin.py `_REGISTERED_PRIMARY_QUERY_ROLE_KEYS` includes it), so `| {"x_axis"}` is a no-op. Keeping it implies the base set lacks `x_axis`, which can mislead future maintainers. Consider removing the line for clarity.
  • tests/unit_tests/common/test_form_data_query_context.py - 1
    • Strengthen bucket assertions · Line 46-48
      The parametrized test only asserts the single target bucket, so it can't detect an alias value leaking into another bucket (e.g. a metric also appearing in `columns`). Assert the full `(columns, metrics, orderby)` tuple with the non-target buckets expected empty. Also `value` is always a string literal, so annotate it `str` rather than `object`.
  • superset/mcp_service/chart/plugins/histogram.py - 1
    • Redundant query_role_keys union · Line 46-46
      `BaseChartPlugin.query_role_keys` already contains "column" (plugin.py:52-63 `_REGISTERED_PRIMARY_QUERY_ROLE_KEYS`), so this union is a no-op. It misleads readers into thinking the plugin adds a role that is already shared. Drop the override or add a clarifying comment.
Review Details
  • Files reviewed - 32 · Commit Range: a609fb8..893bae9
    • superset/mcp_service/chart/chart_utils.py
    • superset/mcp_service/chart/compile.py
    • superset/mcp_service/chart/preview_utils.py
    • superset/mcp_service/chart/sunburst.py
    • superset/mcp_service/chart/tool/generate_chart.py
    • superset/mcp_service/chart/tool/update_chart.py
    • superset/mcp_service/chart/tool/update_chart_preview.py
    • tests/unit_tests/mcp_service/chart/test_sunburst_chart.py
    • superset/mcp_service/chart/chart_helpers.py
    • superset/mcp_service/chart/schemas.py
    • superset/mcp_service/chart/tool/get_chart_data.py
    • superset/mcp_service/chart/tool/get_chart_preview.py
    • tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
    • tests/unit_tests/mcp_service/chart/test_compile.py
    • tests/unit_tests/mcp_service/chart/test_preview_utils.py
    • superset/mcp_service/chart/plugin.py
    • superset/mcp_service/chart/registry.py
    • tests/unit_tests/mcp_service/chart/test_registry.py
    • tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py
    • superset/common/form_data_query_context.py
    • superset/mcp_service/chart/plugins/big_number.py
    • superset/mcp_service/chart/plugins/box_plot.py
    • superset/mcp_service/chart/plugins/handlebars.py
    • superset/mcp_service/chart/plugins/histogram.py
    • superset/mcp_service/chart/plugins/interactive_pivot.py
    • superset/mcp_service/chart/plugins/mixed_timeseries.py
    • superset/mcp_service/chart/plugins/pie.py
    • superset/mcp_service/chart/plugins/pivot_table.py
    • superset/mcp_service/chart/plugins/table.py
    • superset/mcp_service/chart/plugins/waterfall.py
    • superset/mcp_service/chart/plugins/xy.py
    • tests/unit_tests/common/test_form_data_query_context.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 Bito Logo

@bito-code-review

bito-code-review Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #07c451

Actionable Suggestions - 0
Additional Suggestions - 17
  • superset/mcp_service/chart/chart_helpers.py - 4
    • Deck orderby regression · Line 886-886
      The deleted block set `qd["orderby"] = [(deck_metrics[0], not order_desc)]` to mirror `BaseDeckGLViz.query_obj()`. The new `_deck_query_adapter` branches (arc/screengrid/etc.) set `columns`/`metrics`/`filters`/`is_timeseries` but never `orderby`, so deck.gl charts lose the default sort by first metric descending. Verify and restore this ordering.
    • Numeric string treated as metric · Line 402-403
      `_is_deck_metric_value` returns True for any non-empty string, so numeric strings like `point_radius_fixed="100"` (deck_scatter, line 658) and `line_width="5"` (deck_path, line 590) are wrongly added to `metrics`/`orderby`. The legacy `_is_metric_ref` excluded numeric strings via `float()`, and `test_resolve_deck_gl_metrics_numeric_point_radius_fixed_excluded` expects that. Mirror `_is_metric_ref` here.
    • Dead deck helpers remain · Line 886-886
      This diff replaced the old deck query assembly with `_deck_query_adapter`, removing the only production call sites of `_resolve_deck_gl_metrics`, `_deck_gl_null_filters`, and `resolve_deck_gl_columns`. They are now dead in production (only referenced by tests). Consider removing them or marking them test-only to avoid divergence risk.
    • Too many branches and statements · Line 496-496
      `_deck_query_adapter` has 32 branches and 102 statements, exceeding the configured limits (12 and 50). Extract per-viz-type logic (geojson, polygon, path, arc, scatter) into separate helper functions to reduce complexity.
  • superset/mcp_service/chart/tool/get_chart_data.py - 2
    • Empty result rejected as error · Line 789-789
      `validate_query_result_envelope` defaults to `none_as_empty=False`, so a legitimate empty result (`data: None`) for a generic saved chart is rejected as `InvalidQueryResult` before reaching the graceful `EmptyQuery` path below. `get_chart_preview.py` passes `none_as_empty=self.chart.viz_type != "sunburst_v2"` for this reason. Consider passing `none_as_empty=True` here to preserve legacy empty-result behavior.
    • Empty result rejected as error · Line 1155-1155
      Same as line 789: the unsaved-chart path also calls `validate_query_result_envelope` with the default `none_as_empty=False`, so `data: None` is rejected as `InvalidQueryResult` before the `EmptyQuery` branch at line 1171. Pass `none_as_empty=True` for consistency with `get_chart_preview.py`.
  • superset/common/form_data_query_context.py - 3
    • Inherit offset divergence · Line 1416-1417
      `_table_time_offsets` appends the literal `"inherit"` token when `time_compare` contains `"inherit"` but no inherited value is supplied via `extra_form_data.time_compare`. The frontend `buildQuery` only incorporates the inherited shift when `extra_form_data.time_compare` exists, so this yields a spurious `compare` operator and `__inherit`-suffixed labels the frontend would not generate. Guard the append on `extra_form_data.time_compare`.
    • Secondary query regression risk · Line 1478-1485
      This removes the guard that emptied `secondary["metrics"]`/`secondary["groupby"]` when `metrics_b`/`groupby_b` are absent, which the deleted comment says prevented query A from executing twice for older saved charts. If `_finalize_query_objects` doesn't replicate that, the double-execution regression returns. Please confirm the new path preserves the empty-secondary contract.
    • Duplicated BASE_AXIS dict · Line 635-654
      `_box_temporalized_columns` re-inlines the same BASE_AXIS dict (`timeGrain`/`columnType`/`sqlExpression`/`label`/`expressionType`) already built in `_temporalized_columns`. If the pivot/box temporal-column contract changes, the two copies can diverge. Consider extracting a shared `_temporal_column_dict(time_grain, column)` helper and calling it from both functions.
  • superset/mcp_service/chart/query_result.py - 3
    • Rejects valid duplicate colnames · Line 270-270
      This new check rejects valid results: `QueryContextProcessor.get_data` renames columns via `verbose_map` (query_context_processor.py:411), which can collapse two columns onto the same label (see excel.py:64-66), and `extract_dataframe_dtypes` explicitly supports duplicate columns. The consumer `get_chart_data.py` iterates `raw_columns` by index, so duplicates are handled. This would return `InvalidQueryResult` for legitimate charts instead of data.
    • Duplicate colnames regression · Line 316-316
      Routing `first_query_data` through `validate_query_result_envelope` now rejects results whose `colnames` contain duplicates (`len(set(colnames)) != len(colnames)`). The pipeline produces `colnames = list(processed_df.columns)` (client_processing.py:1377), which can contain duplicate names — `extract_dataframe_dtypes` and its unit test explicitly support this. Previews that previously returned data now fail with "malformed column metadata". Consider relaxing the uniqueness check.
    • Too many branches · Line 164-164
      Function has too many branches (25 > 12). Refactor to reduce branches or add a noqa comment.
  • superset/mcp_service/chart/schemas.py - 3
    • Dead always-failing SQL branch · Line 3018-3022
      This branch sets `config["x"] = {"sql_expression": ...}`, but `XYChartConfig.reject_sql_expression_on_dimensions` calls `_reject_sql_expression_on_dimension(self.x, "x")`, which raises `ValueError` for any dimension carrying `sql_expression`. So this path can never produce a valid chart — it always errors. Drop the branch or raise a clear error instead.
    • Cross-class helper coupling · Line 2521-2521
      `coerce_y_column_names` reuses `SunburstChartConfig._coerce_native_metric`, a private Sunburst method whose errors read "Sunburst native metric …" (lines 1334/1376/1378) and which validates via `SunburstNativeMetricColumn`. An XY user with an invalid native metric gets a confusing Sunburst-specific message. Consider a shared chart-agnostic helper with neutral wording.
    • Function has too many branches · Line 2919-2919
      `_normalize_chart_request_input` has 23 branches (limit 12). Consider refactoring into smaller helper functions. This also addresses PLR0915 at the same line.
  • superset/mcp_service/chart/plugin.py - 1
    • Phantom _b role keys · Line 71-75
      The `_b` derivation from `_REGISTERED_PRIMARY_QUERY_ROLE_KEYS` yields 15 phantom keys (`column_b`, `entity_b`, `groupbyColumns_b`, `groupbyRows_b`, `percent_metrics_b`, `x_axis_b`, `metric_b`, `x_b`, `y_b`, `size_b`, `series_b`, ...) that are absent from the authoritative `MIXED_TIMESERIES_SECONDARY_QUERY_KEYS`. The comment claims this prevents vocabulary drift, but the derived set doesn't match the real secondary vocabulary. These dead keys inflate `QUERY_ROLE_KEYS` and are stripped by `query_role_keys_for_viz_type` during same-viz replacement in `chart_utils.py`. Consider deriving only from shared keys with real `_b` counterparts, or dropping the derivation.
  • tests/unit_tests/mcp_service/chart/test_round10_query_contracts.py - 1
    • Duplicated BASE_AXIS literal · Line 193-199
      Consider extracting a module-level `_base_axis_column(name: str) -> dict` helper and using it in both the primary and mixed_timeseries assertions to centralize the BASE_AXIS dict literal and prevent divergence if the contract changes.
Review Details
  • Files reviewed - 21 · Commit Range: 893bae9..efb6961
    • superset/common/form_data_query_context.py
    • superset/mcp_service/chart/chart_helpers.py
    • superset/mcp_service/chart/chart_utils.py
    • superset/mcp_service/chart/plugin.py
    • superset/mcp_service/chart/schemas.py
    • tests/unit_tests/mcp_service/chart/test_round10_query_contracts.py
    • superset/charts/data/form_data.py
    • superset/mcp_service/chart/query_result.py
    • superset/mcp_service/chart/tool/get_chart_data.py
    • superset/mcp_service/chart/tool/get_chart_preview.py
    • tests/unit_tests/common/test_form_data_query_context.py
    • tests/unit_tests/mcp_service/chart/test_chart_helpers.py
    • tests/unit_tests/mcp_service/chart/test_query_result.py
    • tests/unit_tests/mcp_service/chart/test_round11_query_contracts.py
    • tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
    • tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py
    • superset/mcp_service/chart/compile.py
    • tests/unit_tests/mcp_service/chart/test_compile.py
    • tests/unit_tests/mcp_service/chart/test_preview_utils.py
    • tests/unit_tests/mcp_service/chart/test_round12_query_contracts.py
    • tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.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 Bito Logo

@bito-code-review

bito-code-review Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #82b6e5

Actionable Suggestions - 0
Additional Suggestions - 6
  • superset/mcp_service/chart/query_result.py - 1
    • NaN rejected in float bound · Line 110-110
      `math.isfinite(value)` rejects `float('nan')`, but the JSON result path (`query_context_processor` → `df.to_dict(orient="records")`) emits NaN for NULL numeric cells, which the old code accepted. Any chart with a missing numeric value will now fail `validate_query_result_envelope`. Consider allowing NaN (e.g. `math.isnan(value) or math.isfinite(value)`).
  • superset/mcp_service/chart/tool/update_chart.py - 1
    • Redundant live DB compile · Line 860-866
      The removed `run_compile_check=False` (previously passed for dataset-only updates) is now gone, so `_validate_update_against_dataset` uses its default `True` and runs a live DB query via `validate_and_compile`/`ChartDataCommand`. For idempotent same-dataset updates `final_form_data` is the unchanged saved state, so this is redundant work on a plausible path. Pass `run_compile_check=_is_dataset_rebind(request, chart)`.
  • superset/mcp_service/chart/tool/get_chart_data.py - 1
    • dict key order inflates unique_count · Line 195-195
      `_canonical_profile_key` builds the dict key from `value.items()` in insertion order, so equal dicts with different key order (e.g. `{"a":1,"b":2}` vs `{"b":2,"a":1}`) yield different keys and inflate `unique_count`. Sort items by key before canonicalizing. The nested-dict test path makes this reachable.
  • tests/unit_tests/mcp_service/chart/tool/test_update_chart.py - 1
    • Vacuous test assertion · Line 2525-2525
      `mock_validate.call_args.kwargs.get("run_compile_check", True)` is vacuous: the dataset-only call at `update_chart.py:861-866` never passes `run_compile_check`, so `.get(..., True)` always returns the default and the assertion passes regardless of behavior. Assert the effective default (`is not False`) or the absence of the kwarg so a regression that disables the compile check is caught.
  • superset/mcp_service/chart/preview_utils.py - 1
    • Depth guard mismatch · Line 68-68
      `_is_bounded_result_value` accepts containers up to depth 6 (`depth > MAX_RESULT_VALUE_DEPTH` rejects), but `_canonical_preview_value` stops recursing at `depth < 6`, so a valid depth-6 list/dict renders as `"[truncated]"`. Consider sharing the same depth constant/guard so validated nested data isn't collapsed.
  • tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py - 1
    • Duplicate assertion · Line 202-202
      This added line duplicates the assertion on line 201 exactly. It adds no coverage and is dead code in the test. Remove it to keep the assertion single and avoid confusion.
Review Details
  • Files reviewed - 15 · Commit Range: efb6961..813b15f
    • superset/mcp_service/chart/query_result.py
    • superset/mcp_service/chart/tool/update_chart.py
    • superset/mcp_service/chart/tool/update_chart_preview.py
    • tests/unit_tests/mcp_service/chart/test_compile.py
    • tests/unit_tests/mcp_service/chart/test_preview_utils.py
    • tests/unit_tests/mcp_service/chart/test_query_result.py
    • tests/unit_tests/mcp_service/chart/test_sunburst_chart.py
    • tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
    • tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py
    • tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
    • tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py
    • superset/mcp_service/chart/preview_utils.py
    • superset/mcp_service/chart/tool/get_chart_data.py
    • superset/mcp_service/utils/cache_utils.py
    • tests/unit_tests/mcp_service/utils/test_cache_utils.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 Bito Logo

@github-actions github-actions Bot added the github_actions Pull requests that update GitHub Actions code label Sep 3, 2026
Comment thread superset/mcp_service/chart/schemas.py Outdated

@bito-code-review bito-code-review Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Agent Run #bb24a2

Actionable Suggestions - 1
  • superset/mcp_service/chart/tool/get_chart_data.py - 1
Additional Suggestions - 7
  • tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py - 3
    • fold=1 offset expectation wrong · Line 599-599
      `fold=1` on 2024-10-27 01:30 in `Europe/Dublin` denotes the second occurrence after clocks fall back, which is GMT (+00:00), not IST (+01:00). The expected `+01:00` is semantically inconsistent with Python/dateutil fold semantics. Verify `_dateutil_named_offset_without_hooks`'s `if not value.fold and is_ambiguous: index -= 1` branch; either the expectation should be `+00:00` or the source's fold handling is inverted.
    • Vacuous secret-leak assertion · Line 337-337
      `HostileRuntimeError.__str__`/`__repr__` return `"round21-hostile-runtime-secret"`, never the constructor arg `"stored-secret"`. So this assertion checks a literal that can never appear and would pass even if the secret leaked. Assert the real leakable string so the containment test actually verifies its claim.
    • Unmocked DB lookup in unit test · Line 298-313
      In the `context-exit` branch `resolve_dataset` is left unmocked, so the test performs a real `DatasetDAO.find_by_id(1)` DB lookup before the context `__exit__` raises. Every other test in this file mocks `resolve_dataset` (e.g. lines 570, 626). Mock it here too for a deterministic unit test.
  • tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py - 1
    • Duplicated test setup · Line 961-989
      Both new tests duplicate the same `_Command` stub, `form_data`, and five `monkeypatch.setattr` calls (lines 902-923 vs 961-989). If one setup block is later updated (e.g. a new mock) and the other is not, the two tests silently diverge. Consider a shared fixture/helper to keep the timezone assertions the only difference.
  • tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py - 1
    • Test no longer near boundary · Line 428-428
      `test_extract_sql_accepts_multi_query_and_near_response_boundary` only patches `_get_chart_sql_mod.MAX_QUERY_RESULT_VALUE_BYTES`, but the boundary case flows through `response_json_failure`, which reads the constant from `query_result` (still 16 MiB). So `boundary_sql = test_limit - 512` (3584 B) is no longer near the response limit, and the test no longer verifies its stated purpose. Patch `query_result.MAX_QUERY_RESULT_VALUE_BYTES` too, as the sibling test does.
  • superset/mcp_service/chart/response_preflight.py - 1
    • Missing unit tests · Line 45-80
      This new helper is now used at 10+ call sites across `get_chart_data.py`, `query_dataset.py`, and `get_table.py`, yet has no unit tests. Its byte-budget truncation, multi-byte UTF-8 handling, and type-dispatch branches are non-trivial (C901-suppressed). Per BITO.md rules 11730/11731, add tests covering success paths, edge cases, and the 'request failed' fallback.
  • tests/unit_tests/mcp_service/chart/query_result_fixtures.py - 1
    • Nested ternary readability · Line 82-90
      The nested conditional expression for deriving `columns` is hard to read and error-prone. Consider refactoring to use explicit `if/elif/else` statements for clarity and easier maintenance.
Review Details
  • Files reviewed - 12 · Commit Range: 0ae605f..414c7a6
    • superset/mcp_service/chart/response_preflight.py
    • superset/mcp_service/chart/tool/get_chart_data.py
    • superset/mcp_service/dataset/tool/query_dataset.py
    • superset/mcp_service/semantic_layer/tool/get_table.py
    • tests/unit_tests/mcp_service/chart/test_query_result.py
    • tests/unit_tests/mcp_service/chart/test_response_preflight.py
    • tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
    • tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py
    • tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py
    • tests/unit_tests/mcp_service/semantic_layer/tool/test_get_table.py
    • superset/mcp_service/chart/query_result.py
    • tests/unit_tests/mcp_service/chart/query_result_fixtures.py
  • Files skipped - 1
    • .github/workflows/superset-python-unittest.yml - Reason: Filter setting
  • 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 Bito Logo

Comment thread superset/mcp_service/chart/tool/get_chart_data.py
@bito-code-review

bito-code-review Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #4d92c9

Actionable Suggestions - 0
Review Details
  • Files reviewed - 3 · Commit Range: 414c7a6..506c5cb
    • superset/mcp_service/chart/schemas.py
    • tests/unit_tests/mcp_service/chart/test_sunburst_chart.py
    • tests/unit_tests/mcp_service/explore/tool/test_generate_explore_link.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 Bito Logo

Comment thread superset/mcp_service/chart/schemas.py
@bito-code-review

bito-code-review Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #4d423d

Actionable Suggestions - 0
Review Details
  • Files reviewed - 3 · Commit Range: 506c5cb..05c9abc
    • superset/mcp_service/chart/schemas.py
    • tests/unit_tests/mcp_service/chart/test_sunburst_chart.py
    • tests/unit_tests/mcp_service/explore/tool/test_generate_explore_link.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 Bito Logo

*([roles.secondary_metric] if roles.secondary_metric else []),
):
assert field is not None
if not _finite_number(row[field]):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A NULL aggregate value now rejects the entire Sunburst result even though the frontend treats a non-numeric metric as zero. A group whose SUM(revenue) is NULL can therefore make generate/preview fail (and compile sampling makes it intermittent); could this normalize missing metric values to the frontend behavior before validating?

return _invalid_metadata(label)
if "cache_timeout" in payload:
timeout = dict.__getitem__(payload, "cache_timeout")
if timeout is not None and not (

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rejects Superset's supported cache_timeout = -1 sentinel, which QueryContextProcessor includes whenever caching is disabled. Any MCP chart query using that valid setting now fails the new result-envelope check; could the validator allow CACHE_DISABLED_TIMEOUT?

@bito-code-review

bito-code-review Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #686bfa

Actionable Suggestions - 0
Additional Suggestions - 6
  • superset/mcp_service/utils/response_utils.py - 1
    • Asymmetric oversized-decimal handling · Line 252-256
      The exponent>=0 branch (lines 242-246) returns an `oversized_integer` token that still counts the value as unique, but this fractional branch returns `None` when oversized. `None` propagates up through `_profile_value_identity`/`format_data_columns`, which breaks the row loop and zeroes `sampled_rows`, `unique_count`, and `sample_values` for all columns. So `Decimal("1e4096")` and `Decimal("1e-4096")` produce wildly different metadata. Both numerator and denominator are bounded (<=4096 bits), so no unbounded allocation justifies the `None`. Return an oversized token here for consistency.
  • superset/mcp_service/chart/preview_utils.py - 1
    • Ignored width/height params · Line 195-196
      The new `width`/`height` params are only forwarded to the `ASCIIPreview` metadata; every `_generate_safe_ascii_*` generator still uses hardcoded sizes (`"=" * 50`, `data[:20]`, `data[:10]`, bar length 30). So `get_chart_preview.py`'s `self.request.ascii_width/ascii_height` have no effect on the rendered content, and the reported dimensions won't match the output. Either wire the params into the generators or keep fixed metadata.
  • superset/mcp_service/chart/schemas.py - 1
    • Aggregate alias comparison divergence · Line 2956-2961
      `_same_metric_query_reference` compares raw `ColumnRef.aggregate` values, but `create_metric_object` normalizes `STDDEV`->`STDDEV_SAMP` and `VAR`->`VAR_SAMP` when building form_data. `_metric_query_identity` in `sunburst.py` compares post-normalization values, so a primary `STDDEV` + secondary `STDDEV_SAMP` on the same column/label is rejected here yet treated as the intentional categorical fallback at runtime. Normalize both sides before comparing.
  • tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py - 1
    • Stubbed validation weakens test · Line 70-75
      The test stubs `validate_request_with_warnings` to always return `is_valid=True`, so it never exercises the real duplicate-metric acceptance logic in `schemas.py` (the `_same_metric_query_reference` special case). If that logic regressed, this test would still pass. Consider asserting against the real pipeline or adding a non-stubbed variant.
  • superset/mcp_service/chart/sunburst.py - 1
    • Redundant pure-function call · Line 160-162
      `_metric_query_identity(primary_metric)` is a pure function invoked twice in the same boolean condition (lines 160 and 162). Hoist it into a local variable and reuse it for both the equality check and the `is not None` guard to avoid the redundant call.
  • tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py - 1
    • Ineffective test monkeypatch · Line 309-309
      This monkeypatch is a no-op: neither path in this test reaches `response_json_failure` (the only consumer of `query_result.MAX_QUERY_RESULT_VALUE_BYTES`). `too_many` returns at the `query_count` check and `oversized` at the `source_bytes` check, both before the finalizer. Remove it (and the now-unused `_query_result_mod` import) or add a case that reaches the finalizer.
Review Details
  • Files reviewed - 20 · Commit Range: 05c9abc..1990cb9
    • superset/common/form_data_query_context.py
    • superset/mcp_service/chart/chart_helpers.py
    • superset/mcp_service/chart/preview_utils.py
    • superset/mcp_service/chart/query_result.py
    • superset/mcp_service/chart/schemas.py
    • superset/mcp_service/chart/sunburst.py
    • superset/mcp_service/chart/tool/get_chart_data.py
    • superset/mcp_service/utils/response_utils.py
    • tests/unit_tests/common/test_form_data_query_context.py
    • tests/unit_tests/mcp_service/chart/test_chart_helpers.py
    • tests/unit_tests/mcp_service/chart/test_preview_utils.py
    • tests/unit_tests/mcp_service/chart/test_query_result.py
    • tests/unit_tests/mcp_service/chart/test_response_preflight.py
    • tests/unit_tests/mcp_service/chart/test_sunburst_chart.py
    • tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py
    • tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
    • tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py
    • tests/unit_tests/mcp_service/utils/test_response_utils.py
    • superset/mcp_service/chart/tool/generate_chart.py
    • superset/mcp_service/chart/tool/get_chart_preview.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 Bito Logo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

github_actions Pull requests that update GitHub Actions code size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants