-
Notifications
You must be signed in to change notification settings - Fork 867
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
Add negate kernels (#4488) #4494
Conversation
Will add interval tests shortly, realise I forgot |
Should I wait to review this PR until you wrote those tests? Or did you add them to #4493 ? |
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.
Thanks @tustvold -- looks great to me
arrow-arith/src/numeric.rs
Outdated
}}; | ||
} | ||
|
||
/// Perform `!array`, returning an error on overflow |
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.
/// Perform `!array`, returning an error on overflow | |
/// Negates each element of `array`, returning an error on overflow |
I am not really sure what "Performs !array
" means
arrow-arith/src/numeric.rs
Outdated
/// Perform `!array`, returning an error on overflow | ||
/// | ||
/// Note: negation of unsigned arrays is not supported and will return in an error, | ||
/// for wrapping unsigned negation consider using [`neg_wrapping()`] |
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.
/// for wrapping unsigned negation consider using [`neg_wrapping()`] | |
/// for wrapping unsigned negation consider using [`neg_wrapping`] |
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.
This is necessary to avoid ambiguity, will change to a link
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.
sorry it just looked strange to me
arrow-arith/src/numeric.rs
Outdated
} | ||
} | ||
|
||
/// Perform `!array`, wrapping on overflow for [`DataType::is_integer`] |
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.
/// Perform `!array`, wrapping on overflow for [`DataType::is_integer`] | |
/// Negates each element of `array` , wrapping on overflow for [`DataType::is_integer`] |
Duration(Millisecond) => neg_checked!(DurationMillisecondType, array), | ||
Duration(Microsecond) => neg_checked!(DurationMicrosecondType, array), | ||
Duration(Nanosecond) => neg_checked!(DurationNanosecondType, array), | ||
Interval(YearMonth) => neg_checked!(IntervalYearMonthType, array), |
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 double checked that YearMonth intervals are stored as number of whole intervals and thus don't need to be treated field by field
Which issue does this PR close?
Closes #4488
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?