bito-code-review[bot] commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4112895981
##########
tests/unit_tests/mcp_service/chart/test_chart_utils.py:
##########
@@ -2497,3 +2498,630 @@ 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"] == "SUM(sales)"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ 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"] == "SUM(sales)"
+ assert form_data["x_axis_sort_asc"] is True
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ def test_non_temporal_sort_by_metric_with_custom_label(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM", label="Total Sales")],
+ kind="bar",
+ sort_by="sales",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "Total Sales"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+
+ def test_non_temporal_sort_by_saved_metric(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="total_revenue", saved_metric=True)],
+ kind="bar",
+ sort_by="total_revenue",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "total_revenue"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+
+ 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 "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ def test_non_temporal_sort_by_x_axis_case_insensitive(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="Category", label="ProductCategory"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by="category",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+ assert form_data["x_axis_sort"] == "ProductCategory"
+
+ form_data2: dict[str, Any] = {}
+ config2 = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="Category", label="ProductCategory"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by="productcategory",
+ )
+ add_xy_sort_config(form_data2, config2, x_is_temporal=False)
+ assert form_data2["x_axis_sort"] == "ProductCategory"
+
+ def test_non_temporal_sort_by_sql_expression(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[
+ ColumnRef(
+ sql_expression="COUNT(DISTINCT user_id)",
label="unique_users"
+ )
+ ],
+ kind="bar",
+ sort_by="count(distinct user_id)",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+ assert form_data["x_axis_sort"] == "unique_users"
+
+ def test_non_temporal_sort_by_pair_format(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", True],
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+ assert form_data["x_axis_sort"] == "SUM(sales)"
+ assert form_data["x_axis_sort_asc"] 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]
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test docstrings</b></div>
<div id="fix">
BITO.md rule 12148 requires every newly added test function to carry a
docstring documenting purpose and expected behavior; none of the ten tests in
`TestAddXYSortConfig` has one. Adding one-line docstrings (e.g. `sort_by absent
leaves series-sort keys untouched`) aligns the new class with the org standard
and makes each case's intent greppable. Older tests in this file also lack
them, but the rule targets new functions.
</div>
</div>
<div id="suggestion">
<div id="issue"><b>Incomplete series-sort assertion</b></div>
<div id="fix">
`add_xy_sort_config` pops both `x_axis_sort_series_type` and
`x_axis_sort_series_ascending` (chart_utils.py:1202-1203), and sibling tests
assert both keys are absent (lines 2538, 2554, 2600). This custom-label test
checks only `x_axis_sort_series_type`, leaving the ascending-key pop unverified
on this resolution path. Add the missing assertion for parity and coverage.
</div>
</div>
<small><i>Code Review Run #ed1edd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/test_chart_helpers.py:
##########
@@ -1325,3 +1327,119 @@ def
test_shared_query_builder_keeps_mixed_timeseries_ordering_per_query(
expected_secondary["orderby"] = secondary_orderby
assert secondary == expected_secondary
assert form_data["orderby"] == [["count", True]]
+
+
+def test_build_single_query_dict_x_axis_sort_with_metric_label() -> None:
+ metric = {
+ "label": "SUM(sales)",
+ "aggregate": "SUM",
+ "column": {"column_name": "sales"},
+ }
+ form_data = {
+ "x_axis_sort": "SUM(sales)",
+ "x_axis_sort_asc": False,
+ }
+ qd = _build_single_query_dict(form_data, ["category"], [metric])
+ assert qd["orderby"] == [(metric, False)]
+
+
+def test_build_single_query_dict_x_axis_sort_with_metric_column_name() -> None:
+ metric = {
+ "label": "SUM(sales)",
+ "aggregate": "SUM",
+ "column": {"column_name": "sales"},
+ }
+ form_data = {
+ "x_axis_sort": "sales",
+ "x_axis_sort_asc": True,
+ }
+ qd = _build_single_query_dict(form_data, ["category"], [metric])
+ assert qd["orderby"] == [(metric, True)]
+
+
+def test_build_single_query_dict_x_axis_sort_with_saved_metric() -> None:
+ form_data = {
+ "x_axis_sort": "revenue",
+ "x_axis_sort_asc": False,
+ }
+ qd = _build_single_query_dict(form_data, ["category"], ["revenue"])
+ assert qd["orderby"] == [("revenue", False)]
+
+
+def test_build_single_query_dict_x_axis_sort_with_dimension_column() -> None:
+ form_data = {
+ "x_axis_sort": "category",
+ "x_axis_sort_asc": True,
+ }
+ qd = _build_single_query_dict(form_data, ["category"], ["revenue"])
+ assert qd["orderby"] == [("category", True)]
+
+
+def test_build_single_query_dict_prefers_existing_orderby() -> None:
+ form_data = {
+ "orderby": [["count", True]],
+ "x_axis_sort": "sales",
+ "x_axis_sort_asc": False,
+ }
+ qd = _build_single_query_dict(form_data, ["category"], ["sales"])
+ assert qd["orderby"] == [["count", True]]
+
+
+def test_build_query_dicts_from_form_data_xy_bar_with_x_axis_sort() -> None:
+ metric = {
+ "label": "SUM(sales)",
+ "aggregate": "SUM",
+ "column": {"column_name": "sales"},
+ }
+ form_data = {
+ "viz_type": "echarts_timeseries_bar",
+ "x_axis": "category",
+ "metrics": [metric],
+ "x_axis_sort": "SUM(sales)",
+ "x_axis_sort_asc": False,
+ }
+ with patch(
+ "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine",
+ return_value="base",
+ ):
+ queries = build_query_dicts_from_form_data(form_data, 1, "table")
+
+ assert len(queries) == 1
+ assert queries[0]["columns"] == ["category"]
+ assert queries[0]["metrics"] == [metric]
+ assert queries[0]["orderby"] == [(metric, False)]
+
+
+def test_resolve_x_axis_sort_target_case_insensitive_label() -> None:
+ metric = {
+ "label": "Total Sales",
+ "aggregate": "SUM",
+ "column": {"column_name": "sales"},
+ }
+ resolved = _resolve_x_axis_sort_target("total sales", [metric])
+ assert resolved == metric
+
+
+def test_resolve_x_axis_sort_target_case_insensitive_column() -> None:
+ metric = {
+ "label": "SUM(sales)",
+ "aggregate": "SUM",
+ "column": {"column_name": "Sales"},
+ }
+ resolved = _resolve_x_axis_sort_target("sales", [metric])
+ assert resolved == metric
+
+
+def test_resolve_x_axis_sort_target_sql_expression() -> None:
+ metric = {
+ "expressionType": "SQL",
+ "sqlExpression": "COUNT(DISTINCT user_id)",
+ "label": "unique_users",
+ }
+ resolved = _resolve_x_axis_sort_target("count(distinct user_id)", [metric])
+ assert resolved == metric
+
+
+def test_resolve_x_axis_sort_target_string_metric_case_insensitive() -> None:
+ resolved = _resolve_x_axis_sort_target("totalsales", ["TotalSales"])
+ assert resolved == "TotalSales"
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test docstrings</b></div>
<div id="fix">
The ten new test functions (lines 1332-1445) have no docstrings. BITO
adaptive rule 12148 requires every newly added test function to carry a
docstring stating purpose and expected behavior; neighboring tests in this file
(e.g. line 1264) follow that pattern. One-line docstrings keep the file
consistent and make test intent scannable.
</div>
</div>
<small><i>Code Review Run #ed1edd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1110,6 +1110,99 @@ def add_orientation_config(form_data: Dict[str, Any],
config: XYChartConfig) ->
form_data["orientation"] = config.orientation
+def _match_y_metric_label(y_cols: list[ColumnRef], sort_lower: str) -> str |
None:
+ """Find matching metric label for sort_by among Y-axis metrics."""
+ for y_col in y_cols:
+ metric_obj = create_metric_object(y_col)
+ metric_label = (
+ metric_obj
+ if isinstance(metric_obj, str)
+ else (metric_obj.get("label") or "")
+ )
+ agg_expr = (
+ f"{y_col.aggregate}({y_col.name})".lower()
+ if (y_col.aggregate and y_col.name)
+ else None
+ )
+ col_name = (y_col.name or "").lower()
+ col_label = (y_col.label or "").lower()
+ sql_expr = (y_col.sql_expression or "").lower()
+ metric_label_lower = metric_label.lower()
+
+ if (
+ sort_lower == col_name
+ or sort_lower == metric_label_lower
+ or (col_label and sort_lower == col_label)
+ or (agg_expr and sort_lower == agg_expr)
+ or (sql_expr and sort_lower == sql_expr)
+ ):
+ return metric_label
+ return None
+
+
+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: resolves the sort target to the corresponding metric
label
+ (or dimension column name) and sets ``form_data["x_axis_sort"]`` and
+ ``form_data["x_axis_sort_asc"]``.
+ When ``config.sort_by`` is not specified, maintains existing default
behavior.
+ """
+ if not config.sort_by:
+ return
+
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, (list, tuple)):
+ if not sort_entry:
+ return
+ if (
+ len(sort_entry) == 2
+ and isinstance(sort_entry[0], str)
+ and isinstance(sort_entry[1], bool)
+ ):
+ sort_entry = SortByConfig(column=sort_entry[0],
ascending=sort_entry[1])
+ else:
+ sort_entry = sort_entry[0]
+ if isinstance(sort_entry, str):
+ sort_entry = SortByConfig(column=sort_entry, ascending=False)
+ elif isinstance(sort_entry, dict):
+ sort_entry = SortByConfig(**sort_entry)
+
+ if x_is_temporal:
+ x_name = config.x.name if config.x else "x"
+ form_data.setdefault("_mcp_warnings", []).append(
+ f"sort_by='{sort_entry.column}' was ignored because the x-axis "
+ f"column '{x_name}' is temporal. Temporal charts sort "
+ f"chronologically by the time axis."
+ )
+ return
+
+ sort_lower = sort_entry.column.lower()
+ x_name = (config.x.name or "").lower() if config.x else None
+ x_label = (config.x.label or "").lower() if config.x else None
+
+ # If sorting by the x-axis dimension itself (case-insensitive check)
+ if config.x and (
+ (x_name and sort_lower == x_name)
+ or (x_label and sort_lower == x_label)
+ ):
+ sort_target = config.x.label or config.x.name
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>x sort target uses label not name</b></div>
<div id="fix">
`map_xy_config` sets `form_data['x_axis'] = config.x.name`
(chart_utils.py:1470), but here `x_axis_sort` receives `config.x.label`. When
label != name, `_resolve_x_axis_sort_target` (chart_helpers.py:679-698) matches
no metric and returns the raw label string, which flows into `qd['orderby']`
(chart_helpers.py:726-730) as an unresolvable name. Prefer `config.x.name`.
</div>
</div>
<small><i>Code Review Run #ed1edd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1110,6 +1110,99 @@ def add_orientation_config(form_data: Dict[str, Any],
config: XYChartConfig) ->
form_data["orientation"] = config.orientation
+def _match_y_metric_label(y_cols: list[ColumnRef], sort_lower: str) -> str |
None:
+ """Find matching metric label for sort_by among Y-axis metrics."""
+ for y_col in y_cols:
+ metric_obj = create_metric_object(y_col)
+ metric_label = (
+ metric_obj
+ if isinstance(metric_obj, str)
+ else (metric_obj.get("label") or "")
+ )
+ agg_expr = (
+ f"{y_col.aggregate}({y_col.name})".lower()
+ if (y_col.aggregate and y_col.name)
+ else None
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Agg expr mismatch vs metric label</b></div>
<div id="fix">
`create_metric_object` (chart_utils.py:1023-1038) normalizes STDDEV/VAR
aliases and labels SIMPLE metrics as upper-cased `AGG(name)`. `agg_expr` here
uses the raw `y_col.aggregate`, so `sort_by='stddev(height)'` never matches
label 'STDDEV(height)' and falls through to `sort_entry.column`, yielding an
orderby on an unmatched name. Normalize the aggregate before comparing.
</div>
</div>
<small><i>Code Review Run #ed1edd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/test_chart_utils.py:
##########
@@ -2497,3 +2498,630 @@ 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"] == "SUM(sales)"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ 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"] == "SUM(sales)"
+ assert form_data["x_axis_sort_asc"] is True
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ def test_non_temporal_sort_by_metric_with_custom_label(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM", label="Total Sales")],
+ kind="bar",
+ sort_by="sales",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "Total Sales"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+
+ def test_non_temporal_sort_by_saved_metric(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="total_revenue", saved_metric=True)],
+ kind="bar",
+ sort_by="total_revenue",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "total_revenue"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+
+ 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 "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ def test_non_temporal_sort_by_x_axis_case_insensitive(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="Category", label="ProductCategory"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by="category",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+ assert form_data["x_axis_sort"] == "ProductCategory"
+
+ form_data2: dict[str, Any] = {}
+ config2 = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="Category", label="ProductCategory"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by="productcategory",
+ )
+ add_xy_sort_config(form_data2, config2, x_is_temporal=False)
+ assert form_data2["x_axis_sort"] == "ProductCategory"
+
+ def test_non_temporal_sort_by_sql_expression(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[
+ ColumnRef(
+ sql_expression="COUNT(DISTINCT user_id)",
label="unique_users"
+ )
+ ],
+ kind="bar",
+ sort_by="count(distinct user_id)",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+ assert form_data["x_axis_sort"] == "unique_users"
+
+ def test_non_temporal_sort_by_pair_format(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", True],
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+ assert form_data["x_axis_sort"] == "SUM(sales)"
+ assert form_data["x_axis_sort_asc"] 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"] == "SUM(revenue)"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ @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 "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ @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(
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test docstrings</b></div>
<div id="fix">
All four new tests in `TestMapXYConfigWithSortBy` lack docstrings, unlike
sibling tests such as `test_map_xy_config_with_filters` which document intent
inline. Repo rule 12148 requires a docstring on every newly added test
function. Adding one line per test keeps the file consistent and makes the
sort_by scenario under test explicit.
</div>
</div>
<div id="suggestion">
<div id="issue"><b>Missing mock/return type hints</b></div>
<div id="fix">
The new tests leave `mock_is_temporal` unannotated and three of the four
omit `-> None` (only test_map_xy_config_non_temporal_sort_by_metric has it).
Repo rules 12787/12101 require mock and return type annotations; `MagicMock` is
already imported in this file, so the fix is mechanical.
</div>
</div>
<small><i>Code Review Run #ed1edd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/test_chart_schemas.py:
##########
@@ -1219,3 +1220,223 @@ def test_update_chart_identifier_chart_id_alias(self)
-> None:
def test_list_charts_select_columns_columns_alias(self) -> None:
req = ListChartsRequest.model_validate({"columns": ["id",
"slice_name"]})
assert req.select_columns == ["id", "slice_name"]
+
+
+class TestXYChartConfigSortBy:
+ """Test sort_by options, coercions, and aliases in XYChartConfig."""
+
+ def test_sort_by_default_none(self) -> None:
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ )
+ assert config.sort_by is None
+
+ def test_sort_by_bare_string_coerced(self) -> None:
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ sort_by="sales",
+ )
+ assert isinstance(config.sort_by, SortByConfig)
+ assert config.sort_by.column == "sales"
+ assert config.sort_by.ascending is False
+
+ def test_sort_by_single_item_string_list_coerced(self) -> None:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated test method logic</b></div>
<div id="fix">
A syntactic duplication was detected in
tests/unit_tests/mcp_service/chart/test_chart_schemas.py. The same 12-line
block appears at lines 1247-1258 and 1362-1373. Consider refactoring to a
shared helper or parameterized test to reduce maintenance overhead.
</div>
</div>
<small><i>Code Review Run #ed1edd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/test_chart_utils.py:
##########
@@ -2497,3 +2498,630 @@ 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"] == "SUM(sales)"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ 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"] == "SUM(sales)"
+ assert form_data["x_axis_sort_asc"] is True
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ def test_non_temporal_sort_by_metric_with_custom_label(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM", label="Total Sales")],
+ kind="bar",
+ sort_by="sales",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "Total Sales"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+
+ def test_non_temporal_sort_by_saved_metric(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="total_revenue", saved_metric=True)],
+ kind="bar",
+ sort_by="total_revenue",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "total_revenue"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+
+ 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 "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ def test_non_temporal_sort_by_x_axis_case_insensitive(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="Category", label="ProductCategory"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by="category",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+ assert form_data["x_axis_sort"] == "ProductCategory"
+
+ form_data2: dict[str, Any] = {}
+ config2 = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="Category", label="ProductCategory"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by="productcategory",
+ )
+ add_xy_sort_config(form_data2, config2, x_is_temporal=False)
+ assert form_data2["x_axis_sort"] == "ProductCategory"
+
+ def test_non_temporal_sort_by_sql_expression(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[
+ ColumnRef(
+ sql_expression="COUNT(DISTINCT user_id)",
label="unique_users"
+ )
+ ],
+ kind="bar",
+ sort_by="count(distinct user_id)",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+ assert form_data["x_axis_sort"] == "unique_users"
+
+ def test_non_temporal_sort_by_pair_format(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", True],
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+ assert form_data["x_axis_sort"] == "SUM(sales)"
+ assert form_data["x_axis_sort_asc"] 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"] == "SUM(revenue)"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ @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 "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ @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 form_data["x_axis_sort_series_ascending"] is True
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete default-path assertions</b></div>
<div id="fix">
test_map_xy_config_without_sort_by_keeps_defaults only asserts
`x_axis_sort_series_ascending is True`, but the default path it names also sets
`x_axis_sort_series_type = "name"` in `configure_temporal_handling` and must
leave `x_axis_sort` unset (no `sort_by`). Asserting all three would catch
regressions where `add_xy_sort_config` or the temporal handling defaults drift.
</div>
</div>
<small><i>Code Review Run #ed1edd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -701,6 +723,11 @@ def _build_single_query_dict(
# an unordered result (dropping the heaviest rows rather than the top-N).
if form_data.get("sort_by_metric") and metrics and not qd.get("orderby"):
qd["orderby"] = [(metrics[0], False)]
+ elif form_data.get("x_axis_sort") and not qd.get("orderby"):
+ sort_col = form_data["x_axis_sort"]
+ sort_asc = bool(form_data.get("x_axis_sort_asc", False))
+ sort_target = _resolve_x_axis_sort_target(sort_col, metrics)
+ qd["orderby"] = [(sort_target, sort_asc)]
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Temporal sort guard bypassed</b></div>
<div id="fix">
`add_xy_sort_config` (chart_utils.py) deliberately refuses to set
`x_axis_sort` when the x-axis is temporal and warns that temporal charts sort
chronologically. This new branch applies `x_axis_sort` unconditionally for
every viz type built here, including `echarts_timeseries`/`mixed_timeseries`
(and the secondary query via `_build_mixed_timeseries_secondary`), so saved
params carrying `x_axis_sort` flip temporal charts from chronological to value
ordering. Guard temporal viz types before setting `orderby`.
</div>
</div>
<small><i>Code Review Run #ed1edd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
