aminghadersohi commented on code in PR #42070:
URL: https://github.com/apache/superset/pull/42070#discussion_r3600913757
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -951,6 +952,41 @@ 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: the grain (time_grain_sqla) needs the temporal
+ # column it applies to (granularity_sqla), mirroring the xy path's
+ # configure_temporal_handling and the frontend buildQuery's
+ # `x_axis || granularity_sqla`. Providing time_grain signals temporal
Review Comment:
This sets `time_grain_sqla`/`granularity_sqla` purely because `time_grain`
was provided, with no check that `x_axis` is actually temporal.
`map_waterfall_config` doesn't take a `dataset_id`, unlike the XY and BigNumber
mappers, which both call `is_column_truly_temporal(column_name, dataset_id,
...)` first — because Superset can mark a column `is_dttm=True` from name
heuristics alone even when the real SQL type is BIGINT/INTEGER, and DATE_TRUNC
on that column fails at query time. This round's fix actually makes that
scenario a bit more reachable than before, since `granularity_sqla` is now
bound to the column that would get DATE_TRUNC'd (previously only the
comparatively inert `time_grain_sqla` was set).
Properly fixing this means threading `dataset_id` through to
`map_waterfall_config` the way XY/BigNumber do — bigger than a one-liner, so
not blocking, but worth tracking.
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2009,6 +2009,112 @@ 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. Native form_data also names physical columns as
+ bare strings, so a string is coerced to ``{"name": ...}``.
+ """
+
+ def _coerce(value: Any) -> Any:
+ if isinstance(value, list):
+ if len(value) > 1:
+ raise ValueError(
+ "waterfall breakdown is single-select; pass at most
one column"
+ )
+ value = value[0] if value else None
+ if isinstance(value, str):
+ return {"name": value}
+ return value
+
+ if isinstance(data, dict):
+ for key in ("groupby", "breakdown"):
Review Comment:
This coercion loop only covers `groupby`/`breakdown` — `x_axis` (a required
`ColumnRef` field, present on every waterfall config) has no equivalent
handling. Native form_data commonly represents a plain x-axis column as a bare
string too (e.g. `"x_axis": "month"`), which fails validation the same way
`groupby`/`breakdown` did before this round's fix. Since `x_axis` is required
and always present, this is actually a bigger-blast-radius instance of the same
issue.
Worth extending `_coerce()` (or an equivalent) to `x_axis` as well.
--
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]
