bito-code-review[bot] commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4123783618
##########
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:
<!-- Bito Reply -->
The suggestion provided by the reviewer is correct and should be applied.
The current implementation of `columns.extend(form_data.get("groupby") or [])`
will incorrectly treat a scalar string as an iterable, appending each character
as an individual column. Wrapping the scalar value in a list, as suggested,
ensures that the groupby value is correctly treated as a single column entry.
**superset/mcp_service/chart/plugins/waterfall.py**
```
axis = form_data.get("x_axis") or form_data.get("granularity_sqla")
columns = list(axis) if isinstance(axis, list) else [axis] if axis
else []
groupby = form_data.get("groupby")
columns.extend([groupby] if isinstance(groupby, str) else (groupby
or []))
```
##########
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:
<!-- Bito Reply -->
The suggestion to normalize `groupby` before indexing is correct and should
be applied. In the current code, `form_data.get("groupby")` can be a bare
string, and indexing it directly (`groupby[0]`) would incorrectly bind only the
first character of that string as the stage name, leading to empty funnel
renders. Normalizing it ensures that `groupby` is treated as a list of columns,
which is the expected format for the funnel stage binding.
**superset/mcp_service/chart/preview_utils.py**
```
groupby = form_data.get("groupby") or []
if isinstance(groupby, str):
groupby = [groupby]
```
##########
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:
<!-- Bito Reply -->
The pull request has been updated to address the identified chart
correctness issues. The changes include:
* **Geographic Chart Rebinding:** Geographic configurations now use the
strict presentation-only rebind path, preventing the silent retention of old
filters when moving between datasets.
* **Geographic Result Validation:** The validation logic now correctly
accepts finite `Decimal` and real-number scalars before JSON conversion,
ensuring that `NUMERIC` aggregates do not fail with `INVALID_GEOGRAPHIC_RESULT`.
* **Vega-Lite Previews:** Added support for funnel and histogram Vega-Lite
previews, with specific handling for unsupported geometries and correct
metric/groupby resolution.
Regarding the porting request, please proceed with porting these changes to
the specified fork (`fork/mcp-chart-plugin-contract`, commit `21f20f09ec`) as
requested.
--
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]
