aminghadersohi commented on PR #43573: URL: https://github.com/apache/superset/pull/43573#issuecomment-5612425046
Fresh re-review at `eb72811b549c` confirms progress since `bf5628cb`: unsaved compile/preview queries now use metric-descending ordering; cached-preview mapping normalizes column case first; omitted filters/UI state survive updates, and explicit empty filters clear them. Ambiguous non-exact column references and failed-status query envelopes are also handled. The remaining gaps from [the previous review](https://github.com/apache/superset/pull/43573#issuecomment-5574551254) are reproducible: - **Preview fidelity:** the same Sankey edge produces an unsaved Vega-Lite bar with only a color encoding, while saved previews still take the scatter fallback. Neither represents weighted flow; please provide a faithful representation or an explicit unsupported-format error. - **Round-trip/updates:** mapper-produced native form data still fails `GenerateChartRequest` with `Unknown field 'groupby'` for saved/SIMPLE/SQL metrics. Immediate and cached-preview reproductions reset omitted `sort_by_metric=False`, `row_limit=37`, and `color_scheme='lyftColors'` to `True`, `10000`, and `supersetColors`. The [shared merge](https://github.com/apache/superset/blob/eb72811b549c0972e93199093a3c20a1a803b960/superset/mcp_service/chart/chart_utils.py#L675-L721) only protects omitted mapped controls for Gauge, not Sankey. - **Validation:** string/NaN/infinite weights still pass compilation for saved/SIMPLE/SQL metrics, including an invalid second row. `source=Node,target=node` with one physical `Node` column still normalizes to duplicate `Node,Node` roles. A malformed `data: 'malformed'` envelope also reports compile success. The [result validator](https://github.com/apache/superset/blob/eb72811b549c0972e93199093a3c20a1a803b960/superset/mcp_service/chart/query_result.py#L125-L135) remains Gauge-only. - **Recommendations:** two string dimensions plus one numeric column still yields only `bar chart`/`pie chart`, not Sankey. Verification: chart unit suite **1,583 passed, 1 skipped**, plus targeted mocked reproductions including the cached-preview tool. Sankey-specific tests remain confined to schema/mapper/registry coverage. GitHub reports mergeable/clean, with 63 checks passing and 12 skipped. No live app was available; no contributor code was changed 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]
