gabotorresruiz commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4170681259
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -42,6 +43,149 @@
logger = logging.getLogger(__name__)
+def _match_y_metric_name(raw_lower: str, y_configs: list[dict[str, Any]]) ->
str | None:
+ for y_col in y_configs:
+ if y_col.get("label") and y_col["label"].lower() == raw_lower:
+ return y_col["label"]
+ if y_col.get("name") and y_col["name"].lower() == raw_lower:
+ return y_col.get("label") or y_col["name"]
+ if y_col.get("sql_expression") and y_col["sql_expression"].lower() ==
raw_lower:
+ return y_col.get("label") or y_col["sql_expression"]
+ agg = y_col.get("aggregate")
+ name = y_col.get("name")
+ if agg and name and f"{agg}({name})".lower() == raw_lower:
+ return y_col.get("label") or f"{agg.upper()}({name})"
+ return None
+
+
+def _resolve_xy_sort_name_and_metric_status(
+ raw_name: str, config_dict: dict[str, Any], dataset_context: Any
+) -> tuple[str, bool]:
+ raw_lower = raw_name.lower()
+ if matched_y := _match_y_metric_name(raw_lower, config_dict.get("y") or
[]):
+ return matched_y, False
+
+ x_col = config_dict.get("x")
+ if isinstance(x_col, dict):
+ if x_col.get("label") and x_col["label"].lower() == raw_lower:
+ return x_col["label"], False
+ if x_col.get("name") and x_col["name"].lower() == raw_lower:
+ return (
+ DatasetValidator.get_canonical_column_name(
+ x_col["name"], dataset_context
+ ),
+ False,
+ )
+
+ if dataset_context and getattr(dataset_context, "available_metrics", None):
+ for m in dataset_context.available_metrics:
Review Comment:
Not a blocker, but I do not think this branch can reach a working sort.
Probing `generate_chart` against a dataset whose saved metric `total_sales`
is not in `y`: `sort_by: "total_sales"` is rejected at dataset validation with
`saved_metric_not_marked`, because `extract_column_refs` reads the flag off the
incoming config (`saved_metric` is still `None` there) while
`_normalize_xy_sort_by` only sets it in the normalization layer, which runs
after validation. Taking the error's advice and sending `sort_by: {"column":
"total_sales", "saved_metric": true}` does pass validation, and then
`add_xy_sort_config` drops it with `sort_by='total_sales' was ignored: XY
charts can only sort by the x-axis or a y-axis metric.` So both spellings end
with no sort, and the error message points at the one that warns.
The contract itself looks right to me, since `sortOperator` can only sort by
the x-axis label or a label that is in the query's `metrics`. But that means
this `available_metrics` lookup and the `saved_metric=True` ref it produces
only ever buy a warning. Would it be simpler to drop them and have the
validator say the target must be the x column or a `y` metric? Or is there a
path where an independent saved metric does sort that I am missing?
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1268,6 +1268,111 @@ 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."""
+ aggregate_aliases = {"STDDEV": "STDDEV_SAMP", "VAR": "VAR_SAMP"}
+ 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 = y_col.aggregate or ""
+ norm_agg = aggregate_aliases.get(agg.upper(), agg.upper())
+ 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()
+
+ agg_matches: set[str] = set()
+ if agg and y_col.name:
+ agg_matches.add(f"{agg}({y_col.name})".lower())
+ if norm_agg and y_col.name:
+ agg_matches.add(f"{norm_agg}({y_col.name})".lower())
+
+ if (
+ sort_lower == col_name
+ or sort_lower == metric_label_lower
+ or (col_label and sort_lower == col_label)
+ or (sort_lower in agg_matches)
+ 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 ``group_by`` is set: records a warning and ignores ``sort_by`` because
+ x-axis sort applies only to single-series charts in Superset.
+ - If the sort target does not match the x-axis column or a y-axis metric:
+ records a warning and does not apply ``x_axis_sort``.
+ - If valid and 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 not isinstance(sort_entry, SortByConfig):
+ from superset.mcp_service.chart.schemas import _coerce_sort_item
+
+ coerced = _coerce_sort_item(sort_entry)
+ if not isinstance(coerced, SortByConfig):
+ return
+ sort_entry = coerced
+
+ 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
+
+ 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
+
+ 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.name or config.x.label
+ else:
+ # Match against y metrics (by column name, metric label, agg expr, or
sql)
+ 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."
Review Comment:
Worth knowing rather than a change request: these three warnings only reach
the caller from `generate_chart`, which pops `_mcp_warnings` into the response
at `superset/mcp_service/chart/tool/generate_chart.py:333`. `update_chart` and
`update_chart_preview` discard them instead (`update_chart.py:376` and
`update_chart_preview.py:241`), so `update_chart` on a chart that already has a
`group_by` accepts `sort_by`, applies nothing and says nothing.
The discard predates this PR, but these are the first `_mcp_warnings` a user
would actually act on, so it may be worth carrying them through on the update
path as well.
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2623,6 +2642,32 @@ class XYChartConfig(BaseChartConfig):
ge=1,
le=10000,
)
+ sort_by: SortByConfig | str | List[SortByConfig | str] | None = Field(
Review Comment:
Just a small NIT on the annotation: the description promises `[column,
ascending]` and `coerce_sort_by` handles it two lines below, but the list
member serializes as `items: {anyOf: [string, SortByConfig]}`, so the pair form
is not valid against the schema the tool advertises. I ran
`XYChartConfig.model_json_schema()` through `Draft202012Validator`: `["amount",
true]` fails the schema while pydantic accepts it, so a client that validates
against `inputSchema` cannot send the form the description recommends.
A `Tuple[str, bool]` member in the union would line the two up, and
`max_length=1` on the list member would put the single-sort-column rule in the
schema too.
--
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]
