aminghadersohi commented on code in PR #37639:
URL: https://github.com/apache/superset/pull/37639#discussion_r2770383086
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -178,6 +178,17 @@ async def get_chart_data( # noqa: C901
metrics = form_data.get("metrics", [])
groupby_columns = form_data.get("groupby", [])
+ # Build query columns list: include both x_axis and groupby
+ x_axis_config = form_data.get("x_axis")
+ query_columns = groupby_columns.copy()
Review Comment:
Fixed in 53089aa — using `or []` pattern across all three locations.
##########
superset/mcp_service/chart/preview_utils.py:
##########
@@ -36,6 +36,23 @@
logger = logging.getLogger(__name__)
+def _build_query_columns(form_data: Dict[str, Any]) -> list[str]:
+ """Build query columns list from form_data, including both x_axis and
groupby."""
+ x_axis_config = form_data.get("x_axis")
+ groupby_columns: list[str] = form_data.get("groupby", [])
+ raw_columns: list[str] = form_data.get("columns", [])
+
+ columns = raw_columns.copy() if "columns" in form_data else
groupby_columns.copy()
+ if x_axis_config and isinstance(x_axis_config, str):
+ if x_axis_config not in columns:
+ columns.insert(0, x_axis_config)
+ elif x_axis_config and isinstance(x_axis_config, dict):
+ col_name = x_axis_config.get("column_name")
+ if col_name and col_name not in columns:
+ columns.insert(0, col_name)
Review Comment:
Not applicable — in the MCP service, `x_axis` is always set as a string by
`map_xy_config()`. The dict handling is a defensive fallback matching the
existing `ASCIIPreviewStrategy` pattern. Adhoc/expression x-axes aren't part of
the MCP chart generation flow.
##########
superset/mcp_service/chart/preview_utils.py:
##########
@@ -36,6 +36,23 @@
logger = logging.getLogger(__name__)
+def _build_query_columns(form_data: Dict[str, Any]) -> list[str]:
+ """Build query columns list from form_data, including both x_axis and
groupby."""
+ x_axis_config = form_data.get("x_axis")
+ groupby_columns: list[str] = form_data.get("groupby", [])
Review Comment:
Good catch — fixed in 53089aa. Using `or []` instead of `.get(key, [])` to
handle explicit null values.
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -56,6 +56,22 @@ class ChartLike(Protocol):
uuid: Any
+def _build_query_columns(form_data: Dict[str, Any]) -> list[str]:
+ """Build query columns list from form_data, including both x_axis and
groupby."""
+ x_axis_config = form_data.get("x_axis")
+ groupby_columns: list[str] = form_data.get("groupby", [])
+
+ columns = groupby_columns.copy()
+ if x_axis_config and isinstance(x_axis_config, str):
+ if x_axis_config not in columns:
+ columns.insert(0, x_axis_config)
+ elif x_axis_config and isinstance(x_axis_config, dict):
+ col_name = x_axis_config.get("column_name")
+ if col_name and col_name not in columns:
+ columns.insert(0, col_name)
Review Comment:
Not applicable — `x_axis` is always a string in MCP-generated charts. The
dict fallback matches the existing `ASCIIPreviewStrategy` pattern.
##########
superset/mcp_service/chart/preview_utils.py:
##########
@@ -36,6 +36,23 @@
logger = logging.getLogger(__name__)
+def _build_query_columns(form_data: Dict[str, Any]) -> list[str]:
+ """Build query columns list from form_data, including both x_axis and
groupby."""
+ x_axis_config = form_data.get("x_axis")
+ groupby_columns: list[str] = form_data.get("groupby", [])
+ raw_columns: list[str] = form_data.get("columns", [])
Review Comment:
Fixed in 53089aa alongside the groupby null guard.
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -56,6 +56,22 @@ class ChartLike(Protocol):
uuid: Any
+def _build_query_columns(form_data: Dict[str, Any]) -> list[str]:
+ """Build query columns list from form_data, including both x_axis and
groupby."""
+ x_axis_config = form_data.get("x_axis")
+ groupby_columns: list[str] = form_data.get("groupby", [])
Review Comment:
Fixed in 53089aa — using `or []` pattern.
--
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]
