eliaperantoni commented on code in PR #14521:
URL: https://github.com/apache/datafusion/pull/14521#discussion_r1946133768
##########
datafusion/common/src/column.rs:
##########
@@ -299,6 +301,23 @@ impl Column {
.flat_map(|s| s.columns())
.collect(),
})
+ .map_err(|e| match &e {
+ DataFusionError::SchemaError(
+ SchemaError::FieldNotFound {
+ field,
+ valid_fields,
+ },
+ _,
+ ) => {
+ let mut diagnostic = Diagnostic::new_error(
+ format!("column '{}' is ambiguous", &field.name()),
+ field.spans().first(),
+ );
+ add_possible_column_notes(&mut diagnostic, field,
valid_fields);
+ e.with_diagnostic(diagnostic)
+ }
+ _ => e,
+ })
Review Comment:
What I meant is that the same error is covered twice. At the top level, it
looks like this:
https://github.com/apache/datafusion/blob/bab0f54daa99830339c0000a19c1b4e3489278ad/datafusion/sql/src/select.rs#L584-L594
The error that I added is bubbled up at line 593 because `sql_to_expr` calls
`validate_schema_satisfies_exprs`. Yours is bubbled up the line after at
`normalize_col_with_schemas_and_ambiguity_check`. I think it's a bit
unnecessary because this path should never be hit, or you would've noticed
tests started failing.
I would suggest:
- Removing this `Diagnostic` for a field not found, since you already
covered that by adding a call to `add_possible_column_notes` in
`validate_schema_satisfies_exprs`
- Using `add_possible_column_notes` a few lines above to truly cover the
case of an ambiguous column, here:
https://github.com/apache/datafusion/blob/bab0f54daa99830339c0000a19c1b4e3489278ad/datafusion/common/src/column.rs#L268-L288
Also, could you add a new test for suggestions for an ambiguous column too
(you have one for "field not found")
--
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]
