aminghadersohi commented on code in PR #44603:
URL: https://github.com/apache/superset/pull/44603#discussion_r4112265521
##########
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:
VALID — fixed in 182953c812e78768544696e71314369f9622c00e. The multi-column
error now has a plural message and details listing the actual invalid names,
consistent with MULTIPLE_INVALID_COLUMNS. Regression coverage:
test_multiple_column_guidance_uses_real_candidates (with and without
candidates); the new message assertions failed before the fix. Focused MCP
tests: 74 passed. Pre-commit passed on all branch-changed files.
##########
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:
VALID — fixed in 182953c812e78768544696e71314369f9622c00e. Shortened the
empty-candidate fallback to 'No matching columns found.' The template provides
the get_dataset_info hint exactly once. test_generate_chart_column_guidance
asserts the hint occurs once; the no-match cases failed before the fix. Focused
MCP tests: 74 passed. Pre-commit passed on all branch-changed files.
##########
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:
VALID — fixed in 182953c812e78768544696e71314369f9622c00e. The actual
invalid names are preserved in escaped, length-limited details, while candidate
suggestions remain deduplicated. Tests assert both invalid names and
bounded/escaped details; the details regression failed against the previous
validator. Focused MCP tests: 74 passed. Pre-commit passed on all
branch-changed files.
--
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]
