-
Notifications
You must be signed in to change notification settings - Fork 1.9k
RFC: add error macros #6740
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: add error macros #6740
Conversation
comphead
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that
if val.is_null() {
plan_err!(
"Invalid window frame: start bound cannot be UNBOUNDED FOLLOWING"
)?
}
} else if let WindowFrameBound::Preceding(val) = &end_bound {
if val.is_null() {
plan_err!(
"Invalid window frame: end bound cannot be UNBOUNDED PRECEDING"
)?
}
Looks more concise. Reg to format! checking if that can be done better
|
|
@alamb let me know what do you think. |
|
@alamb I can take all the findings and create a PR replacing all plan errors for now, if you think it is a right direction to go |
|
Sorry for the late reply @comphead
I think it looks great to me.
I think that would be good. Perhaps we you could make the macros and do a few changes and we can make sure it looks reasonable before spending the time to migrate the whole codebase? Thanks again @comphead |
|
I have no plans to pursue this PR at this time, so closing it |
Suggested by @comphead on https://github.com/apache/arrow-datafusion/pull/6721/files#r1235622262
I made some macros and then had trouble using them. Issues I ran into:
Result<..>rather thanDataFusionErrorso they can't be used in all places (e.g the one @comphead suggested in https://github.com/apache/arrow-datafusion/pull/6721/files#r1235622262)format!(...)which I couldn't figure out how to make into a nice macro invocationMaybe someone else has some ideas