Re: [PR] feat(mcp): funnel chart type plugin [superset]

["aminghadersohi (via GitHub)" <gi...@apache.org>]

aminghadersohi commented on PR #43567:
URL: https://github.com/apache/superset/pull/43567#issuecomment-5497039076

   @gkneighb Thanks—the earlier Funnel aggregate-dimension and saved get-data ordering fixes remain correct at `1018e8a1`. A fresh Red Hat product-completeness pass found additional end-to-end gaps beyond those prior reviews. Could you please address or coordinate these before we count Funnel support as complete?
   
   1. **Frontend contract:** support the frontend’s ordered multi-`groupby` capability rather than only one typed dimension, and expose/map the meaningful percent, legend, label/tooltip, visibility, number/currency, and color controls.
   2. **Native round-trip + updates:** adapt actual native `groupby` lists and saved/SIMPLE/SQL metric shapes into `FunnelChartConfig`. Preserve omitted `sort_by_metric`, `row_limit`, color, filters, and presentation state across immediate, preview-first, and cached updates; explicit values and empty-filter clears must still override.
   3. **Unsaved query + previews:** compile/unsaved preview currently bypass the Funnel-aware shared builder, losing metric ordering under row limits. Saved preview has an approximation, while unsaved Vega drops the singular metric and ASCII falls back to a table. Add one shared Funnel representation resolving real dimension/metric aliases with consistent ordering, empty, malformed, and query-error behavior.
   4. **Metric/case validation:** require numeric saved/SIMPLE/SQL output, reject a metric output alias colliding with a dimension label, and fail closed on ambiguous case-insensitive column/metric/filter matches.
   5. **Discovery + product tests:** add Funnel to primary guidance and suitable progression-shaped recommendations. Cover generate/update/update-preview/get-data, native round-trip, ordering, and every preview format—not only mapper/registry tests.
   
   I ran the 18 focused Funnel tests and the full MCP chart suite (1,385 passed) plus native-shape, preservation, ordering, alias/type, and ambiguity reproductions; these gaps remain with green CI. Shared builder/canonicalization/error handling from #43737 and omission-aware update work from #43679 should be reused/rebased rather than duplicated. I did not modify or push to your branch.
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org