aminghadersohi commented on code in PR #39922:
URL: https://github.com/apache/superset/pull/39922#discussion_r3328062362
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -320,29 +320,35 @@ def map_config_to_form_data(
| BigNumberChartConfig,
dataset_id: int | str | None = None,
) -> Dict[str, Any]:
- """Map chart config to Superset form_data."""
- if isinstance(config, TableChartConfig):
- return map_table_config(config)
- elif isinstance(config, XYChartConfig):
- return map_xy_config(config, dataset_id=dataset_id)
- elif isinstance(config, PieChartConfig):
- return map_pie_config(config)
- elif isinstance(config, PivotTableChartConfig):
- return map_pivot_table_config(config)
- elif isinstance(config, MixedTimeseriesChartConfig):
- return map_mixed_timeseries_config(config, dataset_id=dataset_id)
- elif isinstance(config, HandlebarsChartConfig):
- return map_handlebars_config(config)
- elif isinstance(config, BigNumberChartConfig):
- if config.show_trendline and config.temporal_column:
- if not is_column_truly_temporal(config.temporal_column,
dataset_id):
- raise ValueError(
- f"Big Number trendline requires a temporal SQL column; "
- f"'{config.temporal_column}' is not temporal."
- )
- return map_big_number_config(config)
- else:
- raise ValueError(f"Unsupported config type: {type(config)}")
+ """Map chart config to Superset form_data via the plugin registry.
+
+ The previous if/elif chain across all 7 chart types has been replaced by a
+ single registry lookup. Cross-field constraints (e.g. BigNumber trendline
+ 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)
+ plugin = get_registry().get(chart_type) if chart_type else None
+
+ if plugin is None:
+ raise ValueError(
+ f"Unsupported config type: {type(config)}
(chart_type={chart_type!r})"
+ )
Review Comment:
Already fixed in the current code. `chart_utils.py` lines 339–344 now reads:
```python
if plugin is None:
if chart_type is None:
raise ValueError(f"Unsupported config type: {type(config)}")
raise ValueError(
f"Unsupported config type: {type(config)}
(chart_type={chart_type!r})"
)
```
When `chart_type is None` the message is just `Unsupported config type:
<class ...>` — no spurious `(chart_type=None)` suffix.
##########
superset/mcp_service/chart/plugins/mixed_timeseries.py:
##########
@@ -0,0 +1,170 @@
+# 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.
+
+"""Mixed timeseries chart type plugin."""
+
+from __future__ import annotations
+
+from typing import Any
+
+from superset.mcp_service.chart.chart_utils import (
+ _mixed_timeseries_what,
+ _summarize_filters,
+ map_mixed_timeseries_config,
+)
+from superset.mcp_service.chart.plugin import BaseChartPlugin
+from superset.mcp_service.chart.schemas import ColumnRef,
MixedTimeseriesChartConfig
+from superset.mcp_service.chart.validation.dataset_validator import
DatasetValidator
+from superset.mcp_service.common.error_schemas import ChartGenerationError
+
+
+class MixedTimeseriesChartPlugin(BaseChartPlugin):
+ """Plugin for mixed_timeseries chart type."""
+
+ chart_type = "mixed_timeseries"
+ display_name = "Mixed Timeseries"
+ native_viz_types = {
+ "mixed_timeseries": "Mixed Timeseries Chart",
+ }
+
+ def pre_validate(
+ self,
+ config: dict[str, Any],
+ ) -> ChartGenerationError | None:
+ missing_fields = []
+
+ if "x" not in config and "x_axis" not in config:
+ missing_fields.append("'x' (X-axis temporal column)")
+ if "y" not in config and "metrics" not in config:
+ missing_fields.append("'y' (primary Y-axis metrics)")
+ if "y_secondary" not in config and "metrics_b" not in config:
+ missing_fields.append("'y_secondary' (secondary Y-axis metrics)")
+
+ if missing_fields:
+ return ChartGenerationError(
+ error_type="missing_mixed_timeseries_fields",
+ message=(
+ f"Mixed timeseries chart missing required fields: "
+ f"{', '.join(missing_fields)}"
+ ),
+ details=(
+ "Mixed timeseries charts require an x-axis, primary
metrics, "
+ "and secondary metrics"
+ ),
+ suggestions=[
+ "Add 'x' field: {'name': 'date_column'}",
+ "Add 'y' field: [{'name': 'revenue', 'aggregate': 'SUM'}]",
+ "Add 'y_secondary': [{'name': 'orders', 'aggregate':
'COUNT'}]",
+ "Optional: 'primary_kind' and 'secondary_kind' for chart
types",
+ ],
Review Comment:
Not applicable — these strings are in `ChartGenerationError` objects
returned by MCP tool calls as structured JSON to LLM clients. MCP is a
machine-to-machine protocol (LLM ↔ JSON); it is not a Flask view or Jinja
template. Wrapping with `_()` would produce `LazyString` objects that cannot be
serialized to JSON by Pydantic and would break the tool response. The i18n
layer belongs at the client (LLM/UI) that renders these messages, not in the
MCP server payload.
--
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]
