aminghadersohi commented on PR #44746:
URL: https://github.com/apache/superset/pull/44746#issuecomment-5889306433
@rebenitez1802 thanks for the thorough review. Every point is addressed, and
master is merged in with a merge commit, so the conflict is cleared.
**🟡 Medium: `allows_empty_result` was a dead flag.** Fixed in 5039119139.
The flag now makes the empty-vs-error decision:
- `VegaLitePreviewStrategy.generate()` (`get_chart_preview.py:494`) returns
`NoDataError` for empty rows unless the owning plugin sets
`allows_empty_result`, before either the plugin renderer or the generic spec
runs.
- The plugins that intentionally render empty results now opt in: bubble,
treemap, gauge and histogram, alongside gantt.
- `test_saved_empty_preview_obeys_plugin_contract` covers all four flag ×
renderer combinations, so a flagged plugin that renders through the generic
spec path no longer gets `NoDataError`.
- `test_empty_rendering_plugins_opt_in` pins the opt-ins.
**🟢 Low: the empty saved Histogram preview change was undocumented.**
Documented in UPDATING.md ("Empty MCP chart previews") and in
`docs/admin_docs/configuration/mcp-server.mdx`, next to the Bubble change.
**🟢 Low: the AST guard had evasion gaps.** Fixed in 5039119139:
- `test_dispatchers_do_not_branch_on_registered_chart_types` now parses the
five library modules whole, as well as the tool modules, instead of a
per-function list.
- `_branches_on_registered_type` matches structurally, independent of
variable names: `Compare` operands (including tuple/list/set literals), `Dict`
keys, the key argument of `.get()`, and subscripts, plus `isinstance(...,
*ChartConfig)`.
- The chart-specific expressions that already exist are pinned in an
exact-multiset baseline (`_LEGACY_TYPE_BRANCHES`), so adding or duplicating a
branch fails, and so does leaving a stale baseline entry.
- `test_dispatch_guard_detects_structural_branches` covers your examples: a
renamed local (`vt == "waterfall"`), membership in a tuple, dict-keyed
dispatch, `.get("waterfall")`, subscripts, and a previously unlisted helper.
`test_dispatch_guard_ignores_get_default` keeps `fd.get("viz_type", "table")`
from being flagged.
**🟢 Low: the Histogram parity comment overstated things.** I softened the
comment rather than dropping the disjunct (`plugins/histogram.py:200`). It now
says the frontend adds the aggregate for adhoc HAVING filters and that MCP also
honors the top-level `having` expression. I kept that path because MCP accepts
top-level `having` elsewhere, and #44744's tests exercise it.
**Also in this round:** a live retest found that MCP gateways cap a
tool-search page at 100 KB, and a page holding both `update_chart` (53 KB) and
`generate_chart` (49 KB) exceeded it.
- The compact config schema in this PR already fixes that; the worst-case
five-tool page is now 25,350 B.
- 3426b5fb7d adds `test_worst_case_search_page_fits_gateway_limit`. It fails
on master and passes here.
**Checks:** `tests/unit_tests/mcp_service`, `tests/unit_tests/charts` and
the query-context tests pass locally: 6645 passed, 4 skipped, 2 xfailed.
Pre-commit is clean.
--
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]
