-
Notifications
You must be signed in to change notification settings - Fork 182
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
feat: add spark_signed_integer_remainder native function for compatibility with spark #1416
base: main
Are you sure you want to change the base?
Conversation
wondering if we can fix in arrow/datafusion It looks addition is working, I wonder what would be the difference between
|
It seems to be by design in rust: https://github.com/rust-lang/rust/blob/f44efbf9e11b1b6bba77c046d7dd150de37e0e0f/library/core/src/num/int_macros.rs#L993 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1416 +/- ##
=============================================
- Coverage 56.12% 39.32% -16.81%
- Complexity 976 2085 +1109
=============================================
Files 119 265 +146
Lines 11743 61132 +49389
Branches 2251 12962 +10711
=============================================
+ Hits 6591 24040 +17449
- Misses 4012 32587 +28575
- Partials 1140 4505 +3365 ☔ View full report in Codecov by Sentry. |
Thanks for digging into the rust source.
But DataFusion is allowing the overflow
It looks postgres also returns 0, I will open a datafusion ticket on this |
Filed apache/datafusion#14771 |
@kazuyukitanimura
result:
|
I also tested
result:
Does this mean we can implement a similar |
The panic is because of the language specification: #1412 (comment) |
|
Yes, I agree that wrapping_rem seems like a reasonable way to address this. |
Thanks @kazuyukitanimura @parthchandra , I will try to fix this issue in upstream. |
Which issue does this PR close?
Closes #1412.
Rationale for this change
Fix overflow happened on: minimum integer value % -1
What changes are included in this PR?
add a
spark_signed_integer_remainder
native function for compatibility with sparkHow are these changes tested?
added unit test and bench