codeant-ai-for-open-source[bot] commented on code in PR #42070:
URL: https://github.com/apache/superset/pull/42070#discussion_r3592124606
##########
superset/mcp_service/chart/validation/dataset_validator.py:
##########
@@ -271,7 +271,7 @@ def _extract_column_references(
"""Extract all column references from configuration via the plugin
registry.
Previously only handled TableChartConfig and XYChartConfig, causing
- 5 of 7 chart types to silently skip column validation. Now delegates
+ most chart types to silently skip column validation. Now delegates
to the plugin for each chart type so all types are covered.
Review Comment:
**Suggestion:** The updated docstring overstates behavior by saying all
chart types are covered, but the implementation still returns an empty list
when no plugin is registered for a chart type. This can mislead maintainers
about validation guarantees; update the wording to clarify it only covers chart
types with registered plugins. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
⚠️ Suggestion affects docstring, not runtime validation behavior.
⚠️ Schema validator already rejects unsupported or disabled chart types.
⚠️ Supported chart types always use plugins for column checks.
⚠️ Docstring change would bring minimal practical improvement.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect SchemaValidator._pre_validate_chart_type in
superset/mcp_service/chart/validation/schema_validator.py:140-219, which uses
get_registry().is_registered(chart_type) and returns INVALID_CHART_TYPE or
DISABLED_CHART_TYPE errors if the chart type is not registered or its plugin
is disabled.
2. Observe that ValidationPipeline._validate_dataset in
superset/mcp_service/chart/validation/pipeline.py:178-195 calls
DatasetValidator.validate_against_dataset only after schema validation
succeeds, so
dataset validation runs only for chart types with a valid, enabled plugin.
3. Inspect DatasetValidator._extract_column_references in
superset/mcp_service/chart/validation/dataset_validator.py:18-43, where
plugin =
get_registry().get(chart_type) is retrieved; in normal validated flows
chart_type is
guaranteed to be registered and enabled, so plugin is non-None and all
supported chart
types are covered via their plugins.
4. Note that the plugin-is-None branch returning [] in
_extract_column_references is a
defensive fallback for misuse (calling DatasetValidator directly with an
unsupported
chart_type) and is not exercised in the main validated pipeline; the
docstring’s “all
types are covered” accurately describes behavior for supported chart types,
making the
suggested wording change a minor documentation nit rather than a concrete
bug.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=10dac841f3264f449c825a23b8c31fde&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=10dac841f3264f449c825a23b8c31fde&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/validation/dataset_validator.py
**Line:** 274:275
**Comment:**
*Comment Mismatch: The updated docstring overstates behavior by saying
all chart types are covered, but the implementation still returns an empty list
when no plugin is registered for a chart type. This can mislead maintainers
about validation guarantees; update the wording to clarify it only covers chart
types with registered plugins.
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=f7f3e62d6677e20fd6a1dadbdc87129bcbb69b650f9cef8f0bb2817304ee5a74&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42070&comment_hash=f7f3e62d6677e20fd6a1dadbdc87129bcbb69b650f9cef8f0bb2817304ee5a74&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]
