You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aminghadersohi (via GitHub)" <gi...@apache.org> on 2026/09/10 23:49:28 UTC
[PR] fix(mcp): complete Treemap round-trip and previews [superset]
aminghadersohi opened a new pull request, #44152:
URL: https://github.com/apache/superset/pull/44152
### SUMMARY
Completes MCP Treemap support on top of the merged community implementation in #43569, without replacing its plugin or chart-type registration.
- Match the frontend's bounded metric/hierarchy ordering, including hierarchy tie-breakers when metric sorting is disabled.
- Accept bounded native hierarchy and saved/SIMPLE/SQL metrics alongside canonical typed references. Expose Treemap presentation controls.
- Preserve omitted controls in saved and cached updates, support explicit clears and partial updates, and restrict dataset rebind inheritance to presentation controls.
- Replace generic bar/scatter previews with bounded metric-proportional, hierarchical slice-and-dice rectangles, labels, categorical colors and tooltips. Unsupported representations return structured errors; Explore remains the native ECharts renderer.
- Validate hierarchy outputs, metric aliases, finite numeric results and query-error envelopes across generation, compilation, previews, chart data and exports.
- Add request/registered FastMCP, cache, update/rebind, cross-chart and rendered Vega regressions.
Tracking: [SC-120298](https://app.shortcut.com/preset/story/120298). Base implementation: [#43569](https://github.com/apache/superset/pull/43569), merged as `1b03ad89fb7a27c36cc9c266d755a6eaf90bb0a4`.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: Treemap previews could return generic scatter/bar geometry, and updates reset omitted controls.
After: previews contain explicit nested rectangles sized by metric values, with hierarchy-path tooltips. The renderer regression checks scenegraph areas and bounds, not snapshots. The bounded Vega-Lite layout is intentionally identified as slice-and-dice, not native ECharts; Explore provides native formatting and interactions.
### TESTING INSTRUCTIONS
1. Use `generate_chart` or `generate_explore_link` with `chart_type: "treemap_v2"`, two hierarchy columns and a saved/SIMPLE/SQL metric.
2. Set `row_limit: 1` and toggle `sort_by_metric`; inspect metric-descending/hierarchy-ascending query ordering.
3. Preview saved and unsaved `form_data_key` states as ASCII, table and Vega-Lite. Inspect hierarchy, tile areas, labels and tooltips.
4. Set nondefault colors, row limit, sorting, labels and formats. Update only one control through `update_chart` and `update_chart_preview`; confirm other controls survive. Verify nullable/empty clears and dataset rebind behavior.
5. Return an invalid metric, missing hierarchy output or query error; confirm generation/preview/data/export returns a structured failure.
Validation results will be updated with the final local suite and exact-head GitHub checks. Eight focused probes failed on the merged baseline and passed after this change. Live database/browser validation is not available in this worktree; query execution and persistence are mocked in the product-path unit tests.
### ADDITIONAL INFORMATION
- [x] Has associated issue: [SC-120298](https://app.shortcut.com/preset/story/120298)
- [x] Required feature flags: existing MCP service configuration
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44152:
URL: https://github.com/apache/superset/pull/44152#issuecomment-5635074924
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44152:
URL: https://github.com/apache/superset/pull/44152#discussion_r3985047478
##########
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:
**Suggestion:** Integer `1` and string `'1'` form separate geometry groups but share the same displayed name, tooltip path, and Vega color, so distinct categories become visually indistinguishable. [logic error]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0d8aaec8c98c434cae7698dcd703042d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0d8aaec8c98c434cae7698dcd703042d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/treemap_preview.py
**Line:** 148:151
**Comment:**
*Logic Error: Integer `1` and string `'1'` form separate geometry groups but share the same displayed name, tooltip path, and Vega color, so distinct categories become visually indistinguishable.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44152&comment_hash=73335367129938fb2d0354e0d46af58daa28fcd57137f69f76b883f45a3713de&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44152&comment_hash=73335367129938fb2d0354e0d46af58daa28fcd57137f69f76b883f45a3713de&reaction=dislike'>๐</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44152:
URL: https://github.com/apache/superset/pull/44152#discussion_r4041438764
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2925,7 +2991,7 @@ class UpdateChartRequest(ChartRequestNormalizerMixin, QueryCacheControl):
description="Chart ID or UUID",
validation_alias=AliasChoices("identifier", "id", "chart_id"),
)
- config: ChartConfig | None = Field(
+ config: ChartConfig | TreemapChartUpdateConfig | None = Field(
Review Comment:
Adding the defaulted partial Treemap model to this union makes a config without `chart_type` validate as a Treemap. On `update_chart(..., generate_preview=false)`, omitting the discriminator for an existing pie or table chart can then overwrite its `viz_type` and params instead of rejecting the request. Could `TreemapChartUpdateConfig.chart_type` be required here, or could this remain a discriminated update union?
##########
superset/mcp_service/chart/query_result.py:
##########
@@ -223,3 +225,83 @@ 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)
Review Comment:
Saved Treemaps can contain an adhoc Custom SQL object in `groupby` via Explore's `DndColumnSelect`, but this check accepts only strings. `get_chart_data`, preview, and export now reject such otherwise-valid saved charts with `InvalidTreemapFormData`. Could this normalize adhoc columns to their output labels instead of rejecting them?
##########
.github/workflows/superset-python-unittest.yml:
##########
@@ -43,7 +43,10 @@ jobs:
needs: changes
if: needs.changes.outputs.python == 'true'
runs-on: ubuntu-26.04
- timeout-minutes: 30
+ # The full coverage run is followed by independent SQL and semantic-layer
+ # 100% gates. Allow all three coverage-preserving phases to finish under
+ # ordinary runner variance rather than cancelling after successful tests.
+ timeout-minutes: 40
Review Comment:
This hunk now conflicts with `master`, where #44178 runs this suite with pytest-xdist and gives it 45 minutes. Resolving the conflict in favor of this 40-minute version would undo that newer guard and can time out otherwise-passing required checks. Could you drop this workflow change when rebasing?
##########
tests/unit_tests/mcp_service/chart/test_treemap_completeness.py:
##########
@@ -0,0 +1,1174 @@
+# 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.
+
+"""Treemap regressions spanning native requests, query output and update semantics."""
+
+from contextlib import nullcontext
+from copy import deepcopy
+from decimal import Decimal
+from types import SimpleNamespace
+from typing import Any
+from unittest.mock import Mock, patch
+
+import pytest
+from pydantic import ValidationError
+
+from superset.mcp_service.chart.chart_helpers import _build_single_query_dict
+from superset.mcp_service.chart.chart_utils import (
+ map_treemap_config,
+ merge_chart_form_data,
+ resolve_treemap_update_config,
+)
+from superset.mcp_service.chart.query_result import normalize_chart_query_result
+from superset.mcp_service.chart.schemas import (
+ ChartError,
+ GenerateChartRequest,
+ GetChartPreviewRequest,
+ TreemapChartConfig,
+ UpdateChartPreviewRequest,
+ UpdateChartRequest,
+)
+from superset.mcp_service.chart.treemap_preview import treemap_ascii, treemap_vega_lite
+from superset.utils import json
+
+FORM_DATA: dict[str, Any] = {
+ "viz_type": "treemap_v2",
+ "groupby": ["region", "product"],
+ "metric": "revenue",
+ "datasource": "7__table",
+ "color_scheme": "lyftColors",
+ "row_limit": 7,
+ "sort_by_metric": False,
+ "show_labels": False,
+ "show_upper_labels": False,
+ "number_format": ",.1f",
+ "date_format": "%Y",
+ "label_type": "value",
+ "currency_format": {"symbol": "USD", "symbolPosition": "prefix"},
+ "adhoc_filters": [
+ {
+ "expressionType": "SIMPLE",
+ "clause": "WHERE",
+ "subject": "region",
+ "operator": "==",
+ "comparator": "West",
+ }
+ ],
+ "time_range": "2025-01-01 : 2026-01-01",
+ "granularity_sqla": "ds",
+ "template_params": '{"scale": 2}',
+}
+ROWS = [
+ {"region": "West", "product": "A", "revenue": 30},
+ {"region": "West", "product": "B", "revenue": 10},
+ {"region": "East", "product": "A", "revenue": 60},
+]
+
+
+@pytest.mark.parametrize("sort", [False, True])
+@pytest.mark.parametrize("limit", [None, 0, 1, 7])
+def test_hierarchy_query_order_matches_frontend(sort: bool, limit: int | None) -> None:
+ """Metric order has precedence, with hierarchy tie-breakers only when bounded."""
+ form = {**FORM_DATA, "sort_by_metric": sort, "row_limit": limit}
+ query = _build_single_query_dict(form, form["groupby"], [form["metric"]])
+ expected = ([("revenue", False)] if sort else []) + [
+ ("region", True),
+ ("product", True),
+ ]
+ assert query.get("orderby", []) == (expected if limit else [])
+
+
+@pytest.mark.parametrize(
+ "metric",
+ [
+ "revenue",
+ {"name": "revenue", "saved_metric": True},
+ {
+ "expressionType": "SIMPLE",
+ "column": {"column_name": "amount"},
+ "aggregate": "SUM",
+ "label": "revenue",
+ "hasCustomLabel": True,
+ },
+ {"expressionType": "SQL", "sqlExpression": "SUM(amount)", "label": "revenue"},
+ ],
+)
+@pytest.mark.parametrize(
+ "request_class", [GenerateChartRequest, UpdateChartPreviewRequest]
+)
+def test_native_request_roundtrip(metric: Any, request_class: Any) -> None:
+ """Native columns and all valid metric shapes survive actual request validation."""
+ native = {**FORM_DATA, "metric": metric, "slice_id": 1}
+ request = request_class(dataset_id=7, config=native)
+ result = map_treemap_config(request.config)
+ assert result["groupby"] == FORM_DATA["groupby"]
+ assert result["show_labels"] is False
+ assert result["number_format"] == ",.1f"
+ assert result["metric"] == "revenue" or result["metric"]["label"] == "revenue"
+
+
+@pytest.mark.parametrize(
+ "field,value",
+ [
+ ("javascript", "alert(1)"),
+ ("groupby", []),
+ ("metric", {"expressionType": "SQL", "sqlExpression": "SUM(amount)"}),
+ ("row_limit", 10001),
+ ],
+)
+def test_native_requests_do_not_accept_unbounded_or_hostile_controls(
+ field: str, value: Any
+) -> None:
+ """Unknown executable controls and invalid native roles remain validation errors."""
+ with pytest.raises(ValidationError):
+ GenerateChartRequest(dataset_id=7, config={**FORM_DATA, field: value})
+
+
+@pytest.mark.parametrize(
+ "patch_data",
+ [{"show_labels": True}, {"groupby": ["product", "region"], "metric": "revenue"}],
+)
+def test_omitted_same_viz_controls_survive(patch_data: dict[str, Any]) -> None:
+ """Partial and full-role updates preserve all omitted native controls."""
+ request = UpdateChartRequest(
+ identifier=1, config={"chart_type": "treemap_v2", **patch_data}
+ )
+ assert request.config is not None
+ config = resolve_treemap_update_config(request.config, FORM_DATA)
+ merged = merge_chart_form_data(FORM_DATA, map_treemap_config(config), config)
+ for key, value in FORM_DATA.items():
+ if key not in patch_data:
+ assert merged[key] == value
+ for key, value in patch_data.items():
+ assert merged[key] == value
+
+
+@pytest.mark.parametrize(
+ "field",
+ [
+ "color_scheme",
+ "currency_format",
+ "time_range",
+ "granularity_sqla",
+ "template_params",
+ "filters",
+ ],
+)
+def test_nullable_clears_are_not_replaced_by_defaults(field: str) -> None:
+ """Explicit null clears native nullable controls rather than restoring defaults."""
+ request = UpdateChartRequest(
+ identifier=1, config={"chart_type": "treemap_v2", field: None}
+ )
+ assert request.config is not None
+ config = resolve_treemap_update_config(request.config, FORM_DATA)
+ merged = merge_chart_form_data(FORM_DATA, map_treemap_config(config), config)
+ assert ("adhoc_filters" if field == "filters" else field) not in merged
+
+
+def test_empty_filters_and_dataset_rebind() -> None:
+ """Rebinds retain presentation only and scrub stale query/template roles."""
+ config = TreemapChartConfig(
+ groupby=[{"name": "other"}], metric="other_metric", filters=[]
+ )
+ existing = {**FORM_DATA, "metrics": ["stale"], "x_axis": "stale"}
+ merged = merge_chart_form_data(
+ existing, map_treemap_config(config), config, dataset_rebind=True
+ )
+ assert merged["color_scheme"] == "lyftColors"
+ assert merged["show_labels"] is False
+ assert merged["groupby"] == ["other"]
+ assert merged["metric"] == "other_metric"
+ for key in (
+ "adhoc_filters",
+ "time_range",
+ "granularity_sqla",
+ "template_params",
+ "metrics",
+ "x_axis",
+ ):
+ assert key not in merged
+ incomplete = UpdateChartRequest(
+ identifier=1, config={"chart_type": "treemap_v2", "show_labels": True}
+ )
+ assert incomplete.config is not None
+ with pytest.raises(ValidationError):
+ resolve_treemap_update_config(incomplete.config, existing, dataset_rebind=True)
+
+
+@pytest.mark.parametrize(
+ "result",
+ [
+ None,
+ {},
+ {"queries": []},
+ {"queries": [{}]},
+ {"queries": [{"data": {}}]},
+ {"queries": [{"data": [None]}]},
+ {"queries": [{"data": [{"region": "West", "revenue": 2}]}]},
+ {"status": "failed", "message": "Database rejected SQL"},
+ {"queries": [{"error": "SQL failed", "data": ROWS}]},
+ ],
+)
+def test_malformed_results_never_produce_success(result: Any) -> None:
+ """Missing hierarchy and malformed/error envelopes are actionable failures."""
+ assert isinstance(normalize_chart_query_result(result, FORM_DATA), ChartError)
+
+
+@pytest.mark.parametrize(
+ "value", [None, True, "3", float("nan"), float("inf"), 10**400]
+)
+def test_metric_outputs_must_be_finite_numeric(value: Any) -> None:
+ """A single invalid metric fails instead of silently dropping a hierarchy node."""
+ rows = [*ROWS, {**ROWS[0], "revenue": value}]
+ result = normalize_chart_query_result({"queries": [{"data": rows}]}, FORM_DATA)
+ assert isinstance(result, ChartError)
+ assert result.error_type == "InvalidTreemapMetric"
+
+
+def test_result_validation_is_non_mutating_and_chart_specific() -> None:
+ """Other charts retain their existing interpretation of data."""
+ result = {"queries": [{"data": ROWS}]}
+ original = deepcopy(result)
+ assert normalize_chart_query_result(result, FORM_DATA) == original
+ assert result == original
+ unrelated = {"queries": [{"data": [{"x": None}]}]}
+ assert normalize_chart_query_result(unrelated, {"viz_type": "table"}) is unrelated
+
+
+def test_treemap_geometry_area_hierarchy_labels_and_color() -> None:
+ """Coordinates partition parents and area follows the metric, not row count."""
+ preview = treemap_vega_lite(
+ ROWS,
+ {
+ **FORM_DATA,
+ "currency_format": None,
+ "show_labels": True,
+ "number_format": None,
+ },
+ )
+ assert not isinstance(preview, ChartError)
+ spec = preview.specification
+ nodes = spec["data"]["values"]
+ leaves = [node for node in nodes if node["leaf"]]
+ for leaf in leaves:
+ area = (leaf["x1"] - leaf["x0"]) * (leaf["y1"] - leaf["y0"])
+ assert area / (600 * 400) == pytest.approx(leaf["value"] / 100)
+ parent = next(
+ node for node in nodes if node["path"] == leaf["path"].split(" > ")[0]
+ )
+ assert parent["x0"] <= leaf["x0"] <= leaf["x1"] <= parent["x1"]
+ assert parent["y0"] <= leaf["y0"] <= leaf["y1"] <= parent["y1"]
+ assert spec["layer"][0]["mark"]["type"] == "rect"
+ assert spec["layer"][0]["encoding"]["color"]["scale"]["range"][0] == "#EA0B8C"
+ assert spec["layer"][0]["encoding"]["tooltip"][1]["title"] == "revenue"
+ assert spec["layer"][1]["mark"]["type"] == "text"
+ assert "West > A | 30" in treemap_ascii(ROWS, FORM_DATA)
+
+
+@pytest.mark.parametrize(
+ "rows,override",
+ [
+ (ROWS, {"color_scheme": "unavailable"}),
+ (ROWS, {"currency_format": {"symbol": "USD"}}),
+ ([{**ROWS[0], "revenue": -1}], {}),
+ (ROWS * 334, {}),
+ ],
+)
+def test_unsupported_geometry_is_explicit(rows: Any, override: Any) -> None:
+ """Unsupported representation must not fall back to a plausible bar or scatter."""
+ preview = treemap_vega_lite(
+ rows, {**FORM_DATA, "currency_format": None, **override}
+ )
+ assert isinstance(preview, ChartError)
+ assert preview.error_type == "UnsupportedTreemapPreview"
+
+
+@pytest.mark.parametrize("format_name", ["ascii", "table", "vega_lite"])
+def test_saved_and_unsaved_preview_dispatch_match(format_name: str) -> None:
+ """Both preview entry paths use the Treemap representation and result contract."""
+ from superset.mcp_service.chart.preview_utils import generate_preview_from_form_data
+ from superset.mcp_service.chart.tool.get_chart_preview import (
+ ASCIIPreviewStrategy,
+ TablePreviewStrategy,
+ VegaLitePreviewStrategy,
+ )
+
+ form = {**FORM_DATA, "currency_format": None}
+ chart = SimpleNamespace(
+ id=1,
+ viz_type="treemap_v2",
+ slice_name="Treemap",
+ datasource_id=7,
+ datasource_type="table",
+ params=json.dumps(form),
+ )
+ context = SimpleNamespace(
+ queries=[SimpleNamespace(metrics=["revenue"], columns=form["groupby"])]
+ )
+ with (
+ patch(
+ "superset.commands.chart.data.get_data_command.ChartDataCommand"
+ ) as command,
+ patch(
+ "superset.mcp_service.chart.tool.get_chart_preview.build_query_context_from_form_data",
+ return_value=context,
+ ),
+ patch(
+ "superset.mcp_service.chart.chart_helpers.build_query_context_from_form_data",
+ return_value=context,
+ ),
+ patch("superset.extensions.db.session") as session,
+ ):
+ session.get.return_value = Mock(id=7)
+ command.return_value.run.return_value = {"queries": [{"data": ROWS}]}
+ strategy = {
+ "ascii": ASCIIPreviewStrategy,
+ "table": TablePreviewStrategy,
+ "vega_lite": VegaLitePreviewStrategy,
+ }[format_name]
+ saved = strategy(
+ chart, GetChartPreviewRequest(identifier=1, format=format_name)
+ ).generate()
+ unsaved = generate_preview_from_form_data(form, 7, format_name)
+ assert not isinstance(saved, ChartError)
+ assert not isinstance(unsaved, ChartError)
+ if format_name == "vega_lite":
+ assert saved.specification == unsaved.specification
+ elif format_name == "ascii":
+ assert "West > A | 30" in saved.ascii_content
+ assert "West > A | 30" in unsaved.ascii_content
+ else:
+ assert "region" in saved.table_data
+ assert "revenue" in unsaved.table_data
+
+
+@pytest.mark.parametrize("rebind", [False, True])
+def test_saved_update_and_update_preview_payloads_agree(rebind: bool) -> None:
+ """Both save and preview-first update paths apply the same omission contract."""
+ from superset.mcp_service.chart.tool.update_chart import (
+ _build_preview_form_data,
+ _build_update_payload,
+ )
+
+ config = TreemapChartConfig(
+ groupby=[{"name": "region"}], metric="revenue", show_labels=True
+ )
+ request = UpdateChartRequest(
+ identifier=1, config=config, dataset_id=8 if rebind else None
+ )
+ chart = Mock(
+ id=1, datasource_id=7, slice_name="Treemap", params=json.dumps(FORM_DATA)
+ )
+ with patch(
+ "superset.mcp_service.chart.chart_utils._bind_dashboard_time_range_filter"
+ ):
+ preview = _build_preview_form_data(request, chart, parsed_config=config)
+ payload = _build_update_payload(request, chart, parsed_config=config)
+ saved = json.loads(payload["params"])
+ for key in (
+ "color_scheme",
+ "row_limit",
+ "sort_by_metric",
+ "groupby",
+ "metric",
+ "number_format",
+ "show_labels",
+ ):
+ assert saved[key] == preview[key]
+ assert saved["color_scheme"] == "lyftColors"
+ assert saved["show_labels"] is True
+ assert ("template_params" not in saved) == rebind
+
+
+def test_normalization_preserves_explicit_field_set_and_rejects_ambiguity() -> None:
+ """Canonicalization must not turn default values into explicit update intent."""
+ from superset.mcp_service.chart.plugins.treemap import TreemapChartPlugin
+ from superset.mcp_service.common.error_schemas import DatasetContext
+
+ context = DatasetContext(
+ id=7,
+ table_name="sales",
+ database_name="database",
+ available_columns=[{"name": "Region", "type": "STRING"}],
+ available_metrics=[{"name": "Revenue"}],
+ )
+ config = TreemapChartConfig(groupby=[{"name": "region"}], metric="revenue")
+ normalized = TreemapChartPlugin().normalize_column_refs(config, context)
+ assert normalized.groupby[0].name == "Region"
+ assert normalized.metric.name == "Revenue"
+ assert normalized.model_fields_set == config.model_fields_set
+ context.available_columns.append({"name": "REGION", "type": "STRING"})
+ with pytest.raises(ValueError, match="[Aa]mbiguous"):
+ TreemapChartPlugin().normalize_column_refs(config, context)
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize(
+ "metric",
+ [
+ "revenue",
+ {
+ "expressionType": "SIMPLE",
+ "column": {"column_name": "amount"},
+ "aggregate": "SUM",
+ "label": "revenue",
+ },
+ {"expressionType": "SQL", "sqlExpression": "SUM(amount)", "label": "revenue"},
+ ],
+)
+@pytest.mark.parametrize("valid_result", [False, True])
+async def test_registered_generate_chart_native_roundtrip(
+ metric: Any, valid_result: bool
+) -> None:
+ """Call the registered FastMCP tool, retaining real request and compile checks."""
+ from fastmcp import Client
+
+ from superset.mcp_service.app import mcp
+
+ request = GenerateChartRequest(
+ dataset_id=7,
+ config={
+ "chart_type": "treemap_v2",
+ "groupby": ["region", "product"],
+ "metric": metric,
+ "currency_format": {"symbol": "USD", "symbolPosition": "suffix"},
+ },
+ preview_formats=["url"],
+ )
+ dataset = Mock(id=7, datasource_name="sales", table_name="sales")
+ user = Mock(id=1, username="admin", roles=[], groups=[])
+ with (
+ patch("superset.mcp_service.auth.get_user_from_request", return_value=user),
+ patch(
+ "superset.mcp_service.chart.validation.ValidationPipeline.validate_request_with_warnings",
+ return_value=Mock(is_valid=True, request=request, warnings={}, error=None),
+ ),
+ patch(
+ "superset.mcp_service.chart.chart_utils.generate_explore_link",
+ return_value="http://localhost/explore/?form_data_key=treemap",
+ ),
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch(
+ "superset.mcp_service.chart.tool.generate_chart.has_dataset_access",
+ return_value=True,
+ ),
+ patch(
+ "superset.mcp_service.chart.chart_helpers.build_query_context_from_form_data",
+ return_value=Mock(),
+ ),
+ patch(
+ "superset.commands.chart.data.get_data_command.ChartDataCommand"
+ ) as command,
+ ):
+ command.return_value.run.return_value = {
+ "queries": [
+ {"data": ROWS if valid_result else [{"region": "West", "revenue": 1}]}
+ ]
+ }
+ async with Client(mcp) as client:
+ result = await client.call_tool(
+ "generate_chart",
+ {
+ "request": {
+ "dataset_id": 7,
+ "config": {
+ "viz_type": "treemap_v2",
+ "groupby": ["region", "product"],
+ "metric": metric,
+ "currency_format": {
+ "symbol": "USD",
+ "symbolPosition": "suffix",
+ },
+ },
+ "preview_formats": ["url"],
+ }
+ },
+ )
+ data = result.structured_content
+ assert data["success"] is valid_result
+ if valid_result:
+ assert data["form_data"]["currency_format"] == {
+ "symbol": "USD",
+ "symbolPosition": "suffix",
+ }
+ assert data["form_data"]["groupby"] == ["region", "product"]
+ assert data["form_data"]["viz_type"] == "treemap_v2"
+ else:
+ assert data["error"]["error_type"] == "InvalidTreemapResult"
+ command.return_value.validate.assert_called_once()
+
+
+@pytest.mark.parametrize("number_format", [None, ",.1f"])
+def test_vega_scenegraph_renders_nested_metric_geometry(
+ number_format: str | None,
+) -> None:
+ """Compile the specification and inspect rendered rectangles, not a snapshot."""
+ import os
+ import shutil
+ import subprocess
+
+ if not os.environ.get("NODE_PATH"):
Review Comment:
This is the only new test that actually compiles and renders the Vega-Lite spec, but the Python-Unit workflow neither installs these Node packages nor sets `NODE_PATH`, so both parametrizations skip in CI. A bad `calculate` field or Vega expression could therefore pass the remaining dict-shape tests and fail only at render time. Could CI run this compiler/scenegraph assertion, or an equivalent schema/compile check?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44152:
URL: https://github.com/apache/superset/pull/44152#issuecomment-5731394766
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44152:
URL: https://github.com/apache/superset/pull/44152#issuecomment-5627264873
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44152?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `14.01274%` with `270 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 66.12%. Comparing base ([`1b03ad8`](https://app.codecov.io/gh/apache/superset/commit/1b03ad89fb7a27c36cc9c266d755a6eaf90bb0a4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b5ea026`](https://app.codecov.io/gh/apache/superset/commit/b5ea026840b3ada3f89de410f183f7356f63cbf6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44152?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/mcp\_service/chart/schemas.py](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Fschemas.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvc2NoZW1hcy5weQ==) | 25.96% | [77 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/treemap\_preview.py](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Ftreemap_preview.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvdHJlZW1hcF9wcmV2aWV3LnB5) | 12.85% | [61 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/chart\_utils.py](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Fchart_utils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvY2hhcnRfdXRpbHMucHk=) | 8.51% | [43 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/query\_result.py](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Fquery_result.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvcXVlcnlfcmVzdWx0LnB5) | 6.06% | [31 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/chart\_helpers.py](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Fchart_helpers.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvY2hhcnRfaGVscGVycy5weQ==) | 0.00% | [13 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...perset/mcp\_service/chart/tool/get\_chart\_preview.py](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_preview.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvdG9vbC9nZXRfY2hhcnRfcHJldmlldy5weQ==) | 9.09% | [10 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/preview\_utils.py](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Fpreview_utils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvcHJldmlld191dGlscy5weQ==) | 10.00% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...set/mcp\_service/chart/tool/update\_chart\_preview.py](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Ftool%2Fupdate_chart_preview.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvdG9vbC91cGRhdGVfY2hhcnRfcHJldmlldy5weQ==) | 0.00% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/tool/get\_chart\_data.py](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_data.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvdG9vbC9nZXRfY2hhcnRfZGF0YS5weQ==) | 0.00% | [8 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/plugins/treemap.py](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Fplugins%2Ftreemap.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvcGx1Z2lucy90cmVlbWFwLnB5) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| ... and [2 more](https://app.codecov.io/gh/apache/superset/pull/44152?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44152 +/- ##
==========================================
- Coverage 66.42% 66.12% -0.30%
==========================================
Files 2925 2926 +1
Lines 172561 172783 +222
Branches 40058 40115 +57
==========================================
- Hits 114619 114251 -368
- Misses 55563 56151 +588
- Partials 2379 2381 +2
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44152/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage ฮ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44152/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.28% <14.01%> (-0.06%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44152/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44152/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44152/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.17% <14.01%> (-0.06%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/44152/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.61% <14.01%> (-0.57%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44152/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.49% <14.01%> (-0.11%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44152?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44152:
URL: https://github.com/apache/superset/pull/44152#issuecomment-5784195889
Re-verified Bitoโs 2026-09-11 โUncaught pydantic ValidationErrorโ finding at current head 5e0dcf94110bf363ebebd526842a038fcb23cf8d: no code change is needed. The review-body link points to discussion_r3989638526, which is already resolved (including the prior explanation in discussion_r3989838698). Pydantic ValidationError inherits ValueError, so update_chart_preview.pyโs existing except ValueError handler returns the structured ValidationError response.
Existing regression evidence at this head: tests/unit_tests/mcp_service/chart/test_treemap_completeness.py::test_resolution_validation_error_is_caught_value_error constructs a genuine failing Treemap resolution and asserts that inheritance. The same suite exercises registered update_chart_preview calls with missing cached roles, asserting success=false, error_type=ValidationError, no transport error, and no cache write.
Fresh verification: PYTHONPATH="$PWD/superset-core/src:$PWD" python -m pytest tests/unit_tests/mcp_service/chart/test_treemap_completeness.py tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py -q --disable-warnings โ 177 passed, 2 skipped, 0 failed. Ruff check on those two test files and update_chart_preview.py โ all checks passed. The initial test invocation without the local superset-core source path failed during conftest import; the corrected invocation above completed successfully.
No files changed, no commit or push, and no review-state changes.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44152:
URL: https://github.com/apache/superset/pull/44152#discussion_r3985134841
##########
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:
Confirmed and fixed in 6a47480. The shared Treemap query builder now maps the selected granularity_sqla to QueryObject granularity, preserving normalized dashboard override precedence. Six query-construction cases exercise selected/cleared chart columns and present/null/omitted dashboard overrides; null dashboard extras retain the shared no-override semantics. The selected-column/override regression failed before the fix. All 96 Treemap completeness cases pass, including real Vega rendering. Full branch-file pre-commit passes; full MCP/common and exact-head CI are running.
##########
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:
Checked against the actual Pydantic/FastMCP entry paths: this is not reproduced. test_native_request_roundtrip passes native FORM_DATA containing viz_type and datasource through GenerateChartRequest and UpdateChartPreviewRequest for saved, SIMPLE and SQL metrics. test_registered_generate_chart_native_roundtrip additionally exercises native viz_type/groupby/metric through the registered FastMCP entry. Subclass before validators adapt the native envelope before the inherited unknown-field check. Hostile/unknown controls remain rejected by the adjacent negative regressions; no relaxation is needed.
##########
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:
Compared with the current frontend: utils/treeBuilder.ts groups by raw Map keys, so integer 1 and string "1" remain separate; Treemap/transformProps.ts then formats the name and calls colorFn(name, sliceId). Keeping distinct geometry with the same display name/color therefore matches the frontend rather than inventing a different categorical encoding. Added test_treemap_mixed_type_categories_remain_separate in 6a47480 to lock in the two distinct metric groups. The test passes alongside renderer geometry checks. A frontend-wide type-disambiguation UX would be a separate behavior change, not a preview-only fix.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44152:
URL: https://github.com/apache/superset/pull/44152#discussion_r3985137869
##########
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:
โ
**Customized review instruction saved!**
**Instruction:**
> Preserve distinct treemap geometry groups for category values that differ by type, even when their formatted display names and colors match the frontend behavior.
**Applied to:**
- `superset/mcp_service/chart/treemap_preview.py`
---
๐ก *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44152:
URL: https://github.com/apache/superset/pull/44152#issuecomment-5628377426
## Finite follow-up complete โ required CI green
Exact head: **6a4748070da3e18f8ffe41c1e11363074bb4c03e**. Normal pushes only; no draft, force-push or PR merge.
Commits:
- 6d2a824812279fc48146e38cf21fc04fd3007310 โ normal merge of upstream master 89565db.
- b7024009d5c3a0ed7145f2047d54d184d8e3cd94 โ shared 40-minute workflow budget, nullable preview format, structured update errors, cached schema compilation and review regressions.
- 6a4748070da3e18f8ffe41c1e11363074bb4c03e โ selected Treemap temporal column reaches query granularity; override/clear and mixed-type grouping regressions.
[Exact-head checks](https://github.com/apache/superset/pull/44152/checks): **all 13 required checks passed**; overall **64 success, 10 skipped, 3 neutral**, no pending or failed checks.
[Python-Unit](https://github.com/apache/superset/actions/runs/34550891815) finished successfully in **31m27s**:
- Main suite: **15,248 passed, 7 skipped, 2 xfailed**.
- SQL: **1,717 passed / 100% coverage**.
- Semantic layers: **437 passed / 100% coverage**.
- Coverage upload and JUnit artifact upload passed. No tests, coverage thresholds or required-gate enforcement were reduced.
Final-head local evidence:
- Full MCP/common: **4,430 passed, 1 skipped**, 335.34s. The skip is the absent optional BigQuery dependency.
- Real Vega 5.33.1 / Vega-Lite 5.23.0 renderer exercised both null and explicit formats, with scenegraph geometry/labels/tooltips assertions. CI does not install this optional Node renderer; both cases were exercised locally.
- SQL **1,717 / 100%**, semantic layers **437 / 100%**.
- Full branch-file pre-commit passed (MyPy, Ruff, Pylint, formatting and workflow audit); compile and diff checks passed.
- Eight strengthened review cases failed on the previous implementation; selected-time-column queries also reproduced the late temporal finding before the fix. Regression/evidence replies are attached to all three inline review findings.
PR remains Amin-authored/assigned, non-draft and conflict-free (MERGEABLE). GitHub still reports REVIEW_REQUIRED; human approval remains necessary. Worktree is clean. No live Superset/browser/database environment was available locally; product-path unit tests mock execution/persistence, while exact-head integration/browser CI passed. No additional review/retry loop was started.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44152:
URL: https://github.com/apache/superset/pull/44152#discussion_r3989638526
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Uncaught pydantic ValidationError</b></div>
<div id="fix">
`resolve_treemap_update_config` calls `TreemapChartConfig.model_validate` (chart_utils.py:692), which raises pydantic v2 `ValidationError` on invalid config. That exception is not a subclass of `ValueError`, so this handler never fires and the structured error is never returned; the `ValidationError` also isn't in the outer `except` tuple (lines 426-434), so the tool fails with an unhandled exception. Catch `(ValueError, ValidationError)`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
dataset_rebind = previous_datasource != str(dataset.id) and (
bool(previous_datasource) or config.chart_type == "treemap_v2"
)
from pydantic import ValidationError
try:
config = resolve_treemap_update_config(
config,
previous_form_data or {},
dataset_rebind=dataset_rebind,
)
except (ValueError, ValidationError) 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",
}
````
</div>
</details>
</div>
<small><i>Code Review Run #e5f050</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44152:
URL: https://github.com/apache/superset/pull/44152#issuecomment-5627875255
<!-- Bito Reply -->
The flagged issue is valid. When integer `1` and string `'1'` are treated as distinct categories in the Treemap preview, they are assigned separate geometry groups but share the same display name and color, making them visually indistinguishable. To resolve this, ensure that all grouping keys are normalized to a consistent type (e.g., string) before they are used to generate the Vega-Lite specification, particularly when constructing the `domain` for the color scale and the `path` for tooltips.
**superset/mcp_service/chart/treemap_preview.py**
```
"scale": {
"range": _PALETTES[scheme],
# Normalize keys to string to prevent type-based collisions
"domain": list(
dict.fromkeys([str(label), *(str(node["name"]) for node in nodes)])
),
},
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44152:
URL: https://github.com/apache/superset/pull/44152#issuecomment-5627073358
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44152:
URL: https://github.com/apache/superset/pull/44152#discussion_r3986057386
##########
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:
SQL DECIMAL/NUMERIC aggregates can still be `decimal.Decimal` in `ChartDataCommand` records before JSON conversion, so this `int`/`float` guard returns `InvalidTreemapMetric` for data the native chart can render. Could this accept finite `Decimal`/real values while still excluding booleans, with a Decimal regression?
##########
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:
This serializes `CurrencyFormat` as `symbol_position`, but the native Treemap formatter reads `symbolPosition`; an explicit suffix can therefore be ignored and replaced by the locale default in Explore. Should this call `CurrencyFormat.to_form_data()` like the other chart mappers?
##########
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:
These saved Treemap previews query with format fallbacks (50/20/1000 rows) because `_preview_row_limit()` only honors Gauge, so a chart configured with `row_limit: 1` can preview categories and geometry absent from the actual chart while the unsaved path honors 1. Should Treemap use its configured row limit here too?
##########
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:
With `filters` explicit and `temporal_column` omitted, the mapper inserts a neutral filter for the dataset default time column; this branch removes only its marker and then overwrites the saved `adhoc_filters`. A Treemap previously bound to another time column therefore starts querying the default column while retaining a stale subject marker. Could the existing generated binding be preserved unless `temporal_column` is explicitly changed?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #44152:
URL: https://github.com/apache/superset/pull/44152
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44152:
URL: https://github.com/apache/superset/pull/44152#discussion_r3985052914
##########
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:
**Suggestion:** `granularity_sqla` is copied into form data but never into the query's `granularity` field, so treemap time filters use the dataset default column instead of the selected column. [api mismatch]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4267314c5ae7412296411f099c70a115&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4267314c5ae7412296411f099c70a115&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/chart_utils.py
**Line:** 1382:1391
**Comment:**
*Api Mismatch: `granularity_sqla` is copied into form data but never into the query's `granularity` field, so treemap time filters use the dataset default column instead of the selected column.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44152&comment_hash=cc55c8b9f0048e622b33c4b714e605d8f5941aae273b8baa1bcbd013b6136d12&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44152&comment_hash=cc55c8b9f0048e622b33c4b714e605d8f5941aae273b8baa1bcbd013b6136d12&reaction=dislike'>๐</a>
##########
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:
**Suggestion:** The inherited unknown-field validator runs before this adapter, so native fields such as `viz_type` and `datasource` are rejected before they can be removed or converted. [api mismatch]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=39a63376540740199f231d76a976620c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=39a63376540740199f231d76a976620c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/schemas.py
**Line:** 1445:1447
**Comment:**
*Api Mismatch: The inherited unknown-field validator runs before this adapter, so native fields such as `viz_type` and `datasource` are rejected before they can be removed or converted.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44152&comment_hash=30a70580ae7f99d9248048e8667b078141069ba1cc218548b30e7ed42c0582aa&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44152&comment_hash=30a70580ae7f99d9248048e8667b078141069ba1cc218548b30e7ed42c0582aa&reaction=dislike'>๐</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44152:
URL: https://github.com/apache/superset/pull/44152#issuecomment-5627093847
### <span aria-hidden="true">โ
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">๐จ</span> Latest commit | b5ea026840b3ada3f89de410f183f7356f63cbf6 |
|<span aria-hidden="true">๐</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aa34209f40d0b0008f499bf |
|<span aria-hidden="true">๐</span> Deploy Preview | [https://deploy-preview-44152--superset-docs-preview.netlify.app](https://deploy-preview-44152--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">๐ฑ</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">๐ค</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpLCoAgEADQu8y6gckktBt0jPFHC83Biojo7rl7i_eCtFrkhAVgANd491v34XFUpKzB4gXPFmNhwVRzrvclfYYouT5r6HdmnrQimzQFckRkkrbWJfh-) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44152--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44152:
URL: https://github.com/apache/superset/pull/44152#discussion_r3989841104
##########
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:
<!-- Bito Reply -->
The reviewer's suggestion to catch `pydantic.ValidationError` in `resolve_treemap_update_config` is unnecessary because `pydantic_core.ValidationError` inherits from `ValueError`, which is already caught by the existing handler. The current implementation correctly handles these validation errors, and the reported uncaught exception is not reproducible with the provided code.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44152:
URL: https://github.com/apache/superset/pull/44152#discussion_r4046255250
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2925,7 +2991,7 @@ class UpdateChartRequest(ChartRequestNormalizerMixin, QueryCacheControl):
description="Chart ID or UUID",
validation_alias=AliasChoices("identifier", "id", "chart_id"),
)
- config: ChartConfig | None = Field(
+ config: ChartConfig | TreemapChartUpdateConfig | None = Field(
Review Comment:
Confirmed and fixed in fcf5905. You were right โ I reproduced it: `UpdateChartRequest.model_validate({"identifier": 1, "config": {"row_limit": 50}})` parsed as a `TreemapChartUpdateConfig` with `chart_type="treemap_v2"`, so a partial update against an existing pie or table chart would have rewritten its `viz_type` and params. Because the model also uses `extra="ignore"`, essentially any object validated.
`TreemapChartUpdateConfig.chart_type` is now required (`Field(...)`), which is the option you suggested. `TreemapChartConfig` re-declares it with the default, since that subclass is only reached through the discriminated `ChartConfig` union โ which already requires the key in client payloads โ and internal call sites construct it directly.
Both request models now reject a config without the discriminator, while a partial Treemap update that does carry `chart_type` still works and still records only the fields the caller set. Covered by `test_update_request_rejects_config_without_discriminator` and `test_update_request_still_accepts_partial_treemap_with_discriminator`.
##########
superset/mcp_service/chart/query_result.py:
##########
@@ -223,3 +225,83 @@ 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)
Review Comment:
Confirmed and fixed in fcf5905 by normalizing rather than rejecting, as you suggested.
`normalize_chart_query_result` now resolves each `groupby` entry through a new `column_result_label` helper that mirrors the frontend `getColumnLabel` rules exactly โ physical string columns pass through, otherwise `label`, otherwise `sqlExpression` โ so an adhoc Custom SQL column is matched against the output key its rows are actually keyed by.
`treemap_preview.py` needed the same treatment: `treemap_ascii` and `treemap_vega_lite` were indexing rows with the raw `form_data["groupby"]` entries, which would have raised on a dict even once validation passed. Both now go through the shared `treemap_hierarchy_labels` helper.
The uniqueness contract is preserved on the resolved labels rather than the raw entries, so two columns that collapse onto the same output label are still rejected, as are entries that resolve to nothing. Covered by `test_adhoc_hierarchy_column_is_normalized_to_its_output_label`, `test_adhoc_hierarchy_column_without_label_falls_back_to_sql`, `test_adhoc_hierarchy_previews_render_with_resolved_labels` and `test_unresolvable_or_duplicate_hierarchy_columns_stay_rejected`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44152:
URL: https://github.com/apache/superset/pull/44152#issuecomment-5771948494
Follow-up on the โUncaught pydantic ValidationErrorโ suggestion in [review run e5f050](https://github.com/apache/superset/pull/44152#pullrequestreview-5179227336): the suggested extra exception type is unnecessary. Pydantic v2's `ValidationError` subclasses `ValueError`, so the existing `except ValueError` around `resolve_treemap_update_config` already returns the structured validation error.
Commit b9d213c4d4ce9e6b5d03c6ea3e706caf7a41d799 already added `test_resolution_validation_error_is_caught_value_error`, which triggers a real invalid Treemap configuration and asserts that the resulting Pydantic exception is a `ValueError`. I reran it as part of the focused chart tests after merging master: 382 passed, 2 skipped. The installed Pydantic 2.13.4 also confirms that inheritance. No additional exception handler is needed.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44152:
URL: https://github.com/apache/superset/pull/44152#issuecomment-5627832810
Finite CI/review follow-up pushed in b7024009d5c3a0ed7145f2047d54d184d8e3cd94, after normal merge 6d2a824812279fc48146e38cf21fc04fd3007310 of upstream master (89565db). No history rewrite.
- Inspected both attempts of [Python-Unit run 34543844720](https://github.com/apache/superset/actions/runs/34543844720/attempts/2): each passed 15,236 tests; the retry also passed all 1,717 SQL tests at 100% coverage. Both were cancelled by the 30-minute job budget, not failing assertions.
- Applied the identical shared 40-minute budget/comment from #43770/#43771. All test commands, scoped 100% coverage thresholds, artifact uploads and required-gate enforcement remain unchanged.
- Addressed the nullable-format review finding: null uses SMART_NUMBER fallback. The real Vega/Vega-Lite renderer tests both explicit and null formats, checking geometry, labels and warnings.
- Invalid fresh/cached partial previews return a structured validation error; simply skipping hydration would allow an incomplete configuration farther into the pipeline. Saved updates already had an outer exception handler, but now report a targeted validation error before mutation/rollback handling. Registered FastMCP regressions cover malformed saved metrics and missing/unknown cache datasource state.
- Deduplicated compile error classification. Cached static discovery schema compilation with defensive copies; a regression proves one compilation and response-mutation isolation. Registry/feature checks still run on every request.
- Retained defensive validation in independently callable preview helpers, type-narrowing assertions, and Gauge's existing numeric contract. Rejection tests remain parametrized over every saved/cache/export path to ensure none bypass validation. Deferred MCP imports avoid changing application-registration order.
Eight added/strengthened review regression cases fail against b5ea026 and pass after the fixes (the same selection also retains five passing controls). Focused suites: **115 passed**, including actual renderer execution. Full branch-file pre-commit (MyPy, Ruff, Pylint, formatting and workflow audit), compile and diff checks passed. Full MCP/common rerun and exact-head CI are in progress; final evidence will follow. No additional review cycle was 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44152:
URL: https://github.com/apache/superset/pull/44152#issuecomment-5637814806
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44152:
URL: https://github.com/apache/superset/pull/44152#discussion_r4046257621
##########
tests/unit_tests/mcp_service/chart/test_treemap_completeness.py:
##########
@@ -0,0 +1,1174 @@
+# 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.
+
+"""Treemap regressions spanning native requests, query output and update semantics."""
+
+from contextlib import nullcontext
+from copy import deepcopy
+from decimal import Decimal
+from types import SimpleNamespace
+from typing import Any
+from unittest.mock import Mock, patch
+
+import pytest
+from pydantic import ValidationError
+
+from superset.mcp_service.chart.chart_helpers import _build_single_query_dict
+from superset.mcp_service.chart.chart_utils import (
+ map_treemap_config,
+ merge_chart_form_data,
+ resolve_treemap_update_config,
+)
+from superset.mcp_service.chart.query_result import normalize_chart_query_result
+from superset.mcp_service.chart.schemas import (
+ ChartError,
+ GenerateChartRequest,
+ GetChartPreviewRequest,
+ TreemapChartConfig,
+ UpdateChartPreviewRequest,
+ UpdateChartRequest,
+)
+from superset.mcp_service.chart.treemap_preview import treemap_ascii, treemap_vega_lite
+from superset.utils import json
+
+FORM_DATA: dict[str, Any] = {
+ "viz_type": "treemap_v2",
+ "groupby": ["region", "product"],
+ "metric": "revenue",
+ "datasource": "7__table",
+ "color_scheme": "lyftColors",
+ "row_limit": 7,
+ "sort_by_metric": False,
+ "show_labels": False,
+ "show_upper_labels": False,
+ "number_format": ",.1f",
+ "date_format": "%Y",
+ "label_type": "value",
+ "currency_format": {"symbol": "USD", "symbolPosition": "prefix"},
+ "adhoc_filters": [
+ {
+ "expressionType": "SIMPLE",
+ "clause": "WHERE",
+ "subject": "region",
+ "operator": "==",
+ "comparator": "West",
+ }
+ ],
+ "time_range": "2025-01-01 : 2026-01-01",
+ "granularity_sqla": "ds",
+ "template_params": '{"scale": 2}',
+}
+ROWS = [
+ {"region": "West", "product": "A", "revenue": 30},
+ {"region": "West", "product": "B", "revenue": 10},
+ {"region": "East", "product": "A", "revenue": 60},
+]
+
+
+@pytest.mark.parametrize("sort", [False, True])
+@pytest.mark.parametrize("limit", [None, 0, 1, 7])
+def test_hierarchy_query_order_matches_frontend(sort: bool, limit: int | None) -> None:
+ """Metric order has precedence, with hierarchy tie-breakers only when bounded."""
+ form = {**FORM_DATA, "sort_by_metric": sort, "row_limit": limit}
+ query = _build_single_query_dict(form, form["groupby"], [form["metric"]])
+ expected = ([("revenue", False)] if sort else []) + [
+ ("region", True),
+ ("product", True),
+ ]
+ assert query.get("orderby", []) == (expected if limit else [])
+
+
+@pytest.mark.parametrize(
+ "metric",
+ [
+ "revenue",
+ {"name": "revenue", "saved_metric": True},
+ {
+ "expressionType": "SIMPLE",
+ "column": {"column_name": "amount"},
+ "aggregate": "SUM",
+ "label": "revenue",
+ "hasCustomLabel": True,
+ },
+ {"expressionType": "SQL", "sqlExpression": "SUM(amount)", "label": "revenue"},
+ ],
+)
+@pytest.mark.parametrize(
+ "request_class", [GenerateChartRequest, UpdateChartPreviewRequest]
+)
+def test_native_request_roundtrip(metric: Any, request_class: Any) -> None:
+ """Native columns and all valid metric shapes survive actual request validation."""
+ native = {**FORM_DATA, "metric": metric, "slice_id": 1}
+ request = request_class(dataset_id=7, config=native)
+ result = map_treemap_config(request.config)
+ assert result["groupby"] == FORM_DATA["groupby"]
+ assert result["show_labels"] is False
+ assert result["number_format"] == ",.1f"
+ assert result["metric"] == "revenue" or result["metric"]["label"] == "revenue"
+
+
+@pytest.mark.parametrize(
+ "field,value",
+ [
+ ("javascript", "alert(1)"),
+ ("groupby", []),
+ ("metric", {"expressionType": "SQL", "sqlExpression": "SUM(amount)"}),
+ ("row_limit", 10001),
+ ],
+)
+def test_native_requests_do_not_accept_unbounded_or_hostile_controls(
+ field: str, value: Any
+) -> None:
+ """Unknown executable controls and invalid native roles remain validation errors."""
+ with pytest.raises(ValidationError):
+ GenerateChartRequest(dataset_id=7, config={**FORM_DATA, field: value})
+
+
+@pytest.mark.parametrize(
+ "patch_data",
+ [{"show_labels": True}, {"groupby": ["product", "region"], "metric": "revenue"}],
+)
+def test_omitted_same_viz_controls_survive(patch_data: dict[str, Any]) -> None:
+ """Partial and full-role updates preserve all omitted native controls."""
+ request = UpdateChartRequest(
+ identifier=1, config={"chart_type": "treemap_v2", **patch_data}
+ )
+ assert request.config is not None
+ config = resolve_treemap_update_config(request.config, FORM_DATA)
+ merged = merge_chart_form_data(FORM_DATA, map_treemap_config(config), config)
+ for key, value in FORM_DATA.items():
+ if key not in patch_data:
+ assert merged[key] == value
+ for key, value in patch_data.items():
+ assert merged[key] == value
+
+
+@pytest.mark.parametrize(
+ "field",
+ [
+ "color_scheme",
+ "currency_format",
+ "time_range",
+ "granularity_sqla",
+ "template_params",
+ "filters",
+ ],
+)
+def test_nullable_clears_are_not_replaced_by_defaults(field: str) -> None:
+ """Explicit null clears native nullable controls rather than restoring defaults."""
+ request = UpdateChartRequest(
+ identifier=1, config={"chart_type": "treemap_v2", field: None}
+ )
+ assert request.config is not None
+ config = resolve_treemap_update_config(request.config, FORM_DATA)
+ merged = merge_chart_form_data(FORM_DATA, map_treemap_config(config), config)
+ assert ("adhoc_filters" if field == "filters" else field) not in merged
+
+
+def test_empty_filters_and_dataset_rebind() -> None:
+ """Rebinds retain presentation only and scrub stale query/template roles."""
+ config = TreemapChartConfig(
+ groupby=[{"name": "other"}], metric="other_metric", filters=[]
+ )
+ existing = {**FORM_DATA, "metrics": ["stale"], "x_axis": "stale"}
+ merged = merge_chart_form_data(
+ existing, map_treemap_config(config), config, dataset_rebind=True
+ )
+ assert merged["color_scheme"] == "lyftColors"
+ assert merged["show_labels"] is False
+ assert merged["groupby"] == ["other"]
+ assert merged["metric"] == "other_metric"
+ for key in (
+ "adhoc_filters",
+ "time_range",
+ "granularity_sqla",
+ "template_params",
+ "metrics",
+ "x_axis",
+ ):
+ assert key not in merged
+ incomplete = UpdateChartRequest(
+ identifier=1, config={"chart_type": "treemap_v2", "show_labels": True}
+ )
+ assert incomplete.config is not None
+ with pytest.raises(ValidationError):
+ resolve_treemap_update_config(incomplete.config, existing, dataset_rebind=True)
+
+
+@pytest.mark.parametrize(
+ "result",
+ [
+ None,
+ {},
+ {"queries": []},
+ {"queries": [{}]},
+ {"queries": [{"data": {}}]},
+ {"queries": [{"data": [None]}]},
+ {"queries": [{"data": [{"region": "West", "revenue": 2}]}]},
+ {"status": "failed", "message": "Database rejected SQL"},
+ {"queries": [{"error": "SQL failed", "data": ROWS}]},
+ ],
+)
+def test_malformed_results_never_produce_success(result: Any) -> None:
+ """Missing hierarchy and malformed/error envelopes are actionable failures."""
+ assert isinstance(normalize_chart_query_result(result, FORM_DATA), ChartError)
+
+
+@pytest.mark.parametrize(
+ "value", [None, True, "3", float("nan"), float("inf"), 10**400]
+)
+def test_metric_outputs_must_be_finite_numeric(value: Any) -> None:
+ """A single invalid metric fails instead of silently dropping a hierarchy node."""
+ rows = [*ROWS, {**ROWS[0], "revenue": value}]
+ result = normalize_chart_query_result({"queries": [{"data": rows}]}, FORM_DATA)
+ assert isinstance(result, ChartError)
+ assert result.error_type == "InvalidTreemapMetric"
+
+
+def test_result_validation_is_non_mutating_and_chart_specific() -> None:
+ """Other charts retain their existing interpretation of data."""
+ result = {"queries": [{"data": ROWS}]}
+ original = deepcopy(result)
+ assert normalize_chart_query_result(result, FORM_DATA) == original
+ assert result == original
+ unrelated = {"queries": [{"data": [{"x": None}]}]}
+ assert normalize_chart_query_result(unrelated, {"viz_type": "table"}) is unrelated
+
+
+def test_treemap_geometry_area_hierarchy_labels_and_color() -> None:
+ """Coordinates partition parents and area follows the metric, not row count."""
+ preview = treemap_vega_lite(
+ ROWS,
+ {
+ **FORM_DATA,
+ "currency_format": None,
+ "show_labels": True,
+ "number_format": None,
+ },
+ )
+ assert not isinstance(preview, ChartError)
+ spec = preview.specification
+ nodes = spec["data"]["values"]
+ leaves = [node for node in nodes if node["leaf"]]
+ for leaf in leaves:
+ area = (leaf["x1"] - leaf["x0"]) * (leaf["y1"] - leaf["y0"])
+ assert area / (600 * 400) == pytest.approx(leaf["value"] / 100)
+ parent = next(
+ node for node in nodes if node["path"] == leaf["path"].split(" > ")[0]
+ )
+ assert parent["x0"] <= leaf["x0"] <= leaf["x1"] <= parent["x1"]
+ assert parent["y0"] <= leaf["y0"] <= leaf["y1"] <= parent["y1"]
+ assert spec["layer"][0]["mark"]["type"] == "rect"
+ assert spec["layer"][0]["encoding"]["color"]["scale"]["range"][0] == "#EA0B8C"
+ assert spec["layer"][0]["encoding"]["tooltip"][1]["title"] == "revenue"
+ assert spec["layer"][1]["mark"]["type"] == "text"
+ assert "West > A | 30" in treemap_ascii(ROWS, FORM_DATA)
+
+
+@pytest.mark.parametrize(
+ "rows,override",
+ [
+ (ROWS, {"color_scheme": "unavailable"}),
+ (ROWS, {"currency_format": {"symbol": "USD"}}),
+ ([{**ROWS[0], "revenue": -1}], {}),
+ (ROWS * 334, {}),
+ ],
+)
+def test_unsupported_geometry_is_explicit(rows: Any, override: Any) -> None:
+ """Unsupported representation must not fall back to a plausible bar or scatter."""
+ preview = treemap_vega_lite(
+ rows, {**FORM_DATA, "currency_format": None, **override}
+ )
+ assert isinstance(preview, ChartError)
+ assert preview.error_type == "UnsupportedTreemapPreview"
+
+
+@pytest.mark.parametrize("format_name", ["ascii", "table", "vega_lite"])
+def test_saved_and_unsaved_preview_dispatch_match(format_name: str) -> None:
+ """Both preview entry paths use the Treemap representation and result contract."""
+ from superset.mcp_service.chart.preview_utils import generate_preview_from_form_data
+ from superset.mcp_service.chart.tool.get_chart_preview import (
+ ASCIIPreviewStrategy,
+ TablePreviewStrategy,
+ VegaLitePreviewStrategy,
+ )
+
+ form = {**FORM_DATA, "currency_format": None}
+ chart = SimpleNamespace(
+ id=1,
+ viz_type="treemap_v2",
+ slice_name="Treemap",
+ datasource_id=7,
+ datasource_type="table",
+ params=json.dumps(form),
+ )
+ context = SimpleNamespace(
+ queries=[SimpleNamespace(metrics=["revenue"], columns=form["groupby"])]
+ )
+ with (
+ patch(
+ "superset.commands.chart.data.get_data_command.ChartDataCommand"
+ ) as command,
+ patch(
+ "superset.mcp_service.chart.tool.get_chart_preview.build_query_context_from_form_data",
+ return_value=context,
+ ),
+ patch(
+ "superset.mcp_service.chart.chart_helpers.build_query_context_from_form_data",
+ return_value=context,
+ ),
+ patch("superset.extensions.db.session") as session,
+ ):
+ session.get.return_value = Mock(id=7)
+ command.return_value.run.return_value = {"queries": [{"data": ROWS}]}
+ strategy = {
+ "ascii": ASCIIPreviewStrategy,
+ "table": TablePreviewStrategy,
+ "vega_lite": VegaLitePreviewStrategy,
+ }[format_name]
+ saved = strategy(
+ chart, GetChartPreviewRequest(identifier=1, format=format_name)
+ ).generate()
+ unsaved = generate_preview_from_form_data(form, 7, format_name)
+ assert not isinstance(saved, ChartError)
+ assert not isinstance(unsaved, ChartError)
+ if format_name == "vega_lite":
+ assert saved.specification == unsaved.specification
+ elif format_name == "ascii":
+ assert "West > A | 30" in saved.ascii_content
+ assert "West > A | 30" in unsaved.ascii_content
+ else:
+ assert "region" in saved.table_data
+ assert "revenue" in unsaved.table_data
+
+
+@pytest.mark.parametrize("rebind", [False, True])
+def test_saved_update_and_update_preview_payloads_agree(rebind: bool) -> None:
+ """Both save and preview-first update paths apply the same omission contract."""
+ from superset.mcp_service.chart.tool.update_chart import (
+ _build_preview_form_data,
+ _build_update_payload,
+ )
+
+ config = TreemapChartConfig(
+ groupby=[{"name": "region"}], metric="revenue", show_labels=True
+ )
+ request = UpdateChartRequest(
+ identifier=1, config=config, dataset_id=8 if rebind else None
+ )
+ chart = Mock(
+ id=1, datasource_id=7, slice_name="Treemap", params=json.dumps(FORM_DATA)
+ )
+ with patch(
+ "superset.mcp_service.chart.chart_utils._bind_dashboard_time_range_filter"
+ ):
+ preview = _build_preview_form_data(request, chart, parsed_config=config)
+ payload = _build_update_payload(request, chart, parsed_config=config)
+ saved = json.loads(payload["params"])
+ for key in (
+ "color_scheme",
+ "row_limit",
+ "sort_by_metric",
+ "groupby",
+ "metric",
+ "number_format",
+ "show_labels",
+ ):
+ assert saved[key] == preview[key]
+ assert saved["color_scheme"] == "lyftColors"
+ assert saved["show_labels"] is True
+ assert ("template_params" not in saved) == rebind
+
+
+def test_normalization_preserves_explicit_field_set_and_rejects_ambiguity() -> None:
+ """Canonicalization must not turn default values into explicit update intent."""
+ from superset.mcp_service.chart.plugins.treemap import TreemapChartPlugin
+ from superset.mcp_service.common.error_schemas import DatasetContext
+
+ context = DatasetContext(
+ id=7,
+ table_name="sales",
+ database_name="database",
+ available_columns=[{"name": "Region", "type": "STRING"}],
+ available_metrics=[{"name": "Revenue"}],
+ )
+ config = TreemapChartConfig(groupby=[{"name": "region"}], metric="revenue")
+ normalized = TreemapChartPlugin().normalize_column_refs(config, context)
+ assert normalized.groupby[0].name == "Region"
+ assert normalized.metric.name == "Revenue"
+ assert normalized.model_fields_set == config.model_fields_set
+ context.available_columns.append({"name": "REGION", "type": "STRING"})
+ with pytest.raises(ValueError, match="[Aa]mbiguous"):
+ TreemapChartPlugin().normalize_column_refs(config, context)
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize(
+ "metric",
+ [
+ "revenue",
+ {
+ "expressionType": "SIMPLE",
+ "column": {"column_name": "amount"},
+ "aggregate": "SUM",
+ "label": "revenue",
+ },
+ {"expressionType": "SQL", "sqlExpression": "SUM(amount)", "label": "revenue"},
+ ],
+)
+@pytest.mark.parametrize("valid_result", [False, True])
+async def test_registered_generate_chart_native_roundtrip(
+ metric: Any, valid_result: bool
+) -> None:
+ """Call the registered FastMCP tool, retaining real request and compile checks."""
+ from fastmcp import Client
+
+ from superset.mcp_service.app import mcp
+
+ request = GenerateChartRequest(
+ dataset_id=7,
+ config={
+ "chart_type": "treemap_v2",
+ "groupby": ["region", "product"],
+ "metric": metric,
+ "currency_format": {"symbol": "USD", "symbolPosition": "suffix"},
+ },
+ preview_formats=["url"],
+ )
+ dataset = Mock(id=7, datasource_name="sales", table_name="sales")
+ user = Mock(id=1, username="admin", roles=[], groups=[])
+ with (
+ patch("superset.mcp_service.auth.get_user_from_request", return_value=user),
+ patch(
+ "superset.mcp_service.chart.validation.ValidationPipeline.validate_request_with_warnings",
+ return_value=Mock(is_valid=True, request=request, warnings={}, error=None),
+ ),
+ patch(
+ "superset.mcp_service.chart.chart_utils.generate_explore_link",
+ return_value="http://localhost/explore/?form_data_key=treemap",
+ ),
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch(
+ "superset.mcp_service.chart.tool.generate_chart.has_dataset_access",
+ return_value=True,
+ ),
+ patch(
+ "superset.mcp_service.chart.chart_helpers.build_query_context_from_form_data",
+ return_value=Mock(),
+ ),
+ patch(
+ "superset.commands.chart.data.get_data_command.ChartDataCommand"
+ ) as command,
+ ):
+ command.return_value.run.return_value = {
+ "queries": [
+ {"data": ROWS if valid_result else [{"region": "West", "revenue": 1}]}
+ ]
+ }
+ async with Client(mcp) as client:
+ result = await client.call_tool(
+ "generate_chart",
+ {
+ "request": {
+ "dataset_id": 7,
+ "config": {
+ "viz_type": "treemap_v2",
+ "groupby": ["region", "product"],
+ "metric": metric,
+ "currency_format": {
+ "symbol": "USD",
+ "symbolPosition": "suffix",
+ },
+ },
+ "preview_formats": ["url"],
+ }
+ },
+ )
+ data = result.structured_content
+ assert data["success"] is valid_result
+ if valid_result:
+ assert data["form_data"]["currency_format"] == {
+ "symbol": "USD",
+ "symbolPosition": "suffix",
+ }
+ assert data["form_data"]["groupby"] == ["region", "product"]
+ assert data["form_data"]["viz_type"] == "treemap_v2"
+ else:
+ assert data["error"]["error_type"] == "InvalidTreemapResult"
+ command.return_value.validate.assert_called_once()
+
+
+@pytest.mark.parametrize("number_format", [None, ",.1f"])
+def test_vega_scenegraph_renders_nested_metric_geometry(
+ number_format: str | None,
+) -> None:
+ """Compile the specification and inspect rendered rectangles, not a snapshot."""
+ import os
+ import shutil
+ import subprocess
+
+ if not os.environ.get("NODE_PATH"):
Review Comment:
Confirmed โ both parametrizations skip in CI, so you are right that the dict-shape tests alone would not catch a bad `calculate` field. Addressed in fcf5905 with the equivalent check you offered as the alternative, since adding Node packages and `NODE_PATH` to the Python-Unit workflow would mean another change to the very workflow file you asked me to leave alone in the next thread.
`test_vega_spec_field_references_all_resolve` walks the generated specification with nothing but the standard library and asserts that every encoding `field`, and every `datum.<name>` reference inside a `filter`, `calculate` or `expr` string, resolves to either a key present in the emitted data rows or a field produced by an earlier transform in the same layer. It also checks each expression only calls Vega functions from an allowlist. It is parametrized over `number_format` and `show_labels`, which is where the `calculate` transforms differ.
I mutation-tested it rather than trusting it: changing `datum.x0` to `datum.x9` in the label filter expression in `treemap_preview.py` fails the new test, and reverting it passes. There is also `test_vega_spec_field_reference_check_catches_a_bad_calculate`, which pins the detection behaviour itself so the check cannot silently degrade into a no-op.
I left the Node scenegraph test in place โ it still asserts real rendered geometry where Node is available, which the static check deliberately does not attempt.
##########
.github/workflows/superset-python-unittest.yml:
##########
@@ -43,7 +43,10 @@ jobs:
needs: changes
if: needs.changes.outputs.python == 'true'
runs-on: ubuntu-26.04
- timeout-minutes: 30
+ # The full coverage run is followed by independent SQL and semantic-layer
+ # 100% gates. Allow all three coverage-preserving phases to finish under
+ # ordinary runner variance rather than cancelling after successful tests.
+ timeout-minutes: 40
Review Comment:
Dropped, as requested. I resolved the conflict in d08280c4 in favour of master, so `.github/workflows/superset-python-unittest.yml` now matches `origin/master` byte for byte โ `git diff origin/master -- .github/workflows/superset-python-unittest.yml` is empty. The pytest-xdist run and the 45-minute timeout from #44178 are intact, and this branch no longer touches the file at all.
I used a normal merge rather than a rebase so the existing review threads stay anchored to their commits.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44152:
URL: https://github.com/apache/superset/pull/44152#discussion_r3989841158
##########
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:
โ
**Customized review instruction saved!**
**Instruction:**
> Preserve type distinctions in internal grouping keys; do not normalize heterogeneous category values to strings when that would merge distinct categories, even if their display names or colors collide.
**Applied to:**
- `superset/mcp_service/chart/treemap_preview.py`
---
๐ก *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44152:
URL: https://github.com/apache/superset/pull/44152#issuecomment-5636000629
## Maintainer pass complete โ required CI green
Head: **b9d213c4d4ce9e6b5d03c6ea3e706caf7a41d799** (normal push).
- Accept finite Decimal/real Treemap metrics; continue rejecting booleans and nonfinite results.
- Align saved/form-data-key preview queries with the configured row limit.
- Serialize currency settings with native symbolPosition.
- Preserve saved predicates and neutral temporal bindings when adding filters with temporal_column omitted; honor explicit clears and safe rebinds.
- Reverify type-distinct geometry, native request validation, temporal query selection and containment of genuine Pydantic resolution errors through registered FastMCP calls.
Validation:
- **273 focused tests passed**, including public previews, updates and data/export paths.
- Exact-head MCP/common run: **4,459 passed, 1 skipped** (optional BigQuery dependency), in 326.05 seconds. SQL **1,717 passed / 100% coverage** and semantic layers **437 passed / 100% coverage**.
- **31 regression cases fail on the prior head**; the strengthened cases pass after these fixes.
- Real Vega/Vega-Lite rendering, branch-file pre-commit (MyPy/Ruff/Pylint/format/workflow audit), compile and diff checks passed.
- [Exact-head CI](https://github.com/apache/superset/actions/runs/34606901158): **all 13 required checks passed**. Overall: **65 success, 10 skipped, 2 neutral**, no pending/failing checks.
- Python-Unit finished in **31m44s**: **15,277 passed, 7 skipped, 2 xfailed**; SQL **1,717 / 100% coverage**, semantic layers **437 / 100% coverage**; coverage and JUnit uploads passed.
- No retries were needed. No coverage thresholds, tests, required gates or timeout budgets were weakened.
- Individual evidence replies were posted on all eight threads; **all eight are resolved** after final verification. A final fetch found no additional actionable threads. Pydantic inheritance and native-adapter ordering claims were disproved with actual public-path regressions, not assumed fixed from earlier replies.
PR remains Amin-authored/assigned, non-draft, unmerged and conflict-free (MERGEABLE). Human approval is still required (REVIEW_REQUIRED). Worktree is clean. Local live Superset/database/browser validation is unavailable; registered product-path tests mock query execution and persistence; exact-head integration/browser CI passed. The existing real Vega/Vega-Lite renderer regressions ran locally.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): complete Treemap round-trip and previews [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44152:
URL: https://github.com/apache/superset/pull/44152#issuecomment-5639428055
Finite maintainer check on unchanged head **b9d213c4d4ce9e6b5d03c6ea3e706caf7a41d799**.
Regarding the [row-limit/renderer advisory](https://github.com/apache/superset/pull/44152#issuecomment-5637814806): the query bound and geometry bound intentionally differ. A configured limit above 1,000 does **not** itself reject a preview; the renderer checks the number of rows actually returned. The existing documentation explicitly limits geometry to 1,000 rows and promises UnsupportedTreemapPreview for oversized results.
Reproduced through registered get_chart_preview calls for saved, saved-cache and unsaved-cache paths, with row_limit=1001/10000 and actual results of 1000/1001 rows:
- The query keeps the configured row limit in every case.
- 1,000 returned rows produce rect geometry containing every leaf.
- 1,001 returned rows produce the declared UnsupportedTreemapPreview, with no substitute or silently truncated geometry.
The 12 temporary public-path boundary probes plus existing oversized-geometry and real Vega renderer regressions all passed: **18 passed**. Temporary probes were removed; no repository files changed. Capping the query at 1,000 would hide categories present in the chart; raising the geometry bound would change the documented bounded-preview contract. Neither change is appropriate for this advisory.
All eight review threads remain resolved; no new unresolved thread or material defect was found. All **13 required exact-head checks are green**. PR remains non-draft, conflict-free and awaiting human approval (REVIEW_REQUIRED). No commit/push, retry, draft, merge, or acceptance-zone move was made.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org