codeant-ai-for-open-source[bot] commented on code in PR #40972:
URL: https://github.com/apache/superset/pull/40972#discussion_r3442765844
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1736,6 +1759,65 @@ def validate_unique_column_labels(self) ->
"XYChartConfig":
# giving LLMs enough context to construct valid configs.
+# Superset viz_type values that LLM clients routinely send where this API
+# expects its chart_type discriminator. Each maps to (chart_type, kind);
+# kind is only meaningful for the xy family.
+_VIZ_TYPE_TO_CHART_TYPE: dict[str, tuple[str, str | None]] = {
+ "bar": ("xy", "bar"),
+ "dist_bar": ("xy", "bar"),
+ "echarts_timeseries_bar": ("xy", "bar"),
+ "line": ("xy", "line"),
+ "echarts_timeseries_line": ("xy", "line"),
+ "echarts_timeseries_smooth": ("xy", "line"),
+ "area": ("xy", "area"),
+ "echarts_area": ("xy", "area"),
+ "scatter": ("xy", "scatter"),
+ "echarts_timeseries_scatter": ("xy", "scatter"),
+ "big_number_total": ("big_number", None),
+ "pivot_table_v2": ("pivot_table", None),
+}
+
+
+def _normalize_chart_request_input(data: Any) -> Any:
+ """Accept common Superset REST/form_data vocabulary in chart requests.
+
+ LLM clients reliably reach for Superset's public field names —
+ ``datasource_id``, ``viz_type``, and concrete viz plugin names such as
+ ``echarts_timeseries_bar`` — before consulting this API's schema. Each
+ rejection costs the client a model round trip, so the unambiguous
+ synonyms are translated instead of refused.
+ """
+ if not isinstance(data, dict):
+ return data
+ if "dataset_id" not in data and "datasource_id" in data:
+ data["dataset_id"] = data.pop("datasource_id")
+ config = data.get("config")
+ if isinstance(config, dict):
+ if "viz_type" in config:
+ viz_type = config["viz_type"]
+ if "chart_type" not in config:
+ config["chart_type"] = viz_type
+ config.pop("viz_type")
+ elif config.get("chart_type") != "table":
Review Comment:
**Suggestion:** When only `viz_type` is provided, the normalizer blindly
copies it into `chart_type` and removes `viz_type`. This breaks valid Superset
table plugin requests like `viz_type="ag-grid-table"` because `chart_type`
becomes an unsupported discriminator value instead of resolving to the table
chart config. Map known table plugin viz types (for example `ag-grid-table`) to
`chart_type="table"` before dropping `viz_type`. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ AG Grid table chart requests rejected despite valid config.
- ⚠️ LLM agents need extra retries, worsening latency.
- ⚠️ Explore link generation fails for ag-grid-table configs.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note the discriminated union `ChartConfig` in
`superset/mcp_service/chart/schemas.py`
(around PR lines 1749–1755) which uses `discriminator="chart_type"` and
documents allowed
values: `'xy'`, `'table'`, `'pie'`, `'pivot_table'`, `'mixed_timeseries'`,
`'handlebars'`,
`'big_number'`.
2. Observe the new normalizer `_normalize_chart_request_input` in the same
file (lines
1782–1810). When `config` contains `"viz_type"` but no `"chart_type"`, it
executes the
block at lines 1799–1801: it sets `config["chart_type"] = viz_type` and then
removes
`"viz_type"` entirely.
3. From `superset/mcp_service/app.py` lines 320–17, see that AG Grid tables
are a
supported Superset viz plugin: `chart_type="table",
viz_type="ag-grid-table": Interactive
AG Grid table`, meaning `viz_type="ag-grid-table"` is a legitimate request
shape in
Superset vocabulary.
4. Call the MCP `generate_chart` tool
(`superset/mcp_service/chart/tool/generate_chart.py:98`) or
`generate_explore_link`
(`superset/mcp_service/explore/tool/generate_explore_link.py:21`) with a
request using
only Superset-style plugin naming in the config, e.g. `{"dataset_id": 1,
"config":
{"viz_type": "ag-grid-table", "columns": [...]}}`. The
`ChartRequestNormalizerMixin` model
validator (schemas.py:113–119) invokes `_normalize_chart_request_input`,
which converts
`chart_type` to the raw string `"ag-grid-table"` and discards `viz_type`.
Since
`"ag-grid-table"` is not an accepted `chart_type` literal in `ChartConfig`
and is also not
mapped in `_VIZ_TYPE_TO_CHART_TYPE` (schemas.py:66–79 only covers
`'pivot_table_v2'` for
table family), Pydantic raises a validation error for an unknown
discriminator value. A
valid Superset table plugin request is thus rejected instead of being
normalized to
`chart_type="table"` and handled by `TableChartConfig`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=780cfbc919344db498465b50a9fa68cf&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=780cfbc919344db498465b50a9fa68cf&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:** 1799:1801
**Comment:**
*Incomplete Implementation: When only `viz_type` is provided, the
normalizer blindly copies it into `chart_type` and removes `viz_type`. This
breaks valid Superset table plugin requests like `viz_type="ag-grid-table"`
because `chart_type` becomes an unsupported discriminator value instead of
resolving to the table chart config. Map known table plugin viz types (for
example `ag-grid-table`) to `chart_type="table"` before dropping `viz_type`.
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%2F40972&comment_hash=1f6ff65cb468073af15306cd3e48367cff0313bed6f7dd3a6efdaefc93050f03&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40972&comment_hash=1f6ff65cb468073af15306cd3e48367cff0313bed6f7dd3a6efdaefc93050f03&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1736,6 +1759,65 @@ def validate_unique_column_labels(self) ->
"XYChartConfig":
# giving LLMs enough context to construct valid configs.
+# Superset viz_type values that LLM clients routinely send where this API
+# expects its chart_type discriminator. Each maps to (chart_type, kind);
+# kind is only meaningful for the xy family.
+_VIZ_TYPE_TO_CHART_TYPE: dict[str, tuple[str, str | None]] = {
+ "bar": ("xy", "bar"),
+ "dist_bar": ("xy", "bar"),
+ "echarts_timeseries_bar": ("xy", "bar"),
+ "line": ("xy", "line"),
+ "echarts_timeseries_line": ("xy", "line"),
+ "echarts_timeseries_smooth": ("xy", "line"),
+ "area": ("xy", "area"),
+ "echarts_area": ("xy", "area"),
+ "scatter": ("xy", "scatter"),
+ "echarts_timeseries_scatter": ("xy", "scatter"),
+ "big_number_total": ("big_number", None),
+ "pivot_table_v2": ("pivot_table", None),
+}
+
+
+def _normalize_chart_request_input(data: Any) -> Any:
+ """Accept common Superset REST/form_data vocabulary in chart requests.
+
+ LLM clients reliably reach for Superset's public field names —
+ ``datasource_id``, ``viz_type``, and concrete viz plugin names such as
+ ``echarts_timeseries_bar`` — before consulting this API's schema. Each
+ rejection costs the client a model round trip, so the unambiguous
+ synonyms are translated instead of refused.
+ """
+ if not isinstance(data, dict):
+ return data
+ if "dataset_id" not in data and "datasource_id" in data:
+ data["dataset_id"] = data.pop("datasource_id")
+ config = data.get("config")
+ if isinstance(config, dict):
+ if "viz_type" in config:
+ viz_type = config["viz_type"]
+ if "chart_type" not in config:
+ config["chart_type"] = viz_type
+ config.pop("viz_type")
+ elif config.get("chart_type") != "table":
+ config.pop("viz_type")
+ chart_type = config.get("chart_type")
Review Comment:
**Suggestion:** This branch discards `viz_type` whenever `chart_type` is
present and not `"table"`, even if the provided `chart_type` is invalid. That
means a request with a typo in `chart_type` but a valid `viz_type` fallback is
unrecoverably rejected. Keep `viz_type` as a fallback when `chart_type` is not
recognized, and only drop it after successful chart-type normalization. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Requests with typoed chart_type fail despite valid viz_type.
- ⚠️ LLM agents lose automatic recovery from minor mistakes.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `_VIZ_TYPE_TO_CHART_TYPE` at `superset/mcp_service/chart/schemas.py`
lines 66–79,
note that several Superset plugin `viz_type` values like
`"echarts_timeseries_bar"` and
`"pivot_table_v2"` are mapped to canonical `(chart_type, kind)` pairs (e.g.
`"echarts_timeseries_bar" -> ("xy", "bar")`), i.e. they can serve as a
fallback source of
truth for `chart_type`.
2. In `_normalize_chart_request_input` (schemas.py lines 1782–1810), when
both
`"viz_type"` and `"chart_type"` are present in `config`, the branch at lines
1802–1803
executes whenever `chart_type` is anything other than `"table"`: `elif
config.get("chart_type") != "table": config.pop("viz_type")`. This happens
before the
subsequent normalization that consults `_VIZ_TYPE_TO_CHART_TYPE`.
3. Invoke the MCP `generate_chart` tool
(`superset/mcp_service/chart/tool/generate_chart.py:98`) with a mixed-input
config that
includes a typoed `chart_type` but a valid Superset plugin `viz_type`, for
example:
`{"dataset_id": 1, "config": {"chart_type": "xyy", "viz_type":
"echarts_timeseries_bar",
"x": {"name": "date"}, "y": [{"name": "sales", "aggregate": "SUM"}]}}`. The
`ChartRequestNormalizerMixin` validator (schemas.py:113–119) calls
`_normalize_chart_request_input`, which sees both keys and, because
`"chart_type" !=
"table"`, executes the branch at 1802–1803 and deletes `"viz_type"`.
4. After `"viz_type"` has been discarded, `_normalize_chart_request_input`
looks up
`chart_type = config.get("chart_type")` and only normalizes values present in
`_VIZ_TYPE_TO_CHART_TYPE` (schemas.py:104–109). The typoed `"xyy"` is not
recognized or
corrected, so `ChartConfig`'s discriminated union (schemas.py:39–56) is
instantiated with
an invalid `chart_type` and Pydantic raises a validation error. If
`viz_type` had been
preserved until after normalization, the valid `"echarts_timeseries_bar"`
value could have
been used as a fallback via `_VIZ_TYPE_TO_CHART_TYPE`, turning this into a
recoverable
request instead of a hard failure.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=953997f100b64cd9bbb2d8fb2aa26b24&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=953997f100b64cd9bbb2d8fb2aa26b24&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:** 1802:1803
**Comment:**
*Logic Error: This branch discards `viz_type` whenever `chart_type` is
present and not `"table"`, even if the provided `chart_type` is invalid. That
means a request with a typo in `chart_type` but a valid `viz_type` fallback is
unrecoverably rejected. Keep `viz_type` as a fallback when `chart_type` is not
recognized, and only drop it after successful chart-type normalization.
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%2F40972&comment_hash=734bc122e1b76c9d5b59396e0ab361ac33dbb30d05cfa1f0d7df41be136d611d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40972&comment_hash=734bc122e1b76c9d5b59396e0ab361ac33dbb30d05cfa1f0d7df41be136d611d&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]
