bito-code-review[bot] commented on code in PR #44603:
URL: https://github.com/apache/superset/pull/44603#discussion_r4109950005


##########
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:
   <div>
   
   
   <div id="suggestion">
   <div id="issue"><b>Duplicated suggestion text</b></div>
   <div id="fix">
   
   The empty-suggestions fallback now repeats guidance the template already 
provides: tracing `column_not_found_error` -> `build_error` -> 
`_format_suggestions`, the rendered list contains both "Use get_dataset_info to 
see available columns" (template line 136) and the fallback's identical 
sentence, so users with no candidate matches see the same hint twice in one 
list. Shorten the fallback to "No matching columns found." ([verified against 
`_format_suggestions` lines 291-312])
   </div>
   
   
   </div>
   
   
   
   
   <small><i>Code Review Run #1f053e</i></small>
   </div>
   
   ---
   Should Bito avoid suggestions like this for future reviews? (<a 
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
   - [ ] Yes, avoid them



##########
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:
   <div>
   
   
   <div id="suggestion">
   <div id="issue"><b>Invalid column names lost</b></div>
   <div id="fix">
   
   The multi-column branch now emits `Column 'multiple requested columns' not 
found in dataset` and drops the offending names the deleted `build_error` call 
listed (`Invalid columns: ...`). Callers can't tell which columns failed. 
Surface the real invalid names in the message/details while keeping the 
deduplicated `candidates` as suggestions.
   </div>
   
   
   </div>
   
   
   
   
   <small><i>Code Review Run #1f053e</i></small>
   </div>
   
   ---
   Should Bito avoid suggestions like this for future reviews? (<a 
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
   - [ ] Yes, avoid them



-- 
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]

Reply via email to