aminghadersohi commented on code in PR #44746:
URL: https://github.com/apache/superset/pull/44746#discussion_r4121933579
##########
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:
Retaining this local style:
superset/mcp_service/chart/plugins/treemap.py:217-230 keeps each policy
entirely within its named method. Compile fallback and cap are both
intentionally 10; preview default 100 and accepted range 1–10000 are separate.
Extracting constants would not change behavior or fix an incorrect limit, so I
am declining this optional style refactor.
##########
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:
Fixed in 21f20f09ec5c8f8beda3ba7745d38bf39946d209: added the 51-entry
tooltip_metrics case to test_malformed_gantt_query_returns_validation_error. It
asserts ValidationError and the field name. Removing the tooltip_metrics
validation guard makes this new case fail (the other five pass); restoring it
passes. Both touched test files: 180 passed.
##########
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:
Retaining the scoped patch:
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:3510-3529 tests
malformed Gantt validation, not datasource-engine signature compatibility. The
caller at superset/mcp_service/chart/chart_helpers.py:826 passes both
datasource_id and datasource_type, matching the definition at :180. A fixed
SQLite return deliberately isolates engine lookup; changing to
monkeypatch/lambda is optional style, not a correction to this path.
##########
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:
Fixed in 21f20f09ec5c8f8beda3ba7745d38bf39946d209: catch get_column_name's
ValueError and return InvalidFormData for an unresolvable funnel stage. Four
malformed-stage regression cases failed before and pass after; three valid
string/label/sqlExpression cases preserve labels. Both touched test files: 180
passed. Pre-commit passed across all branch-changed files, including mypy.
--
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]
