From b16de3622f9c4ceb721ef33fcd9996cff4d86acf Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Thu, 7 May 2026 16:29:53 +0000 Subject: [PATCH] =?UTF-8?q?fix(mcp):=20address=20reviewer=20comments=20?= =?UTF-8?q?=E2=80=94=20local=20import=20rationale,=20x-optional=20correcti?= =?UTF-8?q?ons,=20cardinality=20suggestions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove redundant local imports from BigNumberChartPlugin.post_map_validate() now that BigNumberChartConfig and is_column_truly_temporal are at top level - Add explanatory comments on the two remaining local get_registry imports in chart_utils.py and dataset_validator.py (circular import prevention) - Fix schema_validator.py and generate_chart.py docstring: XY 'x' field is optional (defaults to dataset primary datetime column), not required - Propagate cardinality suggestions alongside warnings in XYChartPlugin - Clarify app.py instructions: chart_type_display_name is null for viz_types outside the 7 generate_chart-supported types Co-Authored-By: Claude Sonnet 4.6 --- superset/mcp_service/app.py | 12 +++++------- superset/mcp_service/chart/chart_utils.py | 3 +++ superset/mcp_service/chart/plugins/big_number.py | 7 ++----- superset/mcp_service/chart/plugins/xy.py | 1 + superset/mcp_service/chart/tool/generate_chart.py | 2 +- .../chart/validation/dataset_validator.py | 3 +++ .../mcp_service/chart/validation/schema_validator.py | 2 +- 7 files changed, 16 insertions(+), 14 deletions(-) diff --git a/superset/mcp_service/app.py b/superset/mcp_service/app.py index f518aa41fbe..9163ab7c863 100644 --- a/superset/mcp_service/app.py +++ b/superset/mcp_service/app.py @@ -223,13 +223,11 @@ Time grain for temporal x-axis (time_grain parameter): Chart Types in Existing Charts (viewable via list_charts/get_chart_info): Each chart returned by list_charts / get_chart_info includes a -chart_type_display_name field with a human-readable name. Use that name -when referring to chart types — do NOT expose raw viz_type identifiers. -Common display names: Line Chart, Bar Chart, Area Chart, Scatter Plot, -Pie Chart, Table, Interactive Table, Pivot Table, Big Number, -Big Number with Trendline, Mixed Timeseries Chart, Custom Template Chart, -Funnel Chart, Gauge Chart, Heatmap, Sankey Chart, Sunburst, Treemap, -Word Cloud, World Map, Box Plot, Bubble Chart. +chart_type_display_name field with a human-readable name when available. +This field is populated only for the 7 chart types supported by generate_chart +(xy, pie, table, pivot_table, big_number, mixed_timeseries, handlebars). +For all other viz_types (Funnel, Gauge, Heatmap, etc.) it will be null — +use the raw viz_type field instead when referring to those chart types. Query Examples: - List all tables: diff --git a/superset/mcp_service/chart/chart_utils.py b/superset/mcp_service/chart/chart_utils.py index c82ad25b94a..f2ac2149379 100644 --- a/superset/mcp_service/chart/chart_utils.py +++ b/superset/mcp_service/chart/chart_utils.py @@ -325,6 +325,9 @@ def map_config_to_form_data( temporal check) are now owned by each plugin's post_map_validate() method rather than being baked into this dispatcher. """ + # Local import: plugins call map_*_config from their to_form_data() methods, + # so chart_utils is loaded before plugins finish registering. A top-level + # import of registry here would trigger plugin loading mid-import = cycle. from superset.mcp_service.chart.registry import get_registry chart_type = getattr(config, "chart_type", None) diff --git a/superset/mcp_service/chart/plugins/big_number.py b/superset/mcp_service/chart/plugins/big_number.py index 17ee2b3d896..bd2b90a7efb 100644 --- a/superset/mcp_service/chart/plugins/big_number.py +++ b/superset/mcp_service/chart/plugins/big_number.py @@ -21,8 +21,9 @@ from __future__ import annotations from typing import Any +from superset.mcp_service.chart.chart_utils import is_column_truly_temporal from superset.mcp_service.chart.plugin import BaseChartPlugin -from superset.mcp_service.chart.schemas import ColumnRef +from superset.mcp_service.chart.schemas import BigNumberChartConfig, ColumnRef from superset.mcp_service.common.error_schemas import ChartGenerationError @@ -144,15 +145,11 @@ class BigNumberChartPlugin(BaseChartPlugin): chart_utils.py as a special case. Moving it here keeps the dispatcher clean and makes the constraint explicit and discoverable. """ - from superset.mcp_service.chart.schemas import BigNumberChartConfig - if not isinstance(config, BigNumberChartConfig): return None if not (config.show_trendline and config.temporal_column): return None - from superset.mcp_service.chart.chart_utils import is_column_truly_temporal - if not is_column_truly_temporal(config.temporal_column, dataset_id): return ChartGenerationError( error_type="non_temporal_trendline_column", diff --git a/superset/mcp_service/chart/plugins/xy.py b/superset/mcp_service/chart/plugins/xy.py index 9176d083f29..b24337cfc1a 100644 --- a/superset/mcp_service/chart/plugins/xy.py +++ b/superset/mcp_service/chart/plugins/xy.py @@ -179,6 +179,7 @@ class XYChartPlugin(BaseChartPlugin): ) if not _ok and card_info: warnings.extend(card_info.get("warnings", [])) + warnings.extend(card_info.get("suggestions", [])) except Exception as exc: logger.warning("XY cardinality validation failed: %s", exc) diff --git a/superset/mcp_service/chart/tool/generate_chart.py b/superset/mcp_service/chart/tool/generate_chart.py index fb4eee1108f..b7b171fcf9b 100644 --- a/superset/mcp_service/chart/tool/generate_chart.py +++ b/superset/mcp_service/chart/tool/generate_chart.py @@ -108,7 +108,7 @@ async def generate_chart( # noqa: C901 other fields in your configuration: - Use chart_type='xy' for charts with x and y axes (line, bar, area, scatter) - Required fields: x, y + Required fields: y (x is optional — defaults to dataset's primary datetime column) - Use chart_type='table' for tabular visualizations Required fields: columns diff --git a/superset/mcp_service/chart/validation/dataset_validator.py b/superset/mcp_service/chart/validation/dataset_validator.py index 7d07cc89055..3646636ee8a 100644 --- a/superset/mcp_service/chart/validation/dataset_validator.py +++ b/superset/mcp_service/chart/validation/dataset_validator.py @@ -377,6 +377,9 @@ class DatasetValidator: if not dataset_context: return config + # Local import: plugins call DatasetValidator helpers from normalize_column_refs(). + # A top-level import of registry in dataset_validator would make loading this + # module implicitly trigger plugin registration, creating a circular dependency. from superset.mcp_service.chart.registry import get_registry chart_type = getattr(config, "chart_type", None) diff --git a/superset/mcp_service/chart/validation/schema_validator.py b/superset/mcp_service/chart/validation/schema_validator.py index 969779c26a8..980c2b56f3a 100644 --- a/superset/mcp_service/chart/validation/schema_validator.py +++ b/superset/mcp_service/chart/validation/schema_validator.py @@ -199,7 +199,7 @@ class SchemaValidator: "details": "The XY chart configuration is missing required " "fields or has invalid structure", "suggestions": [ - "Ensure 'x' field exists with {'name': 'column_name'}", + "Note: 'x' is optional and defaults to the dataset's primary datetime column", "Ensure 'y' is an array: [{'name': 'metric', 'aggregate': 'SUM'}]", "Check that all column names are strings", "Verify aggregate functions are valid: SUM, COUNT, AVG, MIN, MAX",