feat(mcp): add pie, pivot table, and mixed timeseries chart creation support#38375
Merged
Antonio-RiveroMartnez merged 2 commits intoMar 6, 2026
Conversation
Contributor
Code Review Agent Run #5a6b25Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
dosubot
Bot
added
change:backend
![codeant-ai-for-open-source[bot]](https://avatars.github.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+142
to
+174
| @staticmethod | ||
| def _pre_validate_chart_type( | ||
| chart_type: str, | ||
| config: Dict[str, Any], | ||
| ) -> Tuple[bool, ChartGenerationError | None]: | ||
| """Validate chart type and dispatch to type-specific pre-validation.""" | ||
| chart_type_validators = { | ||
| "xy": SchemaValidator._pre_validate_xy_config, | ||
| "table": SchemaValidator._pre_validate_table_config, | ||
| "pie": SchemaValidator._pre_validate_pie_config, | ||
| "pivot_table": SchemaValidator._pre_validate_pivot_table_config, | ||
| "mixed_timeseries": SchemaValidator._pre_validate_mixed_timeseries_config, | ||
| } | ||
|
|
||
| if chart_type not in chart_type_validators: | ||
| valid_types = ", ".join(chart_type_validators.keys()) | ||
| return False, ChartGenerationError( | ||
| error_type="invalid_chart_type", | ||
| message=f"Invalid chart_type: '{chart_type}'", | ||
| details=f"Chart type '{chart_type}' is not supported. Must be 'xy' or " | ||
| f"'table'", | ||
| details=f"Chart type '{chart_type}' is not supported. " | ||
| f"Must be one of: {valid_types}", | ||
| suggestions=[ | ||
| "Use 'chart_type': 'xy' for line, bar, area, or scatter charts", | ||
| "Use 'chart_type': 'table' for tabular data display", | ||
| "Use 'chart_type': 'pie' for pie or donut charts", | ||
| "Use 'chart_type': 'pivot_table' for interactive pivot tables", | ||
| "Use 'chart_type': 'mixed_timeseries' for dual-series time charts", | ||
| "Check spelling and ensure lowercase", | ||
| ], | ||
| error_code="INVALID_CHART_TYPE", | ||
| ) | ||
|
|
||
| # Pre-validate structure based on chart type | ||
| if chart_type == "xy": | ||
| return SchemaValidator._pre_validate_xy_config(config) | ||
| elif chart_type == "table": | ||
| return SchemaValidator._pre_validate_table_config(config) | ||
|
|
||
| return True, None | ||
| return chart_type_validators[chart_type](config) |
Contributor
There was a problem hiding this comment.
Suggestion: If chart_type is not a string (for example, a list or dict due to malformed client input), checking membership in chart_type_validators will raise a TypeError instead of returning a structured ChartGenerationError, causing the validator to crash on bad input instead of failing gracefully. To avoid this, explicitly validate that chart_type is a string before using it as a dict key and treat non-string values as an invalid chart type. [type error]
Severity Level: Major ⚠️
- ❌ Malformed MCP chart requests can crash validation flow.
- ⚠️ MCP chart generation may return HTTP 500 on bad input.
- ⚠️ Clients receive no structured `ChartGenerationError` response.
- ⚠️ Harder to debug invalid chart_type issues for integrators.
Suggested change
| @staticmethod | |
| def _pre_validate_chart_type( | |
| chart_type: str, | |
| config: Dict[str, Any], | |
| ) -> Tuple[bool, ChartGenerationError | None]: | |
| """Validate chart type and dispatch to type-specific pre-validation.""" | |
| chart_type_validators = { | |
| "xy": SchemaValidator._pre_validate_xy_config, | |
| "table": SchemaValidator._pre_validate_table_config, | |
| "pie": SchemaValidator._pre_validate_pie_config, | |
| "pivot_table": SchemaValidator._pre_validate_pivot_table_config, | |
| "mixed_timeseries": SchemaValidator._pre_validate_mixed_timeseries_config, | |
| } | |
| if chart_type not in chart_type_validators: | |
| valid_types = ", ".join(chart_type_validators.keys()) | |
| return False, ChartGenerationError( | |
| error_type="invalid_chart_type", | |
| message=f"Invalid chart_type: '{chart_type}'", | |
| details=f"Chart type '{chart_type}' is not supported. Must be 'xy' or " | |
| f"'table'", | |
| details=f"Chart type '{chart_type}' is not supported. " | |
| f"Must be one of: {valid_types}", | |
| suggestions=[ | |
| "Use 'chart_type': 'xy' for line, bar, area, or scatter charts", | |
| "Use 'chart_type': 'table' for tabular data display", | |
| "Use 'chart_type': 'pie' for pie or donut charts", | |
| "Use 'chart_type': 'pivot_table' for interactive pivot tables", | |
| "Use 'chart_type': 'mixed_timeseries' for dual-series time charts", | |
| "Check spelling and ensure lowercase", | |
| ], | |
| error_code="INVALID_CHART_TYPE", | |
| ) | |
| # Pre-validate structure based on chart type | |
| if chart_type == "xy": | |
| return SchemaValidator._pre_validate_xy_config(config) | |
| elif chart_type == "table": | |
| return SchemaValidator._pre_validate_table_config(config) | |
| return True, None | |
| return chart_type_validators[chart_type](config) | |
| @staticmethod | |
| def _pre_validate_chart_type( | |
| chart_type: str, | |
| config: Dict[str, Any], | |
| ) -> Tuple[bool, ChartGenerationError | None]: | |
| """Validate chart type and dispatch to type-specific pre-validation.""" | |
| chart_type_validators = { | |
| "xy": SchemaValidator._pre_validate_xy_config, | |
| "table": SchemaValidator._pre_validate_table_config, | |
| "pie": SchemaValidator._pre_validate_pie_config, | |
| "pivot_table": SchemaValidator._pre_validate_pivot_table_config, | |
| "mixed_timeseries": SchemaValidator._pre_validate_mixed_timeseries_config, | |
| } | |
| # Ensure chart_type is a string before using it as a dict key | |
| if not isinstance(chart_type, str): | |
| valid_types = ", ".join(chart_type_validators.keys()) | |
| return False, ChartGenerationError( | |
| error_type="invalid_chart_type", | |
| message=f"Invalid chart_type type: {type(chart_type).__name__}", | |
| details="The 'chart_type' field must be a string. " | |
| f"Must be one of: {valid_types}", | |
| suggestions=[ | |
| "Set 'chart_type' to a string value such as 'xy', 'table', " | |
| "'pie', 'pivot_table', or 'mixed_timeseries'", | |
| "Check that your JSON encoder is not serializing chart_type " | |
| "as an object or array", | |
| ], | |
| error_code="INVALID_CHART_TYPE", | |
| ) | |
| if chart_type not in chart_type_validators: | |
| valid_types = ", ".join(chart_type_validators.keys()) | |
| return False, ChartGenerationError( | |
| error_type="invalid_chart_type", | |
| message=f"Invalid chart_type: '{chart_type}'", | |
| details=f"Chart type '{chart_type}' is not supported. " | |
| f"Must be one of: {valid_types}", | |
| suggestions=[ | |
| "Use 'chart_type': 'xy' for line, bar, area, or scatter charts", | |
| "Use 'chart_type': 'table' for tabular data display", | |
| "Use 'chart_type': 'pie' for pie or donut charts", | |
| "Use 'chart_type': 'pivot_table' for interactive pivot tables", | |
| "Use 'chart_type': 'mixed_timeseries' for dual-series time charts", | |
| "Check spelling and ensure lowercase", | |
| ], | |
| error_code="INVALID_CHART_TYPE", | |
| ) | |
| return chart_type_validators[chart_type](config) |
Steps of Reproduction ✅
1. In any Python context (tests, MCP service), call `SchemaValidator.validate_request()`
from `superset/mcp_service/chart/validation/schema_validator.py` with a payload like:
`{"dataset_id": 1, "config": {"chart_type": ["xy"]}}`.
2. Inside `validate_request()` (same file, lines ~36–58), `_pre_validate()` is invoked,
which extracts `config` and then `chart_type = config.get("chart_type")`.
3. Since `chart_type` is a non-empty list (`["xy"]`), the truthiness check `if not
chart_type:` in `_pre_validate()` (same file, lines ~84–101) is skipped, and it calls
`SchemaValidator._pre_validate_chart_type(chart_type, config)` at line 140.
4. In `_pre_validate_chart_type()` at
`superset/mcp_service/chart/validation/schema_validator.py:143`, Python evaluates `if
chart_type not in chart_type_validators:` where `chart_type_validators` is a dict. Because
`chart_type` is a list (an unhashable type), this membership test raises `TypeError:
unhashable type: 'list'`, causing `validate_request()` to raise an uncaught exception
instead of returning a structured `ChartGenerationError` with
`error_code="INVALID_CHART_TYPE"`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/validation/schema_validator.py
**Line:** 142:174
**Comment:**
*Type Error: If `chart_type` is not a string (for example, a list or dict due to malformed client input), checking membership in `chart_type_validators` will raise a `TypeError` instead of returning a structured `ChartGenerationError`, causing the validator to crash on bad input instead of failing gracefully. To avoid this, explicitly validate that `chart_type` is a string before using it as a dict key and treat non-string values as an invalid chart type.
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.
Contributor
Author
There was a problem hiding this comment.
Good catch - fixed in 3fbc4ce. Added isinstance(chart_type, str) guard before the dict lookup so non-string values (list, dict, int, etc.) return a structured ChartGenerationError instead of raising TypeError. Also added parametrized tests covering list, dict, int, and bool inputs.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (99.24%) is below the target coverage (100.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #38375 +/- ##
==========================================
+ Coverage 64.78% 64.96% +0.18%
==========================================
Files 1815 2515 +700
Lines 71917 126848 +54931
Branches 22915 29248 +6333
==========================================
+ Hits 46591 82409 +35818
- Misses 25326 43010 +17684
- Partials 0 1429 +1429
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
Re: Bito suggestion about inaccurate module docstring in The docstring is actually accurate - the test file contains all the test classes mentioned: |
Contributor
Code Review Agent Run #dd1dcdActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
aminghadersohi
force-pushed
the
amin/mcp-chart-types-pie-pivot-mixed
branch
from
3fbc4ce to
5366006
Compare
Contributor
Code Review Agent Run #f45be1Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
aminghadersohi
force-pushed
the
amin/mcp-chart-types-pie-pivot-mixed
branch
from
5366006 to
32d7dd3
Compare
Contributor
Code Review Agent Run #5875b0Actionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…support Add three new chart types to the MCP service chart creation pipeline: - Pie/Donut charts (chart_type="pie"): Category-based proportional visualization with configurable labels, donut mode, and slice limits - Pivot Table V2 (chart_type="pivot_table"): Interactive cross-tabulation with row/column grouping, aggregation functions, and totals - Mixed Timeseries (chart_type="mixed_timeseries"): Dual-series charts combining two chart types (e.g., line + bar) on the same time axis with independent Y-axes Changes: - Add PieChartConfig, PivotTableChartConfig, MixedTimeseriesChartConfig Pydantic schemas with full validation - Add map_pie_config, map_pivot_table_config, map_mixed_timeseries_config form_data mapping functions - Update ChartConfig discriminated union to include new types - Update SchemaValidator with pre-validation for new chart types - Update MCP instructions to list new creatable chart types - Add 58 unit tests covering schemas, form_data mapping, chart name generation, and schema validator pre-validation
Add isinstance(chart_type, str) check before dict lookup in _pre_validate_chart_type to prevent TypeError on malformed input (e.g., chart_type passed as list or dict). Returns structured ChartGenerationError instead of crashing.
aminghadersohi
force-pushed
the
amin/mcp-chart-types-pie-pivot-mixed
branch
from
32d7dd3 to
cc2f466
Compare
codeant-ai-for-open-source
Bot
added
the
size:XXL
![bito-code-review[bot]](https://avatars.github.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #bc324c
Actionable Suggestions - 1
-
tests/unit_tests/mcp_service/chart/test_new_chart_types.py - 1
- Incomplete Test Coverage · Line 1-360
Additional Suggestions - 1
-
tests/unit_tests/mcp_service/chart/test_new_chart_types.py - 1
-
Unused Imports · Line 30-45Several imports are declared but not used in the test code, which can trigger linter warnings and clutter the codebase.
-
Review Details
-
Files reviewed - 5 · Commit Range:
41f0a76..cc2f466- superset/mcp_service/app.py
- superset/mcp_service/chart/chart_utils.py
- superset/mcp_service/chart/schemas.py
- superset/mcp_service/chart/validation/schema_validator.py
- tests/unit_tests/mcp_service/chart/test_new_chart_types.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- 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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Comment on lines
+1
to
+360
| # 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. | ||
|
|
||
| """ | ||
| Unit tests for new MCP chart types: pie, pivot_table, mixed_timeseries. | ||
|
|
||
| Tests cover schema validation, form_data mapping, chart name generation, | ||
| and schema validator pre-validation for all three new chart types. | ||
| """ | ||
|
|
||
| from unittest.mock import patch | ||
|
|
||
| import pytest | ||
| from pydantic import ValidationError | ||
|
|
||
| from superset.mcp_service.chart.chart_utils import ( | ||
| generate_chart_name, | ||
| map_config_to_form_data, | ||
| map_mixed_timeseries_config, | ||
| map_pie_config, | ||
| map_pivot_table_config, | ||
| ) | ||
| from superset.mcp_service.chart.schemas import ( | ||
| AxisConfig, | ||
| ColumnRef, | ||
| FilterConfig, | ||
| MixedTimeseriesChartConfig, | ||
| PieChartConfig, | ||
| PivotTableChartConfig, | ||
| ) | ||
| from superset.mcp_service.chart.validation.schema_validator import SchemaValidator | ||
|
|
||
| # ============================================================ | ||
| # Pie Chart Schema Tests | ||
| # ============================================================ | ||
|
|
||
|
|
||
| class TestPieChartConfigSchema: | ||
| """Test PieChartConfig Pydantic schema validation.""" | ||
|
|
||
| def test_basic_pie_config(self) -> None: | ||
| config = PieChartConfig( | ||
| chart_type="pie", | ||
| dimension=ColumnRef(name="product"), | ||
| metric=ColumnRef(name="revenue", aggregate="SUM"), | ||
| ) | ||
| assert config.chart_type == "pie" | ||
| assert config.dimension.name == "product" | ||
| assert config.metric.aggregate == "SUM" | ||
| assert config.donut is False | ||
| assert config.show_labels is True | ||
| assert config.label_type == "key_value_percent" | ||
|
|
||
| def test_donut_chart_config(self) -> None: | ||
| config = PieChartConfig( | ||
| chart_type="pie", | ||
| dimension=ColumnRef(name="category"), | ||
| metric=ColumnRef(name="count", aggregate="COUNT"), | ||
| donut=True, | ||
| inner_radius=40, | ||
| outer_radius=80, | ||
| ) | ||
| assert config.donut is True | ||
| assert config.inner_radius == 40 | ||
| assert config.outer_radius == 80 | ||
|
|
||
| def test_pie_config_with_all_options(self) -> None: | ||
| config = PieChartConfig( | ||
| chart_type="pie", | ||
| dimension=ColumnRef(name="region"), | ||
| metric=ColumnRef(name="sales", aggregate="SUM", label="Total Sales"), | ||
| donut=True, | ||
| show_labels=False, | ||
| label_type="percent", | ||
| sort_by_metric=False, | ||
| show_legend=False, | ||
| row_limit=50, | ||
| number_format="$,.2f", | ||
| show_total=True, | ||
| labels_outside=False, | ||
| outer_radius=90, | ||
| inner_radius=50, | ||
| filters=[FilterConfig(column="status", op="=", value="active")], | ||
| ) | ||
| assert config.show_labels is False | ||
| assert config.label_type == "percent" | ||
| assert config.row_limit == 50 | ||
| assert config.show_total is True | ||
| assert config.filters is not None | ||
| assert len(config.filters) == 1 | ||
|
|
||
| def test_pie_config_rejects_extra_fields(self) -> None: | ||
| with pytest.raises(ValidationError): | ||
| PieChartConfig( | ||
| chart_type="pie", | ||
| dimension=ColumnRef(name="product"), | ||
| metric=ColumnRef(name="revenue", aggregate="SUM"), | ||
| unknown_field="bad", | ||
| ) | ||
|
|
||
| def test_pie_config_missing_dimension(self) -> None: | ||
| with pytest.raises(ValidationError): | ||
| PieChartConfig( | ||
| chart_type="pie", | ||
| metric=ColumnRef(name="revenue", aggregate="SUM"), | ||
| ) | ||
|
|
||
| def test_pie_config_missing_metric(self) -> None: | ||
| with pytest.raises(ValidationError): | ||
| PieChartConfig( | ||
| chart_type="pie", | ||
| dimension=ColumnRef(name="product"), | ||
| ) | ||
|
|
||
| def test_pie_config_row_limit_bounds(self) -> None: | ||
| with pytest.raises(ValidationError): | ||
| PieChartConfig( | ||
| chart_type="pie", | ||
| dimension=ColumnRef(name="product"), | ||
| metric=ColumnRef(name="revenue", aggregate="SUM"), | ||
| row_limit=0, | ||
| ) | ||
|
|
||
| def test_pie_config_valid_label_types(self) -> None: | ||
| for label_type in [ | ||
| "key", | ||
| "value", | ||
| "percent", | ||
| "key_value", | ||
| "key_percent", | ||
| "key_value_percent", | ||
| "value_percent", | ||
| ]: | ||
| config = PieChartConfig( | ||
| chart_type="pie", | ||
| dimension=ColumnRef(name="product"), | ||
| metric=ColumnRef(name="revenue", aggregate="SUM"), | ||
| label_type=label_type, | ||
| ) | ||
| assert config.label_type == label_type | ||
|
|
||
|
|
||
| # ============================================================ | ||
| # Pie Chart Form Data Mapping Tests | ||
| # ============================================================ | ||
|
|
||
|
|
||
| class TestMapPieConfig: | ||
| """Test map_pie_config form_data generation.""" | ||
|
|
||
| def test_basic_pie_form_data(self) -> None: | ||
| config = PieChartConfig( | ||
| chart_type="pie", | ||
| dimension=ColumnRef(name="product"), | ||
| metric=ColumnRef(name="revenue", aggregate="SUM"), | ||
| ) | ||
| result = map_pie_config(config) | ||
|
|
||
| assert result["viz_type"] == "pie" | ||
| assert result["groupby"] == ["product"] | ||
| assert result["metric"]["aggregate"] == "SUM" | ||
| assert result["metric"]["column"]["column_name"] == "revenue" | ||
| assert result["show_labels"] is True | ||
| assert result["show_legend"] is True | ||
| assert result["label_type"] == "key_value_percent" | ||
| assert result["sort_by_metric"] is True | ||
| assert result["row_limit"] == 100 | ||
| assert result["donut"] is False | ||
| assert result["color_scheme"] == "supersetColors" | ||
|
|
||
| def test_donut_form_data(self) -> None: | ||
| config = PieChartConfig( | ||
| chart_type="pie", | ||
| dimension=ColumnRef(name="category"), | ||
| metric=ColumnRef(name="count", aggregate="COUNT"), | ||
| donut=True, | ||
| inner_radius=40, | ||
| outer_radius=80, | ||
| ) | ||
| result = map_pie_config(config) | ||
|
|
||
| assert result["donut"] is True | ||
| assert result["innerRadius"] == 40 | ||
| assert result["outerRadius"] == 80 | ||
|
|
||
| def test_pie_form_data_with_filters(self) -> None: | ||
| config = PieChartConfig( | ||
| chart_type="pie", | ||
| dimension=ColumnRef(name="product"), | ||
| metric=ColumnRef(name="revenue", aggregate="SUM"), | ||
| filters=[FilterConfig(column="region", op="=", value="US")], | ||
| ) | ||
| result = map_pie_config(config) | ||
|
|
||
| assert "adhoc_filters" in result | ||
| assert len(result["adhoc_filters"]) == 1 | ||
| assert result["adhoc_filters"][0]["subject"] == "region" | ||
| assert result["adhoc_filters"][0]["operator"] == "==" | ||
| assert result["adhoc_filters"][0]["comparator"] == "US" | ||
|
|
||
| def test_pie_form_data_custom_options(self) -> None: | ||
| config = PieChartConfig( | ||
| chart_type="pie", | ||
| dimension=ColumnRef(name="status"), | ||
| metric=ColumnRef(name="count", aggregate="COUNT"), | ||
| show_labels=False, | ||
| label_type="percent", | ||
| show_legend=False, | ||
| number_format="$,.2f", | ||
| show_total=True, | ||
| labels_outside=False, | ||
| ) | ||
| result = map_pie_config(config) | ||
|
|
||
| assert result["show_labels"] is False | ||
| assert result["label_type"] == "percent" | ||
| assert result["show_legend"] is False | ||
| assert result["number_format"] == "$,.2f" | ||
| assert result["show_total"] is True | ||
| assert result["labels_outside"] is False | ||
|
|
||
| def test_pie_form_data_custom_metric_label(self) -> None: | ||
| config = PieChartConfig( | ||
| chart_type="pie", | ||
| dimension=ColumnRef(name="product"), | ||
| metric=ColumnRef(name="revenue", aggregate="SUM", label="Total Revenue"), | ||
| ) | ||
| result = map_pie_config(config) | ||
|
|
||
| assert result["metric"]["label"] == "Total Revenue" | ||
| assert result["metric"]["hasCustomLabel"] is True | ||
|
|
||
|
|
||
| # ============================================================ | ||
| # Pivot Table Schema Tests | ||
| # ============================================================ | ||
|
|
||
|
|
||
| class TestPivotTableChartConfigSchema: | ||
| """Test PivotTableChartConfig Pydantic schema validation.""" | ||
|
|
||
| def test_basic_pivot_table_config(self) -> None: | ||
| config = PivotTableChartConfig( | ||
| chart_type="pivot_table", | ||
| rows=[ColumnRef(name="product")], | ||
| metrics=[ColumnRef(name="revenue", aggregate="SUM")], | ||
| ) | ||
| assert config.chart_type == "pivot_table" | ||
| assert len(config.rows) == 1 | ||
| assert len(config.metrics) == 1 | ||
| assert config.aggregate_function == "Sum" | ||
| assert config.show_row_totals is True | ||
| assert config.show_column_totals is True | ||
|
|
||
| def test_pivot_table_with_columns(self) -> None: | ||
| config = PivotTableChartConfig( | ||
| chart_type="pivot_table", | ||
| rows=[ColumnRef(name="product")], | ||
| columns=[ColumnRef(name="region")], | ||
| metrics=[ColumnRef(name="revenue", aggregate="SUM")], | ||
| ) | ||
| assert config.columns is not None | ||
| assert len(config.columns) == 1 | ||
| assert config.columns[0].name == "region" | ||
|
|
||
| def test_pivot_table_with_all_options(self) -> None: | ||
| config = PivotTableChartConfig( | ||
| chart_type="pivot_table", | ||
| rows=[ColumnRef(name="product"), ColumnRef(name="category")], | ||
| columns=[ColumnRef(name="region")], | ||
| metrics=[ | ||
| ColumnRef(name="revenue", aggregate="SUM"), | ||
| ColumnRef(name="orders", aggregate="COUNT"), | ||
| ], | ||
| aggregate_function="Average", | ||
| show_row_totals=False, | ||
| show_column_totals=False, | ||
| transpose=True, | ||
| combine_metric=True, | ||
| row_limit=5000, | ||
| value_format="$,.2f", | ||
| filters=[FilterConfig(column="year", op="=", value=2024)], | ||
| ) | ||
| assert config.aggregate_function == "Average" | ||
| assert config.show_row_totals is False | ||
| assert config.transpose is True | ||
| assert config.combine_metric is True | ||
| assert config.row_limit == 5000 | ||
|
|
||
| def test_pivot_table_missing_rows(self) -> None: | ||
| with pytest.raises(ValidationError): | ||
| PivotTableChartConfig( | ||
| chart_type="pivot_table", | ||
| metrics=[ColumnRef(name="revenue", aggregate="SUM")], | ||
| ) | ||
|
|
||
| def test_pivot_table_missing_metrics(self) -> None: | ||
| with pytest.raises(ValidationError): | ||
| PivotTableChartConfig( | ||
| chart_type="pivot_table", | ||
| rows=[ColumnRef(name="product")], | ||
| ) | ||
|
|
||
| def test_pivot_table_empty_rows(self) -> None: | ||
| with pytest.raises(ValidationError): | ||
| PivotTableChartConfig( | ||
| chart_type="pivot_table", | ||
| rows=[], | ||
| metrics=[ColumnRef(name="revenue", aggregate="SUM")], | ||
| ) | ||
|
|
||
| def test_pivot_table_rejects_extra_fields(self) -> None: | ||
| with pytest.raises(ValidationError): | ||
| PivotTableChartConfig( | ||
| chart_type="pivot_table", | ||
| rows=[ColumnRef(name="product")], | ||
| metrics=[ColumnRef(name="revenue", aggregate="SUM")], | ||
| unknown_field="bad", | ||
| ) | ||
|
|
||
| def test_pivot_table_valid_aggregate_functions(self) -> None: | ||
| for agg in ["Sum", "Average", "Median", "Count", "Minimum", "Maximum"]: | ||
| config = PivotTableChartConfig( | ||
| chart_type="pivot_table", | ||
| rows=[ColumnRef(name="product")], | ||
| metrics=[ColumnRef(name="revenue", aggregate="SUM")], | ||
| aggregate_function=agg, | ||
| ) | ||
| assert config.aggregate_function == agg | ||
|
|
||
|
|
||
| # ============================================================ | ||
| # Pivot Table Form Data Mapping Tests | ||
| # ============================================================ | ||
|
|
||
|
|
||
| class TestMapPivotTableConfig: | ||
| """Test map_pivot_table_config form_data generation.""" | ||
|
|
||
| def test_basic_pivot_form_data(self) -> None: | ||
| config = PivotTableChartConfig( | ||
| chart_type="pivot_table", | ||
| rows=[ColumnRef(name="product")], | ||
| metrics=[ColumnRef(name="revenue", aggregate="SUM")], | ||
| ) | ||
| result = map_pivot_table_config(config) |
Contributor
There was a problem hiding this comment.
Incomplete Test Coverage
The test file's docstring indicates coverage for mixed_timeseries charts, chart name generation, and schema validator pre-validation, but these are not implemented in the tests. This could allow bugs in untested code paths to go undetected.
Code Review Run #bc324c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Antonio-RiveroMartnez
approved these changes
Mar 6, 2026
aminghadersohi
added a commit
to aminghadersohi/superset
that referenced
this pull request
Mar 17, 2026
qfcwell
pushed a commit
to qfcwell/superset
that referenced
this pull request
May 12, 2026
This was referenced Jun 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
SUMMARY
Add three new chart types to the MCP service chart creation pipeline, expanding the set of charts that can be programmatically created via the Model Context Protocol:
chart_type="pie"): Category-based proportional visualization with configurable labels, donut mode, slice limits, and number formattingchart_type="pivot_table"): Interactive cross-tabulation with row/column grouping, multiple aggregation functions, totals, and value formattingchart_type="mixed_timeseries"): Dual-series charts combining two chart types (e.g., line + bar) on the same time axis with independent Y-axes and groupby supportPreviously, only XY charts (line, bar, area, scatter) and tables could be created via MCP. These three chart types were viewable but not creatable, requiring users to fall back to the Superset UI.
Key changes:
PieChartConfig,PivotTableChartConfig,MixedTimeseriesChartConfigwith full field validationmap_pie_config,map_pivot_table_config,map_mixed_timeseries_configChartConfigdiscriminated union to include new typesSchemaValidatorwith pre-validation for all new chart typesBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - Backend-only changes to MCP service
TESTING INSTRUCTIONS
Run the new unit tests:
Expected: 58 tests pass
Run the full MCP test suite to verify no regressions:
Expected: All tests pass
Test chart creation via MCP client:
generate_chart(dataset_id=X, config={"chart_type": "pie", "dimension": {"name": "category"}, "metric": {"name": "revenue", "aggregate": "SUM"}})generate_chart(dataset_id=X, config={"chart_type": "pivot_table", "rows": [{"name": "product"}], "metrics": [{"name": "sales", "aggregate": "SUM"}]})generate_chart(dataset_id=X, config={"chart_type": "mixed_timeseries", "x": {"name": "date"}, "y": [{"name": "revenue", "aggregate": "SUM"}], "y_secondary": [{"name": "orders", "aggregate": "COUNT"}]})ADDITIONAL INFORMATION
MCP Testing Results (localhost, 2026-03-04)
Tested via localhost MCP tools on branch
amin/mcp-chart-types-pie-pivot-mixed:Pie Chart
generate_chartwithchart_type="pie"config using dataset 7 (vehicle_sales)groupby=["product_line"],metric={"name": "quantity", "aggregate": "SUM"}viz_type: "pie"✅Pivot Table
generate_chartwithchart_type="pivot_table"config using dataset 7groupby=["product_line"],metrics=[{"name": "quantity", "aggregate": "SUM"}],columns=["status"]viz_type: "pivot_table_v2"✅Mixed Timeseries
generate_chartwithchart_type="mixed_timeseries"config using dataset 7x={"name": "order_date"},y=[{"name": "quantity", "aggregate": "SUM"}],y_secondary=[{"name": "sales", "aggregate": "SUM"}],time_grain="P1M"yfield (noty_primary) is the correct field name for primary metricsviz_type: "mixed_timeseries"✅All three new chart types work correctly via MCP.
CodeAnt-AI Description
Add pie, pivot table, and mixed timeseries chart creation support to MCP
What Changed
Impact
✅ Create pie/donut charts via API✅ Create interactive pivot tables via API✅ Fewer server crashes on malformed chart_type💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.