aminghadersohi commented on PR #44148:
URL: https://github.com/apache/superset/pull/44148#issuecomment-5901579011
@rebenitez1802 thanks for the thorough review. Merged upstream `master` in
5edf6b993b (the only conflict was `UPDATING.md`; both entries are kept). Each
finding:
**1. 🟡 Gantt compile errors escape as `QueryObjectValidationError`**: fixed
in 832e7262bd. `_compile_chart` now catches `QueryObjectValidationError` next
to `CommandException`/`ValueError`/`KeyError`, so malformed Gantt form data
returns a structured `CHART_COMPILE_FAILED` again. The Gantt plugin keeps
raising `QueryObjectValidationError` because `get_chart_data` maps it to
`ValidationError`. The saved preview, SQL and data paths already catch
`SupersetException`. Regression:
`test_compile_chart_returns_structured_error_for_malformed_gantt_form_data` (51
`tooltip_columns`). It fails before the fix and passes after.
**2. 🟡 Cross-filter/drill emits the ISO id for blank-metric regions**: fixed
in 6bc655e038. `transformProps` builds an `ISO → source value` map from every
normalized row, including rows it drops for a non-finite metric, and passes it
as `sourceValues`. `CountryMap`'s `sourceValue()` reads that map instead of
scanning the rendered `data`. New tests: `source values cover regions whose
metric is blank` checks the map. A real-GeoJSON render test clicks and
right-clicks a NULL-metric California and asserts both the cross-filter `val:
['CA']` and the drill `val: 'CA'`.
**3. 🟢 `world_map` rejects a bubble-less choropleth over an unused
`secondary_metric`**: fixed in 624c53f292. `result_metrics` and
`size_metric_labels` only include the secondary metric when `show_bubbles` is
set. The typed frontend transform (`transformData`, `showBubbles`) now applies
the same rule, so the render layer doesn't throw where the backend accepts.
Tests cover `-1`/NULL/NaN/non-numeric secondary values, which are accepted with
bubbles off and rejected with bubbles on, in both pytest and Jest.
**4. 🟢 Empty non-Gantt Vega-Lite preview no longer returns `NoDataError`;
`allows_empty_result` unused**: fixed in 832e7262bd. The saved Vega-Lite path
reads `plugin.allows_empty_result` and returns `NoDataError` for an empty
result before running any plugin preview. Only Gantt skips that check.
`test_saved_vega_preview_empty_result_honors_allows_empty_result` covers
`bubble_v2`, `gauge_chart`, `treemap_v2` → `NoDataError` and `gantt_chart` →
empty `VegaLitePreview`. `bubble_v2` and `gauge_chart` failed before the fix.
**5. 🟢 `deck_scatter` render silently drops invalid rows**: fixed by
documenting the behavior and making the skips visible, in af5a06246a. Rendering
keeps skipping the rows instead of throwing, because @rusackas explicitly asked
for sparse/non-numeric rows to render blank rather than fail the chart. The MCP
data/export contract still rejects these rows. The filter is now a named,
exported helper (`filterDrawableGeographicPoints`) that logs `Skipped N of M
geographic points …` through `logging.warn`. The MCP guide documents the
render-layer skip and the warning. Jest covers the warning and the no-warning
path.
**6. 🟢 Size-metric nonnegative guard untested**: fixed in 624c53f292.
`test_negative_size_metric_is_rejected` is parametrized over `world_map`
bubbles and `deck_scatter` radius, with `-1`, `-0.5` and `Decimal("-0.001")`,
and runs through `normalize_chart_query_result` asserting
`InvalidGeographicResult`. Companion tests cover zero being accepted and a
negative *color* metric being accepted with bubbles on.
**7. 🟢 `GB-WRL` mislabeled "Halton"**: fixed in f83e84409c. The bundled
`uk.geojson`, `superset/utils/geographic_regions.py` and the frontend
`regions.ts` all change together, so the geometry parity tests still pass.
"Halton" → `GB-HAL` and "Wirral" → `GB-WRL` in both the backend and the
frontend. A new test asserts that no bundled region name in any boundary set
shares a folded key, so a second duplicate like this would fail CI.
**8. 🟢 `select_country` casing asymmetry**: fixed in 6bc655e038.
`transformProps` lowercases `select_country` once and uses that value for both
`normalizeRegions` and the render prop. Jest cases `USA`/`Usa`/`usa` all
resolve.
**9. 🟢 Internal tracker links in the PR body**: done. The tracker link is
gone from the PR body and the title has none. None of the branch's commit
messages and not the branch name (`mcp-geographic-chart-types`) contain tracker
IDs either. Existing pushed commits couldn't have been rewritten anyway, since
this PR takes new commits only and is never force-pushed. None needed it.
Local validation at af5a06246a: MCP unit tests 6,543 passed. Three
`test_mcp_caching` catalog tests fail identically on untouched `master` because
of the locally installed FastMCP 3.4.2 (pinned 3.4.7), so they're unrelated to
this branch. Country-map, world-map and deck.gl Scatter Jest suites: 15 suites
/ 169 tests pass. Pre-commit on the changed files passes (MyPy, Ruff, Pylint,
oxfmt, oxlint, custom rules, stylelint, frontend type check).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
