aminghadersohi opened a new pull request, #44746:
URL: https://github.com/apache/superset/pull/44746
### SUMMARY
The MCP chart tools branch on `viz_type` in several shared modules: query
construction (`chart_helpers`), result normalization (`query_result`), the
compile check (`compile`), saved-chart previews (`get_chart_preview`),
form-data previews (`preview_utils`), update merges
(`chart_utils.merge_chart_form_data`) and the update tools' rebind rules. The
saved and unsaved preview paths each keep their own copy of the per-chart
dispatch. Every new chart type adds another branch to each of these, and the
open chart-type PRs each add their own.
This PR puts that per-chart behavior behind one plugin lifecycle contract:
- **Hooks on `BaseChartPlugin`**, each with a default that keeps the shared
behavior:
- query construction: `resolve_query_fields`, `build_query_dicts`
- result normalization, applied by compile, previews and (when
`normalize_data_results` is set) `get_chart_data` and its CSV/XLSX exports:
`normalize_query_result`
- row limits: `compile_row_limit`, `preview_row_limit`
- previews: `ascii_preview`, `vega_lite_preview`
- update and rebind: `resolve_update_config`, `merge_update_form_data`,
`validate_merged_form_data`
- **Contract flags** that replace the hard-coded viz-type lists:
`requires_compile_check`, `requires_config_for_dataset_rebind`,
`unbound_form_data_is_rebind`, `normalize_data_results`, `allows_empty_result`,
`resizes_saved_preview`, `supports_column_append`, `preview_note`,
`invalid_result_error_code` / `invalid_result_message`, and
`additional_viz_types` for saved legacy or sibling viz types (`bubble`,
`pop_kpi`).
- **`registry.plugin_for_viz_type()`** resolves the plugin that owns a saved
chart's `viz_type`. It ignores runtime enablement, so disabling a type only
stops new charts of that type from being created.
- **One dispatcher per concern.** Each shared tool calls that dispatcher
instead of branching on viz type. Treemap, Gauge, Gantt, Big Number, Bubble,
Mixed Timeseries, Table and Handlebars behavior moves into their plugins as it
is.
**Bug fix found by the contract suite:** Histogram charts queried through
the shared builder (`get_chart_data`, previews, compile) selected no columns
and no metrics. The Histogram plugin now mirrors the frontend `buildQuery` and
`histogramOperator`: `[...groupby, column]`, a `COUNT(*)` metric when a HAVING
filter is present, and `histogram` post-processing.
**Other visible differences:**
- Saved-chart Vega-Lite previews now call the plugin renderer first, the
same order the form-data preview path already used. A saved Bubble chart with
zero rows now returns the same empty Bubble spec that an unsaved preview
returns, where it used to return `NoDataError`.
- The rebind error for Gauge now uses the plugin display name: "Gauge Chart
dataset rebind requires a complete Gauge Chart config."
- `resolve_metrics(form_data, viz_type)` returns Big Number's singular
metric, matching `resolve_metrics_and_groupby` and the executed query.
**Registry-wide regression suite** (`test_chart_plugin_contract.py`). It is
parametrized over the live registry, so it covers every chart type registered
now and any added later. Each registered type must:
- implement every hook and declare typed flags;
- publish at least one schema example;
- own its viz types uniquely, and keep owning them when disabled;
- round-trip the viz type it maps back to itself;
- build non-empty, well-formed queries, and agree with its own
`build_query_dicts`;
- handle malformed and failed result envelopes without raising or mutating
them;
- return a typed preview or error from every preview format;
- render saved and unsaved Vega-Lite previews from the same renderer;
- keep newly mapped query roles in a same-viz update;
- drop stale roles and filters on a dataset rebind;
- drop every saved control when the viz type changes.
An AST guard also fails if a shared dispatcher compares a viz or chart type
against a registered chart type, so new chart types have to use hooks.
**Out of scope:** Jinja `g.form_data` seeding in the compile, preview and
form-data `get_chart_data` paths (the gap reported for #40570) is handled by
#43176. That PR also carries the approach from #43711 by
@Abdulrehman-PIAIC80387. This PR does not touch those call sites, so the two
merge independently.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/mcp_service tests/unit_tests/charts \
tests/unit_tests/common/test_form_data_query_context.py \
tests/unit_tests/common/test_query_context_factory.py \
tests/unit_tests/common/test_query_context_processor.py \
tests/unit_tests/common/test_query_context_processor_timing.py
```
Locally: 6437 passed, 4 skipped, 2 xfailed. The new Histogram test fails on
master (the query selects only `['region']`) and passes with this change.
Manual check: call `get_chart_data` on a saved Histogram chart. It returns
binned rows instead of failing on an empty query. Treemap, Gauge and Gantt
previews, `update_chart` and `update_chart_preview` behave as before.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is 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
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
