gkneighb commented on PR #43573:
URL: https://github.com/apache/superset/pull/43573#issuecomment-6029757928
@rusackas Following up on the CI question from your sankey review — the
rebase is done on all four and the picture turned out different from what
either of us expected. One correction first: **the byte budget isn't failing.**
All four now fail exactly one test,
`test_dispatchers_do_not_branch_on_registered_chart_types` from #44746, and
nothing else:
| PR | what the guard flags |
| --- | --- |
| #43567 funnel | `preview_utils: {'funnel':
generate_funnel_vega_lite_preview}` |
| #43570 heatmap | `chart_helpers: viz_type == 'heatmap_v2'` |
| #43571 radar | `get_chart_data: _VIZ_CATEGORY` → `radar` |
| #43573 sankey | `get_chart_data: _VIZ_CATEGORY` → `sankey_v2` |
**The mechanism is worth stating plainly, because it isn't "these PRs add
bad code."** The guard flags dispatcher branches keyed on *registered* chart
types. Registering a type is therefore enough to turn pre-existing code into a
violation.
#43567 is the clean illustration. `_FALLBACK_VEGA_RENDERERS = {"funnel":
generate_funnel_vega_lite_preview}` is **master's line**, added by #44746
itself at `preview_utils.py:1382`. It's legal today only because `funnel` isn't
registered, so it isn't in `_LEGACY_TYPE_BRANCHES`. The moment #43567 registers
the plugin, master's own dict becomes the violation. My diff doesn't touch that
file.
#43570 is the mirror image: the flagged `viz_type == 'heatmap_v2'` is a line
**I wrote during this rebase**, adopting your `with_x_axis_column()` helper to
keep the heatmap x-axis reaching `GROUP BY`. Correct for the pre-#44746 shape,
illegal by construction now.
So all four need the same thing — move the per-type behavior into the plugin
hooks the new contract provides, so no shared dispatcher names the type. That's
a real port, not a line edit, and I don't want to guess at your intent on two
points:
1. **Was this anticipated?** `preview_utils` on master already mentions
`funnel` (3×), `radar` and `sankey`. If #44746 was written with these four in
mind, you may already have a shape in mind for how they should land — I'd
rather implement that than invent a parallel one.
2. **Who should do it?** Happy to port all four onto the contract. But if
it's cleaner for whoever holds #44746 to absorb them — four approved plugins is
a decent forcing function for the hooks — that's fine by me and probably
faster. Parking them until the contract settles is also a reasonable answer.
Separately, your non-blocking nit on this PR is valid: `SankeyChartConfig`
has no `record_implicit_metric_aggregate`, so a bare metric on a text column
fails at the database instead of returning the clean `invalid_aggregation`.
I'll fold that in with whatever port shape you pick rather than pushing it
standalone.
All four are rebased, mergeable, and green apart from that one test.
--
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]
