fix: Fixed error handling for generate_series/range#16391
Conversation
github-actions
Bot
added
sqllogictest
| _ => return plan_err!("First argument must be an integer literal"), | ||
| other => { | ||
| return plan_err!( | ||
| "Argument #{} must be an integer literal or null value, got {:?}", |
There was a problem hiding this comment.
Is there a better way to word this, Argument #... seems awkward 🤷
There was a problem hiding this comment.
I don' have any good suggestions
I do think the {:?} might be hard to read for complex expressions. Maybe you could change this to have the display name instead. Something like
| "Argument #{} must be an integer literal or null value, got {:?}", | |
| "Argument #{} must be an integer literal or null value, got {} ({:?})", expr_index, other, other, |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Looks good to me -- thank you @jonathanc-n
| _ => return plan_err!("First argument must be an integer literal"), | ||
| other => { | ||
| return plan_err!( | ||
| "Argument #{} must be an integer literal or null value, got {:?}", |
There was a problem hiding this comment.
I don' have any good suggestions
I do think the {:?} might be hard to read for complex expressions. Maybe you could change this to have the display name instead. Something like
| "Argument #{} must be an integer literal or null value, got {:?}", | |
| "Argument #{} must be an integer literal or null value, got {} ({:?})", expr_index, other, other, |
|
Fixed @alamb! Should be good now |
| return plan_err!( | ||
| "Argument #{} must be an integer literal or null value, got {} ({:?})", | ||
| expr_index + 1, | ||
| other, |
There was a problem hiding this comment.
do we need to output other twice?
There was a problem hiding this comment.
I think @alamb's intention here was to have one less verbose and one with more information in case the user wants.
There was a problem hiding this comment.
yeah, I'm checking the message in the slt test
got Utf8\("foo"\) \(Literal\(Utf8\("foo"\), None\)\)
would be that self explanatory to keep the last one only?
There was a problem hiding this comment.
Yeah I think it would be, let me change that
|
Thanks again @jonathanc-n |
3 participants
Which issue does this PR close?
Rationale for this change
I was reviewing #16388 and noticed that the linked issue had an incorrect error message.
What changes are included in this PR?
Correct the argument that was being returned and made the message more verbose. Also did a driveby clean on some nits for other error messages
Are these changes tested?
Yes