diff --git a/superset/mcp_service/chart/chart_utils.py b/superset/mcp_service/chart/chart_utils.py index 5b865c1f0dd..ba5a885cd4d 100644 --- a/superset/mcp_service/chart/chart_utils.py +++ b/superset/mcp_service/chart/chart_utils.py @@ -37,6 +37,7 @@ MixedTimeseriesChartConfig, PieChartConfig, PivotTableChartConfig, + SortByConfig, TableChartConfig, XYChartConfig, ) @@ -466,7 +467,14 @@ def map_table_config(config: TableChartConfig) -> Dict[str, Any]: _add_adhoc_filters(form_data, config.filters) if config.sort_by: - form_data["order_by_cols"] = config.sort_by + form_data["order_by_cols"] = [ + json.dumps( + [entry.column, entry.ascending] + if isinstance(entry, SortByConfig) + else [entry, False] + ) + for entry in config.sort_by + ] form_data["row_limit"] = config.row_limit diff --git a/superset/mcp_service/chart/schemas.py b/superset/mcp_service/chart/schemas.py index 1fdb4f43ab6..67aa6983563 100644 --- a/superset/mcp_service/chart/schemas.py +++ b/superset/mcp_service/chart/schemas.py @@ -803,6 +803,30 @@ def validate_value_type_matches_operator(self) -> FilterConfig: return self +class SortByConfig(BaseModel): + """Sort specification with explicit direction. + + Accepts either this object or a bare column-name string in `sort_by` + lists. Bare strings default to descending, which matches the + sort-by-metric "top N" pattern most commonly used for tables. + """ + + model_config = ConfigDict(populate_by_name=True) + + column: str = Field( + ..., + min_length=1, + max_length=255, + validation_alias=AliasChoices("column", "col"), + description="Column to sort by", + ) + ascending: bool = Field( + False, + description="Sort ascending. Defaults to False (descending) to match " + "the typical sort-by-metric top-N use case.", + ) + + # Actual chart types class PieChartConfig(UnknownFieldCheckMixin): model_config = ConfigDict(extra="ignore", populate_by_name=True) @@ -1189,8 +1213,12 @@ class TableChartConfig(UnknownFieldCheckMixin): description="Structured filters (column/op/value). " "Do NOT use adhoc_filters or raw SQL expressions.", ) - sort_by: List[str] | None = Field( + sort_by: List[str | SortByConfig] | None = Field( None, + description="Columns to sort by. Each entry is either a column-name " + "string (defaults to descending) or a SortByConfig object with " + "explicit `ascending`. Descending matches the sort-by-metric " + "top-N pattern most common for tables.", validation_alias=AliasChoices("sort_by", "order_by_cols", "order_by"), ) row_limit: int = Field(1000, description="Max rows returned", ge=1, le=50000) diff --git a/tests/unit_tests/mcp_service/chart/test_chart_utils.py b/tests/unit_tests/mcp_service/chart/test_chart_utils.py index 7c2da4e5de7..26d04fe4f5b 100644 --- a/tests/unit_tests/mcp_service/chart/test_chart_utils.py +++ b/tests/unit_tests/mcp_service/chart/test_chart_utils.py @@ -43,6 +43,7 @@ ColumnRef, FilterConfig, LegendConfig, + SortByConfig, TableChartConfig, XYChartConfig, ) @@ -273,7 +274,7 @@ def test_map_table_config_with_mixed_filters(self) -> None: assert result["adhoc_filters"][2]["comparator"] == ["Sports", "Racing"] def test_map_table_config_with_sort(self) -> None: - """Test table config mapping with sort""" + """Bare strings default to descending.""" config = TableChartConfig( chart_type="table", columns=[ColumnRef(name="product")], @@ -281,7 +282,26 @@ def test_map_table_config_with_sort(self) -> None: ) result = map_table_config(config) - assert result["order_by_cols"] == ["product", "revenue"] + assert result["order_by_cols"] == ['["product", false]', '["revenue", false]'] + + def test_map_table_config_with_sort_direction(self) -> None: + """SortByConfig entries honor the explicit ascending flag.""" + config = TableChartConfig( + chart_type="table", + columns=[ColumnRef(name="product")], + sort_by=[ + SortByConfig(column="product", ascending=True), + SortByConfig(column="revenue", ascending=False), + "category", + ], + ) + + result = map_table_config(config) + assert result["order_by_cols"] == [ + '["product", true]', + '["revenue", false]', + '["category", false]', + ] def test_map_table_config_ag_grid_table(self) -> None: """Test table config mapping with AG Grid Interactive Table viz_type"""