You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by as...@apache.org on 2026/05/12 13:01:59 UTC
(superset) branch alexandrusoare/fix/multiple-chart-preview-bugs created (now 752434ce9a2)
This is an automated email from the ASF dual-hosted git repository.
alexandrusoare pushed a change to branch alexandrusoare/fix/multiple-chart-preview-bugs
in repository https://gitbox.apache.org/repos/asf/superset.git
at 752434ce9a2 fix(preview): fix chart preview bugs
This branch includes the following new commits:
new 752434ce9a2 fix(preview): fix chart preview bugs
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 01/01: fix(preview): fix chart preview bugs
Posted by as...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
alexandrusoare pushed a commit to branch alexandrusoare/fix/multiple-chart-preview-bugs
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 752434ce9a2b4858eef1636b9b1f1a7457bb2978
Author: alexandrusoare <al...@gmail.com>
AuthorDate: Tue May 12 16:01:33 2026 +0300
fix(preview): fix chart preview bugs
---
.../mcp_service/chart/tool/get_chart_preview.py | 100 ++++++++++++++++-----
.../chart/tool/test_get_chart_preview.py | 99 ++++++++++++++++++++
2 files changed, 176 insertions(+), 23 deletions(-)
diff --git a/superset/mcp_service/chart/tool/get_chart_preview.py b/superset/mcp_service/chart/tool/get_chart_preview.py
index 33281ec6fb2..64758a3a255 100644
--- a/superset/mcp_service/chart/tool/get_chart_preview.py
+++ b/superset/mcp_service/chart/tool/get_chart_preview.py
@@ -146,21 +146,85 @@ class ChartLike(Protocol):
def _build_query_columns(form_data: Dict[str, Any]) -> list[str]:
- """Build query columns list from form_data, including both x_axis and groupby."""
- x_axis_config = form_data.get("x_axis")
- groupby_columns: list[str] = form_data.get("groupby") or []
+ """Build query columns list from form_data, including both x_axis and groupby.
- columns = groupby_columns.copy()
+ Handles chart-type-specific keys:
+ - Standard charts: ``groupby`` + ``x_axis``
+ - Pivot tables: ``groupbyColumns`` + ``groupbyRows`` (when ``groupby`` is absent)
+ - Mixed timeseries: ``groupby_b`` (secondary groupby)
+ """
+ x_axis_config = form_data.get("x_axis")
+ groupby_columns: list[Any] = form_data.get("groupby") or []
+
+ # Pivot tables store dimensions under groupbyColumns / groupbyRows
+ if not groupby_columns:
+ pivot_rows: list[Any] = form_data.get("groupbyRows") or []
+ pivot_cols: list[Any] = form_data.get("groupbyColumns") or []
+ groupby_columns = list(pivot_rows) + list(pivot_cols)
+
+ # Mixed timeseries stores secondary groupby under groupby_b
+ groupby_b: list[Any] = form_data.get("groupby_b") or []
+ for col in groupby_b:
+ if col not in groupby_columns:
+ groupby_columns.append(col)
+
+ # Deduplicate while preserving order
+ seen: set[Any] = set()
+ columns: list[Any] = []
if x_axis_config and isinstance(x_axis_config, str):
- if x_axis_config not in columns:
- columns.insert(0, x_axis_config)
+ columns.append(x_axis_config)
+ seen.add(x_axis_config)
elif x_axis_config and isinstance(x_axis_config, dict):
col_name = x_axis_config.get("column_name")
- if col_name and col_name not in columns:
- columns.insert(0, col_name)
+ if col_name:
+ columns.append(col_name)
+ seen.add(col_name)
+
+ for col in groupby_columns:
+ if col not in seen:
+ columns.append(col)
+ seen.add(col)
+
return columns
+def _build_query_metrics(form_data: Dict[str, Any]) -> list[Any]:
+ """Extract metrics from form_data, handling chart-type variations.
+
+ Handles:
+ - ``metrics`` (plural) — most chart types
+ - ``metric`` (singular) — Pie charts
+ - ``metrics_b`` — secondary y-axis in Mixed Timeseries charts
+ """
+ metrics: list[Any] = list(form_data.get("metrics") or [])
+ if not metrics:
+ singular = form_data.get("metric")
+ if singular:
+ metrics = [singular]
+
+ # Mixed timeseries stores the second y-axis metrics under metrics_b
+ metrics_b: list[Any] = form_data.get("metrics_b") or []
+ for m in metrics_b:
+ if m not in metrics:
+ metrics.append(m)
+
+ return metrics
+
+
+def _build_chart_description(chart: ChartLike) -> str:
+ """Build a human-readable chart description, with hints for special chart types."""
+ base = (
+ f"Preview of {chart.viz_type or 'chart'}: "
+ f"{chart.slice_name or f'Chart {chart.id}'}"
+ )
+ if chart.viz_type == "handlebars":
+ base += (
+ ". Note: Handlebars charts use browser-side template rendering; "
+ "this preview shows the raw underlying data, not the rendered template"
+ )
+ return base
+
+
class PreviewFormatStrategy:
"""Base class for preview format strategies."""
@@ -215,9 +279,7 @@ class ASCIIPreviewStrategy(PreviewFormatStrategy):
)
# Build query for chart data
- x_axis_config = form_data.get("x_axis")
- groupby_columns = form_data.get("groupby", [])
- metrics = form_data.get("metrics", [])
+ metrics = _build_query_metrics(form_data)
# Table charts in raw mode use all_columns or columns
all_columns = form_data.get("all_columns", [])
@@ -225,12 +287,7 @@ class ASCIIPreviewStrategy(PreviewFormatStrategy):
if form_data.get("query_mode") == "raw" and (all_columns or raw_columns):
columns = list(all_columns or raw_columns)
else:
- columns = groupby_columns.copy()
- if x_axis_config and isinstance(x_axis_config, str):
- columns.append(x_axis_config)
- elif x_axis_config and isinstance(x_axis_config, dict):
- if "column_name" in x_axis_config:
- columns.append(x_axis_config["column_name"])
+ columns = _build_query_columns(form_data)
if not columns and not metrics:
return ChartError(
@@ -327,7 +384,7 @@ class TablePreviewStrategy(PreviewFormatStrategy):
{
"filters": form_data.get("filters", []),
"columns": columns,
- "metrics": form_data.get("metrics", []),
+ "metrics": _build_query_metrics(form_data),
"row_limit": 20,
"order_desc": True,
}
@@ -433,7 +490,7 @@ class VegaLitePreviewStrategy(PreviewFormatStrategy):
{
"filters": form_data.get("filters", []),
"columns": columns,
- "metrics": form_data.get("metrics", []),
+ "metrics": _build_query_metrics(form_data),
"row_limit": 1000, # More data for visualization
"order_desc": True,
}
@@ -1371,10 +1428,7 @@ async def _get_chart_preview_internal( # noqa: C901
chart_type=chart.viz_type or "unknown",
explore_url=f"{get_superset_base_url()}/explore/?slice_id={chart.id}",
content=content,
- chart_description=(
- f"Preview of {chart.viz_type or 'chart'}: "
- f"{chart.slice_name or f'Chart {chart.id}'}"
- ),
+ chart_description=_build_chart_description(chart),
accessibility=accessibility,
performance=performance,
)
diff --git a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py
index e5fcf909f7f..a0cf780c66a 100644
--- a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py
+++ b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py
@@ -19,6 +19,8 @@
Unit tests for get_chart_preview MCP tool
"""
+from unittest.mock import MagicMock
+
import pytest
from superset.mcp_service.chart.schemas import (
@@ -33,6 +35,9 @@ from superset.mcp_service.chart.schemas import (
VegaLitePreview,
)
from superset.mcp_service.chart.tool.get_chart_preview import (
+ _build_chart_description,
+ _build_query_columns,
+ _build_query_metrics,
_sanitize_chart_preview_for_llm_context,
)
from superset.mcp_service.utils import sanitize_for_llm_context
@@ -597,6 +602,100 @@ Market Share
# These demonstrate the expected ASCII formats for different chart types
+def test_build_query_columns_standard_groupby():
+ form_data = {"x_axis": "date", "groupby": ["region"]}
+ assert _build_query_columns(form_data) == ["date", "region"]
+
+
+def test_build_query_columns_pivot_table():
+ """Pivot tables use groupbyColumns/groupbyRows instead of groupby."""
+ form_data = {
+ "groupbyRows": ["product"],
+ "groupbyColumns": ["region"],
+ "metrics": [{"label": "SUM(sales)"}],
+ }
+ columns = _build_query_columns(form_data)
+ assert "product" in columns
+ assert "region" in columns
+
+
+def test_build_query_columns_mixed_timeseries_groupby_b():
+ """Mixed timeseries stores secondary groupby under groupby_b."""
+ form_data = {
+ "x_axis": "date",
+ "groupby": ["series_a"],
+ "groupby_b": ["series_b"],
+ }
+ columns = _build_query_columns(form_data)
+ assert "date" in columns
+ assert "series_a" in columns
+ assert "series_b" in columns
+
+
+def test_build_query_columns_no_duplicates():
+ form_data = {
+ "x_axis": "date",
+ "groupby": ["date", "region"],
+ }
+ columns = _build_query_columns(form_data)
+ assert columns.count("date") == 1
+
+
+def test_build_query_metrics_plural():
+ form_data = {"metrics": [{"label": "SUM(sales)"}, {"label": "COUNT(*)"}]}
+ assert _build_query_metrics(form_data) == [
+ {"label": "SUM(sales)"},
+ {"label": "COUNT(*)"},
+ ]
+
+
+def test_build_query_metrics_singular_for_pie():
+ """Pie charts use metric (singular) instead of metrics."""
+ form_data = {"metric": "SUM(amount)"}
+ assert _build_query_metrics(form_data) == ["SUM(amount)"]
+
+
+def test_build_query_metrics_mixed_timeseries():
+ """Mixed timeseries stores secondary metrics under metrics_b."""
+ form_data = {
+ "metrics": [{"label": "SUM(revenue)"}],
+ "metrics_b": [{"label": "AVG(cost)"}],
+ }
+ result = _build_query_metrics(form_data)
+ assert {"label": "SUM(revenue)"} in result
+ assert {"label": "AVG(cost)"} in result
+
+
+def test_build_query_metrics_empty():
+ assert _build_query_metrics({}) == []
+
+
+def test_build_query_columns_pivot_overlapping_rows_and_columns():
+ """Overlapping values in groupbyRows and groupbyColumns are deduplicated."""
+ form_data = {
+ "groupbyRows": ["country", "region"],
+ "groupbyColumns": ["region", "city"],
+ }
+ columns = _build_query_columns(form_data)
+ assert columns.count("region") == 1
+ assert "country" in columns
+ assert "city" in columns
+
+
+def test_build_chart_description_standard():
+ chart = MagicMock(viz_type="line", slice_name="Sales Trend", id=1)
+ desc = _build_chart_description(chart)
+ assert desc == "Preview of line: Sales Trend"
+
+
+def test_build_chart_description_handlebars():
+ chart = MagicMock(viz_type="handlebars", slice_name="My Template", id=2)
+ desc = _build_chart_description(chart)
+ assert "Handlebars" in desc
+ assert "raw underlying data" in desc
+ assert "template rendering" in desc
+
+
class TestDetachedInstanceError:
"""Tests that DetachedInstanceError is handled gracefully.