chore: attach Diagnostic to unary operator type errors#21288
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
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
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.