aminghadersohi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4113849706
##########
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:
Saved multi-series charts store a series-sort type here (`sum`, `name`,
`max`…), which becomes `orderby=[("sum", …)]`; `get_chart_preview` then fails
with `Unknown column used in orderby: sum` (fine at the merge base). Gate on
what `sortOperator` can sort by:
```suggestion
elif (
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_target = _resolve_x_axis_sort_target(sort_col, metrics)
if sort_target in metrics or sort_target in columns:
sort_asc = bool(form_data.get("x_axis_sort_asc", False))
qd["orderby"] = [(sort_target, sort_asc)]
```
##########
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
+
Review Comment:
With `group_by`, `sortOperator` skips the sort (`isEmpty(groupby)`), so
`sort_by` is a silent no-op in Explore while the MCP query still orders by it.
Warn like the temporal case:
```suggestion
if form_data.get("groupby"):
form_data.setdefault("_mcp_warnings", []).append(
f"sort_by='{sort_entry.column}' was ignored because group_by is "
"set; the x-axis sort applies only to single-series charts."
)
return
```
##########
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
+ else:
+ # Match against y metrics (by column name, metric label, agg expr, or
sql)
+ matched_label = _match_y_metric_label(config.y, sort_lower)
+ sort_target = matched_label or sort_entry.column
Review Comment:
A saved metric or column not in `y` lands in `x_axis_sort`, but
`sortOperator` only sorts by the x-axis or a query metric label, so the saved
chart stays unsorted while the MCP query orders by it (`ORDER BY profit` on a
grouped query). Warn and skip like the temporal case:
```suggestion
matched_label = _match_y_metric_label(config.y, sort_lower)
if not matched_label:
form_data.setdefault("_mcp_warnings", []).append(
f"sort_by='{sort_entry.column}' was ignored: XY charts can "
"only sort by the x-axis or a y-axis metric."
)
return
sort_target = matched_label
```
--
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]
