codeant-ai-for-open-source[bot] commented on code in PR #42070:
URL: https://github.com/apache/superset/pull/42070#discussion_r3583947457
##########
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:
**Suggestion:** The waterfall mapper never sets temporal granularity fields,
so temporal `x_axis` values are sent as raw timestamps instead of being
bucketed (for example by day/month). This causes incorrect step grouping for
datetime columns and makes time-grain behavior inconsistent with other temporal
charts. Add waterfall support for temporal bucketing (e.g., map a time-grain
config to `granularity_sqla`/`time_grain_sqla` when the axis is temporal).
[incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ MCP waterfall charts cannot accept time_grain parameter.
- ⚠️ Temporal x_axis columns lack standard time-grain bucketing.
- ⚠️ Time-grain behavior inconsistent with XY and mixed_timeseries.
- ⚠️ LLM clients see schema rejection for time_grain on waterfall.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the MCP `generate_chart` tool
(superset/mcp_service/chart/tool/generate_chart.py:98-140) with
`request.config.chart_type="waterfall"` and a temporal x-axis column, e.g.
`{"chart_type":
"waterfall", "x_axis": {"name": "order_date"}, "metric": {"name":
"revenue_delta",
"aggregate": "SUM"}}`.
2. The request is validated against `WaterfallChartConfig` in
`superset/mcp_service/chart/schemas.py:1849-1900`, which does not define a
`time_grain`
field and inherits `UnknownFieldCheckMixin` (lines 781-788); any
`time_grain` key in the
config is rejected as an unknown field, so MCP clients cannot specify time
grain for
waterfall charts.
3. After validation, `generate_chart` passes the config into the chart
registry, where
`WaterfallChartPlugin.to_form_data()`
(superset/mcp_service/chart/plugins/waterfall.py:88-91) calls
`map_waterfall_config(config)` in `chart_utils.py:895-919`.
4. `map_waterfall_config` builds `form_data` with `"viz_type": "waterfall",
"x_axis":
config.x_axis.name, "groupby": [...]` and other display options (lines
903-915) but never
calls `configure_temporal_handling` (chart_utils.py:681-708) and never sets
`granularity_sqla` or `time_grain_sqla`, so the standard temporal bucketing
path used by
XY and mixed_timeseries charts (see `map_xy_config` at 833-835 and
`map_mixed_timeseries_config` at 1170-1172) is unavailable for waterfall
charts, leaving
temporal x-axis values unbucketed and making time-grain behavior
inconsistent with other
temporal chart types.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=18a720946f0947689fbdde0e76666d2c&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=18a720946f0947689fbdde0e76666d2c&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/chart_utils.py
**Line:** 904:915
**Comment:**
*Incomplete Implementation: The waterfall mapper never sets temporal
granularity fields, so temporal `x_axis` values are sent as raw timestamps
instead of being bucketed (for example by day/month). This causes incorrect
step grouping for datetime columns and makes time-grain behavior inconsistent
with other temporal charts. Add waterfall support for temporal bucketing (e.g.,
map a time-grain config to `granularity_sqla`/`time_grain_sqla` when the axis
is temporal).
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=d07c3a2081c03076d61a504704bd47bb5a8f95c66fde658d89aa6a555284db47&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42070&comment_hash=d07c3a2081c03076d61a504704bd47bb5a8f95c66fde658d89aa6a555284db47&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]
