gkneighb commented on code in PR #42070: URL: https://github.com/apache/superset/pull/42070#discussion_r3599830197
########## superset/mcp_service/chart/plugins/waterfall.py: ########## @@ -0,0 +1,147 @@ +# 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. + +"""Waterfall chart type plugin.""" + +from __future__ import annotations + +from collections.abc import Mapping +from typing import Any, ClassVar + +from superset.mcp_service.chart.chart_utils import ( + _summarize_filters, + map_waterfall_config, +) +from superset.mcp_service.chart.plugin import BaseChartPlugin +from superset.mcp_service.chart.schemas import ColumnRef, WaterfallChartConfig +from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +class WaterfallChartPlugin(BaseChartPlugin): + """Plugin for waterfall chart type.""" + + chart_type = "waterfall" + display_name = "Waterfall Chart" Review Comment: Declining, standing rationale (validated by aminghadersohi's review on the sibling PRs): bare class-level display_name constants follow BaseChartPlugin's convention shared by all in-tree plugins; mypy passes. ########## superset/mcp_service/chart/schemas.py: ########## @@ -2009,6 +2009,105 @@ def validate_percentiles_and_dimensions(self) -> "BoxPlotChartConfig": return self +class WaterfallChartConfig(UnknownFieldCheckMixin): + """Config for waterfall charts (viz_type ``waterfall``).""" + + model_config = ConfigDict(extra="ignore", populate_by_name=True) + + chart_type: Literal["waterfall"] = "waterfall" + x_axis: ColumnRef = Field( + ..., + description="Category or period column along the x-axis (often " + "temporal, e.g. month); each value is one waterfall step", + ) + metric: ColumnRef = Field( + ..., + description="Metric whose per-step change is plotted (use aggregate " + "e.g. SUM for ad-hoc, or saved_metric=True for a saved metric)", + ) + breakdown: ColumnRef | None = Field( + None, + validation_alias=AliasChoices("breakdown", "groupby"), + description="Optional single category column that breaks each " + "x-axis period into per-category steps (form_data 'groupby'; the " + "frontend Breakdowns control is single-select)", + ) + time_grain: TimeGrain | None = Field( + None, + description="Time bucket for a temporal x_axis (PT1H, P1D, P1W, " + "P1M, P1Y); each bucket becomes one waterfall step. Ignored for a " + "non-temporal x_axis.", + validation_alias=AliasChoices("time_grain", "time_grain_sqla"), + ) + show_total: bool = Field( + True, description="Append a total bar per period (frontend default)" + ) + show_legend: bool = Field( + False, description="Show the legend (frontend default: off)" + ) + increase_label: str = Field("Increase", max_length=50) + decrease_label: str = Field("Decrease", max_length=50) + total_label: str = Field("Total", max_length=50) + x_axis_time_format: str = Field( + "smart_date", + description="Time format for a temporal x-axis (e.g. 'smart_date', '%Y-%m-%d')", + max_length=50, + ) + y_axis_format: str = Field("SMART_NUMBER", max_length=50) + currency_format: CurrencyFormat | None = Field( + None, + description="Currency symbol applied to the metric value", + ) + filters: List[FilterConfig] | None = Field( + None, + description="Structured filters (column/op/value). " + "Do NOT use adhoc_filters or raw SQL expressions.", + ) + row_limit: int = Field( + 10000, + description="Max grouped rows (frontend shared default)", + ge=1, + le=50000, + ) + + @model_validator(mode="before") + @classmethod + def unwrap_list_breakdown(cls, data: Any) -> Any: + """Accept the native form_data shape where ``groupby`` is a list. + + The frontend Breakdowns control is single-select but stores its value + as a one-item list (e.g. ``["region"]``), so a config round-tripped + from existing waterfall form_data sends a list. Unwrap a length-1 + list to the single value; reject longer lists rather than silently + dropping breakdowns. + """ + if isinstance(data, dict): + for key in ("groupby", "breakdown"): + value = data.get(key) + if isinstance(value, list): + if len(value) > 1: + raise ValueError( + "waterfall breakdown is single-select; pass at " + "most one column" + ) + data[key] = value[0] if value else None + return data Review Comment: Confirmed and fixed in 573c819f35 — ColumnRef does reject bare strings, so a native groupby: ['region'] now coerces the string to {'name': 'region'}; scalar-string and list-of-strings both covered by tests. ########## superset/mcp_service/chart/chart_utils.py: ########## @@ -951,6 +952,37 @@ def map_box_plot_config(config: "BoxPlotChartConfig") -> Dict[str, Any]: return form_data +def map_waterfall_config(config: WaterfallChartConfig) -> Dict[str, Any]: + """Map waterfall config to Superset form_data (viz_type waterfall). + + Matches the frontend Waterfall buildQuery contract: a single ``x_axis`` + column, an optional single-select ``groupby`` breakdown, and one + ``metric``; the query orders by the axis columns ascending, which the + frontend derives from these keys. + """ + form_data: Dict[str, Any] = { + "viz_type": "waterfall", + "x_axis": config.x_axis.name, + "groupby": [config.breakdown.name] if config.breakdown else [], + "metric": create_metric_object(config.metric), + "show_total": config.show_total, + "show_legend": config.show_legend, + "increase_label": config.increase_label, + "decrease_label": config.decrease_label, + "total_label": config.total_label, + "x_axis_time_format": config.x_axis_time_format, + "y_axis_format": config.y_axis_format, + "row_limit": config.row_limit, + } + # Bucket a temporal x_axis; Superset ignores time_grain_sqla for + # non-temporal columns, matching the frontend control's behavior. + if config.time_grain: + form_data["time_grain_sqla"] = config.time_grain Review Comment: Confirmed and fixed in 573c819f35 — when time_grain is set the mapper now also sets granularity_sqla to the x_axis column, matching the xy path's configure_temporal_handling and the frontend buildQuery's 'x_axis || granularity_sqla'. Test asserts both fields. -- 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]
