codeant-ai-for-open-source[bot] commented on code in PR #42070:
URL: https://github.com/apache/superset/pull/42070#discussion_r3592129658
##########
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:
**Suggestion:** This hard-codes the total chart-type count, which will
create brittle test failures whenever a new chart type is added even if schema
support is correctly implemented. Replace the fixed-length assertion with
assertions over required members (or parity with the adapter/example
registries) so the test validates behavior without reintroducing stale count
maintenance. [code quality]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Adding new chart type breaks schema tests.
- ❌ CI fails blocking MCP chart-type additions.
- ⚠️ Maintainers must update brittle length assertion manually.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note that `VALID_CHART_TYPES` is defined as
`sorted(_CHART_TYPE_ADAPTERS.keys())` in
`superset/mcp_service/chart/tool/get_chart_type_schema.py:46-59`, so its
length equals the
number of registered adapters.
2. Add a new chart type adapter (e.g., `"scatter":
TypeAdapter(ScatterChartConfig)`) to
`_CHART_TYPE_ADAPTERS` in
`superset/mcp_service/chart/tool/get_chart_type_schema.py:46-57`
as part of expanding supported MCP chart types.
3. Run `pytest
tests/unit_tests/mcp_service/chart/tool/test_get_chart_type_schema.py::TestGetChartTypeSchema::test_valid_chart_types_constant`
to execute the constant-length assertion at
`tests/unit_tests/mcp_service/chart/tool/test_get_chart_type_schema.py:96-99`.
4. Observe the test failure because `len(VALID_CHART_TYPES)` now reflects
the new adapter
count (e.g., 11) while the test still asserts `== 10`, causing CI to fail
even though the
schema implementation correctly supports the new chart type.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0b6d58d533514fef951f123b1e34aa2e&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=0b6d58d533514fef951f123b1e34aa2e&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:**
tests/unit_tests/mcp_service/chart/tool/test_get_chart_type_schema.py
**Line:** 97:97
**Comment:**
*Code Quality: This hard-codes the total chart-type count, which will
create brittle test failures whenever a new chart type is added even if schema
support is correctly implemented. Replace the fixed-length assertion with
assertions over required members (or parity with the adapter/example
registries) so the test validates behavior without reintroducing stale count
maintenance.
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=fa6825d600c1a0b2535ec0951e41e802d374408287c5604fa980596a18014896&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42070&comment_hash=fa6825d600c1a0b2535ec0951e41e802d374408287c5604fa980596a18014896&reaction=dislike'>👎</a>
##########
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:
**Suggestion:** The schema advertises native `groupby` support, but this
field only accepts a single `ColumnRef` object; Superset form_data uses
`groupby` as a list, so round-tripping a waterfall config from existing chart
form_data will fail validation. Add a pre-validator to accept a one-item
`groupby` list (and normalize it to a single breakdown) so `groupby` aliases
work with real form_data payloads. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ generate_chart rejects waterfall configs with groupby lists.
- ⚠️ LLM reuse of Superset form_data needs manual fixing.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the MCP server and call the `generate_chart` tool implemented in
`superset/mcp_service/chart/tool/generate_chart.py:98-207`, sending a JSON
body that
matches `GenerateChartRequest` in
`superset/mcp_service/chart/schemas.py:2268-2272`.
2. Construct `request_data` with `dataset_id` and a `config` dict using
Superset-style
waterfall form_data semantics: for example `config = {"chart_type":
"waterfall", "x_axis":
{"name": "month"}, "metric": {"name": "revenue", "aggregate": "SUM"},
"groupby": [{"name":
"region"}]}` where `groupby` is a one-item list, mirroring the form_data
produced by
`map_waterfall_config` in
`superset/mcp_service/chart/chart_utils.py:963-967` (which
always emits `"groupby": [config.breakdown.name] if config.breakdown else
[]`).
3. The request passes SchemaValidator pre-validation in
`superset/mcp_service/chart/validation/schema_validator.py:64-144` (it is a
dict, has
`dataset_id`, `config`, and `config["chart_type"] = "waterfall"`), and plugin
pre-validation in `superset/mcp_service/chart/plugins/waterfall.py:11-42`
(which only
checks for presence of `"x_axis"` and `"metric"`).
4. During Pydantic parsing of `GenerateChartRequest` (line 56 in
`schema_validator.py`),
`ChartRequestNormalizerMixin._normalize_request_vocabulary` in
`superset/mcp_service/chart/schemas.py:2161-2168` runs but does not adjust
`config["groupby"]`. The `config` union `ChartConfig` at
`superset/mcp_service/chart/schemas.py:120-140` selects
`WaterfallChartConfig` (since
`chart_type="waterfall"`), whose `breakdown` field at lines 2028-2034 uses
`validation_alias=AliasChoices("breakdown", "groupby")` but has type
`ColumnRef | None`.
Because the alias receives a list of dicts instead of a single `ColumnRef`
object,
Pydantic raises a `ValidationError` for the `groupby`/`breakdown` field,
causing
`SchemaValidator.validate_request` to return a `ChartGenerationError` and the
`generate_chart` tool to reject this otherwise native-form_data-shaped
waterfall
configuration.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d914fda562de4fe6a3f49b293a80fa7b&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=d914fda562de4fe6a3f49b293a80fa7b&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:** 2028:2034
**Comment:**
*Api Mismatch: The schema advertises native `groupby` support, but this
field only accepts a single `ColumnRef` object; Superset form_data uses
`groupby` as a list, so round-tripping a waterfall config from existing chart
form_data will fail validation. Add a pre-validator to accept a one-item
`groupby` list (and normalize it to a single breakdown) so `groupby` aliases
work with real form_data payloads.
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=693e25fe3646bc0a5ffefde64770755c238cc03d3cd95138035cccac518f8088&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42070&comment_hash=693e25fe3646bc0a5ffefde64770755c238cc03d3cd95138035cccac518f8088&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]
