rusackas commented on PR #44148: URL: https://github.com/apache/superset/pull/44148#issuecomment-5769983926
Thanks for all the work here, @aminghadersohi, this is a lot of ground covered and most of it holds up well. Sadpandajoe's two threads got real fixes with good evidence, and most of codeant's other flags turned out to be false alarms you backed up with actual runtime checks. One thing before this can merge, though. In `CountryMap/transformProps.ts`, the new per-row validation only checks `formData.regionFormat`, and that control is available to anyone in Explore, not just MCP-generated charts. It throws for *any* row with a missing or non-numeric metric, not just unresolved/ambiguous regions like the control's own description promises. A perfectly normal sparse dataset (a state with no data, a NULL aggregate) will now crash the whole chart instead of rendering it blank. Same pattern shows up in the deck_scatter `Scatter/transformProps.ts`, gated behind `mcp_geographic` there so it's a smaller blast radius, but needs the same treatment: skip/blank the row instead of throwing. Also, GitHub shows conflicts with `master` right now despite CI being green on the last head, so it'll need a rebase regardless. There's a handful of bito's DRY/perf nits still open (duplicated `GEOGRAPHIC_VIZ_TYPES`, some per-row recomputation) but those can ride as a follow-up, not blocking. Holler if you want to talk through the fix. -- 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]
