bito-code-review[bot] commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4123787835
##########
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:
<!-- Bito Reply -->
The suggestion to extract the duplicated `compile_row_limit` logic into a
shared helper in `plugin.py` is appropriate and improves maintainability by
preventing drift between the Gauge and Treemap implementations. The user has
already addressed this in the latest commit (19c44fe) by performing this
refactor and delegating both plugins to the new `capped_compile_row_limit`
helper, while preserving the existing cap and fallback behavior.
##########
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:
<!-- Bito Reply -->
The reviewer's suggestion to consolidate the duplicated row-limit logic into
a shared helper is appropriate. Since both Gauge and Treemap plugins share the
same cap/fallback logic, moving this to a shared location in `plugin.py`
improves maintainability and prevents silent divergence if the cap requirements
change in the future.
--
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]
