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 i256 (#2637) #2781
Add i256 (#2637) #2781
Conversation
} | ||
|
||
impl Ord for i256 { | ||
fn cmp(&self, other: &Self) -> Ordering { |
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 related to #2360, performing the comparison this way is significantly faster than using memcmp, which is itself significantly faster than using BigInt.
FYI @liukun4515
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.
genius method.
arrow-buffer/src/bigint.rs
Outdated
/// Performs wrapping multiplication | ||
#[inline] | ||
pub fn wrapping_mul(self, other: Self) -> Self { | ||
let l = BigInt::from_signed_bytes_le(&self.0); | ||
let r = BigInt::from_signed_bytes_le(&other.0); | ||
Self::from_bigint_with_overflow(l * r).0 | ||
} | ||
|
||
/// Performs checked multiplication | ||
#[inline] | ||
pub fn checked_mul(self, other: Self) -> Option<Self> { | ||
let l = BigInt::from_signed_bytes_le(&self.0); | ||
let r = BigInt::from_signed_bytes_le(&other.0); | ||
let (val, overflow) = Self::from_bigint_with_overflow(l * r); | ||
(!overflow).then(|| val) | ||
} | ||
|
||
/// Performs wrapping division | ||
#[inline] | ||
pub fn wrapping_div(self, other: Self) -> Self { | ||
let l = BigInt::from_signed_bytes_le(&self.0); | ||
let r = BigInt::from_signed_bytes_le(&other.0); | ||
Self::from_bigint_with_overflow(l / r).0 | ||
} | ||
|
||
/// Performs checked division | ||
#[inline] | ||
pub fn checked_div(self, other: Self) -> Option<Self> { | ||
let l = BigInt::from_signed_bytes_le(&self.0); | ||
let r = BigInt::from_signed_bytes_le(&other.0); | ||
let (val, overflow) = Self::from_bigint_with_overflow(l / r); | ||
(!overflow).then(|| val) | ||
} | ||
|
||
/// Performs wrapping remainder | ||
#[inline] | ||
pub fn wrapping_rem(self, other: Self) -> Self { | ||
let l = BigInt::from_signed_bytes_le(&self.0); | ||
let r = BigInt::from_signed_bytes_le(&other.0); | ||
Self::from_bigint_with_overflow(l % r).0 | ||
} | ||
|
||
/// Performs checked division | ||
#[inline] | ||
pub fn checked_rem(self, other: Self) -> Option<Self> { | ||
if other.0 == [0; 32] { | ||
return None; | ||
} | ||
|
||
let l = BigInt::from_signed_bytes_le(&self.0); | ||
let r = BigInt::from_signed_bytes_le(&other.0); | ||
let (val, overflow) = Self::from_bigint_with_overflow(l % r); | ||
(!overflow).then(|| val) | ||
} | ||
} |
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.
Theoretically these would all benefit from an implementation not using BigInt, as this elides allocations and branches, but unlike the addition and subtraction kernels these are significantly more complicated to implement. I leave this as a potential future optimisation for someone who really cares about decimal256 performance
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. I'm a bit busy since last week. I will find some time to look at this in the weekend.
I need more time to review it tomorrow. Thanks @tustvold |
match overflow { | ||
true => assert!(checked.is_none()), | ||
false => assert_eq!(checked.unwrap(), actual), | ||
} |
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 doesn't include tests for div, rem because they are implemented using BigInt?
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.
Yeah I figured that would be a bit circular
} | ||
|
||
#[inline] | ||
fn mulx(a: u128, b: u128) -> (u128, u128) { |
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.
A simple doc might be nicer.
Benchmark runs are scheduled for baseline = 9b59081 and contender = 4df1f3c. 4df1f3c is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
There appears to be a mistake somewhere in this 😢 will look into https://github.com/apache/arrow-rs/actions/runs/3174304664/jobs/5170929134 |
Which issue does this PR close?
Part of #2637
Rationale for this change
This adds an i256 that can be used as the basis for a Decimal256Array, and implements common arithmetic for it.
What changes are included in this PR?
Are there any user-facing changes?