gkneighb commented on code in PR #42070:
URL: https://github.com/apache/superset/pull/42070#discussion_r3592262232
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -891,6 +892,33 @@ def map_pie_config(config: PieChartConfig) -> 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,
Review Comment:
Confirmed and fixed in 3a65b67281 — the frontend Waterfall control panel
does expose time_grain_sqla. Added a time_grain field (alias time_grain_sqla)
that maps to time_grain_sqla only when set, matching the xy/mixed/big_number
pattern; Superset ignores it for non-temporal columns. Tests cover mapping,
omission, and the alias.
##########
superset/mcp_service/chart/tool/get_chart_type_schema.py:
##########
@@ -223,6 +239,7 @@ def get_chart_type_schema(
Valid chart_type values: xy, table, pie, pivot_table,
mixed_timeseries, handlebars, big_number, histogram, box_plot.
+ mixed_timeseries, handlebars, big_number, waterfall.
Review Comment:
Fixed in 3a65b67281 — merge artifact from the #41860 rebase; the docstring
had two contradictory type lists. Now a single line: xy, table, pie,
pivot_table, mixed_timeseries, handlebars, big_number, histogram, box_plot,
waterfall.
##########
superset/mcp_service/chart/validation/dataset_validator.py:
##########
@@ -271,7 +271,7 @@ def _extract_column_references(
"""Extract all column references from configuration via the plugin
registry.
Previously only handled TableChartConfig and XYChartConfig, causing
- 5 of 7 chart types to silently skip column validation. Now delegates
+ most chart types to silently skip column validation. Now delegates
to the plugin for each chart type so all types are covered.
Review Comment:
Fixed in 3a65b67281 — reworded to 'delegates to the plugin for each
registered chart type; a config whose type has no registered plugin yields no
refs (rather than raising)', so it no longer overstates coverage.
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2009,6 +2009,73 @@ 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)",
+ )
Review Comment:
Confirmed and fixed in 3a65b67281 — added a before-validator that unwraps a
one-item groupby list (the native form_data shape; the frontend Breakdowns
control stores a single value as a list) and rejects multi-item lists rather
than silently dropping. Tests cover list, empty-list, and multi-item cases.
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_type_schema.py:
##########
@@ -94,7 +94,7 @@ def test_examples_match_chart_type(self) -> None:
assert example["chart_type"] == "pie"
def test_valid_chart_types_constant(self) -> None:
- assert len(VALID_CHART_TYPES) == 9
+ assert len(VALID_CHART_TYPES) == 10
Review Comment:
Fixed in 3a65b67281 — the assertion now checks parity with the adapter
registry (set(VALID_CHART_TYPES) == set(_CHART_TYPE_ADAPTERS)) plus a few
load-bearing members, so adding a chart type won't spuriously fail it.
--
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]
