aminghadersohi commented on code in PR #39922: URL: https://github.com/apache/superset/pull/39922#discussion_r3482745870
########## superset/mcp_service/chart/plugins/pie.py: ########## @@ -0,0 +1,139 @@ +# 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. + +"""Pie chart type plugin.""" + +from __future__ import annotations + +from typing import Any + +from superset.mcp_service.chart.chart_utils import ( + _pie_chart_what, + _summarize_filters, + map_pie_config, +) +from superset.mcp_service.chart.plugin import BaseChartPlugin +from superset.mcp_service.chart.schemas import ColumnRef, PieChartConfig +from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +class PieChartPlugin(BaseChartPlugin): + """Plugin for pie chart type.""" + + chart_type = "pie" + display_name = "Pie / Donut Chart" + native_viz_types = { + "pie": "Pie Chart", + } + + def pre_validate( + self, + config: dict[str, Any], + ) -> ChartGenerationError | None: + missing_fields = [] + + if "dimension" not in config and "groupby" not in config: + missing_fields.append("'dimension' (category column for slices)") + if "metric" not in config: + missing_fields.append("'metric' (value metric for slice sizes)") + + if missing_fields: + return ChartGenerationError( + error_type="missing_pie_fields", + message=( + f"Pie chart missing required fields: {', '.join(missing_fields)}" + ), + details=( + "Pie charts require a dimension (categories) and a metric (values)" + ), + suggestions=[ + "Add 'dimension' field: {'name': 'category_column'}", + "Add 'metric' field: {'name': 'value_column', 'aggregate': 'SUM'}", + "Example: {'chart_type': 'pie', 'dimension': {'name': 'product'}, " + "'metric': {'name': 'revenue', 'aggregate': 'SUM'}}", + ], + error_code="MISSING_PIE_FIELDS", + ) + + return None + + def extract_column_refs(self, config: Any) -> list[ColumnRef]: + if not isinstance(config, PieChartConfig): + return [] + refs: list[ColumnRef] = [config.dimension, config.metric] Review Comment: Fixed in `429e3987df`. Added a schema validator to `PieChartConfig` that rejects `saved_metric=True` on the dimension field (mirrors the existing `sql_expression` rejection), and a defensive guard in `normalize_column_refs` to skip name canonicalization when `saved_metric` is set — so the normalization path never calls `_get_canonical_column_name` with a metric name. ########## superset/mcp_service/app.py: ########## @@ -668,6 +670,7 @@ def create_mcp_app( # NOTE: Always add new prompt/resource imports here when creating new prompts/resources. # Prompts use @mcp.prompt decorators and resources use @mcp.resource decorators. # They register automatically on import, similar to tools. +import superset.mcp_service.chart.plugins # noqa: F401, E402 — registers all chart type plugins Review Comment: This eager import is intentional — it runs the plugin-registration side effects that populate the chart type registry at startup, following the same pattern as every other tool module imported unconditionally in `app.py`. The registry itself uses a circuit-breaker sentinel (`_plugins_load_failed`) to handle per-plugin import failures gracefully. If this import raises, it means the chart plugin package itself is broken, which is a real startup error that should surface rather than be swallowed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
