-
Notifications
You must be signed in to change notification settings - Fork 839
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
Remove ArrowSignedNumericType
to Simplify and reduce code duplication in arithmetic kernels
#1161
Conversation
/// SIMD vectorized version of `divide`. | ||
/// | ||
/// The divide kernels need their own implementation as there is a need to handle situations | ||
/// where a divide by `0` occurs. This is complicated by `NULL` slots and padding. |
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.
The comment needs to be updated too?
Codecov Report
@@ Coverage Diff @@
## master #1161 +/- ##
==========================================
+ Coverage 82.55% 82.59% +0.04%
==========================================
Files 173 173
Lines 50673 50612 -61
==========================================
- Hits 41833 41804 -29
+ Misses 8840 8808 -32
Continue to review full report at Codecov.
|
This looks great @jhorstmann -- I am sorry I ran out of time today to review Arrow PRs -- i will give this one a careful look tomorrow |
Marking as an API change due to One datapoint: the only place I can find it seems to be in a copy of the arrow-rs codebase: https://sourcegraph.com/search?q=context:global+ArrowSignedNumericType&patternType=literal Likewise github shows results only from forks / copies of Arrow as well: https://github.com/search?q=ArrowSignedNumericType&type=code |
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 looks a lot simpler to me. 👍 very nice -- thank you for the contribution @jhorstmann ❤️
FWIW we are going to try releasing arrow 8.0.0 next week from master
next week (as it contains some other breaking changes)
ArrowSignedNumericType
to Simplify and reduce code duplication in arithmetic kernels
Which issue does this PR close?
Closes #1160.
Rationale for this change
Reduces some code duplication and hopefully makes the whole file a bit easier to understand.
What changes are included in this PR?
ArrowSignedNumericType
which was only introduced for a single kernelAre there any user-facing changes?
The removal of
ArrowSignedNumericType
is a breaking change but I don't think there were any usages outside of the arrow crate or even thearithmetic.rs
file.cc @alamb @viirya