aminghadersohi commented on PR #43573: URL: https://github.com/apache/superset/pull/43573#issuecomment-5574551254
Re-reviewed `bf5628cb86ad82aef28adf26b0c37e7a84211fcd` against refreshed base `87f138cc`. The shared-builder grouping/descending metric ordering remains correct, but the material gaps in [the prior request](https://github.com/apache/superset/pull/43573#issuecomment-5496889029) remain: - **Unsaved ordering and previews:** with `row_limit=1`, the shared builder emits metric-descending `orderby`, while `generate_preview_from_form_data` emits `orderby=[]` ([query construction](https://github.com/apache/superset/blob/bf5628cb86ad82aef28adf26b0c37e7a84211fcd/superset/mcp_service/chart/preview_utils.py#L101-L112)). The same edge data produces an unsaved Vega-Lite bar with only a color encoding and a saved-preview scatter fallback—not a weighted flow. Please use a faithful representation or an explicit unsupported-format error. - **Round-trip/update preservation:** passing mapper-produced native form data into `GenerateChartRequest` fails for saved/SIMPLE/SQL metrics. An immediate update with omitted controls resets `sort_by_metric=False`, `row_limit=37`, and `lyftColors` to `True`, `10000`, and `supersetColors`, and drops existing filters/UI state ([payload construction](https://github.com/apache/superset/blob/bf5628cb86ad82aef28adf26b0c37e7a84211fcd/superset/mcp_service/chart/tool/update_chart.py#L232-L251)). Preview-first merging also resets those three controls; cached preview updates still lack general Sankey preservation. - **Validation/casing:** mocked successful query envelopes containing string, infinity, or NaN weights all pass `_compile_chart`. `source=Node,target=node` is accepted and normalizes to `Node,Node`, including with both case-distinct columns present. `update_chart_preview` still maps before normalization. These need explicit finite-weight and unambiguous-role checks across tool paths. - **Recommendations:** two string dimensions plus one numeric column still returns only `bar chart`/`pie chart`; the added current-viz category does not create a Sankey candidate. Verification: **21 Sankey tests passed; the full MCP chart unit suite passed (1,408 tests)**, plus targeted mocked reproductions above. Sankey-specific tests remain confined to the schema/mapper/registry test file; public generate/update/update-preview/get-data and saved/cached/unsaved preview regressions are still needed, including error/empty/malformed envelopes. No live-app verification was available. No contributor code was modified or pushed. -- 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]
