aminghadersohi commented on PR #43570: URL: https://github.com/apache/superset/pull/43570#issuecomment-5590453024
@rusackas @gkneighb One independent re-review at `e8b270167a4b5a3125fa72bdee66b03afb4178d2`: **wait on the runtime failure**. The Heatmap patches are effectively unchanged after the rebase; the earlier axis/GROUP BY and schema-discovery fixes remain correct. I independently reproduced [Gabo’s compile finding](https://github.com/apache/superset/pull/43570#discussion_r3961103156) through the registered FastMCP tools, with authentication/dataset fixtures and no chart writes: - `generate_chart({"dataset_id":1,"config":{"chart_type":"heatmap_v2","x_axis":{"name":"day"},"y_axis":{"name":"hour"},"metric":{"name":"trips","aggregate":"COUNT"}},"save_chart":false})` raises `'str' object has no attribute 'copy'`; `save_chart:true` does too. - The same config in `update_chart` with `identifier:1` fails with structured `AttributeError` for both `generate_preview:false` and `true`. Please cover all four public paths when coercing scalar `groupby` in the shared column builder. The [September 1 product-path requests](https://github.com/apache/superset/pull/43570#issuecomment-5496940225) also remain open, independently of that blocker: - Mapper output fails typed native round-trip. Update-builder probes reset omitted `normalize_across:"y"/row_limit:42` to `"heatmap"/10000`; immediate replacement additionally drops saved presentation/filters/template parameters, unlike preview-first merging. Preserve omitted fields consistently; explicit values and empty-filter clears should win. - Public saved/key/combined-key Vega previews produce **circle**, not rect, marks. With query execution mocked, previews/data also accept rows alongside `status:"failed",error:"database error"`, missing metric values, or a nonnumeric last-row metric. Use shared Heatmap cell/result validation, or explicitly reject unsupported previews. - Sorting/rank normalization and distinct/unambiguous canonical axes remain unimplemented. Checks: focused chart/query suites **1,446 passed**; common/chart-data API **350 passed**; **51** temporary Heatmap probes plus **9** Jinja-context checks. Broader MCP suite: **3,739 passed, 1 health-check smoke failure**, independently reproduced on upstream master `c45fc6a31a2e`. Ruff, mypy, and applicable pre-commit hooks passed. These are remaining contract gaps, not regressions in the original GROUP BY fix. No branch edits/pushes, approval, or thread resolutions. -- 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]
