chore: Attach Diagnostic to "incompatible type in unary expression" error #15209
Conversation
|
@eliaperantoni could you help review? :) |
onlyjackfrost
force-pushed
the
chore/unary-function
branch
from
94230d5 to
3310087
Compare
There was a problem hiding this comment.
Hey @onlyjackfrost, thank you for your contribution!
The PR looks good, I left some comments. Though, I don't think we can close issue #14433 just with this PR because only a very specific operator is being handled. Is there any way you can extend it to all unary expressions?
datafusion/sql/src/expr/unary_op.rs
Outdated
| let spans = operand.spans(); | ||
| let span = if let Some(s) = spans.as_ref() { s.first() } else { None }; |
There was a problem hiding this comment.
| let spans = operand.spans(); | |
| let span = if let Some(s) = spans.as_ref() { s.first() } else { None }; | |
| let spans = operand.spans().map(|s| s.first()); |
datafusion/sql/src/expr/unary_op.rs
Outdated
| let diagnostic = | ||
| Diagnostic::new_error("Unary operator '+' only supports numeric, interval and timestamp types".to_string(), span); |
There was a problem hiding this comment.
Not a big deal but I think this message is a bit long for a Diagnostic and it addresses the problem in a very broad way, saying "+ only works with X, Y, and Z" instead of simply "+ cannot be used with W".
Is it possible to make the Diagnostic look like this:
- message = "+ cannot be used with {type-name}"
- note (without span) = "+ can only be used with numbers, intervals, and timestamps"
- help (without span) = "perhaps you need to cast {expression}"
There was a problem hiding this comment.
Sure, it will be better this way.
Sure. I was thinking about doing the same data type check in other unary operator. |
I'm not 100% what you mean by that. For I just hope that all necessary checks already happen in that file, and not "further up" cause that would require more work probably. |
onlyjackfrost
force-pushed
the
chore/unary-function
branch
from
8733dc0 to
43132f8
Compare
onlyjackfrost
force-pushed
the
chore/unary-function
branch
from
43132f8 to
16a181b
Compare
|
@eliaperantoni could I raise another PR for the others unary expressions and keep this PR for the PLUS unary expression? |
Yes absolutely 😊 |
datafusion/sql/src/expr/unary_op.rs
Outdated
| if span.is_none() { | ||
| diagnostic.add_note("+ can only be used with numbers, intervals, and timestamps", None); | ||
| diagnostic.add_help(format!("perhaps you need to cast {operand}"), None); | ||
| } |
There was a problem hiding this comment.
Why do this only when the span is None? I think it should always be there
There was a problem hiding this comment.
note (without span) = "+ can only be used with numbers, intervals, and timestamps"
help (without span) = "perhaps you need to cast {expression}"
oops... I might have misunderstood your reply.
I'll remove the condition from here.
There was a problem hiding this comment.
delivered in ef3fca8
There was a problem hiding this comment.
Ahh sorry! Hahah. Yeah I meant to say that the note and the help should have no Span attached to them, in my opinion. But they (the note and the help) should always be there.
| #[test] | ||
| fn test_unary_op_plus_with_non_column() -> Result<()> { | ||
| // create a table with a column of type varchar | ||
| let query = "SELECT /*whole*/+'a'/*whole*/ "; |
There was a problem hiding this comment.
I would remove the span tags /*whole*/ since they aren't used by the test anyway
There was a problem hiding this comment.
delivered in ef3fca8
There was a problem hiding this comment.
Other than the comments from @eliaperantoni I think this PR looks good to me
Thank you @onlyjackfrost (and @eliaperantoni 🙏 )
|
Thanks again everyone! |
Which issue does this PR close?
Diagnosticto "incompatible type in unary expression" error #14433What changes are included in this PR?
attach a Diagnostic when dealing with "incompatible type in unary expression" error
Are these changes tested
Yes, I've tested the PLUS operator with column and non-column operands.