aminghadersohi commented on code in PR #44152: URL: https://github.com/apache/superset/pull/44152#discussion_r3989837360
########## superset/mcp_service/chart/treemap_preview.py: ########## @@ -0,0 +1,293 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Bounded slice-and-dice Treemap previews using explicit rectangle geometry.""" + +from __future__ import annotations + +import math +from typing import Any + +from superset.mcp_service.chart.query_result import ( + metric_result_label, + normalize_chart_query_result, +) +from superset.mcp_service.chart.schemas import ChartError, VegaLitePreview + +# Match the built-in frontend categorical schemes. Unknown schemes are not guessed. +_PALETTES = { + "supersetColors": [ + "#1FA8C9", + "#454E7C", + "#5AC189", + "#FF7F44", + "#666666", + "#E04355", + "#FCC700", + "#A868B7", + "#3CCCCB", + "#A38F79", + "#8FD3E4", + "#A1A6BD", + "#ACE1C4", + "#FEC0A1", + "#B2B2B2", + "#EFA1AA", + "#FDE380", + "#D3B3DA", + "#9EE5E5", + "#D1C6BC", + ], + "lyftColors": [ + "#EA0B8C", + "#6C838E", + "#29ABE2", + "#33D9C1", + "#9DACB9", + "#7560AA", + "#2D5584", + "#831C4A", + "#333D47", + "#AC2077", + ], +} +_MAX_ROWS = 1000 + + +def treemap_ascii( + data: list[dict[str, Any]], form_data: dict[str, Any], width: int = 80 +) -> str | ChartError: + """Show the ordered hierarchy and values rather than unrelated bar geometry.""" + checked = normalize_chart_query_result({"queries": [{"data": data}]}, form_data) + if isinstance(checked, ChartError): + return checked + label = metric_result_label(form_data["metric"]) + assert label is not None + lines = [f"Treemap hierarchy | {label}"] + for row in data[:_MAX_ROWS]: + path = " > ".join(str(row[column]) for column in form_data["groupby"]) + lines.append(f"{path} | {row[label]}") + if len(data) > _MAX_ROWS: + lines.append(f"Showing {_MAX_ROWS} of {len(data)} rows") + return "\n".join(line[:width] for line in lines) + + +def treemap_vega_lite( # noqa: C901 + data: list[dict[str, Any]], form_data: dict[str, Any] +) -> VegaLitePreview | ChartError: + """Render nested metric-proportional rectangles; never substitute scatter/bar marks. + + Vega-Lite has no hierarchy transform. A bounded slice-and-dice layout is + computed here and sent as explicit coordinates, without executable hooks. + Native ECharts layout and dashboard interactions remain available in Explore. + """ + checked = normalize_chart_query_result({"queries": [{"data": data}]}, form_data) + if isinstance(checked, ChartError): + return checked + label = metric_result_label(form_data["metric"]) + assert label is not None + if not data: + return ChartError(error="No Treemap data available.", error_type="NoDataError") + scheme = form_data.get("color_scheme") or "supersetColors" + if ( + scheme not in _PALETTES + or form_data.get("currency_format") + or form_data.get("label_position", "insideTopLeft") != "insideTopLeft" + ): + return ChartError( + error=( + "This Treemap color/currency/label format requires the native " + "Explore renderer; use url or table preview." + ), + error_type="UnsupportedTreemapPreview", + ) + if ( + len(data) > _MAX_ROWS + or any(row[label] < 0 for row in data) + or not any(row[label] > 0 for row in data) + ): + return ChartError( + error=( + "Treemap geometry requires at most 1000 nonnegative rows and " + "a positive total; use table or url preview." + ), + error_type="UnsupportedTreemapPreview", + ) + hierarchy = form_data["groupby"] + if len(hierarchy) > 20 or not math.isfinite(sum(float(row[label]) for row in data)): + return ChartError( + error="Treemap preview requires at most 20 levels and a finite total.", + error_type="UnsupportedTreemapPreview", + ) + nodes: list[dict[str, Any]] = [] + + def layout( + rows: list[dict[str, Any]], + depth: int, + path: list[str], + x: float, + y: float, + width: float, + height: float, + ) -> None: + """Recursively partition the parent rectangle in hierarchy order.""" + groups: dict[tuple[str, str], list[dict[str, Any]]] = {} + for row in rows: + value = row[hierarchy[depth]] + groups.setdefault((type(value).__name__, str(value)), []).append(row) Review Comment: Reverified in b9d213c with registered get_chart_preview calls, not just the earlier reply. Across saved, saved-cache and unsaved-cache paths, integer 1 and string "1" produce two separate parent rectangles, values 30 and 10, with areas 0.3 and 0.1 of the canvas even though names/colors collide. The raw type/value grouping key remains intact; normalizing both keys to strings would incorrectly merge categories. The frontend's Map grouping and display-name coloring are preserved. The expanded public-path regression and real renderer suite pass. ########## superset/mcp_service/chart/chart_utils.py: ########## @@ -1276,6 +1379,16 @@ def map_treemap_config(config: TreemapChartConfig) -> Dict[str, Any]: "row_limit": config.row_limit, "color_scheme": config.color_scheme or "supersetColors", } + for key in _TREEMAP_PRESENTATION_KEYS | { + "time_range", + "granularity_sqla", + "template_params", + }: + value = getattr(config, key) + if value is not None: + form_data[key] = ( + value.model_dump() if hasattr(value, "model_dump") else value + ) Review Comment: Reverified on b9d213c: test_treemap_selected_time_column_reaches_query exercises actual shared query construction with selected/cleared chart columns and present/null/omitted dashboard overrides (six cases). The existing granularity mapping still reaches the query correctly, including shared null-extra/no-override semantics. These cases pass in the broad MCP/common suite; no additional mapping change is needed. ########## superset/mcp_service/chart/schemas.py: ########## @@ -1410,9 +1415,58 @@ class TreemapChartConfig(BaseChartConfig): max_length=100, ) + show_labels: bool = True + show_upper_labels: bool = True + label_type: Literal["key", "Key", "value", "key_value"] = "key_value" + label_position: Literal[ + "top", + "left", + "right", + "bottom", + "inside", + "insideLeft", + "insideRight", + "insideTop", + "insideBottom", + "insideTopLeft", + "insideBottomLeft", + "insideTopRight", + "insideBottomRight", + ] = "insideTopLeft" + number_format: str = Field("SMART_NUMBER", max_length=100) + date_format: str = Field("smart_date", max_length=100) + currency_format: CurrencyFormat | None = None + time_range: str | None = Field(None, max_length=1000) + granularity_sqla: str | None = Field(None, min_length=1, max_length=255) + template_params: str | None = Field(None, max_length=10000) + + @model_validator(mode="before") + @classmethod + def adapt_native_form_data(cls, data: Any) -> Any: + """Accept native hierarchy and saved, SIMPLE, and SQL metric inputs.""" + return _adapt_native_single_metric_form_data(data) Review Comment: Reverified with the installed Pydantic types and registered FastMCP entry calls on b9d213c. Native viz_type/datasource/groupby plus saved, SIMPLE and SQL metrics pass request adaptation; unknown executable controls still fail adjacent negative tests. The strengthened generation regression also checks the resulting native currency shape. The subclass before-adapter runs before inherited unknown-field rejection; there is no reproduced validation-order failure to fix. ########## superset/mcp_service/chart/query_result.py: ########## @@ -223,3 +223,79 @@ def validate_gauge_query_result( """Check Gauge results using the same finite-dial contract as rendering.""" normalized = normalize_gauge_query_result(result, form_data) return normalized if isinstance(normalized, ChartError) else None + + +def normalize_chart_query_result(result: Any, form_data: Mapping[str, Any]) -> Any: + """Validate chart-specific result contracts before consumers use rows.""" + if form_data.get("viz_type") != "treemap_v2": + return normalize_gauge_query_result(result, form_data) + if failure := query_result_failure(result): + return failure + label = metric_result_label(form_data.get("metric")) + hierarchy = form_data.get("groupby") + if ( + not label + or not isinstance(hierarchy, list) + or not hierarchy + or not all(isinstance(column, str) and column for column in hierarchy) + or len(set(hierarchy)) != len(hierarchy) + or label in hierarchy + ): + return ChartError( + error=( + "Treemap requires unique hierarchy columns and a distinct metric label." + ), + error_type="InvalidTreemapFormData", + ) + queries = result.get("queries") if isinstance(result, Mapping) else None + if not isinstance(queries, list) or len(queries) != 1: + return ChartError( + error="Treemap requires exactly one query result.", + error_type="InvalidTreemapResult", + ) + query = queries[0] + rows = query.get("data") if isinstance(query, Mapping) else None + if not isinstance(rows, list): + return ChartError( + error="Treemap query data must be an array of rows.", + error_type="InvalidTreemapResult", + ) + if failure := _validate_treemap_rows(rows, hierarchy, label): + return failure + return result + + +def _validate_treemap_rows( + rows: list[Any], hierarchy: list[str], label: str +) -> ChartError | None: + """Require complete hierarchy outputs and finite numeric metric values.""" + for index, row in enumerate(rows): + if not isinstance(row, Mapping) or any( + column not in row for column in [*hierarchy, label] + ): + return ChartError( + error=f"Treemap row {index} is missing hierarchy or metric outputs.", + error_type="InvalidTreemapResult", + ) + value = row[label] + try: + valid = ( + not isinstance(value, bool) + and isinstance(value, (int, float)) Review Comment: Fixed in b9d213c: Treemap accepts numbers.Real and Decimal while excluding booleans. Decimal.is_finite() rejects quiet/signaling NaNs and both infinities without coercing numeric strings. Regression coverage includes positive/nonfinite Decimal values, registered saved/cached ASCII/table/Vega previews, and registered saved/cache JSON/CSV/Excel exports. Gauge's existing numeric contract is unchanged. Decimal contract cases fail on the previous head and pass with this change. ########## superset/mcp_service/chart/tool/get_chart_preview.py: ########## @@ -470,6 +475,8 @@ def generate(self) -> VegaLitePreview | ChartError: # noqa: C901 if result and "queries" in result and len(result["queries"]) > 0: Review Comment: Fixed in b9d213c: _preview_row_limit now honors bounded Treemap row_limit instead of representation-specific 50/20/1000 fallbacks. Registered get_chart_preview regressions assert the actual query-context row_limit for 1 and 7 across ASCII/table/Vega, saved charts, saved+form_data_key and unsaved form_data_key paths (18 cases). These fail before/pass after; the unsaved generation path continues to use the configured limit. Large unsupported geometry still returns a structured unsupported preview rather than substitute marks. ########## superset/mcp_service/chart/chart_utils.py: ########## @@ -1276,6 +1379,16 @@ def map_treemap_config(config: TreemapChartConfig) -> Dict[str, Any]: "row_limit": config.row_limit, "color_scheme": config.color_scheme or "supersetColors", } + for key in _TREEMAP_PRESENTATION_KEYS | { + "time_range", + "granularity_sqla", + "template_params", + }: + value = getattr(config, key) + if value is not None: + form_data[key] = ( Review Comment: Fixed in b9d213c by using CurrencyFormat.to_form_data() in the Treemap mapper. Registered generate_chart and both registered update paths assert exactly {"symbol":"USD","symbolPosition":"suffix"}, without the internal symbol_position field. The suffix round-trip assertions fail on the previous head and pass after the change. ########## superset/mcp_service/chart/chart_utils.py: ########## @@ -672,6 +673,104 @@ def _without_generated_gauge_time_filter( ] +def resolve_treemap_update_config( + config: ChartConfig | TreemapChartUpdateConfig, + existing: dict[str, Any], + *, + dataset_rebind: bool = False, +) -> ChartConfig: + """Fill omitted required roles only from an authorized same-dataset Treemap.""" + if not isinstance(config, TreemapChartUpdateConfig) or isinstance( + config, TreemapChartConfig + ): + return config + values = config.model_dump(exclude_unset=True) + if existing.get("viz_type") == "treemap_v2" and not dataset_rebind: + for field in ("groupby", "metric"): + if field not in config.model_fields_set and field in existing: + values[field] = existing[field] + resolved = TreemapChartConfig.model_validate(values) + resolved.__pydantic_fields_set__ = set(config.model_fields_set) + return resolved + + +_TREEMAP_PRESENTATION_KEYS = frozenset( + { + "color_scheme", + "show_labels", + "show_upper_labels", + "label_type", + "label_position", + "number_format", + "date_format", + "currency_format", + } +) + + +def _merge_treemap_filters( + existing: dict[str, Any], + patch: dict[str, Any], + config: TreemapChartConfig, + dataset_rebind: bool, +) -> None: + """Separate explicit filter/temporal changes from mapper-generated defaults.""" + fields = config.model_fields_set + if "temporal_column" not in fields: + patch.pop(MCP_DASHBOARD_TIME_FILTER_SUBJECT, None) + if "filters" not in fields: Review Comment: Reproduced and fixed in b9d213c. For same-dataset updates with omitted temporal_column, the mapper-generated default filter is removed BEFORE its provenance marker; saved predicates and the existing neutral temporal binding are then retained while explicit new filters are added. Explicit empty filters still clear; explicit temporal clears also remove mapper defaults instead of silently rebinding to the dataset default. Dataset rebinds keep fresh bindings without importing saved ones. Registered saved-update and cached update-preview regressions use real mapping/binding/merge with a dataset whose default_time differs from saved_time; they verify old/new predicates, one neutral binding, no default_time predicate, and explicit clear/change behavior. These pass alongside the broad suite. ########## superset/mcp_service/chart/tool/update_chart_preview.py: ########## @@ -177,6 +178,39 @@ def update_chart_preview( # noqa: C901 NORMALIZATION_EXCEPTIONS, ) + warnings: list[str] = [] + previous_form_data: dict[str, Any] | None = None + + if request.form_data_key: + previous_form_data = _get_previous_form_data(request.form_data_key) + if previous_form_data is None: + warnings.append(INVALID_FORM_DATA_KEY_WARNING) + previous_datasource = str( + (previous_form_data or {}).get("datasource") + or (previous_form_data or {}).get("datasource_id") + or "" + ).split("__", 1)[0] + dataset_rebind = previous_datasource != str(dataset.id) and ( + bool(previous_datasource) or config.chart_type == "treemap_v2" + ) + try: + config = resolve_treemap_update_config( + config, + previous_form_data or {}, + dataset_rebind=dataset_rebind, + ) + except ValueError as ex: + return { + "chart": None, + "error": { + "error_type": "ValidationError", + "message": "Invalid Treemap update configuration", + "details": str(ex), + }, + "success": False, + "schema_version": "2.0", + "api_version": "v1", + } Review Comment: Verified against the actual installed Pydantic implementation: pydantic_core.ValidationError.__mro__ includes ValueError. Therefore the existing handler DOES catch resolution errors; adding a second subclass entry is unnecessary. test_resolution_validation_error_is_caught_value_error constructs a genuine failing Treemap resolution and verifies this inheritance. Registered update-preview calls with missing hierarchy/metric and saved update calls with a malformed native SIMPLE metric return structured ValidationError responses, success=false and no FastMCP transport error; the saved response validates against GenerateChartResponse and no mutation occurs. These pass on b9d213c. The claimed uncaught exception is not reproduced. -- 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]
