bito-code-review[bot] commented on code in PR #44746:
URL: https://github.com/apache/superset/pull/44746#discussion_r4121503906
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Missing tooltip_metrics case</b></div>
<div id="fix">
The parametrize set covers `tooltip_columns` overflow but omits
`tooltip_metrics`, which `resolve_gantt_query_fields` validates identically
(`Gantt tooltip_metrics must contain at most 50 entries`,
chart_helpers.py:626-627). No test in this file exercises `tooltip_metrics`
(grep: 0 matches). Adding one parametrize case closes the gap in the
docstring's promised "malformed cached Gantt roles" coverage.
</div>
</div>
<small><i>Code Review Run #bd88ea</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Inconsistent mock pattern</b></div>
<div id="fix">
`resolve_datasource_engine` is patched with `patch(...,
return_value="sqlite")`, but its real signature takes `(datasource_id,
datasource_type)` (chart_helpers.py:180). A MagicMock swallows any call, so a
future signature change would go undetected here. Sibling tests in this file
use `monkeypatch.setattr` with lambdas (lines 572-575, 2474-2477); following
that pattern keeps mocking consistent and signature-sensitive.
</div>
</div>
<details>
<summary><b>Citations</b></summary>
<ul>
<li>
Rule Violated: <a
href="https://github.com/apache/superset/blob/49b2e0a/.cursor/rules/dev-standard.mdc#L119";>dev-standard.mdc:119</a>
</li>
</ul>
</details>
<small><i>Code Review Run #bd88ea</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Magic numbers encode limit policy</b></div>
<div id="fix">
The changed lines hardcode the business policy three times: `or 10`
(fallback for absent/falsy `row_limit`), `return 10` (fallback for unparseable
values), and `min(10, ...)` (cap). The cap and the fallback coincidentally
share the value 10, but nothing distinguishes 'default when unset' from
'maximum allowed for the compile check', so a future change to one (e.g.
raising the cap) will likely miss the other. The sibling `preview_row_limit`
(lines 224-230) hardcodes 100/10000 the same way. Please extract named
constants for the fallback and the cap.
</div>
</div>
<small><i>Code Review Run #bd88ea</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Unguarded ValueError from get_column_name</b></div>
<div id="fix">
`generate_funnel_vega_lite_preview` calls `get_column_name(groupby[0])`
unguarded. Per `superset/utils/core.py:1391`, `get_column_name` raises
`ValueError("Missing label")` for any entry that is not a str, dict with
label/sqlExpression, or object with column_name. `groupby` comes straight from
`form_data` (line 1303) with no shape validation, unlike the gauge path (lines
816-829) which validates each entry. A malformed stage entry raises ValueError
instead of returning the `ChartError` contract; the caller's except tuple does
catch ValueError, but the failure is then mislabeled `VegaLiteGenerationError`
rather than `InvalidFormData`.
</div>
</div>
<small><i>Code Review Run #bd88ea</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
