bito-code-review[bot] commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4123521468
##########
superset/mcp_service/chart/plugins/waterfall.py:
##########
@@ -184,3 +184,29 @@ def schema_error_hint(self) -> ChartGenerationError | None:
],
error_code="WATERFALL_VALIDATION_ERROR",
)
+
+ 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 (
+ build_single_query_dict,
+ resolve_shared_metrics,
+ )
+
+ # Match Waterfall buildQuery: the x-axis category (or legacy time
+ # column) plus breakdown, ordered by those columns so the running total
+ # and grand total follow the axis.
+ axis = form_data.get("x_axis") or form_data.get("granularity_sqla")
+ columns = list(axis) if isinstance(axis, list) else [axis] if axis
else []
+ columns.extend(form_data.get("groupby") or [])
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-20: String groupby char-split</b></div>
<div id="fix">
`form_data.get("groupby")` can be a scalar string: native waterfall charts
save the single-value `groupby` control as a string (frontend `buildQuery`
wraps it with `ensureIsArray`), and the shared `resolve_groupby` guards
`isinstance(raw_groupby, str)`. `columns.extend(...)` on a string appends
individual characters as columns, corrupting `columns` and `orderby`. Wrap
scalars: `columns.extend([groupby] if isinstance(groupby, str) else groupby)`.
([CWE-20](https://cwe.mitre.org/data/definitions/20.html))
</div>
</div>
<small><i>Code Review Run #c99270</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])
+ return VegaLitePreview(
+ type="vega_lite",
+ specification={
+ "$schema": "https://vega.github.io/schema/vega-lite/v5.json";,
+ "data": {"values": data},
+ "mark": "bar",
+ "width": "container",
+ "height": 400,
+ "encoding": {
+ "y": {"field": stage, "type": "nominal", "sort": None},
+ "x": {"field": metric, "type": "quantitative"},
+ "tooltip": [
+ {"field": stage, "type": "nominal"},
+ {"field": metric, "type": "quantitative"},
+ ],
+ },
+ },
+ supports_streaming=False,
+ )
+
+
+def generate_histogram_vega_lite_preview(
+ data: list[dict[str, Any]], form_data: dict[str, Any]
+) -> VegaLitePreview:
+ """Render histogram operator output without re-binning its counts."""
+ from superset.utils.core import get_column_name
+
+ groupby = [get_column_name(column) for column in form_data.get("groupby")
or []]
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>String groupby corrupts histogram bins</b></div>
<div id="fix">
Same string-groupby hole as the funnel path: iterating a bare-string
`groupby` yields characters, so `get_column_name` maps each char, the real
stage column leaks into `bins` and is charted as a bin, and `series` collapses
to `All`. Siblings (`preview_utils.py:817`, `plugins/histogram.py:196`)
normalize `isinstance(raw_groupby, str)` first; do the same here.
</div>
</div>
<small><i>Code Review Run #c99270</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 []
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>String groupby misbinds funnel stage</b></div>
<div id="fix">
`form_data.get("groupby")` can be a bare string: sibling paths normalize it
(`preview_utils.py:817-818` gauge validation and `plugins/histogram.py:196`
both wrap `isinstance(raw_groupby, str)`). Here `groupby[0]` on a string yields
its first character, so `stage` binds the y field to a nonexistent column and
the funnel renders empty. Normalize before indexing.
</div>
</div>
<small><i>Code Review Run #c99270</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/gauge.py:
##########
@@ -193,3 +203,55 @@ def schema_error_hint(self) -> ChartGenerationError | None:
],
error_code="GAUGE_VALIDATION_ERROR",
)
+
+ def normalize_query_result(self, result: Any, form_data: Mapping[str,
Any]) -> Any:
+ from superset.mcp_service.chart.query_result import (
+ normalize_gauge_query_result,
+ )
+
+ return normalize_gauge_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>Duplicated row-limit logic</b></div>
<div id="fix">
This `compile_row_limit` body is byte-identical to the one added in
`treemap.py` (lines 217-222). Both plugins now duplicate the same cap/fallback
logic; if one cap changes (e.g. a different max) the other silently diverges.
Consider a shared helper in `plugin.py` next to the base `compile_row_limit`.
</div>
</div>
<small><i>Code Review Run #c99270</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>Duplicated row-limit logic</b></div>
<div id="fix">
This `compile_row_limit` is a verbatim duplicate of the one added in
`gauge.py` (214-219). The shared cap/fallback should be a single helper (e.g.
in `plugin.py`) called by both plugins so the two copies cannot drift apart.
</div>
</div>
<small><i>Code Review Run #c99270</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]
