codeant-ai-for-open-source[bot] commented on code in PR #42070:
URL: https://github.com/apache/superset/pull/42070#discussion_r3599014980
##########
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:
**Suggestion:** When `time_grain` is provided, the mapper only sets
`time_grain_sqla` but never sets `granularity_sqla`/`granularity` to the x-axis
column. In Superset query building, grain application depends on a time column
being set; without it, the requested bucketing can be ignored or fail in
stricter paths. Set the granularity field alongside `time_grain_sqla`
(typically to `config.x_axis.name`) so time bucketing is actually applied. [api
mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Waterfall MCP charts ignore requested time_grain bucketing.
- ⚠️ Temporal waterfall steps use raw dates, not buckets.
- ⚠️ Granularity inference may target wrong temporal filter column.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a WaterfallChartConfig with temporal x_axis and time_grain, e.g. in
tests/unit_tests/mcp_service/chart/test_waterfall_chart.py:168-175 using
x_axis={"name":
"order_date"} and time_grain="P1M".
2. Call WaterfallChartPlugin.to_form_data() from
superset/mcp_service/chart/plugins/waterfall.py:88-91, which delegates to
map_waterfall_config() in superset/mcp_service/chart/chart_utils.py:56-84.
3. Observe the resulting form_data contains "x_axis": "order_date" and
"time_grain_sqla":
"P1M" but no "granularity_sqla" field (only lines 979-980 set
time_grain_sqla without
granularity_sqla).
4. Use this form_data in build_query_context_from_form_data() at
superset/mcp_service/chart/chart_helpers.py:110-140, which builds
QueryObjects via
QueryContextFactory.create(); in QueryContextFactory._apply_granularity()
(superset/common/query_context_factory.py:36-132) query_object.granularity
remains None,
so no explicit time column is bound and the requested time_grain bucketing
is either
ignored or inferred against an unintended temporal column instead of the
x_axis.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e79be314f98a47fba8aa6496bd42a807&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=e79be314f98a47fba8aa6496bd42a807&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:** 979:980
**Comment:**
*Api Mismatch: When `time_grain` is provided, the mapper only sets
`time_grain_sqla` but never sets `granularity_sqla`/`granularity` to the x-axis
column. In Superset query building, grain application depends on a time column
being set; without it, the requested bucketing can be ignored or fail in
stricter paths. Set the granularity field alongside `time_grain_sqla`
(typically to `config.x_axis.name`) so time bucketing is actually applied.
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=81dde640658baf97b06d4e84fa0307bebcab482681c780f3f112f9b2c6c380fb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42070&comment_hash=81dde640658baf97b06d4e84fa0307bebcab482681c780f3f112f9b2c6c380fb&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]
