FrancescoCastaldi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4115308064
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -699,8 +722,19 @@ def _build_single_query_dict(
# metric descending. buildQuery derives this on the frontend; translate
# the flag here when there is no explicit ordering or a row_limit truncates
# an unordered result (dropping the heaviest rows rather than the top-N).
+ viz_type = form_data.get("viz_type") or ""
+ is_temporal = (
+ is_timeseries
+ or viz_type == "mixed_timeseries"
+ or bool(form_data.get("granularity_sqla"))
+ )
if form_data.get("sort_by_metric") and metrics and not qd.get("orderby"):
qd["orderby"] = [(metrics[0], False)]
+ elif not is_temporal and 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:
Good catch. Updated `_build_single_query_dict` to check that `sort_target`
is a valid metric or column before adding it to `orderby`, avoiding invalid
series-sort aggregates from being propagated. Also added unit test
`test_build_single_query_dict_x_axis_sort_ignores_unmatched_series_sort` in
commit `a8ab8a7`.
##########
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 `bc6d9ff`. Added check `if form_data.get("groupby"):` with
warning in `add_xy_sort_config`, along with unit tests
`test_group_by_sort_by_ignored_with_warning` and
`test_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 `bc6d9ff`. If `sort_by` targets a column not matching the
x-axis or a y-axis metric, `add_xy_sort_config` now warns and skips setting
`x_axis_sort`, covered by unit tests
`test_unmatched_sort_by_ignored_with_warning` and
`test_map_xy_config_unmatched_sort_by_ignores_and_warns`.
##########
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:
Applied in commit `bc6d9ff`. Added `and not form_data.get("groupby")` and
validated `sort_target in metrics or sort_target in col_labels` in
`_build_single_query_dict`, covered by unit test
`test_build_single_query_dict_x_axis_sort_ignored_when_groupby_set`.
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -863,13 +898,19 @@ def build_query_dicts_from_form_data(
if x_axis_col and x_axis_col not in groupby:
groupby = [x_axis_col] + groupby
+ is_temporal = bool(
+ form_data.get("granularity_sqla")
+ or form_data.get("time_grain_sqla")
+ or viz_type == "mixed_timeseries"
+ )
Review Comment:
Applied in commit `bc6d9ff` (and `a8ab8a7`). Removed `time_grain_sqla` from
the temporal check so Explore-saved categorical bar charts preserve
`x_axis_sort`, covered by unit test
`test_build_query_dicts_from_form_data_xy_bar_explore_default_grain`.
--
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]
