codeant-ai-for-open-source[bot] commented on code in PR #42070:
URL: https://github.com/apache/superset/pull/42070#discussion_r3599841538
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2019,13 +2125,14 @@ def validate_percentiles_and_dimensions(self) ->
"BoxPlotChartConfig":
| HandlebarsChartConfig
| BigNumberChartConfig
| HistogramChartConfig
- | BoxPlotChartConfig,
+ | BoxPlotChartConfig
+ | WaterfallChartConfig,
Review Comment:
**Suggestion:** This adds `waterfall` as a first-class chart type in request
validation, but the chart-data recommendation classifier still lacks a
`waterfall` category mapping, so downstream recommendation dedup treats
existing waterfall charts as an unknown category and can suggest redundant `bar
chart` recommendations for the same semantic family. Update the viz-category
mapping to classify waterfall with bar-like charts to keep recommendation
behavior consistent with other registered chart types. [incomplete
implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ get_chart_data suggests redundant bar-style charts for waterfalls.
- ⚠️ Recommendation dedup ignores waterfall’s bar-like category semantics.
- ⚠️ LLM chart guidance less coherent for datasets with waterfall charts.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use the MCP `get_chart_data` tool implemented in
`superset/mcp_service/chart/tool/get_chart_data.py`, which constructs
human-readable
chart-type candidates (e.g. "line chart", "bar chart", "waterfall chart")
and normalizes
them via the `_CANDIDATE_CATEGORY` map at `get_chart_data.py:207-213` where
each candidate
string is mapped to a canonical category for dedup against the current
`viz_type`.
2. Create or retrieve a Superset chart with `viz_type="waterfall"` (the
Waterfall plugin
is registered in `superset/mcp_service/chart/plugins/__init__.py:40` and
resolves
`viz_type` to `"waterfall"` in
`superset/mcp_service/chart/plugins/waterfall.py:102-103`),
then invoke `get_chart_data` so it can compute recommended chart-type
candidates for the
same dataset.
3. Because `_CANDIDATE_CATEGORY` currently includes mappings for strings
like `"bar
chart": "bar"` but lacks an entry for `"waterfall chart"`, any candidate
phrase referring
to waterfall charts is treated as an unclassified category when deduping
recommendations
against the existing `viz_type="waterfall"`.
4. The dedup logic in `get_chart_data.py` (commented at lines 207-208 as
“Maps each
candidate string to a canonical category for dedup against the current
viz_type”)
therefore fails to recognize waterfall as part of the bar-like family,
allowing redundant
recommendations such as “bar chart” for data already visualized as a
waterfall chart,
leading to inconsistent and noisy suggestion behavior for the new chart type.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b5a8957bc9cf4e0ea4176ccf3a019dc6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b5a8957bc9cf4e0ea4176ccf3a019dc6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/schemas.py
**Line:** 2127:2129
**Comment:**
*Incomplete Implementation: This adds `waterfall` as a first-class
chart type in request validation, but the chart-data recommendation classifier
still lacks a `waterfall` category mapping, so downstream recommendation dedup
treats existing waterfall charts as an unknown category and can suggest
redundant `bar chart` recommendations for the same semantic family. Update the
viz-category mapping to classify waterfall with bar-like charts to keep
recommendation behavior consistent with other registered chart types.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42070&comment_hash=3d1d44418fa0e343bb04794eed243c48f56764d6f0d4027914cbe4cbdbf076c5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42070&comment_hash=3d1d44418fa0e343bb04794eed243c48f56764d6f0d4027914cbe4cbdbf076c5&reaction=dislike'>👎</a>
##########
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"):
+ if key in data:
+ data[key] = _coerce(data[key])
+ return data
Review Comment:
**Suggestion:** The pre-validation coercion only normalizes
`groupby`/`breakdown`, but native waterfall form_data also uses a bare string
for `x_axis` (for example `"x_axis": "month"`). Because `x_axis` is typed as
`ColumnRef`, those valid round-tripped payloads fail schema parsing with a
validation error. Extend this same coercion path to normalize `x_axis` string
input into `{"name": ...}` so existing waterfall form_data can be accepted
consistently. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MCP generate_chart rejects valid waterfall configs with string x_axis.
- ⚠️ LLM clients copying Superset form_data see validation errors.
- ⚠️ WaterfallChartPlugin.normalize_column_refs cannot run on rejected
configs.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the MCP `generate_chart` tool so that request data is validated via
`SchemaValidator.validate_request` in
`superset/mcp_service/chart/validation/schema_validator.py:40-56`, passing a
config like
`{"chart_type": "waterfall", "x_axis": "month", "metric": {"name":
"revenue", "aggregate":
"SUM"}}` copied from native Superset waterfall form_data (`x_axis` as bare
string).
2. The `GenerateChartRequest` Pydantic model (defined in
`superset/mcp_service/chart/schemas.py` later in the file) runs
`ChartRequestNormalizerMixin._normalize_request_vocabulary`, which calls
`_normalize_chart_request_input` at
`superset/mcp_service/chart/schemas.py:29-59` to
translate Superset vocabulary (e.g. `viz_type` to `chart_type`) but does not
modify the
`x_axis` field.
3. The discriminated union `ChartConfig` at
`superset/mcp_service/chart/schemas.py:220-240` selects
`WaterfallChartConfig` based on
`chart_type="waterfall"` and invokes its `@model_validator(mode="before")
unwrap_list_breakdown` at `superset/mcp_service/chart/schemas.py:174-203`,
which only
coerces `groupby`/`breakdown` values and leaves `x_axis` as the original
string.
4. Pydantic then attempts to populate the `x_axis: ColumnRef` field of
`WaterfallChartConfig` (lines 118-123 in
`superset/mcp_service/chart/schemas.py`) from the
bare string `"month`; since `ColumnRef` expects a structured column object,
this results
in a `ValidationError`, causing `SchemaValidator.validate_request` to return
a
`ChartGenerationError` instead of a parsed request and blocking waterfall
chart generation
for otherwise valid native form_data.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5f16c672479247bbb6e85743ab5a709a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5f16c672479247bbb6e85743ab5a709a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/schemas.py
**Line:** 2097:2101
**Comment:**
*Api Mismatch: The pre-validation coercion only normalizes
`groupby`/`breakdown`, but native waterfall form_data also uses a bare string
for `x_axis` (for example `"x_axis": "month"`). Because `x_axis` is typed as
`ColumnRef`, those valid round-tripped payloads fail schema parsing with a
validation error. Extend this same coercion path to normalize `x_axis` string
input into `{"name": ...}` so existing waterfall form_data can be accepted
consistently.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42070&comment_hash=ea9b45f90eb5dfb177e236c4c2342e7d9bd3167532f69dab5ba8f64ae1bd814e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42070&comment_hash=ea9b45f90eb5dfb177e236c4c2342e7d9bd3167532f69dab5ba8f64ae1bd814e&reaction=dislike'>👎</a>
--
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]
