chore: attach Diagnostic to unary operator type errors#21288
chore: attach Diagnostic to unary operator type errors#21288hcrosse wants to merge 3 commits intoapache:mainfrom
Conversation
Closes apache#14433 - Attach rich `Diagnostic` (primary message, note, help) to the 'NOT requires boolean' and 'minus requires signed numeric' errors raised during SQL planning in `unary_op.rs` - Use `BinaryTypeCoercer` for the NOT boolean-coercibility check so `Dictionary(_, Boolean)` and other coercible types are accepted, matching the analyzer's acceptance rule - Extend `Expr::spans()` to recurse through `Not` and `Negative` wrappers so column source spans propagate to the error - Update `select_neg_filter` test to expect the new planning-time error - Add four diagnostic tests covering column and non-column operands for both NOT and unary minus
github-actions
bot
added
sql
|
|
||
| #[test] | ||
| fn select_neg_filter() { | ||
| // NOT requires a boolean expression; applying it to a Utf8 column is an error |
There was a problem hiding this comment.
Can we also ensure we have test coverage for planning an actual valid not expression?
| ))), | ||
| UnaryOperator::Not => { | ||
| let operand = | ||
| self.sql_expr_to_logical_expr(expr, schema, planner_context)?; |
There was a problem hiding this comment.
This checking is normally done as part of analysis (not the sql planner) (aka in an Analyzer rule) -- if we put it in sql planning then it won't apply to queries that don't come from SQL.
There was a problem hiding this comment.
Thanks for the review! The + operator diagnostic was done the same way in #15209, do you think we should move all three to the analyzer, or do you want me to add a similar check there in addition to what I'm adding here?
There was a problem hiding this comment.
It would be great to move this into the analyzer too (as a follow on PR once we have the pattern sorted out for this one)
- Add select_not_bool_filter test for the happy path (NOT on a boolean column) to complement the error-case test in select_neg_filter
- Update invalid_wrapped_negation test to expect error at planning time - Update scalar.slt expected error messages for NOT and minus
github-actions
bot
added
core
2 participants
Which issue does this PR close?
Diagnosticto "incompatible type in unary expression" error #14433What changes are included in this PR?
Attaches
Diagnostic(message, note, help) to theNOTand-error paths inunary_op.rs. Also extendsExpr::spans()to recurse throughNot/Negativeso column source locations propagate to the error.+already had this, this PR covers the remaining two operators.Are these changes tested?
Yes, added tests in
diagnostic.rsfor column and non-column operands for both operators.Are there any user-facing changes?
Error messages for
NOT <non-boolean>and- <non-numeric>now include source location and a fix suggestion.