Skip to content

feat(mcp): add typed Bullet chart support - #43770

Open
aminghadersohi wants to merge 36 commits into
apache:masterfrom
aminghadersohi:sc-119157-mcp-bullet
Open

feat(mcp): add typed Bullet chart support#43770
aminghadersohi wants to merge 36 commits into
apache:masterfrom
aminghadersohi:sc-119157-mcp-bullet

Conversation

@aminghadersohi

@aminghadersohi aminghadersohi commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

SUMMARY

Adds complete typed MCP support for the frontend ECharts Bullet plugin (viz_type: bullet).

  • Adds the exact chart_type: bullet discriminated-union member, plugin registry entry, schema discovery examples, tool guidance, and recommendation metadata.
  • Models the frontend controlPanel / buildQuery / transformProps contract, including SIMPLE, saved, and SQL metrics; grouped row hierarchies; filters/time controls; ordering/limits; and range, marker, label, legend, and number-format controls.
  • Adds dataset-aware exact/casefold normalization, ambiguity detection, role/output validation, numeric-output constraints, and order-independent conflict detection for semantic dimensions versus native groupby aliases.
  • Uses the shared MCP query builder for Bullet's singular metric + groupby transformation and preserves frontend-only orderby semantics for compile/data paths.
  • Preserves omitted native Bullet query, filter, and presentation state across update_chart and update_chart_preview, including the exact native adhoc-filter sequence and in-place provenance binding replacement, while explicit empty/false/null values remain authoritative.
  • Adds bounded iterative query-error extraction and a strict shared ChartData result envelope used by compile, saved/unsaved preview, and saved/unsaved get-data paths.
  • Uses shortest round-trip-safe finite-number serialization for Bullet ranges, markers, and marker lines, including extreme finite values.
  • Adds layered horizontal ASCII/Vega-Lite preview support for saved and unsaved Bullet charts, including metric aliases, the complete dimension hierarchy, ranges, targets, and marker lines.
  • Adds product-path coverage through generate_chart, update_chart, update_chart_preview, compile/query builders, registry/schema discovery, dataset normalization, get-data, and preview paths; includes non-Bullet regression guards.

Round-4 fixes:

  • Treats every native metric string—including repository names such as sum__num and sum__SP_POP_TOTL—as a predefined metric; adhoc SIMPLE metrics require their explicit object shape, while a strict bounded adapter retains the documented legacy label-only saved-metric object.
  • Separates explicit temporal subject changes from explicit range changes. Range-only updates preserve the saved subject, replace the provenance-owned binding in place, retain user filters, and reject missing or duplicate provenance candidates instead of guessing.
  • Preserves positional empty label tokens such as Low,,High independently from numeric token parsing, while retaining cardinality and comma-safety validation.
  • Allocates collision-free internal Vega Bullet category fields even when metric, dimension, or result aliases use the reserved-looking key.
  • Bounds strings and binary values before scalar rendering, never invokes arbitrary object str/repr, and retains the iterative depth/width/item/output failure bounds.
  • Advertises Bullet's interactive behavior, implemented URL/ASCII/Vega-Lite previews (not table), metric role, and optional categorical role.
  • Routes empty saved and unsaved Bullet Vega results through the same Bullet no-data validation contract.

Round-5 fixes:

  • Uses exact builtin-type checks for strings, integers, floats, booleans, bytes, bytearrays, and memoryviews before any scalar operation, so subclasses cannot dispatch overridden slicing, conversion, comparison, magnitude, or buffer-adjacent hooks.
  • Extracts only exact supported builtin values from enums without consulting public value or string hooks; unsupported enum values and all scalar subclasses/unknown objects use a byte-bounded type descriptor read without instance or metaclass overrides.
  • Keeps scalar/container classification identity-based, preserves bounded iterative depth/width/item/output extraction, and strictly bounds small remaining UTF-8 budgets as well as exact builtin rendering.
  • Adds direct and Bullet compile/saved-preview/unsaved-preview regressions for hostile scalar subclasses, bool- and bytes-like objects, enum hooks, multi-megabyte values/type names, exact builtins, and byte-output limits.

Round-14 fixes:

  • Normalizes only exact trusted pandas/NumPy result scalars at the shared ChartData validation boundary, including timestamps, NaT/NA, timedeltas, periods, intervals, datetime/timedelta scalars, and finite numeric/boolean scalars. Timestamp normalization preserves the represented instant, timezone offset/name, fold, and nanosecond precision without allowing arbitrary subclass hooks.
  • Makes the result validator explicitly bounded across query count, rows, nested values/work, aggregate string bytes, UTF-8 string/key sizes, integer magnitude, Decimal digits/exponent/storage, and metadata items/bytes. Non-finite values, inconsistent metadata row counts, and unsupported subclasses fail through the existing structured malformed-result contract before uniqueness or JSON serialization.
  • Seeds the final semantic get_table query context before ChartDataCommand construction, preserving virtual-dataset url_param, filter_values, and get_filters behavior and completing the audit of every MCP ChartDataCommand call site.
  • Shares saved/unsaved get-data column metadata construction and honors aligned validated temporal, boolean, numeric, and string coltypes for empty and non-empty results.
  • Adds real DataFrame-to-QueryContextProcessor/ChartDataCommand coverage for Big Number, Waterfall, XY, Mixed, and cached results, plus adversarial exact/beyond-boundary regressions.

Tracking: Shortcut SC-119157
Agor worktree: sc-119157-mcp-bullet

Deduplication/coordination: upstream master was inspected at exact head 45b903b19ad0217c8175fe451d742515ec62ac56; related PRs were re-inspected without modification at #43737 (6fd8ab5a13fa7481fd5b083544f74abf90e08ddc), #43679 (364f479c810f815bac51a8bee0a33a53c880bf54), and #43711 (e06be4f694a091bbc93af23fad69fd2e23c587e9). Work remains scoped to Amin's sc-119157-mcp-bullet branch; no contributor, community, or Sunburst branch was modified. The separate active #43771 fixer has semantic overlap in shared query-result/cache handling; Round 14 retains the existing MalformedQueryResult public error contract, exact-container/no-hook rules, and compatible cache metadata for later reconciliation. Prior BASE_AXIS, Deck.gl, Pivot, query-parity, explicit-clear/provenance/identity, and #43711 Jinja contracts remain covered. Round-14 implementation is delivered at 1b6b88e81acd132e92eafbb7e86b491cbca7b9f8; exact-head CI is queued/running, a fresh independent round-15 review follows, and no clean-review claim is made here.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — this changes the MCP server contract/query/preview paths and does not modify the frontend Bullet plugin UI. A live Superset server was not available in this worktree, so no UI screenshot was captured.

TESTING INSTRUCTIONS

Validated with SQLAlchemy 2.0.52 on exact commit 1b6b88e81acd132e92eafbb7e86b491cbca7b9f8:

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

# Focused Round-14 result-boundary, compile, preview, get-data, and semantic regressions
$PY -m pytest -q tests/unit_tests/mcp_service/chart/test_query_result.py
$PY -m pytest -q \
  tests/unit_tests/mcp_service/chart/test_compile.py \
  tests/unit_tests/mcp_service/chart/test_bullet_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/semantic_layer/tool/test_get_table.py

# Full MCP chart and service suites
$PY -m pytest -q tests/unit_tests/mcp_service/chart
$PY -m pytest -q tests/unit_tests/mcp_service

# Broad shared QueryObject/QueryContext coverage
$PY -m pytest -q \
  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

# Relevant frontend Pivot and Deck.gl regressions
cd superset-frontend
npm run test -- \
  plugins/plugin-chart-pivot-table/test/plugin/buildQuery.test.ts \
  plugins/plugin-chart-pivot-table/test/plugin/utilities.test.ts \
  plugins/preset-chart-deckgl/src/layers/Geojson/Geojson.test.tsx \
  plugins/preset-chart-deckgl/src/layers/Polygon/Polygon.test.tsx \
  plugins/preset-chart-deckgl/src/layers/Path/Path.test.tsx

# Quality gates from the repository root
ruff format --check <round-14 Python files>
ruff check <round-14 Python files>
$PY -m py_compile <round-14 production and test Python files>
pre-commit run mypy --files <round-14 Python files>
PYTHONPATH="$PWD:$PWD/superset-core/src" \
  $PY -m pylint --rcfile=.pylintrc \
  superset/mcp_service/chart/query_result.py \
  superset/mcp_service/chart/tool/get_chart_data.py \
  superset/mcp_service/semantic_layer/tool/get_table.py
pre-commit run
git diff --check
git diff --cached --check

Exact results:

  • Focused shared result validator: 126 passed
  • Full tests/unit_tests/mcp_service/chart: 1808 passed
  • Full tests/unit_tests/mcp_service: 4144 passed
  • Broad shared QueryObject/QueryContext suite: 195 passed
  • Relevant Pivot and Deck.gl Jest regressions: 5 suites, 83 passed
  • Ruff format/check, repository-configured MyPy, direct scoped Pylint (10.00/10), Python compilation, and diff checks: passed

Round-14 coverage includes real DataFrames flowing through QueryContextProcessor.get_data and real ChartDataCommand.run for timestamped Big Number, Waterfall, XY, Mixed, and cache results; timezone/fold/null/timedelta/numeric/boolean normalization; one-megabyte strings/keys, 100,000 rows, 10,000-bit integers, Decimal and metadata limits at and beyond boundaries; saved/unsaved empty/non-empty coltype round trips; and virtual-dataset Jinja helpers seeded before command construction. The full suites retain prior BASE_AXIS, Deck.gl, Pivot, Bullet, explicit-clear/provenance/identity, strict exact-container/no-hook, cache metadata, query-parity, and #43711 integration guards.

Environment caveats:

  • curl -f http://localhost:8088/health fails because no live Superset server is running in this worktree; live/manual UI verification was unavailable. See the Working with LLMs setup guide.
  • Staged pre-commit passed every runnable hook except its isolated Pylint environment: the Superset Pylint plugin could not import sqlglot.dialects.singlestore and exited before linting. The same three production files passed direct repository-configured Pylint in the SQLAlchemy 2.0.52 environment at 10.00/10.
  • The exact-head frontend declares Node ^24.16.0/npm ^11.13.0, while the worktree provides Node 22.22.2/npm 10.9.7; focused frontend tests passed with the existing install.

ROUND-15 TESTING AND COORDINATION

Round-15 fixes are delivered at exact commit f8c142aacd4f0d653b3a04d6755dd6e31d51534b, based directly on requested head 1b6b88e81acd132e92eafbb7e86b491cbca7b9f8. The original exact-head CI was reconciled: 72 checks succeeded and the three reported failures (unit-tests (current), unit-tests-required, and the aggregate Python result check) all traced to the single dateutil/US/Pacific pandas-timezone regression. The new exact-commit CI is queued/running; an independent round-16 review follows, and no clean-review claim is made.

Round-15 coverage and contracts:

  • exact-type/no-public-hook timezone canonicalization for dateutil tzfile/tzoffset/UTC and pytz named/fixed/UTC implementations, including dateutil/US/Pacific, positive/negative fixed offsets, fold, DataFrame materialization, pandas Timestamp/Timedelta/NaT, NumPy temporal values, and actual QueryContextProcessor/ChartDataCommand producer paths; custom/hostile tzinfo remains rejected;
  • a 50,000-row per-query cap, 100,000-row aggregate two-leg cap, 2.5-million aggregate value/work allowance, existing 16 MiB encoded-text and bounded nested-work contracts, plus a separate one-million row-by-column profiling budget with explicit sampled-statistics metadata; boundary tests cover exact/beyond Big Number and Mixed two-query shapes and 50,000 sparse rows with 4,096 declared columns;
  • semantic-layer get_table and dataset query_dataset normalize exactly once immediately after ChartDataCommand.run, consume only normalized rows/validated metadata, and map ChartError to their public error schemas; hostile scalar, oversized, pandas/NumPy, and malformed-envelope tests prove formatting is not reached before validation;
  • all ChartDataCommand data consumers were audited. Compile, preview, preview-utils, saved/unsaved get-data, semantic-layer, and dataset paths use the shared normalizer; get_chart_sql consumes the distinct SQL-only ChartDataResultType.QUERY envelope. Final Jinja form-data seeding before command construction, coltypes, cache metadata, provenance/identity, scalar bounds, BASE_AXIS, Deck.gl, and Pivot guards remain covered.

Validation used Python 3.11, SQLAlchemy 2.0.52, pandas 2.3.3, NumPy 2.4.6, and sqlglot 30.17.0:

  • focused Round-15 MCP chart/dataset/semantic/utils files: 512 passed; after final profiling-budget refinement, targeted affected files: 395 passed and 298 passed;
  • full tests/unit_tests/mcp_service/chart: 1824 passed;
  • full tests/unit_tests/mcp_service: 4165 passed;
  • relevant common QueryContext/QueryObject suite: 161 passed;
  • relevant frontend Geojson Jest suite: 1 suite, 17 passed;
  • Python compilation, git diff --check, MyPy, Ruff format/check, and every other applicable staged pre-commit hook: passed;
  • direct repository-configured Pylint over all five changed production modules: 10.00/10 with zero messages. The isolated pre-commit Pylint hook exits before linting because its hook environment cannot import sqlglot.dialects.singlestore; the direct SQLAlchemy 2.0.52 environment run passes.

Coordination/reconciliation: upstream master was inspected at cf77deb678a2421a347e44d4cd0647916dab5514; related PRs were read-only inspected at #43737 (6fd8ab5a13fa7481fd5b083544f74abf90e08ddc), #43679 (364f479c810f815bac51a8bee0a33a53c880bf54), and #43711 (e06be4f694a091bbc93af23fad69fd2e23c587e9). Agor review-zone/session inspection found no competing same-PR implementation. Active #43771 shared-result work was read-only compared through fa5d90f99f51e4aaef5769add352ffe3ad6d8edb; task 01a061a4-1a2c-74ec-bcef-b65f1c00d996 carries the Bullet findings to that work. Both contracts align on 50,000 rows per query, 100,000 aggregate rows, 2.5-million aggregate values, pandas/NumPy normalization, and final Jinja seeding; Bullet additionally bounds downstream profiling work. No contributor/community or Sunburst branch was modified.

Environment caveat: curl -f http://localhost:8088/health fails because this worktree has no live Superset server, so live/manual UI verification was unavailable. See the Working with LLMs setup guide.

ROUND-16 TESTING AND COORDINATION

Round-16 fixes are delivered at exact commit a0e9bd3624836d354dc8b7b7fdb305e34adb6d30, based directly on requested head f8c142aacd4f0d653b3a04d6755dd6e31d51534b.

Round-16 coverage and contracts:

  • the aggregate 16 MiB guard meters compact JSON bytes for every exact normalized scalar, integer digits without decimal rendering, Decimal/temporal/UUID/enum values, UTF-8/non-ASCII and JSON escaping, keys, mapping/array delimiters, nesting, metadata, and all query legs; the 17,000 × 10**999 reproduction fails in the shared result boundary before Pydantic/FastMCP serialization;
  • a bounded Pydantic response preflight meters the actual returned projection, including multi-query first-leg aliases, DataColumn.sample_values, generated column/cache/performance metadata, and CSV/Excel export payloads. The compact 2 × 50,000 × 20 ordinary-value source envelope is accepted by source work/row limits but rejected when its actual aliased ChartData response exceeds 16 MiB;
  • semantic get_table, dataset query_dataset, and saved/unsaved chart get-data share one coltype-aware bounded metadata builder. It gathers up to three late non-null samples during the existing row×column pass, uses hook-free value-semantic identities (equal int/float/Decimal values collapse while string representations remain distinct), excludes null from uniqueness, preserves authoritative temporal/boolean/numeric/string coltypes on empty data, and exposes statistics.sampled_rows for approximation;
  • all ChartDataCommand consumers, cache metadata, response schemas, and exception paths were re-audited. The distinct SQL-only query-result consumer now enforces exact containers, bounded query count/text, response size, and safe exception rendering; source-result and complete-response guards retain the established MalformedQueryResult contract;
  • the previous exact-head CI's three failures were one environment-dependent dateutil/US/Pacific packaged-zone tzfile regression. Exact known dateutil packaged tzfile types are canonicalized through bounded internal names without accepting arbitrary subclasses or timezone hooks; the reproducing compile/query-result tests pass locally.

Validation used Python 3.11, SQLAlchemy 2.0.52, pandas 2.3.3, NumPy 2.4.6, and sqlglot 30.17.0:

  • exact-commit full tests/unit_tests/mcp_service: 4192 passed;
  • focused final query-result plus Big Number compile regression: 149 passed;
  • common QueryContext/QueryObject and SQLAlchemy extension suite: 268 passed;
  • staged pre-commit run, including repository-configured MyPy, Ruff format/check, and custom Pylint: all applicable hooks passed;
  • direct scoped MyPy (--follow-imports=skip) passed all six changed production modules; direct repository-configured Pylint over those modules passed at 10.00/10; Python compilation and staged/unstaged diff checks passed.

No frontend files changed, so frontend testing was not relevant to Round 16. The exact-commit CI is running; pre-commit, CodeQL, dependency/license, frontend build, validation, and several database/build jobs have passed, while unit/E2E/browser jobs remain pending. No clean-review claim is made; an independent round-17 review follows.

Coordination/reconciliation: upstream master was read-only inspected at cf77deb678a2421a347e44d4cd0647916dab5514; #43737 (6fd8ab5a13fa7481fd5b083544f74abf90e08ddc), #43679 (364f479c810f815bac51a8bee0a33a53c880bf54), and #43711 (e06be4f694a091bbc93af23fad69fd2e23c587e9) were also re-inspected read-only. Active Sunburst shared-result work was compared through its remote/latest fa5d90f99f51e4aaef5769add352ffe3ad6d8edb; it retains the same raw-string-only byte-meter gap and divergent metadata identity/sample semantics, so no code was copied or branch modified. Task 01a061da-67d0-71be-8810-4754496859cc carries the findings to that separate work. Agor context confirmed Amin's sc-119157-mcp-bullet branch in the mcp chart types zone with no competing same-PR session. No contributor, community, or Sunburst branch was changed.

Environment caveat: curl -f http://localhost:8088/health and the recorded branch app URL both fail because no live Superset server is running in this worktree, so live/manual UI verification was unavailable. See the Working with LLMs setup guide.

ROUND-17 TESTING AND COORDINATION

Round-17 fixes are delivered at exact commit 9ba1f0b7d10a540249bd8c3ac32b9d8abae0fb4c, based directly on requested head a0e9bd3624836d354dc8b7b7fdb305e34adb6d30.

Round-17 coverage and contracts:

  • source-result cell normalization retains the 64 KiB scalar cap, while final Pydantic response preflight meters derived strings directly against the remaining 16 MiB compact-JSON aggregate budget. Keys, escaping, containers, aliases, and every serialized occurrence remain charged exactly; 70+ KiB SQL/CSV and generated base64 Excel responses succeed, the exact 16 MiB boundary succeeds, and one byte beyond returns the aggregate malformed-result error;
  • exact builtin/NumPy NaN plus pandas NA/NaT forms canonicalize to None before accounting and profiling. Superset's producer converts infinities to NaN; a trusted infinity injected after that producer boundary remains rejected as malformed. Real QueryContextProcessor/ChartDataCommand, compile, preview, chart, semantic, and dataset paths cover nullable numeric output;
  • iterative row and metadata traversal tracks active ancestry instead of globally visited identities. Genuine cycles remain rejected, while shared acyclic list/dict references are normalized and charged once per serialized occurrence, including a real object-dtype DataFrame alias and exact byte-boundary coverage;
  • completeness uses statistics.sampled_rows whenever null counts are sampled and adds completeness_is_approximate plus sampled_rows to sampled quality output. All-null 10,000/50,000-row inputs report 0.0, while mixed unsampled and empty inputs retain exact results; chart, semantic, and dataset response metadata are covered;
  • exact Python datetime/time values support builtin/ZoneInfo plus packaged dateutil named/fixed/local/UTC and pytz named/fixed/UTC timezone implementations without invoking source timezone hooks. Pandas Timestamp supports dateutil tzlocal; selected fold/offset and null semantics are preserved, while hostile custom timezone implementations remain rejected.

Validation used Python 3.11, SQLAlchemy 2.0.52, pandas 2.3.3, NumPy 1.26.4, and sqlglot 30.17.0:

  • focused Round-17 response/result, chart compile/preview/get-data/SQL, semantic, dataset, and shared utility suite: 619 passed;
  • exact-tree full tests/unit_tests/mcp_service: 4213 passed (includes the full MCP chart, semantic, and dataset suites);
  • common QueryContext/QueryObject/chart-data plus SQLAlchemy extension suite: 211 passed;
  • relevant Pivot and Deck.gl Jest regressions: 5 suites, 83 passed;
  • staged repository pre-commit, repository-configured MyPy, Ruff format/check, custom Pylint, direct scoped Pylint (10.00/10), Python compilation, and staged/unstaged diff checks: passed.

Coordination/reconciliation: upstream master was read-only inspected at cf77deb678a2421a347e44d4cd0647916dab5514; #43737 (6fd8ab5a13fa7481fd5b083544f74abf90e08ddc), #43679 (364f479c810f815bac51a8bee0a33a53c880bf54), and #43711 (e06be4f694a091bbc93af23fad69fd2e23c587e9) were re-inspected read-only. Sunburst shared work was fetched and compared read-only through 63c6cdc2fa7dc608817ab3135b10576ff0bed134 (fix(mcp): bound serialized results and metadata); Round 17 aligns its packaged dateutil-name validation and bounded serialized-result/metadata principles while retaining Bullet's established query_result_data/MalformedQueryResult, separate source/final-response budgets, ancestry-based repeated-occurrence semantics, metadata accounting, and sampled unique-statistics contracts. Queue 01a06220-c2cd-73f9-b0e6-1aff19539caf remains the cross-workstream handoff. No contributor/community or Sunburst branch was modified, and Agor context found no competing same-PR session.

Exact-head CI completed with 75 successful, 3 neutral, and 3 skipped checks. Frontend Jest shard 6 timed out/cancelled on its first 25-minute attempt; the failed-job rerun (attempt 2) passed in 4m33s. No clean-review claim is made; round 18 follows.

Environment caveats:

  • curl -f http://localhost:8088/health fails because no live Superset server is running in this worktree, so live/manual UI verification was unavailable. See the Working with LLMs setup guide.
  • The frontend declares Node ^24.16.0/npm ^11.13.0, while the worktree provides Node 22.22.2/npm 10.9.7; the focused frontend suite passed with the existing install.

ROUND-18 TESTING AND COORDINATION

Round-18 fixes are delivered at exact commit bc2ce63e549d4368858b5d06981cdbcec742c950, based directly on requested head 9ba1f0b7d10a540249bd8c3ac32b9d8abae0fb4c.

Round-18 coverage and contracts:

  • the 64 KiB scalar-string cap remains limited to source row cells. FULL result metadata strings, including the query/sql aliases, are normalized and charged by exact compact-JSON escaped UTF-8 bytes against both the 1 MiB metadata budget and the 16 MiB aggregate budget. Real ChartDataCommand envelopes with 65,537-byte and 70 KiB rendered SQL succeed through compile, chart preview/get-data, semantic get_table, and dataset query_dataset; metadata and aggregate exact-boundary/+1 behavior, aliases, non-ASCII, and escaping are covered without double counting or bypass;
  • trusted built-in QueryObject compare ratio/percentage/difference results canonicalize pandas nulls, NaN/NaT, and positive/negative infinity to None after all postprocessing and before record materialization. Finite values, large integers, source DataFrames, dtypes, and coltypes remain intact, while infinity injected after ChartData materialization continues to be rejected by the untrusted-envelope validator;
  • the shared Excel scalar projection converts exact UUID values to strings for both openpyxl and xlsxwriter. Real get_chart_data(format=excel) workbooks cover both engines, the missing-engine path retains ExportError, CSV/JSON UUID behavior is unchanged, and final response accounting still validates the serialized export.

Validation used Python 3.11, SQLAlchemy 2.0.52, pandas 2.3.3, NumPy 1.26.4, and sqlglot 30.17.0:

  • exact-commit full tests/unit_tests/mcp_service: 4232 passed;
  • focused real-producer and cross-consumer Round-18 suite: 192 passed, plus exact boundary and final SQL/UUID targets (10 passed and 5 passed);
  • common QueryContext/QueryObject/chart-data and SQLAlchemy extension suite: 223 passed;
  • relevant Pivot and Deck.gl Jest regressions: 5 suites, 83 passed;
  • staged repository pre-commit run, repository-configured MyPy, Ruff format/check, custom Pylint, direct scoped Pylint (10.00/10), Python compilation, and staged/unstaged diff checks: passed.

Coordination/reconciliation: upstream master was initially inspected read-only at cf77deb678a2421a347e44d4cd0647916dab5514; #43737 (6fd8ab5a13fa7481fd5b083544f74abf90e08ddc), #43679 (364f479c810f815bac51a8bee0a33a53c880bf54), and #43711 (e06be4f694a091bbc93af23fad69fd2e23c587e9) were re-inspected read-only and retain those heads. Sunburst shared-result work was fetched and compared read-only through its latest 923d925437f6317ba461154a102e8aa9eb902329; handoff task 01a06288-c5c0-77c7-a662-7dddf49b3a36 completed at Sunburst bab761b310517918290cd5dbf4a6199107a54d61, with subsequent shared metadata/source-boundary follow-ups included in the latest comparison. Agor confirms Amin's sole sc-119157-mcp-bullet worktree/session in the mcp chart types zone. Only Amin's branch was normal-pushed; no contributor/community/Sunburst branch, GitHub comment, or review was modified. During exact-head CI, upstream master advanced to 9b2bb15ececb8c5af41713901d417e03c76922e7; GitHub therefore reports this exact head as conflicting in semantic get_table and dataset query_dataset. Those new-base conflicts were inspected with a read-only merge tree and deliberately not resolved so the delivered commit remains based directly on the requested head.

Exact-head CI completed with 72 successful and 6 skipped checks. The Python unit-test runner hit its fixed 30-minute limit on all three attempts, so unit-tests (current) is cancelled and its required aggregator reports failure. This was a timeout rather than a test assertion: attempt 2 completed the main suite (14,331 passed, 4 skipped, 3 xfailed) and both subsequent 100%-coverage groups (1,486 passed, 1 xfailed; 402 passed) before cancellation during post-test coverage upload; attempt 3 again completed the main suite (14,331 passed, 4 skipped, 3 xfailed) before cancellation in the coverage phase. No clean-review claim is made; round 19 follows.

Environment caveats:

  • curl -f http://localhost:8088/health fails because no live Superset server is running in this worktree, so live/manual UI verification was unavailable. See the Working with LLMs setup guide.
  • The frontend declares Node ^24.16.0/npm ^11.13.0, while the worktree provides Node 22.22.2/npm 10.9.7; the focused frontend suite passed with the existing install.

ROUND-19 TESTING AND COORDINATION

Round-19 is delivered by merge commit b8c5f79142533b1cbecbeb148ea86578d7797817 (parents: requested head bc2ce63e549d4368858b5d06981cdbcec742c950 and upstream master 46c8c67a400f57031e3ec61298279470971071ce) plus implementation commit 889208c24f5756191941b16f1ba94808b8077754 and cleanup commit 276f6511a6f4d0da7f4c620ee378053026c2be36. The integration was a normal merge and the branch was normal-pushed; no rebase or force-push was used.

Round-19 coverage and contracts:

  • shared bounded, coltype-aware column metadata assigns booleans their own JSON-scalar identity, so [True, 1, False, 0] has unique_count=4; chart get-data, semantic get_table, dataset query_dataset, and the shared builder each have consumer coverage;
  • exact builtin timedelta values canonicalize at trusted normalization to the same stable ISO text emitted by the existing Pydantic JSON contract; exact date and UUID values likewise canonicalize to their existing JSON strings. Accounting uses the canonical string's exact escaped UTF-8 compact-JSON size, with real result-envelope and exact-boundary/+1 tests, while CSV/JSON/Excel UUID and temporal export guards remain covered;
  • DataFrame producer cleanup materializes records before inspecting exact trusted scalar types. It converts only exact builtin/NumPy floating nonfinites and exact pandas missing sentinels to None, never performs frame-wide replacement/object equality, preserves Enum values for the established consumer normalization, and retains rejection of infinity injected after materialization. Direct df_to_records and real QueryContextProcessor hostile-__eq__ regressions cover the boundary;
  • the merge resolution retains feat(datasource): headless query API for Explorable datasources #43527's shared validate_query_names, build_query_dict, and execute_tabular_query paths, including dimension/metric/filter/order validation, ascending/descending inversion, temporal range/grain semantics, cache controls, execution, and final Jinja form-data seeding. This PR's query_result_data normalization/budgets, exact result extraction, coltype-aware metadata, response preflight, and structured public errors run immediately above that shared execution result. Both the shared tabular-query tests and the MCP semantic/dataset families pass.

Validation used Python 3.11 and SQLAlchemy 2.0.52:

  • focused shared response/DataFrame/query-result suite: 206 passed;
  • full MCP chart plus affected semantic/dataset/cache/response service suite: 2030 passed;
  • all branch-changed Python test files: 1374 passed;
  • shared tabular query, common QueryContextProcessor, and DataFrame suite: 103 passed;
  • relevant Geojson Jest suite: 1 suite, 17 passed;
  • staged repository pre-commit run (including MyPy, Ruff format/check, custom Pylint, YAML and zizmor), separate scoped MyPy/Pylint, Python compilation, and staged/unstaged diff checks: passed.

CI timing was inspected separately from functional failures. At old exact head bc2ce63..., unit-tests (current) started the main pytest phase at 16:15:36Z, completed 14,331 passed, 4 skipped, 3 xfailed in 26:42, entered the independent 100%-coverage phase, and was cancelled at the fixed 30-minute job deadline while the SQL gate was at 61%; no assertion failure was observed. Round-19 replaces two avoidable multi-gigabyte-equivalent test constructions with equivalent bounded/exact arithmetic or reduced-budget checks (without removing a test or relaxing a production limit), and raises the job timeout to 40 minutes so the full suite plus independent SQL/semantic 100% gates and upload can complete under runner variance. At exact head 276f6511a6f4d0da7f4c620ee378053026c2be36, required unit-tests (current) run 33667465444 / job 100373127800 passed in about 28m45s: the main phase reported 14,428 passed, 4 skipped, 3 xfailed in 23m55s, the SQL 100% gate reported 1,486 passed, 1 xfailed in 27.42s, the semantic 100% gate reported 415 passed in 29.01s, and Codecov accepted the upload. Required aggregator job 100382902161 passed. The exact-head pre-commit check also passed. At the reporting snapshot, the rollup was 70 success, 4 neutral, 3 skipped, 2 failure, plus one pending docs-preview context; the only failures were CodeQL Python/JavaScript jobs, whose logs fail before analysis because the workflow initialized CodeQL action 4.37.9 but ran analyzer 4.37.8. This upstream action-version mismatch is reported separately from the successful functional matrix and was not papered over. This is not a clean-review claim.

Shared post-merge Sunburst follow-up (8a053c8ff905f3ed6997f0fd1873c2863532352d): every ChartError construction in get_chart_sql now passes through one final complete-response preflight, including the error-only QueryGenerationFailed path. A real exact-16MiB source error regression verifies that the duplicated message/error projection is replaced by a bounded structured fallback rather than serializing at roughly 32MiB. A fixed-envelope test accepts the exact escaped-JSON limit and rejects the same envelope at limit-minus-one (one byte over), while asserting that the fallback itself remains within the configured bound. Focused get_chart_sql reported 81 passed; combined query-result/get-SQL/get-data reported 418 passed; the full MCP chart suite reported 1,883 passed; staged pre-commit, MyPy, Ruff, Pylint, compilation, and diff checks passed. At exact head 8a053c8ff905f3ed6997f0fd1873c2863532352d, Python-Unit run 33671883797 passed: unit-tests (current) job 100387307158 completed in 29m11s with 14,430 passed, 4 skipped, 3 xfailed in the main phase, 1,486 passed, 1 xfailed in the SQL 100% gate, 415 passed in the semantic 100% gate, and an accepted Codecov upload; required aggregator unit-tests-required job 100396942734 passed. The exact-head rollup was 72 success, 4 neutral, 3 skipped, the same two upstream CodeQL action-version failures (4.37.9 initialization versus 4.37.8 analyzer), and one pending docs-preview context.

Coordination/reconciliation: Agor session/zone context was inspected before work and showed the completed Round-19 reviewer archived with no competing same-PR fixer. Upstream master was fetched and merged at 46c8c67a400f57031e3ec61298279470971071ce. #43737 (6fd8ab5a13fa7481fd5b083544f74abf90e08ddc), #43679 (364f479c810f815bac51a8bee0a33a53c880bf54), and #43711 (e06be4f694a091bbc93af23fad69fd2e23c587e9) were re-inspected read-only. Future reconciliation should preserve #43737's shared failure/query changes, #43679's saved-config update behavior, and #43711's compile/preview Jinja seed-shape assertions alongside Bullet's exact-container budgets, explicit-clear/provenance/update semantics, and structured errors. Sunburst #43771 was fetched and compared read-only at 923d925437f6317ba461154a102e8aa9eb902329; it already separates boolean metadata identity and canonicalizes date/UUID/duration values but retains a distinct metadata builder/duration spelling and the producer DataFrame.replace path, so future shared consolidation must retain this round's exact JSON contract and hook-free post-materialization cleanup. No contributor/community/Sunburst branch, GitHub comment, review, Shortcut item, or Agor object was modified.

Environment caveats:

  • curl -f http://localhost:8088/health fails because no live Superset server is running in this worktree, so live/manual UI verification was unavailable. See the Working with LLMs setup guide.
  • The frontend declares Node ^24.16.0/npm ^11.13.0, while the worktree provides Node 22.22.2/npm 10.9.7; the focused frontend suite passed with the existing install.

ROUND-20 TESTING AND COORDINATION

Round-20 starts from the requested clean head 8a053c8ff905f3ed6997f0fd1873c2863532352d. The implementation is commit 8c17f7f6c78bad2c0d359d76a7ecdb509eceb908; final delivery is normal merge commit c3c07fda339dc58f21d9aaed4aeeb662995dd2f3 (parents 8c17f7f6c78bad2c0d359d76a7ecdb509eceb908 and upstream master e8540b7c49583a8ca89cfbcfd95a6a6e6a66a6ed). Upstream was integrated with normal merges and the branch was normal-pushed; no rebase or force-push was used.

Round-20 coverage and contracts:

  • every preview-bearing public entry point and shared preview helper preflights its complete Pydantic wire projection, including success and error responses from all get_chart_preview strategies, generate_chart, immediate and preview-first update_chart, update_chart_preview, cache aliases, and shared form-data preview generation. Actual MCP entry-point tests accept the exact 16 MiB limit and return a bounded, schema-correct structured fallback at +1; they cover empty/non-empty envelopes, aliases/caches, and Bullet derived-category amplification;
  • response accounting uses Pydantic JSON-mode wire values rather than Python-mode values, preserving the UTC Z projection and avoiding +00:00 overcount. Complete outer models are metered once without serializing a second full response;
  • one bounded, hook-free Bullet category helper matches frontend JavaScript String(value) semantics for null versus empty string, lowercase booleans, integral/non-integral floats, negative zero, finite Decimal values, temporal values, and UUIDs. Strings remain unchanged, Vega preserves normalized raw dimensions where possible, ASCII uses the same category spelling, and collision/budget guards remain enforced;
  • valid zero-row Bullet results compile and preview. Ungrouped results use the frontend zero-valued fallback; grouped results retain no fabricated category and produce explicit no-data ASCII/Vega output. Saved, unsaved, create/update/cached, get-data, and both preview-format contracts retain product-path coverage;
  • prior Round-19 SQL error, tabular-query, result-budget, scalar/DataFrame, QueryContext/Jinja, cache, provenance, and exact-container contracts remain covered.

Validation used Python 3.11 and SQLAlchemy 2.0.52:

  • final focused generate/update/preview/compile/data/response suite: 564 passed;
  • full MCP chart/service plus shared DataFrame and QueryContextProcessor suite: 4,329 passed in 5m40s;
  • SQL and semantic-layer unit suites: 1,901 passed, 1 xfailed;
  • frontend Bullet transform suite: 1 suite, 12 passed;
  • staged backend MyPy, Ruff format/check, Pylint, oxfmt, oxlint, custom/style hooks, Python compilation, and staged/unstaged diff checks: passed.

Environment caveats:

  • curl -f http://localhost:8088/health fails because no live Superset server is running in this worktree, so live/manual verification was unavailable. See the Working with LLMs setup guide.
  • the targeted frontend type-check is blocked by missing generated package declaration outputs (TS6305) in the existing worktree install; the focused Jest suite and all runnable frontend formatting/lint hooks pass.
  • the local pytest environment does not include pytest-cov, so the independent SQL/semantic coverage gates are verified by exact-head CI rather than claimed from the local run.

Coordination/reconciliation: Agor session and zone context was inspected before work and showed the completed reviewer archived with no parallel same-PR reviewer/fixer. #43737, #43679, and #43711 were fetched and re-inspected read-only for shared failure/query, saved-update, and compile/preview overlaps. Sunburst was compared read-only at 781698ba7c660d561759aff6a22495cc55c3b89e; no contributor, community, or Sunburst branch, GitHub comment/review, Shortcut item, or Agor object was modified. Exact-head CI completed at c3c07fda339dc58f21d9aaed4aeeb662995dd2f3 with 76 successful, 2 neutral, and 3 skipped check runs plus a successful docs-preview status. Required unit-tests (current) and unit-tests-required, pre-commit, and CodeQL Python/JavaScript all passed. The current-unit job completed in 30m55s within its 40-minute limit: the main phase reported 14,452 passed, 4 skipped, 3 xfailed in 26m18s; the SQL gate reported 1,486 passed, 1 xfailed at 100% coverage; the semantic gate reported 415 passed at 100% coverage; and Codecov accepted the upload. GitHub reports the head mergeable with merge-state BLOCKED. A fresh independent review follows, and no clean-review claim is made.

ROUND-21 TESTING AND COORDINATION

Round-21 starts from the requested exact head c3c07fda339dc58f21d9aaed4aeeb662995dd2f3 and is delivered by implementation commit 13adf37c12ca50a366f701173b2c9f6913894c44 (parent c3c07fda339dc58f21d9aaed4aeeb662995dd2f3). The branch was normal-pushed; no rebase or force-push was used.

Round-21 coverage and contracts:

  • Bullet numeric dimensions now follow the actual frontend JSON-number pipeline: exact builtin integers, floats, and finite Decimals are rounded to IEEE-754 binary64 before JavaScript String(Number) spelling. Coverage includes values beyond 2**53, tie-to-even rounding, excess Decimal precision, the fixed/exponent boundaries at 1e-6, 1e-7, 1e20, and 1e21, maximum finite binary64, finite-source overflow to positive/negative infinity, negative zero, non-finite rejection/null normalization, and exact-type no-hook guards;
  • ASCII and Vega use the same bounded category helper. Vega retains raw normalized dimension values for tooltips but consumes the precomputed corrected category directly; the later formula transform that overwrote it with a second coercion contract was removed. Backend, real FastMCP preview, and Node/Jest frontend parity tests cover the same boundary spellings;
  • after strict Bullet result validation, saved and form-data-key get_chart_data paths bypass the generic original-row NoData check and return the shared normalized result: one 0.0 metric row for ungrouped empty data and no fabricated rows/categories for grouped empty data. Actual FastMCP tests cover saved id/chart_id aliases, form-data-key-only queries, JSON, CSV, and Excel; the existing non-Bullet empty-result test retains NoData;
  • all prior complete-response preflight, exact 16 MiB Pydantic JSON-wire and UTC Z accounting, SQL/tabular-query, result-budget, scalar/DataFrame, cache/Jinja, provenance/update/rebind, empty compile/preview, and cross-chart contracts remain covered.

Validation used Python 3.11 and SQLAlchemy 2.0.52:

  • focused Bullet, saved/unsaved get-data, and actual preview-entry files: 411 passed;
  • full MCP service plus DataFrame, QueryContext, QueryObject, form-data, and shared tabular-query suites: 4,514 passed in 5m50s;
  • SQL and semantic-layer suites: 1,901 passed, 1 xfailed;
  • frontend Bullet Node/Jest parity suite: 1 suite, 13 passed;
  • staged MyPy, Ruff format/check, custom Pylint, oxfmt, oxlint, custom/style hooks, Python compilation, and staged/unstaged diff checks: passed.

Environment caveats:

  • curl -f http://localhost:8088/health fails because no live Superset server is running in this worktree, so live/manual verification was unavailable. See the Working with LLMs setup guide.
  • the targeted frontend type-check remains blocked by missing generated package declaration outputs (TS6305) in the existing worktree install; the focused Jest suite and all runnable frontend formatting/lint hooks pass.

Coordination/reconciliation: Agor session/zone context was inspected first and showed the completed independent reviewer idle with no parallel same-PR fixer. Latest #43771/Sunburst was fetched and compared read-only at be228ec3f7aeed11756be345124eaffd6c47839b, including its saved/cached get-data and shared result-validation changes; no Sunburst/community/contributor branch was touched or cherry-picked. No GitHub comment/review, Shortcut item, or Agor object was modified. Exact-head CI completed at 13adf37c12ca50a366f701173b2c9f6913894c44; gh pr checks --required reports every required context passing, including frontend build, pre-commit, MySQL/SQLite/Presto/Hive, Cypress/Playwright aggregators, and unit-tests-required. The current-unit job passed in 30m35s within its 40-minute limit: the main phase reported 14,488 passed, 4 skipped, 3 xfailed in 26m08s; SQL reported 1,486 passed, 1 xfailed at 100% coverage; semantic layers reported 415 passed at 100% coverage; and Codecov accepted the upload. CodeQL Python and JavaScript passed with action 4.37.9. Initial duplicate workflow attempts at the same head were superseded/cancelled; the latest required contexts are green. GitHub reports MERGEABLE with merge-state BLOCKED. A fresh independent round is requested, and no clean-review claim is made.

ROUND-22 TESTING AND COORDINATION

Round-22 starts from the requested exact head 13adf37c12ca50a366f701173b2c9f6913894c44. Current upstream master 52a4cb6314aea332f8061a7b3babd814cdebc33b was fetched and integrated by normal merge commit 5d11cc8aa0132ef0ba51f0969027b40c6d8a53fc (parents 13adf37c12ca50a366f701173b2c9f6913894c44 and 52a4cb6314aea332f8061a7b3babd814cdebc33b). The Round-22 implementation and delivery head is 50106d7b81b342271a60fe6df09f1be17b6a5abf (parent 5d11cc8aa0132ef0ba51f0969027b40c6d8a53fc). Both commits were normal-pushed; no rebase or force-push was used.

Round-22 coverage and contracts:

  • complete final-response preflight now measures floats with pydantic-core's actual JSON spelling while retaining the existing source-result and metadata budget rules. Complete ChartPreview models at the exact 16 MiB boundary and +1 byte cover both Python-repr mismatches: 1e-5 undercharging and 1e-6 overcharging, with the existing bounded structured fallbacks preserved;
  • every Bullet Vega category-bearing encoding uses the one precomputed frontend-equivalent category. The axis/grouping and bar tooltip share the same collision-safe derived field, joined multi-dimension tooltip title, and IEEE-754-coerced large-integer/high-precision-Decimal spelling; raw dimensions remain inspectable but cannot overwrite the category and no later Vega transform re-coerces it;
  • exact date/datetime Bullet dimensions follow Chart Data's epoch-millisecond number wire projection before JavaScript String(value). Backend, actual FastMCP ASCII/Vega, and frontend parity coverage includes dates, UTC datetimes, ZoneInfo ambiguous folds, the two distinct instants, bounded raw epoch values, and hostile timezone no-hook rejection;
  • grouped empty Bullet CSV and Excel exports preserve known column headers. Public FastMCP coverage asserts CSV headers and decodes both openpyxl and forced-xlsxwriter workbooks to assert their header row; ungrouped synthesized-zero and non-Bullet NoData behavior remain unchanged;
  • prior complete response-preflight/UTC Z, SQL/tabular-query, result-budget, scalar/DataFrame, cache/Jinja, provenance/update/rebind, saved/unsaved/cached zero-row preview, and cross-chart contracts remain covered.

Validation used Python 3.11 and SQLAlchemy 2.0.52:

  • complete focused response/Bullet/get-data/get-preview files: 420 passed;
  • full MCP service plus DataFrame, QueryContext factory/processor/timing, QueryObject, form-data, and shared tabular-query suites: 4,496 passed in 5m24s;
  • SQL and semantic-layer suites: 1,901 passed, 1 xfailed;
  • frontend Bullet Node/Jest parity suite: 1 suite, 13 passed;
  • changed-range pre-commit with backend MyPy, Ruff format/check, custom Pylint, oxfmt, oxlint, custom/style hooks, Python compilation, and staged/unstaged diff checks: passed.

Environment caveats:

  • curl -f http://localhost:8088/health fails because no live Superset server is running in this worktree, so live/manual verification was unavailable. See the Working with LLMs setup guide.
  • the targeted frontend type-check remains blocked by missing generated package declaration outputs (TS6305) in the existing worktree install; the focused Jest suite and all runnable frontend formatting/lint hooks pass.
  • a broad optional db-engine-spec collection attempt was blocked at collection by the existing environment's missing sqlalchemy_bigquery, pyhive, and trino extras; the requested SQL/semantic suites and exact-head database CI contexts are reported separately.

Coordination/reconciliation: Agor session and zone context was inspected first and showed the completed/archived reviewer with no parallel same-PR reviewer or fixer. The five upstream commits through 52a4cb6314aea332f8061a7b3babd814cdebc33b had no direct Round-22 path overlap and merged without conflict; upstream #43711's Jinja request-body reuse and both test families are retained. Latest #43771/Sunburst was fetched and compared read-only at 8b204b5cb5b0b7fca2db837eaec0a2cec4fe5aaf, including its separate pydantic-core final-float sizing helper; no Sunburst/community/contributor branch was modified or cherry-picked. No GitHub comment/review, Shortcut item, or Agor object was modified.

Exact-head CI completed at 50106d7b81b342271a60fe6df09f1be17b6a5abf with 74 successful, 3 neutral, and 3 skipped check runs. gh pr checks --required reports every required context passing, including frontend build, pre-commit, MySQL/SQLite/Postgres, Presto/Hive, Cypress/Playwright aggregators, and unit-tests-required; CodeQL Python and JavaScript also passed. Python-Unit run 33695455382, job 100463283994, completed in 30m57s within the 40-minute gate: the main phase reported 14,514 passed, 4 skipped, 3 xfailed in 26m30s, SQL reported 1,486 passed, 1 xfailed at 100% coverage, semantic layers reported 415 passed at 100% coverage, and Codecov accepted the exact-head upload. GitHub reports MERGEABLE with merge-state BLOCKED.

A fresh independent round is requested, and no clean-review claim is made.

ROUND-23 TESTING AND COORDINATION

Round-23 starts from the requested exact head 50106d7b81b342271a60fe6df09f1be17b6a5abf and is delivered by implementation commit 30bb2e29169a9465c2380326ba3fc10e5d3a9735 (parent 50106d7b81b342271a60fe6df09f1be17b6a5abf). The branch was normal-pushed; no rebase or force-push was used. The integrated upstream parent remains 52a4cb6314aea332f8061a7b3babd814cdebc33b through Round-22 merge commit 5d11cc8aa0132ef0ba51f0969027b40c6d8a53fc. Upstream was re-fetched through d921131c5d95ccc3d442d4389d5e42acb234f017; its three subsequent frontend/chart-list commits have no direct changed-path overlap with Round-23, so no additional master merge was needed.

Round-23 coverage and contracts:

  • the shared hook-free Chart Data temporal projection now recognizes exact trusted pd.Timestamp, exact np.datetime64, pandas/NumPy NaT, builtin datetime, and builtin date before generic ISO scalar normalization. Exact pandas timestamps are canonicalized to safe timezone implementations and then passed through Superset's production datetime_to_epoch, preserving its actual float behavior rather than approximating epoch milliseconds;
  • coverage matches json_int_dttm_ser for naive and aware timestamps, UTC and fixed-offset equivalent instants, ZoneInfo folds, pytz folds, date-only values, pre-epoch fractions, one-nanosecond/sub-millisecond values, and NaT-to-null behavior. Unsupported timestamp timezone implementations are rejected without invoking armed timezone hooks;
  • compile, saved get-data, ASCII, Vega grouping/axis/tooltips, and the shared direct renderer all consume the same epoch-number projection before JavaScript number/string coercion. Real producer regressions construct rows with df_to_records(pd.DataFrame(...)) and carry them through actual FastMCP saved get-data and both ASCII/Vega preview entry points;
  • the generic non-Bullet query-result path retains canonical ISO normalization, while exact np.datetime64 receives Bullet epoch semantics only in the explicit temporal-number mode;
  • all Round-22 Pydantic float sizing, derived tooltip category, zero-row/export-header, result-budget, SQL/Jinja/DataFrame/tabular, cache, provenance, update/rebind, and cross-chart contracts remain covered.

Validation used Python 3.11 and SQLAlchemy 2.0.52:

  • complete focused Bullet/query-result/get-data/get-preview files: 602 passed;
  • full MCP service plus DataFrame, QueryContext factory/processor/timing, QueryObject, form-data, and shared tabular-query suites: 4,504 passed in 5m19s;
  • SQL and semantic-layer suites: 1,901 passed, 1 xfailed;
  • frontend Bullet Node/Jest suite: 1 suite, 13 passed;
  • staged pre-commit including backend MyPy, Ruff format/check, custom Pylint, blacklist and standard hooks, Python compilation, and staged/unstaged diff checks: passed.

Environment caveat: curl -f http://localhost:8088/health fails because no live Superset server is running in this worktree, so live/manual UI verification was unavailable. See the Working with LLMs setup guide.

Coordination/reconciliation: Agor session and board zones were inspected first; the completed Round-23 independent reviewer was idle and no parallel reviewer/fixer was started. Current upstream master was fetched through d921131c5d95ccc3d442d4389d5e42acb234f017; its three newer commits were inspected and have no direct Round-23 path overlap, so no new merge was needed. Latest #43771/Sunburst was fetched and compared read-only at 0ae605f5ce6df8a9bff0cee13aa0d42c4b7a70a5, including its trusted timestamp canonicalization and separate producer-side non-finite Decimal normalization; no Sunburst/community/contributor branch was modified or cherry-picked. No GitHub comment/review, Shortcut item, or Agor object was modified.

Exact-head CI completed at 30bb2e29169a9465c2380326ba3fc10e5d3a9735; all code-head workflows and the subsequent body-only PR-lint completed successfully. gh pr checks --required reports every required context passing, including frontend build, pre-commit, MySQL/SQLite/Postgres, Presto/Hive, Cypress/Playwright aggregators, and unit-tests-required; CodeQL also passed. Python-Unit run 33700932458, job 100479917918, completed in 30m27s within the 40-minute gate: the main phase reported 14,522 passed, 4 skipped, 3 xfailed in 26m04s, SQL reported 1,486 passed, 1 xfailed at 100% coverage, semantic layers reported 415 passed at 100% coverage, and Codecov accepted the exact-head upload. GitHub reports MERGEABLE with merge-state BLOCKED.

A fresh independent round is requested, and no clean-review claim is made.

ROUND-24 TESTING AND COORDINATION

Round-24 starts from the requested exact head 30bb2e29169a9465c2380326ba3fc10e5d3a9735 and is delivered by implementation commit a502ae46285ccbba6adc72d9a9be6a1932b367c2 (parent 30bb2e29169a9465c2380326ba3fc10e5d3a9735). The branch was normal-pushed; no rebase or force-push was used.

Round-24 coverage and contracts:

  • df_to_records now recognizes only exact builtin Decimal values and calls the non-dispatching Decimal.is_finite(value) descriptor. Exact NaN, signaling NaN, positive infinity, and negative infinity become producer-boundary None; exact finite Decimal objects retain identity and precision until the intended JSON wire conversion. The record rewrite uses builtin dict operations and leaves Decimal subclasses untouched for the bounded consumer validator, without calling their equality, finite-check, or float hooks;
  • real DataFrame → QueryContextProcessorChartDataCommand coverage verifies strict Chart Data JSON, finite-Decimal provenance, and hostile-subclass rejection. Sync SQL Lab, async SQL result serialization, view deserialization, and public form-data-key FastMCP get-data regressions verify that Decimal non-finite tokens cannot escape while a finite high-precision Decimal remains valid;
  • exact dateutil named tzfiles now derive the source-selected offset from their own bounded, exact transition table rather than reinterpreting the wall time through ZoneInfo. The projection mirrors dateutil fold/gap/post-table selection without invoking timezone hooks and fails closed when the exact table cannot be trusted;
  • coverage includes Dublin's 2024 negative-DST fold, New York's 2024 spring gap and 2040 post-transition-table behavior, UTC/fixed/pytz/ZoneInfo/dateutil equivalent instants, pre-epoch fractions, microseconds, pandas nanoseconds, named and fixed dateutil zones, hostile methods/tables, real df_to_records(pd.DataFrame(...)) producers, compile, get-data, and public FastMCP ASCII/Vega Bullet paths. All compare directly with json_int_dttm_ser/datetime_to_epoch wire values;
  • all prior numeric/temporal category coercion, complete Pydantic response sizing, zero-row/export-header, source/metadata/result budgets, SQL/Jinja/DataFrame/tabular, cache, provenance, update/rebind, and cross-chart contracts remain covered.

Validation used Python 3.11 and SQLAlchemy 2.0.52:

  • complete focused DataFrame/Bullet/query-result/get-data/get-preview plus new SQL producer tests: 625 passed;
  • full MCP service plus the relevant DataFrame/QueryContext/QueryObject/form-data/tabular suites: 4,512 passed in 5m14s;
  • full common suite: 283 passed;
  • SQL, SQL Lab, and semantic-layer suites: 1,918 passed, 1 xfailed;
  • a hook-free parity audit compared 7,200 dateutil named-zone cases directly with datetime_to_epoch;
  • frontend Bullet Node/Jest suites: 2 suites, 16 passed;
  • staged pre-commit including backend MyPy, Ruff format/check, custom Pylint, blacklist and standard hooks, Python compilation, and staged/unstaged diff checks: passed.

Environment caveat: curl -f http://localhost:8088/health fails because no live Superset server is running in this worktree, so live/manual UI verification was unavailable. See the Working with LLMs setup guide.

Coordination/reconciliation: Agor session and board zones were inspected first; the completed Round-24 independent reviewer was idle and no parallel reviewer/fixer was started. Current upstream master was fetched through b457280b57ac79be13877fb6ab1132cad75d1118; its four commits after the already integrated master parent have no direct Round-24 changed-path overlap, so no additional merge was needed. Latest #43771/Sunburst was fetched and compared read-only at 28bbacacd0421f79913761c922b0428a9be52f23, including its separate exact-Decimal producer normalization commit 0ae605f5ce6df8a9bff0cee13aa0d42c4b7a70a5; no Sunburst/community/contributor branch was modified or cherry-picked. No GitHub comment/review, Shortcut item, or Agor object was modified.

Exact-head code CI completed at a502ae46285ccbba6adc72d9a9be6a1932b367c2 with 74 successful, 3 neutral, and 3 skipped check runs. gh pr checks --required reports every required context passing, including frontend build, pre-commit, MySQL/SQLite/Postgres, Presto/Hive, Cypress/Playwright aggregators, and unit-tests-required; CodeQL also passed. Python-Unit run 33706637574, job 100497103354, completed in 31m29s within the 40-minute gate: the main phase reported 14,543 passed, 4 skipped, 3 xfailed in 26m53s, SQL reported 1,487 passed, 1 xfailed at 100% coverage, semantic layers reported 415 passed at 100% coverage, and Codecov accepted the exact-head upload. GitHub reports MERGEABLE with merge-state BLOCKED.

A fresh independent round is requested, and no clean-review claim is made.

ROUND-25 TESTING AND COORDINATION

Round-25 starts from the requested exact head a502ae46285ccbba6adc72d9a9be6a1932b367c2 and is delivered by implementation commit 4131a07494af7046c0dd53d8d5ba23293796c4bf (parent a502ae46285ccbba6adc72d9a9be6a1932b367c2). The branch was normal-pushed; no rebase or force-push was used.

Round-25 coverage and contracts:

  • bounded exact-type dateutil named tzfiles with an empty transition table no longer require _ttinfo_before; they validate and use exact _ttinfo_std. Non-empty tables retain the exact before-record requirement, transition/container bounds, and fail-closed behavior. System and packaged dateutil UTC, GMT, Universal, Zulu, EST, HST, MST, Etc/GMT+1, and Etc/GMT-2 values are compared directly with datetime_to_epoch without timezone hook dispatch;
  • real df_to_records(pd.DataFrame(...))ChartDataCommand producer results cover compile and public FastMCP get-data/Vega preview for all of those system and packaged transitionless zones. Source numbers, derived grouping/axis categories, and Vega tooltips retain the Chart Data JSON/browser coercion contract;
  • empty ungrouped Bullet query results remain empty in _strict_bullet_result_data, saved aliases, and unsaved/form-data-key FastMCP get-data. Top-level and nested query data/counts remain zero. Saved CSV and openpyxl/xlsxwriter Excel exports emit known headers only, while the separate saved/unsaved preview contract still renders its private zero-valued fallback safely. Grouped empty and non-Bullet NoData behavior are unchanged;
  • prior Decimal non-finite producer handling, dynamic dateutil fold/gap/post-2037 parity, pandas/NumPy temporal projection, JavaScript number/category coercion, tooltip consistency, Pydantic-wire response sizing, result/source/metadata budgets, SQL/Jinja/DataFrame/tabular, cache, provenance, update/rebind, and cross-chart contracts remain covered.

Local validation used Python 3.11 and SQLAlchemy 2.0.52:

  • focused modified chart/query-result/generate/update/preview/data tests: 755 passed;
  • full MCP service suite: 4,330 passed in 5m04s;
  • full common/DataFrame/SQL/SQL Lab/semantic-layer suites: 2,215 passed, 1 xfailed;
  • frontend Bullet Node/Jest suites: 2 suites, 16 passed;
  • staged pre-commit including backend MyPy, Ruff format/check, custom Pylint, blacklist and standard hooks, Python compilation, and staged diff checks: passed.

Environment caveat: curl -f http://localhost:8088/health fails because no live Superset server is running in this worktree, so live/manual UI verification was unavailable. See the Working with LLMs setup guide.

Coordination/reconciliation: Agor session and board zones were inspected first; the completed independent reviewer was idle and no parallel reviewer/fixer was started. Upstream master was fetched through b457280b57ac79be13877fb6ab1132cad75d1118; its four commits after the already integrated 52a4cb6314aea332f8061a7b3babd814cdebc33b master parent have no changed-path overlap, so no additional merge was needed. Latest #43771/Sunburst was compared read-only at 28bbacacd0421f79913761c922b0428a9be52f23; overlap PRs #43737, #43679, and #43711 were also checked read-only. No branch was modified or cherry-picked, and no GitHub comment/review, Shortcut item, or unrelated Agor object was modified.

Exact-head code CI completed at 4131a07494af7046c0dd53d8d5ba23293796c4bf with 75 successful, 3 neutral, and 3 skipped check runs. Every required context passes, including frontend build, pre-commit, MySQL/SQLite/Postgres, Presto/Hive, Cypress/Playwright aggregators, and unit-tests-required; both Python and JavaScript CodeQL analyses pass. Python-Unit run 33711127353, job 100510674235, completed in 31m21s within the 40-minute gate: the main phase reported 14,568 passed, 4 skipped, 3 xfailed in 26m49s, SQL reported 1,487 passed, 1 xfailed at 100% coverage, semantic layers reported 415 passed at 100% coverage, and Codecov accepted the exact-head upload. GitHub reports MERGEABLE with merge-state BLOCKED because a fresh review is required.

A fresh independent round is requested, and no clean-review claim is made.

ROUND-26 TESTING AND COORDINATION

Round-26 starts from the requested exact head 4131a07494af7046c0dd53d8d5ba23293796c4bf and is delivered by implementation commit 6d0041f459aa319dad115d033fbf7b4b284ff338 (parent 4131a07494af7046c0dd53d8d5ba23293796c4bf). The branch was normal-pushed; no rebase or force-push was used.

Round-26 coverage and contracts:

  • Bullet's Chart Data wire projection now handles exact builtin timedelta, exact trusted pandas.Timedelta, and exact numpy.timedelta64. Builtin and pandas values reproduce format_timedelta spelling without comparison/string hooks; exact NumPy durations follow the real DataFrame producer boundary by promoting supported unambiguous units to pandas semantics, converting NaT to null, and failing closed for ambiguous units or overflow;
  • coverage compares directly with public json_int_dttm_ser output for negative, zero, day, microsecond, nanosecond, maximum/minimum, and coarse-unit extremes. Hostile builtin/pandas subclasses are rejected without hook dispatch. Real df_to_records(pd.DataFrame(...))ChartDataCommand results cover compile, saved cached get-data, form-data-key get-data, public FastMCP ASCII/Vega preview, CSV, and both openpyxl/xlsxwriter exports;
  • Vega measure bars now expose a collision-safe derived Range tooltip only where the frontend supplies a containing label. Selection matches the stable ascending-threshold measure <= threshold rule, blank-hit behavior, and > last-label result beyond the final threshold. Coverage includes exact boundaries, within/beyond ranges, negative and zero values, reordered thresholds, integer/float/Decimal measures, custom/default/blank labels, grouped/ungrouped and empty results, hostile source-key collisions, invalid presentation validation, and final-response amplification rejection;
  • prior timezone/dateutil, Decimal nonfinite, JavaScript numeric/temporal categories, zero-row/export headers, Pydantic-wire response preflight, source/metadata/result budgets, SQL/Jinja/DataFrame/tabular, cache, provenance, update/rebind, and cross-chart behavior remain covered.

Local validation used Python 3.11 and SQLAlchemy 2.0.52:

  • focused complete query-result/Bullet/get-data/get-preview/generate/update files: 799 passed;
  • full MCP service suite: 4,374 passed in 5m10s;
  • full common/DataFrame/SQL/SQL Lab/semantic-layer suites: 2,215 passed, 1 xfailed;
  • direct public duration serializer parity: 25,004 builtin/pandas cases plus 56 NumPy unit/extreme cases, zero mismatches;
  • frontend Bullet Node/Jest suites: 2 suites, 17 passed;
  • targeted core/ECharts declaration builds and staged pre-commit including backend MyPy, frontend type checking, Ruff format/check, Oxformat/Oxlint, custom Pylint, blacklist and standard hooks, Python compilation, and staged diff checks: passed.

Environment caveat: curl -f http://localhost:8088/health fails because no live Superset server is running in this worktree, so live/manual UI verification was unavailable. See the Working with LLMs setup guide.

Coordination/reconciliation: Agor session and board zones were inspected first; the completed Round-26 independent reviewer was idle and no parallel reviewer/fixer was started. Upstream master remains b457280b57ac79be13877fb6ab1132cad75d1118; its commits after the already integrated 52a4cb6314aea332f8061a7b3babd814cdebc33b parent have no Round-26 changed-path overlap, so no merge was needed. #43771/Sunburst was compared read-only at 28bbacacd0421f79913761c922b0428a9be52f23; #43737, #43679, and #43711 were also checked read-only. No related branch was modified or cherry-picked, and no GitHub comment/review, Shortcut item, or unrelated Agor object was modified.

Exact-head CI at 6d0041f459aa319dad115d033fbf7b4b284ff338 (GitHub merge ref 862912e7fe9ea831d309fde5937ad42326bdb0b9) is complete: all required checks passed; 75 checks succeeded, 3 were neutral docs-preview checks, and 3 were intentionally skipped. CodeQL and both JavaScript/Python analyses passed. Python Unit run 33716016893, job 100525293147, completed successfully in 26m05s; its current-unit suite reported 14,612 passed, 4 skipped, 3 xfailed in 21m54s, and the two 100%-coverage suites reported 1,487 passed, 1 xfailed and 415 passed, both at required 100.00% coverage. Codecov project and patch checks passed. GitHub reports the PR MERGEABLE; its BLOCKED state is the expected REVIEW_REQUIRED policy state.

A fresh independent round is requested, and no clean-review claim is made.

REVIEW-COMMENT RESOLUTION PASS (ROUND 27 FOLLOW-UP)

This finite pass starts from the verified clean local/tracking/fork/live PR head 6d0041f459aa319dad115d033fbf7b4b284ff338 and is delivered by 703bb6cd8c9a96c19e0e76c0b1e3ec968ec26489 (parent 6d0041f459aa319dad115d033fbf7b4b284ff338). The branch was normal-pushed; no rebase or force-push was used.

Confirmed reviewer findings addressed:

  • every Bullet Vega range rectangle and optional range label uses the same collision-safe category y encoding as the measure bar. The dead saved-preview Bullet builder was removed so all public saved/unsaved/cache paths use the single strict shared renderer; grouped public FastMCP and direct saved-preview tests assert per-row range encodings;
  • conflicting semantic dimensions and native groupby aliases are rejected deterministically. Exact spelling remains required at schema-only adaptation because quoted physical identifiers may differ by case; dataset-aware canonicalization remains exact-first/casefold-unambiguous;
  • the shared update merge retains the saved temporal subject while replacing its comparator for range-only updates and replaces/removes provenance-owned bindings for explicit subject changes. The unused compatibility wrapper and its test-only call sites were removed so tests exercise the production merge path;
  • config-less dataset-only Bullet rebind validates a filter-free typed projection while preserving the byte-faithful native SQL/HAVING sequence for target-dataset Tier-2 compilation and persistence. Direct and public FastMCP regressions cover the target datasource and saved filter sequence;
  • saved SQL metrics without an explicit label resolve their sqlExpression result field, HAVING saved-metric fallback survives ambiguous physical-column casefold matches, supported native frontend operator names adapt to typed operators, malformed nested error objects no longer hide a valid top-level query message, XY log scale uses the frontend logAxis key, and final oversize failures after chart creation retain the persisted chart ID with an explicit do-not-retry instruction;
  • the two reported NumPy-duration test crashes were reproduced and found stale: df_to_records promotes exact np.timedelta64 values to exact pandas.Timedelta and converts NumPy NaT to None. Producer assertions were added to the compile, get-data, and preview tests before their direct json_int_dttm_ser parity comparisons;
  • secondary test/static feedback was addressed by consuming the cached Deck result, replacing optimization-sensitive post-validation assertions in dataset/semantic tools with structured malformed-result fallbacks, removing loop-local JSON imports, tightening misleading names/docs, and making SQL Lab/DataFrame assertions independent and literal.

Local validation used Python 3.11 and SQLAlchemy 2.0.52:

  • complete focused affected backend suite: 1,451 passed in 1m10s;
  • full tests/unit_tests/mcp_service: 4,386 passed in 5m45s;
  • full common plus DataFrame and SQL Lab suites: 313 passed;
  • frontend Bullet Jest: 1 suite, 14 passed;
  • staged repository pre-commit passed all applicable hooks, including MyPy, Ruff format/check, and custom Pylint; Python compilation and staged/unstaged diff checks passed.

Coordination/reconciliation: Agor sessions and board zones were inspected before edits and showed this as the only active writer for sc-119157-mcp-bullet. Upstream master was fetched through d2d965e0; its 12 commits after the previously inspected b457280b have no changed-path overlap with this PR, so no merge was needed. #43771/Sunburst was compared read-only at 05c9abcaa8a91c636b309f85bd0db29f4a7a0188; no Sunburst, contributor, or community branch was modified or cherry-picked. All ten live unresolved/outdated threads and all six review summaries/nine issue comments were fetched directly from GitHub and triaged individually. Exact-head CI completed at 703bb6cd8c9a96c19e0e76c0b1e3ec968ec26489: 75 check runs succeeded, the docs-preview status succeeded, 3 checks were neutral, and 3 were intentionally skipped. Every required context passed, including pre-commit, frontend build, MySQL/SQLite/Postgres, Presto/Hive, Cypress/Playwright aggregators, and unit-tests-required; Python and JavaScript CodeQL passed. Python-Unit run 33786401386, job 100752237100, completed in 21m10s within the 40-minute gate: the main suite reported 14,649 passed, 4 skipped, 3 xfailed in 16m51s, SQL reported 1,487 passed, 1 xfailed at 100% coverage, semantic layers reported 415 passed at 100% coverage, and Codecov accepted the upload. GitHub reports MERGEABLE; BLOCKED reflects REVIEW_REQUIRED. All ten review threads are individually resolved after evidence replies. This is the requested finite comment-resolution pass; it does not initiate another review loop.

ADDITIONAL INFORMATION

  • Has associated issue: Shortcut SC-119157
  • 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

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

Copy link
Copy Markdown
Contributor

The issue described is a logic error in the Bullet chart configuration mapping where range rectangles lack a category y encoding. This causes thresholds to render as chart-wide bands instead of being scoped to individual bullet rows.

To resolve this, you need to ensure that the range_rectangles (or equivalent threshold configuration) in the form_data mapping includes the necessary y encoding or grouping information that links them to the specific bullet row. Since the file superset/mcp_service/chart/tool/get_chart_preview.py is not present in the provided PR context, I cannot implement the fix directly. Please verify the mapping logic in that file to ensure the y encoding is correctly passed to the frontend configuration.

Comment thread superset/mcp_service/chart/preview_utils.py Outdated
Comment thread superset/mcp_service/chart/preview_utils.py
Comment thread superset/mcp_service/chart/schemas.py
Comment thread superset/mcp_service/chart/chart_utils.py
Comment thread superset/mcp_service/chart/plugins/bullet.py Outdated
@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 74.56186% with 987 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.40%. Comparing base (52a4cb6) to head (703bb6c).
⚠️ Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
superset/mcp_service/chart/query_result.py 74.61% 184 Missing and 109 partials ⚠️
superset/mcp_service/chart/chart_helpers.py 80.35% 66 Missing and 66 partials ⚠️
superset/mcp_service/chart/preview_utils.py 72.10% 81 Missing and 42 partials ⚠️
superset/mcp_service/chart/compile.py 61.94% 75 Missing and 46 partials ⚠️
superset/mcp_service/chart/schemas.py 74.11% 41 Missing and 32 partials ⚠️
superset/mcp_service/chart/chart_utils.py 77.92% 42 Missing and 26 partials ⚠️
superset/mcp_service/chart/plugins/bullet.py 72.00% 18 Missing and 10 partials ⚠️
superset/mcp_service/chart/tool/get_chart_data.py 74.48% 15 Missing and 10 partials ⚠️
superset/mcp_service/utils/response_utils.py 82.63% 14 Missing and 11 partials ⚠️
superset/mcp_service/chart/tool/get_chart_sql.py 68.75% 19 Missing and 1 partial ⚠️
... and 11 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #43770      +/-   ##
==========================================
+ Coverage   79.36%   79.40%   +0.04%     
==========================================
  Files        2893     2896       +3     
  Lines      167614   171258    +3644     
  Branches    38816    39793     +977     
==========================================
+ Hits       133026   135989    +2963     
- Misses      32088    32432     +344     
- Partials     2500     2837     +337     
Flag Coverage Δ
hive 36.69% <10.48%> (-1.11%) ⬇️
javascript 74.88% <ø> (+0.01%) ⬆️
mysql 55.61% <10.67%> (-1.91%) ⬇️
postgres 55.65% <10.67%> (-1.91%) ⬇️
presto 38.50% <10.51%> (-1.19%) ⬇️
python 83.69% <74.56%> (-0.13%) ⬇️
sqlite 55.35% <10.61%> (-1.90%) ⬇️
unit 74.57% <74.56%> (+0.26%) ⬆️

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.

@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 #658aec

Actionable Suggestions - 1
  • superset/mcp_service/chart/chart_utils.py - 1
Additional Suggestions - 7
  • superset/mcp_service/chart/preview_utils.py - 2
    • SQL metric label gap · Line 328-341
      `_form_metric_label` reimplements `superset.utils.core.get_metric_name` but drops its SQL branch (`expressionType == "SQL"` returns `sqlExpression`). For a SQL adhoc metric without a `label`, this returns `None`, so `_bullet_result_roles` falls back to the first numeric column — which may be a dimension or a different metric, producing a wrong Bullet preview. Consider reusing `get_metric_name` or adding the SQL branch.
    • Empty ranges band divergence · Line 620-620
      When `ranges` is empty, `_bullet_numeric_tokens` returns `[]` and no range band is drawn. The native Bullet plugin (`transformProps.ts`) falls back to `[0, measure * 1.1]` when ranges are empty, so the preview diverges from the real chart. Consider deriving a default band from the metric values to stay faithful.
  • superset/mcp_service/chart/tool/get_chart_preview.py - 1
    • Duplicated bullet spec logic · Line 704-822
      `_bullet_chart_spec` reimplements metric/dimension resolution (lines 729-745) and the full layer-building already provided by `_bullet_result_roles`/`_canonical_result_field` and `_generate_bullet_vega_lite_preview` in preview_utils.py. The metric fallback also diverges: existing code picks the first quantitative field from the data row, this picks the last from `reversed(fields)`. Reuse the shared helpers to avoid divergent bullet rendering.
  • superset/mcp_service/chart/chart_utils.py - 1
    • Unhandled KeyError · Line 1122-1122
      `dimension_targets[order.column.casefold()]` raises `KeyError` when an `order_by` column matches neither the metric nor any dimension. The schema's `_adapt_native_order_by` accepts arbitrary bare column-name strings, so this is reachable from user input and crashes the update/preview tool. Use `.get(...)` and surface a validation error.
  • superset/mcp_service/chart/schemas.py - 2
    • Incomplete operator mapping · Line 2231-2232
      `_adapt_native_filters` maps `operator` using `{"==": "=", "IS_NOT_NULL": "IS NOT NULL"}`, but saved `adhoc_filters` store the `Operators` enum value (e.g. `'EQUALS'`, `'LESS_THAN'`, `'GREATER_THAN'`, `'IN'`, `'IS_NULL'`). Only `IS_NOT_NULL` maps correctly; `'=='` never occurs, and all other operators pass through unmapped and fail `FilterConfig.op` Literal validation, so saved bullet charts with filters won't load. Map the full enum set.
    • order_by validation/mapper divergence · Line 2371-2379
      `valid_order_targets` adds a dimension's `label` even when the dimension has no `name`, but `map_bullet_config`'s `dimension_targets` dict only maps label→name when `dimension.name` is present (`if candidate and dimension.name`). A dimension `{"label": "Region"}` passes this validation yet crashes the mapper with `KeyError` on `dimension_targets[order.column.casefold()]`. Gate the label on `dimension.name` to match.
  • tests/unit_tests/mcp_service/chart/test_bullet_chart.py - 1
    • Vacuous compile test · Line 347-348
      Within the provided diff, `test_bullet_compile_path_uses_groupby_metric_orderby_and_empty_results` ends at line 348 with `query` assigned but never asserted; `result` (line 347) is also unused. If the function is complete, the test is vacuous and would pass even if compiled queries were wrong. Add assertions on `query`/`result`, or remove the dead assignments. (If assertions follow line 348 and the diff was truncated, please disregard.)
Review Details
  • Files reviewed - 14 · Commit Range: 6411173..6411173
    • superset/mcp_service/chart/chart_helpers.py
    • superset/mcp_service/chart/chart_utils.py
    • superset/mcp_service/chart/plugins/__init__.py
    • superset/mcp_service/chart/plugins/bullet.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_preview.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
    • tests/unit_tests/mcp_service/chart/test_bullet_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/chart_utils.py Outdated
@netlify

netlify Bot commented Sep 1, 2026

Copy link
Copy Markdown

Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit c3c07fd
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6a988d6682eb1b00084a1bca
😎 Deploy Preview https://deploy-preview-43770--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 #d1f367

Actionable Suggestions - 0
Additional Suggestions - 6
  • superset/mcp_service/chart/query_result.py - 1
    • Error misclassified as malformed · Line 125-135
      A non-empty mapping with no recognized message field (e.g. `{"code": 500}`) now returns `malformed`, so `_failure_for_query_payload` short-circuits with `MalformedQueryResult` before checking other fields like a top-level `message`. Previously `_query_error_text` returned `None` and the caller fell through to the real message. Consider `continue` here so legitimate error objects with unrecognized keys still surface the actual message.
  • superset/mcp_service/chart/compile.py - 1
    • HAVING metric fallback unreachable · Line 233-234
      When `resolve_dataset_column` raises `ValueError` (ambiguous casefold match among physical columns), this branch returns `False` before the HAVING metric check at L237-240 runs. A valid HAVING saved-metric reference whose name casefold-matches 2+ physical columns is then rejected, whereas the previous `_column_exists` path accepted it. Fall through for HAVING clauses.
  • superset/mcp_service/chart/schemas.py - 1
    • Case-sensitive alias conflict check · Line 2361-2361
      The conflict check compares canonical names case-sensitively (`dimensions != groupby`), but `validate_roles_and_outputs` treats physical column identity case-insensitively via `name.casefold()` (line 2466), and SQL column identifiers are case-insensitive in most backends. So `dimensions=["Region"]` with `groupby=["region"]` (same column) is falsely rejected. Canonicalize with `name.casefold()` in `_canonical_dimension_alias` for consistency.
  • superset/mcp_service/chart/validation/dataset_validator.py - 1
    • Duplicated column resolution logic · Line 56-88
      `resolve_dataset_column` re-implements the exact-then-casefold matching and ambiguity `ValueError` already in `get_canonical_column_name` (lines 460-490) in this same file. Duplicating the logic risks divergence (e.g. one path changes matching rules, the other doesn't). Consider delegating to `get_canonical_column_name` and mapping the resolved name back to the column dict.
  • superset/mcp_service/chart/tool/update_chart_preview.py - 1
    • Dead code in production · Line 129-141
      `_preserve_previous_adhoc_filters` is never called in production — the update path at line 264 calls `merge_update_form_data` directly, and grep shows only the `def` plus test references. The new `previous_has_new_subject_filter`/`temporal_binding_changed` logic therefore has no effect on real previews, while the tests give false confidence it is active. Wire the wrapper into the production path or remove it.
  • superset/mcp_service/chart/plugins/bullet.py - 1
    • Misleading error message · Line 174-174
      `resolve_dataset_column` raises `ValueError` for two distinct causes: duplicate exact column names (dataset_validator.py:74) and casefold ambiguity (line 86). The hardcoded message "is ambiguous by case" is misleading for the duplicate-exact case. Consider wording that covers both, since `details=str(ex)` already carries the real reason.
Review Details
  • Files reviewed - 23 · Commit Range: 6411173..da1a1f1
    • 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/bullet.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/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
    • superset/mcp_service/chart/validation/pipeline.py
    • tests/unit_tests/mcp_service/chart/test_bullet_chart.py
    • tests/unit_tests/mcp_service/chart/test_chart_helpers.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_column_name_normalization.py
    • tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.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_generate_chart.py
    • tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.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 #76a9c8

Actionable Suggestions - 0
Additional Suggestions - 6
  • superset/mcp_service/chart/chart_utils.py - 2
    • Wrong native control key · Line 1646-1646
      `y_axis_scale` is not a control name anywhere in the frontend plugins (0 matches), so writing it to `new_form_data` is ignored and the y-axis log scale won't apply for "xy" charts. The echarts timeseries control is `logAxis` — the same diff already uses `logAxis`/`logAxisSecondary` for `mixed_timeseries`. Use `logAxis` here for consistency.
    • Granularity clear unreliable · Line 1843-1844
      The current logic sets `granularity_sqla` whenever `is_column_truly_temporal` returns true, but that function defaults to true if the dataset or column isn't found. To ensure non-temporal columns clear `granularity_sqla`, update the code to only set it when the dataset lookup succeeds and `is_column_truly_temporal` explicitly confirms temporality.
  • tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py - 1
    • Decimal serialization mismatch · Line 2634-2634
      This asserts special Decimals serialize to strings (`"sNaN"`, `"NaN"`, `"Infinity"`), but the repo's Decimal encoder in `superset/utils/json.py` (line 93-94) returns `float(obj)`, and `float(Decimal("sNaN"))` raises `ValueError: cannot convert float NaN to integer`. If the tool-result path uses that encoder, this assertion fails. Verify the serializer actually used and align the expectation.
  • superset/mcp_service/chart/preview_utils.py - 1
    • Unsupported datetime dimension · Line 609-646
      The new dispatch raises BulletOutputError for any dimension value that isn't None/str/bool/int/float/Decimal. But the upstream validator `_unsafe_row_value` (query_result.py `_SAFE_ROW_SCALAR_TYPES`) explicitly whitelists `date`, `datetime`, `time`, `timedelta`, and `UUID` as safe row scalars, so a bullet chart with a date/datetime dimension passes validation then hits the `else` branch and fails with "unsupported value type". The prior code passed raw values through (`copied[dimension] = row[row_dimension]`), so this is a regression. Consider handling these types explicitly.
  • superset/mcp_service/chart/query_result.py - 1
    • Wrong metadata key · Line 426-426
      `cache_dttm` is not a key in the chart-data payload: `query_context_processor.py` builds it as `cached_dttm` (line 237). So `dict.get(query, "cache_dttm")` always returns `None` and this validation never fires. Use `cached_dttm` to match the payload key, otherwise the intended cache-timestamp guard is dead.
  • tests/unit_tests/mcp_service/chart/test_dashboard_time_binding.py - 1
    • Misleading test name · Line 489-489
      The test name `test_non_temporal_waterfall_granularity_falls_back_to_dataset_time_column` now contradicts its assertion: `granularity_sqla` is asserted `is None`, not a fallback to `order_date`. The name was left stale when the assertion changed from `== "region"`. Rename it so future maintainers don't misread the intended behavior.
Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

  • superset/mcp_service/chart/tool/get_chart_preview.py - 1
Review Details
  • Files reviewed - 19 · Commit Range: da1a1f1..105c0f9
    • superset/mcp_service/chart/chart_utils.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/tool/get_chart_preview.py
    • tests/unit_tests/mcp_service/chart/test_bullet_chart.py
    • tests/unit_tests/mcp_service/chart/test_query_result.py
    • tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py
    • superset/mcp_service/chart/compile.py
    • superset/mcp_service/chart/tool/get_chart_data.py
    • tests/unit_tests/mcp_service/chart/test_compile.py
    • tests/unit_tests/mcp_service/chart/test_preview_utils.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_dashboard_time_binding.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
    • superset/mcp_service/chart/plugins/waterfall.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 #0e799f

Actionable Suggestions - 0
Additional Suggestions - 2
  • superset/mcp_service/chart/tool/get_chart_data.py - 1
    • Duplicated temporal identity logic · Line 514-561
      The `datetime` (514-535) and `datetime_time` (541-561) branches duplicate the same offset/naive/opaque resolution via `_trusted_utc_offset` and `id(value.tzinfo)`. If one branch is later updated (e.g. a new trusted tzinfo type), the other can silently diverge. Consider extracting a shared helper for the aware/naive/opaque decision.
  • tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py - 1
    • Duplicated test scaffolding · Line 2654-2710
      New test `test_saved_generic_get_data_rejects_misaligned_coltypes` duplicates ~40 lines of scaffolding (imports, `chart` construction, `_Command`, the four `patch` calls, `client.call_tool`) from `test_saved_get_data_rejects_hostile_rows_and_scalars`. Consider a shared helper so both malformed-result tests stay in sync when the tool's error handling changes.
Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

  • superset/mcp_service/chart/tool/get_chart_data.py - 1
Review Details
  • Files reviewed - 7 · Commit Range: 105c0f9..ddd3296
    • superset/mcp_service/chart/chart_helpers.py
    • superset/mcp_service/chart/query_result.py
    • superset/mcp_service/chart/tool/get_chart_data.py
    • tests/unit_tests/mcp_service/chart/test_bullet_chart.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/tool/test_get_chart_data.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 #400d68

Actionable Suggestions - 0
Additional Suggestions - 6
  • superset/mcp_service/chart/tool/update_chart.py - 1
    • Rebind now fail-closed · Line 719-719
      This changes dataset-only rebinds from 'verify target dataset exists' to a full live compile against the target (`_compile_chart` via `validate_and_compile`). A rebind whose existing form_data references columns/metrics missing in the new dataset will now be rejected with no escape hatch, since `run_compile_check=False` was removed here and at line 755. Confirm this is intended and consider an opt-out for legitimate schema-differing rebinds.
  • superset/mcp_service/utils/cache_utils.py - 1
    • Duplicated security-relevant constants · Line 34-35
      `_TRUSTED_TZINFO_TYPES` and the 4096 length bound are redefined here but already exist in `chart/query_result.py` (`_TRUSTED_TZINFO_TYPES`, `_MAX_CACHE_STRING_LENGTH`). Since this allowlist gates which tzinfo values are accepted, a future change in one file would silently diverge from the other. Consider importing the shared constants.
  • superset/mcp_service/chart/compile.py - 1
    • Error message context lost · Line 551-551
      The refactor into `metric_error()` changes the saved-metric ambiguity error from `"saved metric"` to the generic `"query {query_index} metric"` role. The old message explicitly guided users toward saved metric names; the new one loses that context. Consider retaining the saved-metric qualifier in the role string.
  • superset/mcp_service/chart/tool/update_chart_preview.py - 1
    • Doc/design drift on fast path · Line 294-294
      This flips `update_chart_preview` to run Tier 2, which executes a live DB query via `_compile_chart` (`ChartDataCommand.run()`, row_limit=2) on every call — including when `generate_preview=False`. The `validate_and_compile` docstring (compile.py:750-752) and CLAUDE.md section 11 both still document this tool as opting out of Tier 2 for SLA reasons. If intentional, update both docs; otherwise keep `False`.
  • tests/unit_tests/mcp_service/chart/test_big_number_chart.py - 1
    • Cached branch omits merge chain · Line 102-104
      In the 'cached' branch of the test, after `merge_update_form_data(existing, form_data, config)`, also call `merge_table_column_config(existing, form_data)`, `merge_interactive_pivot_ui_config(existing, form_data)`, and `merge_same_viz_form_data(existing, form_data, config)` to match the merge sequence in update_chart_preview.py.
  • tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py - 1
    • Test coverage gap · Line 638-638
      The test discards the return of `_query_from_form_data`, so the `ChartDataCommand.run()` stub's data never flows through `query_result_data`, column inference, or `get_cache_status_from_result`. A regression in those steps wouldn't be caught. Consider asserting on the returned `ChartData` (e.g. `row_count`, `columns`) in addition to the captured query dicts.
Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

  • tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py - 1
Review Details
  • Files reviewed - 18 · Commit Range: ddd3296..63558bf
    • superset/mcp_service/chart/chart_helpers.py
    • superset/mcp_service/chart/compile.py
    • superset/mcp_service/chart/query_result.py
    • superset/mcp_service/chart/schemas.py
    • superset/mcp_service/chart/tool/get_chart_data.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_chart_helpers.py
    • tests/unit_tests/mcp_service/chart/test_compile.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/mcp_service/chart/tool/test_update_chart.py
    • tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py
    • superset/mcp_service/utils/cache_utils.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
    • tests/unit_tests/mcp_service/utils/test_cache_utils.py
    • tests/unit_tests/mcp_service/chart/test_big_number_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/chart_utils.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 #d53793

Actionable Suggestions - 2
  • tests/unit_tests/mcp_service/chart/test_bullet_chart.py - 1
  • tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py - 1
Additional Suggestions - 7
  • superset/mcp_service/chart/tool/generate_chart.py - 1
    • Commit-before-gate inconsistency · Line 879-879
      The chart is committed at `command.run()` (line 486-501) before this gate runs. If the response exceeds `MAX_QUERY_RESULT_VALUE_BYTES` (16 MiB), `_bounded_generate_response` returns a `CHART_RESPONSE_TOO_LARGE` failure even though the chart was already persisted. The client sees a failed generation and may retry, creating duplicate charts. Consider gating before commit or surfacing the created `chart_id` in the error.
  • tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py - 1
    • np.timedelta64 reference raises · Line 4279-4279
      `expected` serializes each `row["Duration"]` with `json.dumps(..., default=json.json_int_dttm_ser)`. `base_json_conv` handles `datetime.timedelta` (line 97) but not `np.timedelta64`, so the `np.timedelta64(123456789, 'ns')` source value raises `TypeError` here, erroring the test before the tool is even called. Use the producer's `_chart_data_duration_text` normalizer for the reference, or drop that source value.
  • superset/mcp_service/chart/response_preflight.py - 1
    • Duplicate error payload · Line 47-61
      The error payloads in `preflight_generate_chart_response` (lines 47-61) and `preflight_update_preview_response` (lines 69-83) are near-identical: same `error_type`, `suggestions`, `error_code: CHART_RESPONSE_TOO_LARGE`, and `details: failure.error`, differing only in message text and version keys. Consider extracting a shared `_wire_size_error(failure, message)` helper so the error contract stays in one place and cannot diverge.
  • superset/mcp_service/chart/query_result.py - 1
    • Pandas negative duration spelling · Line 490-512
      `_chart_data_pandas_timedelta_text` claims to "Reproduce pandas Timedelta formatting", but for negative durations it emits a normalized spelling (e.g. `-0 days 00:00:01` for `pd.Timedelta(seconds=-1)`) that diverges from pandas' canonical `str()` (`-1 days +23:59:59`). The bullet test computes expected via this same function, so the divergence is unpinned. Align the docstring with the actual normalized output.
  • superset/mcp_service/chart/tool/get_chart_preview.py - 1
    • Dead always-false flag · Line 372-375
      Remove the `temporal_json_numbers` argument from `query_result_data(result)` in `TablePreviewStrategy.generate`, since `self.chart.viz_type == "bullet"` is already handled above, making the flag always False.
  • tests/unit_tests/sql_lab_test.py - 1
    • Tautological test assertion · Line 94-96
      This assertion is tautological: `_deserialize_results_payload` (views/utils.py:673) calls `json.loads` on the same `superset.utils.json` module as `superset_json.loads`, so both sides always produce identical output and the check can never fail. It verifies nothing about the round-trip. Assert the deserialized `data` rows directly (finite Decimal becomes float 0.1).
  • tests/unit_tests/dataframe_test.py - 1
    • Misleading precision assertion · Line 335-335
      `superset_json.dumps` routes Decimals through `base_json_conv` (`superset/utils/json.py`), which returns `float(obj)`, so `Decimal("0.10000000000000000001")` serializes to `0.1`. This assertion passes only because Python's float literal `0.10000000000000000001 == 0.1` is `True`. Assert `0.1` so the test verifies actual behavior and would catch a precision regression.
Review Details
  • Files reviewed - 33 · Commit Range: 9ba1f0b..6d0041f
    • superset/common/query_context_processor.py
    • superset/dataframe.py
    • superset/mcp_service/chart/query_result.py
    • superset/mcp_service/chart/tool/get_chart_data.py
    • tests/unit_tests/common/test_query_context_processor.py
    • tests/unit_tests/dataframe_test.py
    • tests/unit_tests/mcp_service/chart/query_result_test_utils.py
    • tests/unit_tests/mcp_service/chart/test_compile.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/mcp_service/chart/tool/test_get_chart_preview.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/utils/response_utils.py
    • tests/unit_tests/mcp_service/utils/test_response_utils.py
    • superset/mcp_service/semantic_layer/tool/get_table.py
    • superset/mcp_service/chart/tool/get_chart_sql.py
    • tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py
    • superset-frontend/plugins/plugin-chart-echarts/test/Bullet/transformProps.test.ts
    • superset/mcp_service/chart/preview_utils.py
    • superset/mcp_service/chart/response_preflight.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
    • tests/unit_tests/mcp_service/chart/test_bullet_chart.py
    • tests/unit_tests/mcp_service/chart/test_response_preflight.py
    • tests/unit_tests/mcp_service/chart/tool/test_generate_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
    • superset/mcp_service/chart/compile.py
    • tests/unit_tests/sql/execution/test_celery_task.py
    • tests/unit_tests/sql_lab_test.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
    • Eslint (Linter) - ✔︎ 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 tests/unit_tests/mcp_service/chart/test_bullet_chart.py
Comment thread tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py
@bito-code-review

bito-code-review Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #550f70

Actionable Suggestions - 0
Additional Suggestions - 5
  • superset/mcp_service/chart/chart_utils.py - 1
    • String written to boolean control · Line 1646-1646
      The diff renames the scale control to `logAxis`, which is a boolean-typed frontend control (`logAxis: boolean` in `Timeseries/types.ts`). But the write path at line 1678 still assigns `axis.scale` (string "log"/"linear"). An explicit `scale="linear"` would write the truthy string "linear" and wrongly enable the log axis. The mapping path `_apply_axis_to_form_data` (line 2063) already converts via `== "log"`; apply the same here.
  • superset/mcp_service/chart/response_preflight.py - 1
    • Duplicate error payload · Line 65-80
      The `persisted_chart_id` branch rebuilds the same 5-key error payload (`error_type`, `details`, `suggestions`, `error_code`) inline instead of reusing the `_wire_size_error` helper introduced in this same diff. This duplicates the shared `CHART_RESPONSE_TOO_LARGE` shape and risks divergence if the helper evolves. Consider parameterizing `_wire_size_error` with optional `details`/`suggestions` and calling it from both branches.
  • superset/mcp_service/dataset/tool/query_dataset.py - 1
    • Duplicated validation block · Line 317-341
      This validation block duplicates the identical logic in `semantic_layer/tool/get_table.py` (only the error class differs: `DatasetError` vs `SemanticLayerError`). If one is updated (e.g. a new malformed case or changed message), the other silently diverges. Consider extracting a shared helper from `query_result_data` and reusing it in both tools.
  • superset/mcp_service/chart/schemas.py - 1
    • Duplicated operator mapping · Line 2348-2359
      This inline map duplicates the operator-translation concept already in `map_filter_operator` (chart_utils.py:2162), which maps the inverse direction (`=`→`==`). Two maps now translate between the same two vocabularies, so updating one without the other will silently diverge. Consider a single shared constant used by both.
  • superset/mcp_service/chart/tool/update_chart_preview.py - 1
    • Test coverage gap · Line 114-114
      Removing the dead `_preserve_previous_adhoc_filters` is correct, but its test `test_rejects_ambiguous_temporal_filters_on_generated_subject` was the only coverage for the ambiguous-binding ValueError. That path is now reachable via the live `merge_update_form_data` call (line 236) → `_temporal_binding_filter` (chart_utils.py:1312). No merge test covers it; consider porting the scenario to the `merge_update_form_data` suite.
Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

  • superset/mcp_service/semantic_layer/tool/get_table.py - 1
Review Details
  • Files reviewed - 27 · Commit Range: 6d0041f..703bb6c
    • 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/bullet.py
    • superset/mcp_service/chart/preview_utils.py
    • superset/mcp_service/chart/query_result.py
    • superset/mcp_service/chart/response_preflight.py
    • superset/mcp_service/chart/schemas.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_preview.py
    • superset/mcp_service/dataset/tool/query_dataset.py
    • superset/mcp_service/semantic_layer/tool/get_table.py
    • tests/unit_tests/dataframe_test.py
    • tests/unit_tests/mcp_service/chart/test_big_number_chart.py
    • tests/unit_tests/mcp_service/chart/test_bullet_chart.py
    • tests/unit_tests/mcp_service/chart/test_chart_helpers.py
    • tests/unit_tests/mcp_service/chart/test_chart_utils.py
    • tests/unit_tests/mcp_service/chart/test_compile.py
    • tests/unit_tests/mcp_service/chart/test_dashboard_time_binding.py
    • tests/unit_tests/mcp_service/chart/test_query_result.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_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
    • tests/unit_tests/sql_lab_test.py
  • Files skipped - 1
    • superset/mcp_service/CLAUDE.md - 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

("marker", marker_labels, markers),
("marker line", marker_line_labels, marker_lines),
):
if labels and len(labels) != len(values):

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 a saved Bullet chart when its label list is shorter than its ranges or markers, although Bullet/transformProps.ts intentionally renders missing labels as blank or as the formatted value. That makes MCP previews fail for charts that still render in Explore. Could the preview use the same per-index fallback instead of requiring one label per value?

f"dimensions[{index}] must be a physical dimension, not a metric"
)
name = dimension.name or ""
if name.casefold() in seen_names:

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 treats Region and region as duplicate Bullet dimensions before dataset-aware resolution can preserve their exact physical names. Databases with quoted case-distinct columns can return both fields, and the frontend accesses each exact result key, so those charts are rejected even though they can be queried and rendered. Could role validation keep exact names distinct and reserve case-folding for ambiguous lookup only?

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 plugins size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants