bito-code-review[bot] commented on code in PR #44603:
URL: https://github.com/apache/superset/pull/44603#discussion_r4112266409
##########
superset/mcp_service/utils/error_builder.py:
##########
@@ -363,7 +363,10 @@ def column_not_found_error(
) -> ChartGenerationError:
"""Build a column not found error."""
suggestion_text = (
- ", ".join(suggestions[:3]) if suggestions else "Check available
columns"
+ f"Did you mean: {', '.join(suggestions[:3])}?"
+ if suggestions
+ else "No matching columns found. "
+ "Use get_dataset_info to see available columns."
Review Comment:
<!-- Bito Reply -->
The suggestion provided by the reviewer is appropriate and addresses the
issue of redundant guidance text. Applying this change improves the user
experience by ensuring the fallback message is concise and does not repeat
information already present in the template.
**superset/mcp_service/utils/error_builder.py**
```
suggestion_text = (
f"Did you mean: {', '.join(suggestions[:3])}?"
if suggestions
else "No matching columns found. "
"Use get_dataset_info to see available columns."
```
##########
superset/mcp_service/chart/validation/dataset_validator.py:
##########
@@ -690,34 +691,43 @@ def _build_column_error(
)
if len(invalid_columns) == 1:
- col = invalid_columns[0]
- col_name = col.name or "<unknown column>"
- suggestions = suggestions_map.get(col_name, [])
-
- if suggestions:
- return ChartErrorBuilder.column_not_found_error(
- col_name, [s.name for s in suggestions]
- )
- else:
- return ChartErrorBuilder.column_not_found_error(col_name)
+ col_name = invalid_columns[0].name or "<unknown column>"
+ error = ChartErrorBuilder.column_not_found_error(
+ col_name, [s.name for s in suggestions_map.get(col_name, [])]
+ )
else:
- # Multiple invalid columns
- invalid_names: list[str] = [col.name for col in invalid_columns if
col.name]
- return ChartErrorBuilder.build_error(
- error_type="multiple_invalid_columns",
- template_key="column_not_found",
- template_vars={
- "column": ", ".join(invalid_names[:3])
- + ("..." if len(invalid_names) > 3 else ""),
- "suggestions": "Use get_dataset_info to see all available
columns",
- },
- custom_suggestions=[
- f"Invalid columns: {', '.join(invalid_names)}",
- "Check spelling and case sensitivity",
- "Use get_dataset_info to list available columns",
- ],
- error_code="MULTIPLE_INVALID_COLUMNS",
+ candidates = list(
+ dict.fromkeys(
+ suggestion.name
+ for suggestions in suggestions_map.values()
+ for suggestion in suggestions
+ )
+ )
+ error = ChartErrorBuilder.column_not_found_error(
+ "multiple requested columns", candidates
)
+ error.error_type = "multiple_invalid_columns"
+ error.error_code = "MULTIPLE_INVALID_COLUMNS"
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to surface the actual invalid column names in the
error message is appropriate. The current implementation in the diff hunk
replaces the detailed `build_error` call with a generic `multiple requested
columns` message, which loses the specific context of which columns failed.
Applying the suggestion to include the invalid column names while maintaining
the deduplicated candidates will improve the clarity of the error for the user.
--
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]
