FrancescoCastaldi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4115334902
##########
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:
Applied in commit \c6d9ff\. Added check \if form_data.get(\groupby\):\ with
warning in \dd_xy_sort_config\, along with unit tests \
est_group_by_sort_by_ignored_with_warning\ and \
est_map_xy_config_with_group_by_ignores_sort_by\.
##########
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:
Applied in commit \c6d9ff\. If \sort_by\ targets a column not matching the
x-axis or a y-axis metric, \dd_xy_sort_config\ now warns and skips setting
\x_axis_sort\, covered by unit tests \
est_unmatched_sort_by_ignored_with_warning\ and \
est_map_xy_config_unmatched_sort_by_ignores_and_warns\.
--
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]
