aminghadersohi commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4123780696
##########
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:
Fixed in 19c44fe93255392e98ae36f2e5ad0ad5a7d0dc49. Confirmed reachability
before changing code: `superset/charts/schemas.py:313-317` validates saved
`params` only as a JSON string; `chart/tool/get_chart_preview.py:458-467` loads
those params directly, without the typed ChartConfig validation used for chart
creation, and line 496 passes the same form_data to plugin previews.
`chart_helpers.build_query_dicts_from_form_data` dispatches those native params
to plugin query builders. Extracted one shared `normalize_groupby`, also reused
by `resolve_groupby`, without introducing alias fallback into these paths.
Waterfall query columns and ordering preserve the complete breakdown name. The
three scalar regression cases failed before the fix and pass afterward;
list-valued controls also pass. This demonstrates a chart correctness bug, not
a demonstrated role/capability boundary violation.
Validation: 802 focused unit tests passed; pre-commit for all 8 touched
files passed, including mypy. Branch-wide pre-commit was also run: frontend
checks were blocked by missing glob, postcss-styled-syntax and tscw-config
dependencies.
Porting note for the coordinator: the same affected code exists on #44746
(`fork/mcp-chart-plugin-contract`, inspected at 21f20f09ec). Please port this
change there; this session only pushed #44148.
##########
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:
Fixed in 19c44fe93255392e98ae36f2e5ad0ad5a7d0dc49. Confirmed reachability
before changing code: `superset/charts/schemas.py:313-317` validates saved
`params` only as a JSON string; `chart/tool/get_chart_preview.py:458-467` loads
those params directly, without the typed ChartConfig validation used for chart
creation, and line 496 passes the same form_data to plugin previews.
`chart_helpers.build_query_dicts_from_form_data` dispatches those native params
to plugin query builders. Extracted one shared `normalize_groupby`, also reused
by `resolve_groupby`, without introducing alias fallback into these paths.
Funnel binds the complete stage name. The three scalar regression cases failed
before the fix and pass afterward; list-valued controls also pass. This
demonstrates a chart correctness bug, not a demonstrated role/capability
boundary violation.
Validation: 802 focused unit tests passed; pre-commit for all 8 touched
files passed, including mypy. Branch-wide pre-commit was also run: frontend
checks were blocked by missing glob, postcss-styled-syntax and tscw-config
dependencies.
Porting note for the coordinator: the same affected code exists on #44746
(`fork/mcp-chart-plugin-contract`, inspected at 21f20f09ec). Please port this
change there; this session only pushed #44148.
##########
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:
Fixed in 19c44fe93255392e98ae36f2e5ad0ad5a7d0dc49. Confirmed reachability
before changing code: `superset/charts/schemas.py:313-317` validates saved
`params` only as a JSON string; `chart/tool/get_chart_preview.py:458-467` loads
those params directly, without the typed ChartConfig validation used for chart
creation, and line 496 passes the same form_data to plugin previews.
`chart_helpers.build_query_dicts_from_form_data` dispatches those native params
to plugin query builders. Extracted one shared `normalize_groupby`, also reused
by `resolve_groupby`, without introducing alias fallback into these paths.
Funnel binds the complete stage name. The three scalar regression cases failed
before the fix and pass afterward; list-valued controls also pass. This
demonstrates a chart correctness bug, not a demonstrated role/capability
boundary violation.
Validation: 802 focused unit tests passed; pre-commit for all 8 touched
files passed, including mypy. Branch-wide pre-commit was also run: frontend
checks were blocked by missing glob, postcss-styled-syntax and tscw-config
dependencies.
Porting note for the coordinator: the same affected code exists on #44746
(`fork/mcp-chart-plugin-contract`, inspected at 21f20f09ec). Please port this
change there; this session only pushed #44148.
##########
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:
Fixed in 19c44fe93255392e98ae36f2e5ad0ad5a7d0dc49. Extracted
`capped_compile_row_limit` in `chart/plugin.py` and delegated both Gauge and
Treemap compile limits to it, preserving the existing cap/fallback behavior.
The existing parameterized `test_compile_row_limit_handles_persisted_values`
covers both plugins and malformed, nonpositive, capped and valid saved limits.
This is a small behavior-preserving refactor, not a security fix.
Validation: 802 focused unit tests passed; pre-commit for all 8 touched
files passed, including mypy. Branch-wide pre-commit was also run: frontend
checks were blocked by missing glob, postcss-styled-syntax and tscw-config
dependencies.
Porting note for the coordinator: the same affected code exists on #44746
(`fork/mcp-chart-plugin-contract`, inspected at 21f20f09ec). Please port this
change there; this session only pushed #44148.
--
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]
