aminghadersohi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4066340246
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -103,6 +126,21 @@ def extract_column_refs(self, config: Any) ->
list[ColumnRef]:
if config.filters:
for f in config.filters:
refs.append(ColumnRef(name=f.column))
+ if config.sort_by:
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, list) and sort_entry:
+ sort_entry = sort_entry[0]
+ sort_col = (
+ sort_entry.column
+ if isinstance(sort_entry, SortByConfig)
+ else sort_entry
+ if isinstance(sort_entry, str)
+ else sort_entry.get("column")
+ if isinstance(sort_entry, dict)
+ else None
+ )
+ if sort_col:
+ refs.append(ColumnRef(name=sort_col))
Review Comment:
codeant C1 confirmed, and broader: it fails on **every** path, not only
pre-normalization. Probed: sort by saved metric -> `ref doesn't have
saved_metric=true`; sort by a y label -> `Column 'Total Sales' not found`.
Skipping refs already covered by `y` fixes both; suite 1884 passed.
```suggestion
if sort_col and not any(
sort_col.lower() in {n.lower() for n in (yc.name, yc.label)
if n}
for yc in config.y
):
refs.append(ColumnRef(name=sort_col))
```
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -42,6 +42,29 @@
logger = logging.getLogger(__name__)
+def _normalize_xy_sort_by(config_dict: dict[str, Any], dataset_context: Any)
-> None:
+ """Resolve canonical column or metric name for sort_by in XY charts."""
+ sort_by = config_dict.get("sort_by")
+ if not sort_by:
+ return
+
+ get_canonical = DatasetValidator.get_canonical_column_name
+
+ def _resolve_name(raw_name: str) -> str:
+ raw_lower = raw_name.lower()
+ for y_col in config_dict.get("y") or []:
+ if y_col.get("label") and y_col["label"].lower() == raw_lower:
+ return y_col["label"]
Review Comment:
bito B1 is **wrong** — returning `label` here is correct. `map_xy_config`
emits the metric as `label: 'Total Sales'` (hasCustomLabel), and
Timeseries/transformProps.ts:502 sorts series by that label, so
`x_axis_sort='Total Sales'` matches. Returning `name` would be the bug. No
change needed.
##########
tests/unit_tests/mcp_service/chart/test_chart_utils.py:
##########
@@ -2497,3 +2498,250 @@ def
test_normalize_column_names_skips_sql_metric_dicts(self) -> None:
)
assert normalized.y[0].sql_expression == _SQL_EXPR
assert normalized.y[0].name is None
+
+
+class TestAddXYSortConfig:
+ """Test add_xy_sort_config helper function."""
+
+ def test_no_sort_by_does_nothing(self) -> None:
+ form_data: dict[str, Any] = {
+ "x_axis_sort_series_type": "name",
+ "x_axis_sort_series_ascending": True,
+ }
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert "x_axis_sort" not in form_data
+ assert "x_axis_sort_asc" not in form_data
+ assert form_data["x_axis_sort_series_type"] == "name"
+ assert form_data["x_axis_sort_series_ascending"] is True
+
+ def test_non_temporal_sort_by_metric_descending(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by="sales",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "sales"
+ assert form_data["x_axis_sort_asc"] is False
+ assert form_data["x_axis_sort_series_type"] == "value"
+ assert form_data["x_axis_sort_series_ascending"] is False
+
+ def test_non_temporal_sort_by_metric_ascending(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by=SortByConfig(column="sales", ascending=True),
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "sales"
+ assert form_data["x_axis_sort_asc"] is True
+ assert form_data["x_axis_sort_series_type"] == "value"
+ assert form_data["x_axis_sort_series_ascending"] is True
+
+ def test_non_temporal_sort_by_x_axis_column(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by=SortByConfig(column="category", ascending=True),
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "category"
+ assert form_data["x_axis_sort_asc"] is True
+ assert form_data["x_axis_sort_series_type"] == "name"
+ assert form_data["x_axis_sort_series_ascending"] is True
+
+ def test_temporal_sort_by_ignored_with_warning(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="order_date"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="line",
+ sort_by="sales",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=True)
+
+ assert "x_axis_sort" not in form_data
+ assert "x_axis_sort_asc" not in form_data
+ assert "x_axis_sort_series_type" not in form_data
+ assert len(form_data.get("_mcp_warnings", [])) == 1
+ expected_msg = "was ignored because the x-axis column 'order_date' is
temporal"
+ assert expected_msg in form_data["_mcp_warnings"][0]
+
+
+class TestMapXYConfigWithSortBy:
+ """Test map_xy_config integration with sort_by."""
+
+ @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal")
+ def test_map_xy_config_non_temporal_sort_by_metric(self, mock_is_temporal)
-> None:
+ mock_is_temporal.return_value = False
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="department"),
+ y=[ColumnRef(name="revenue", aggregate="SUM")],
+ kind="bar",
+ sort_by="revenue",
+ )
+ form_data = map_xy_config(config, dataset_id=1)
+
+ assert form_data["x_axis_sort"] == "revenue"
+ assert form_data["x_axis_sort_asc"] is False
+ assert form_data["x_axis_sort_series_type"] == "value"
+ assert form_data["x_axis_sort_series_ascending"] is False
+
+ @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal")
+ def test_map_xy_config_non_temporal_sort_by_x_axis(self, mock_is_temporal)
-> None:
+ mock_is_temporal.return_value = False
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="department"),
+ y=[ColumnRef(name="revenue", aggregate="SUM")],
+ kind="bar",
+ sort_by=SortByConfig(column="department", ascending=True),
+ )
+ form_data = map_xy_config(config, dataset_id=1)
+
+ assert form_data["x_axis_sort"] == "department"
+ assert form_data["x_axis_sort_asc"] is True
+ assert form_data["x_axis_sort_series_type"] == "name"
+ assert form_data["x_axis_sort_series_ascending"] is True
+
+ @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal")
+ def test_map_xy_config_temporal_ignores_sort_by(self, mock_is_temporal) ->
None:
+ mock_is_temporal.return_value = True
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="order_date"),
+ y=[ColumnRef(name="revenue", aggregate="SUM")],
+ kind="line",
+ sort_by="revenue",
+ )
+ form_data = map_xy_config(config, dataset_id=1)
+
+ assert "x_axis_sort" not in form_data
+ assert len(form_data.get("_mcp_warnings", [])) == 1
+ expected_msg = "was ignored because the x-axis column 'order_date' is
temporal"
+ assert expected_msg in form_data["_mcp_warnings"][0]
+
+ @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal")
+ def test_map_xy_config_without_sort_by_keeps_defaults(
+ self, mock_is_temporal
+ ) -> None:
+ mock_is_temporal.return_value = False
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="department"),
+ y=[ColumnRef(name="revenue", aggregate="SUM")],
+ kind="bar",
+ )
+ form_data = map_xy_config(config, dataset_id=1)
+
+ assert "x_axis_sort" not in form_data
+ assert form_data["x_axis_sort_series_type"] == "name"
+ assert form_data["x_axis_sort_series_ascending"] is True
+
+
+class TestXYChartPluginSortBy:
+ """Test XYChartPlugin extract_column_refs and normalize_column_refs with
sort_by."""
+
+ def test_extract_column_refs_includes_sort_by(self) -> None:
+ from superset.mcp_service.chart.plugins.xy import XYChartPlugin
+
+ plugin = XYChartPlugin()
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ sort_by=SortByConfig(column="sales", ascending=False),
+ )
+ refs = plugin.extract_column_refs(config)
+ ref_names = [r.name for r in refs]
+
+ assert "category" in ref_names
+ assert "sales" in ref_names
Review Comment:
bito B2 confirmed by mutation. Control arm (no-op edit): 297 passed. Then
`if False and config.sort_by:` in `extract_column_refs` -> still **297
passed**, zero tests red. `sales` is already in `y`, so the assertion cannot
see the branch. Sorting by a distinct column kills the mutant.
```suggestion
sort_by=SortByConfig(column="profit", ascending=False),
)
refs = plugin.extract_column_refs(config)
ref_names = [r.name for r in refs]
assert "category" in ref_names
assert "profit" in ref_names
```
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2350,6 +2350,38 @@ class XYChartConfig(BaseChartConfig):
ge=1,
le=10000,
)
+ sort_by: SortByConfig | str | List[SortByConfig | str] | None = Field(
+ None,
+ description=(
+ "Sort specification for the chart. Accepts a SortByConfig object, "
+ "a bare column/metric name string (defaults to descending), or a "
+ "single-item list containing either."
+ ),
+ validation_alias=AliasChoices("sort_by", "x_axis_sort", "order_by"),
+ )
+
+ @field_validator("sort_by", mode="before")
+ @classmethod
+ def coerce_sort_by(cls, v: Any) -> Any:
+ """Coerce bare string, dict, or single-item list into SortByConfig."""
+ if v is None:
+ return None
+ if isinstance(v, str):
+ return SortByConfig(column=v, ascending=False)
+ if isinstance(v, list):
+ if not v:
+ return None
+ first = v[0]
Review Comment:
codeant C2 confirmed. `model_json_schema()` advertises `type: array` with no
`maxItems`, so a client may legitimately send 2 items; they silently become
`SortByConfig(column='a')`. Only the prose says single-item. Raising leaves
1-item and empty unchanged; suite 1884 passed.
```suggestion
if isinstance(v, list):
if not v:
return None
if len(v) > 1:
raise ValueError(
"sort_by accepts at most one sort criterion, "
f"received {len(v)}."
)
first = v[0]
```
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1110,6 +1110,50 @@ def add_orientation_config(form_data: Dict[str, Any],
config: XYChartConfig) ->
form_data["orientation"] = config.orientation
+def add_xy_sort_config(
+ form_data: Dict[str, Any], config: XYChartConfig, x_is_temporal: bool
+) -> None:
+ """Apply sort configuration to form_data for XY charts.
+
+ When ``config.sort_by`` is present:
+ - If ``x_is_temporal``: records a warning in ``form_data["_mcp_warnings"]``
+ and does not override temporal sorting.
+ - If non-temporal: sets ``form_data["x_axis_sort"]`` and
+ ``form_data["x_axis_sort_asc"]``, along with backward-compatibility keys
Review Comment:
Not back-compat keys: `x_axis_sort_series_type` occurs nowhere else in the
repo, and transformProps.ts:502 derives `xAxisSortSeries` from `x_axis_sort`.
Migration 378cecfdba9f pops `x_axis_sort_series*` into `x_axis_sort`. Both
writes are dead (`:1176` predates this PR).
--
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]
