aminghadersohi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4231389169
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -1086,13 +1110,30 @@ def build_single_query_dict(
order_desc if order_desc is not None else
form_data.get("order_desc", True)
)
qd["orderby"] = [(sort_metric, not descending)]
+ elif (
+ not is_temporal
+ and form_data.get("x_axis_sort")
+ and not form_data.get("groupby")
+ 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)
+ col_labels = [
+ c if isinstance(c, str) else (c.get("label") or
c.get("column_name"))
+ for c in columns
+ if isinstance(c, (str, dict))
+ ]
+ if sort_target in metrics or sort_target in col_labels:
+ qd["orderby"] = [(sort_target, sort_asc)]
apply_form_data_filters_to_query(qd, form_data)
granularity = form_data.get("granularity",
form_data.get("granularity_sqla"))
if granularity is not None:
qd["granularity"] = granularity
return qd
+
Review Comment:
The merge left a third blank line here; `ruff-format` fails `pre-commit
(current)` on it.
```suggestion
```
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -1086,13 +1110,30 @@ def build_single_query_dict(
order_desc if order_desc is not None else
form_data.get("order_desc", True)
)
qd["orderby"] = [(sort_metric, not descending)]
+ elif (
Review Comment:
Unreachable since https://github.com/apache/superset/pull/43771: XY charts
no longer call `build_single_query_dict`, and its four plugin callers never set
`x_axis_sort` or `is_timeseries`. Consider dropping this branch,
`_resolve_x_axis_sort_target` and the new param.
##########
tests/unit_tests/mcp_service/chart/test_chart_helpers.py:
##########
@@ -1515,6 +1517,276 @@ def
test_shared_query_builder_keeps_mixed_timeseries_ordering_per_query(
assert form_data["orderby"] == [["count", True]]
+def test_build_single_query_dict_x_axis_sort_with_metric_label() -> None:
+ """
+ Verify build_single_query_dict maps x_axis_sort metric label to
+ query orderby.
+ """
+ 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:
+ """
+ Verify build_single_query_dict matches x_axis_sort column name to
+ metric dict.
+ """
+ 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:
+ """Verify build_single_query_dict handles saved string metric in
x_axis_sort."""
+ 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:
+ """Verify build_single_query_dict preserves dimension column sort."""
+ 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:
+ """Verify explicit orderby in form_data takes precedence over
x_axis_sort."""
+ 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_single_query_dict_temporal_chart_guards_against_x_axis_sort()
-> None:
+ """
+ Verify temporal charts do not override chronological order with
+ x_axis_sort.
+ """
+ metric = {
+ "label": "SUM(sales)",
+ "aggregate": "SUM",
+ "column": {"column_name": "sales"},
+ }
+ # Case 1: is_timeseries flag passed
+ form_data = {
+ "x_axis": "order_date",
+ "x_axis_sort": "SUM(sales)",
+ "x_axis_sort_asc": False,
+ }
+ qd = build_single_query_dict(
+ form_data, ["order_date"], [metric], is_timeseries=True
+ )
+ assert "orderby" not in qd
+
+ # Case 2: granularity_sqla present indicates temporal chart
+ form_data_temporal = {
+ "viz_type": "echarts_timeseries_line",
+ "x_axis": "order_date",
+ "granularity_sqla": "order_date",
+ "x_axis_sort": "SUM(sales)",
+ "x_axis_sort_asc": False,
+ }
+ qd_temporal = build_single_query_dict(form_data_temporal, ["order_date"],
[metric])
+ assert "orderby" not in qd_temporal
+
+ # Case 3: time_grain_sqla indicates temporal chart
+ form_data_grain = {
+ "viz_type": "echarts_timeseries_bar",
+ "x_axis": "order_date",
+ "granularity_sqla": "order_date",
+ "time_grain_sqla": "P1D",
+ "x_axis_sort": "SUM(sales)",
+ "x_axis_sort_asc": False,
+ }
+ qd_grain = build_single_query_dict(form_data_grain, ["order_date"],
[metric])
+ assert "orderby" not in qd_grain
+
+
+def test_build_query_dicts_from_form_data_xy_bar_with_x_axis_sort() -> None:
+ """
+ Verify build_query_dicts_from_form_data sets orderby for categorical
+ xy bar chart.
+ """
+ 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
+ col = queries[0]["columns"][0]
+ assert (
+ col if isinstance(col, str) else col.get("sqlExpression") or
col.get("label")
+ ) == "category"
+ assert queries[0]["metrics"] == [metric]
+ assert queries[0]["orderby"] == [[metric, False]]
+
+
+def test_resolve_x_axis_sort_target_case_insensitive_label() -> None:
+ """Verify _resolve_x_axis_sort_target matches metric label
case-insensitively."""
+ 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:
+ """
+ Verify _resolve_x_axis_sort_target matches metric column name
+ case-insensitively.
+ """
+ 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:
+ """Verify _resolve_x_axis_sort_target matches adhoc SQL expression
metric."""
+ 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:
+ """
+ Verify _resolve_x_axis_sort_target matches bare string metric
+ case-insensitively.
+ """
+ resolved = _resolve_x_axis_sort_target("totalsales", ["TotalSales"])
+ assert resolved == "TotalSales"
+
+
+def test_build_single_query_dict_x_axis_sort_ignores_unmatched_series_sort()
-> None:
+ """
+ Verify build_single_query_dict does not set orderby when x_axis_sort
+ contains a series-sort value (like 'sum' or 'max') that is neither
+ a metric nor a column, preventing 'Unknown column used in orderby: sum'.
+ """
+ metric = {
+ "label": "SUM(sales)",
+ "aggregate": "SUM",
+ "column": {"column_name": "sales"},
+ }
+ form_data = {
+ "x_axis_sort": "sum",
+ "x_axis_sort_asc": False,
+ }
+ qd = build_single_query_dict(form_data, ["category"], [metric])
+ assert "orderby" not in qd
+
+ # Also test with 'max'
+ form_data_max = {
+ "x_axis_sort": "max",
+ "x_axis_sort_asc": True,
+ }
+ qd_max = build_single_query_dict(form_data_max, ["category"], [metric])
+ assert "orderby" not in qd_max
+
+
+def test_build_query_dicts_from_form_data_xy_bar_explore_default_grain() ->
None:
+ """
+ Verify explore default time_grain_sqla ('P1D') on categorical bar chart
+ (without granularity_sqla) does not cause x_axis_sort to be dropped.
+ """
+ metric = {
+ "label": "SUM(sales)",
+ "aggregate": "SUM",
+ "column": {"column_name": "sales"},
+ }
+ form_data = {
+ "viz_type": "echarts_timeseries_bar",
+ "x_axis": "category",
+ "time_grain_sqla": "P1D",
+ "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)]
Review Comment:
Fails at this head: since https://github.com/apache/superset/pull/43771 XY
queries come from `form_data_query_context`, so columns are axis dicts and the
sort is a post-processing op (`orderby` stays metric-desc even when ascending).
```suggestion
col = queries[0]["columns"][0]
assert (col if isinstance(col, str) else col.get("label")) == "category"
assert queries[0]["metrics"] == [metric]
sort_op = {"operation": "sort", "options": {"ascending": False, "by":
"SUM(sales)"}}
assert sort_op in queries[0]["post_processing"]
```
--
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]
