FrancescoCastaldi commented on PR #44464: URL: https://github.com/apache/superset/pull/44464#issuecomment-5849842086
Thanks a lot for the thorough review and real-dataset testing, @gabotorresruiz and @aminghadersohi! All points raised have been addressed and covered with unit and end-to-end pipeline tests in the latest commits (`fdb3ea5`): ### 1. `x_axis_sort` resolution to metric label & `sortOperator` compatibility - In `superset/mcp_service/chart/chart_utils.py` (`add_xy_sort_config`), the sort target is resolved against metrics in `config.y` (by name, custom label, or `sql_expression`). When matched, it derives the metric label (e.g., `SUM(sales)` or custom label `Total Sales`) matching what frontend `sortOperator.ts` expects in `sortableLabels`. - Case-insensitivity is supported for both metric matching and dimension column matching against the x-axis. ### 2. `ValidationPipeline` & `extract_column_refs` - In `superset/mcp_service/chart/plugins/xy.py`, `_get_covered_xy_names` and `_collect_y_metric_names` ensure that sort targets covered by `y`, `x`, or `group_by` are not emitted as raw physical columns, preventing premature `column_not_found` validation failures at Layer 2. - For independent saved metrics not in `y`, `_extract_sort_col_info` checks `dataset_context.available_metrics` and marks `saved_metric=True` on the `ColumnRef`, allowing `DatasetValidator` to accept them. ### 3. Schema validation & multi-item rejection - In `superset/mcp_service/chart/schemas.py`, `SortByConfig` rejects multi-item sort lists with an explicit `ValueError` stating that XY charts support at most one sort criterion. ### 4. Query Dictionary `orderby` mapping - In `chart_helpers.py`, `_resolve_x_axis_sort_target` now resolves `x_axis_sort` to metric dictionaries in `queries[0]["orderby"]` so MCP data/preview calls also receive the expected ordering. ### 5. Tests - Added `TestValidationPipelineWithXYChartSortBy` in `tests/unit_tests/mcp_service/chart/test_chart_utils.py` driving requests through `ValidationPipeline.validate_request_with_warnings` and `map_xy_config` to assert `form_data["x_axis_sort"] == "SUM(sales)"`, custom labels, and saved metrics. - Added tests in `test_dataset_validator.py` verifying that unmarked saved metrics fail appropriately while marked ones pass. - All 500 unit tests across affected suites pass, and `ruff check` passes with 0 issues. -- 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]
