bito-code-review[bot] commented on code in PR #44746:
URL: https://github.com/apache/superset/pull/44746#discussion_r4121935445
##########
superset/mcp_service/chart/plugins/treemap.py:
##########
@@ -150,3 +162,114 @@ def schema_error_hint(self) -> ChartGenerationError |
None:
],
error_code="TREEMAP_VALIDATION_ERROR",
)
+
+ def resolve_query_fields(
+ self, form_data: Mapping[str, Any], viz_type: str
+ ) -> tuple[list[Any], list[Any]] | None:
+ # Treemap has exactly these roles; stale controls from another plugin
+ # must not override its singular metric or ordered hierarchy.
+ metric = form_data.get("metric")
+ hierarchy = form_data.get("groupby") or []
+ return ([metric] if metric else []), (
+ [hierarchy] if isinstance(hierarchy, str) else list(hierarchy)
+ )
+
+ def build_query_dicts(
+ self,
+ form_data: dict[str, Any],
+ *,
+ viz_type: str,
+ engine: str,
+ row_limit: int | None,
+ order_desc: bool | None,
+ ) -> list[dict[str, Any]] | None:
+ from superset.mcp_service.chart.chart_helpers import (
+ apply_treemap_query_fields,
+ build_single_query_dict,
+ )
+
+ metrics, hierarchy = cast(
+ tuple[list[Any], list[Any]],
+ self.resolve_query_fields(form_data, viz_type),
+ )
+ query = build_single_query_dict(
+ form_data,
+ hierarchy,
+ metrics,
+ row_limit=row_limit,
+ order_desc=order_desc,
+ )
+ apply_treemap_query_fields(
+ query,
+ form_data,
+ hierarchy,
+ row_limit if row_limit is not None else form_data.get("row_limit"),
+ )
+ return [query]
+
+ def normalize_query_result(self, result: Any, form_data: Mapping[str,
Any]) -> Any:
+ from superset.mcp_service.chart.query_result import (
+ normalize_treemap_query_result,
+ )
+
+ return normalize_treemap_query_result(result, form_data)
+
+ def compile_row_limit(self, form_data: Mapping[str, Any]) -> int:
+ try:
+ limit = int(form_data.get("row_limit") or 10)
+ except (TypeError, ValueError, OverflowError):
+ return 10
+ return min(10, limit) if limit > 0 else 10
Review Comment:
<!-- Bito Reply -->
The decision to keep the row limit logic localized within the method is
reasonable, as it maintains clear ownership of the policy for this specific
plugin. Since the fallback and cap values are intentionally distinct for
different contexts, extracting them into constants would not necessarily
improve clarity or prevent errors in this case. Your approach of keeping the
policy encapsulated within the method is a valid design choice.
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:
##########
@@ -3488,3 +3488,40 @@ def run(self) -> dict[str, Any]:
await client.call_tool("get_chart_data", {"request":
{"identifier": "11"}})
assert captured["order_desc"] is False
+
+
[email protected]
[email protected](
+ "invalid_fields, message",
+ [
+ ({"start_time": None}, "start_time"),
+ ({"end_time": None}, "end_time"),
+ ({"y_axis": None}, "y_axis"),
+ ({"tooltip_columns": ["task"] * 51}, "tooltip_columns"),
+ ({"order_by_cols": [["start", "yes"]]}, "ascending_boolean"),
+ ],
Review Comment:
<!-- Bito Reply -->
The suggestion provided by the reviewer is appropriate. It correctly
identifies that the `tooltip_metrics` field was missing validation coverage in
the test suite, mirroring the existing validation logic in the application
code. Adding this test case ensures that the `tooltip_metrics` field is
properly validated, consistent with the other fields in the `parametrize` set.
**tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py**
```
{"tooltip_columns": ["task"] * 51}, "tooltip_columns"),
{"tooltip_metrics": ["metric"] * 51}, "tooltip_metrics"),
{"order_by_cols": [["start", "yes"]]}, "ascending_boolean"),
```
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:
##########
@@ -3488,3 +3488,40 @@ def run(self) -> dict[str, Any]:
await client.call_tool("get_chart_data", {"request":
{"identifier": "11"}})
assert captured["order_desc"] is False
+
+
[email protected]
[email protected](
+ "invalid_fields, message",
+ [
+ ({"start_time": None}, "start_time"),
+ ({"end_time": None}, "end_time"),
+ ({"y_axis": None}, "y_axis"),
+ ({"tooltip_columns": ["task"] * 51}, "tooltip_columns"),
+ ({"order_by_cols": [["start", "yes"]]}, "ascending_boolean"),
+ ],
+)
+async def test_malformed_gantt_query_returns_validation_error(
+ invalid_fields: dict[str, Any],
+ message: str,
+) -> None:
+ """Reject malformed cached Gantt roles without reporting an internal
failure."""
+ with patch(
+ "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine",
+ return_value="sqlite",
+ ):
Review Comment:
<!-- Bito Reply -->
The suggestion to use `monkeypatch.setattr` with a lambda is a valid
alternative that improves test robustness by ensuring the mock remains
sensitive to the function's signature. While the current `patch` approach is
functional for isolating the engine lookup, adopting the suggested pattern
aligns with existing test standards in the file and prevents potential silent
failures if the function signature changes in the future.
**tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py**
```
with patch(
"superset.mcp_service.chart.chart_helpers.resolve_datasource_engine",
return_value="sqlite",
):
```
##########
superset/mcp_service/chart/preview_utils.py:
##########
@@ -1257,19 +1275,129 @@ def generate_bubble_vega_lite_preview(
)
+# Native geometries the Vega-Lite adapter cannot represent faithfully.
+_UNSUPPORTED_VEGA_GEOMETRIES: frozenset[str] = frozenset(
+ {"sankey", "sankey_v2", "radar"}
+)
+
+
+def unsupported_vega_geometry(viz_type: str) -> ChartError | None:
+ """Reject native geometries the Vega-Lite adapter cannot represent."""
+ if viz_type not in _UNSUPPORTED_VEGA_GEOMETRIES:
+ return None
+ return ChartError(
+ error=(
+ f"Vega-Lite previews do not support {viz_type} geometry. "
+ "Use Explore for the native visualization or ASCII/table for data."
+ ),
+ error_type="UnsupportedFormat",
+ )
+
+
+def generate_funnel_vega_lite_preview(
+ data: list[dict[str, Any]], form_data: dict[str, Any]
+) -> VegaLitePreview | ChartError:
+ """Render funnel stages as horizontal value bars, preserving query
order."""
+ from superset.utils.core import get_column_name
+
+ groupby = form_data.get("groupby") or []
+ metric = metric_result_label(form_data.get("metric"))
+ if not groupby or not metric:
+ return ChartError(
+ error="Funnel requires a stage and metric",
error_type="InvalidFormData"
+ )
+ stage = get_column_name(groupby[0])
Review Comment:
<!-- Bito Reply -->
The suggestion provided by the reviewer is appropriate and improves the
code. It correctly identifies that `get_column_name` can raise a `ValueError`
when encountering malformed input, and wrapping this call ensures the function
adheres to the expected `ChartError` contract instead of allowing an unhandled
exception to propagate.
**superset/mcp_service/chart/preview_utils.py**
```
try:
stage = get_column_name(groupby[0])
except ValueError:
return ChartError(
error=f"Invalid funnel stage: {groupby[0]}",
error_type="InvalidFormData"
)
```
--
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]
