Jefffrey commented on code in PR #11315:
URL: https://github.com/apache/arrow-rs/pull/11315#discussion_r4176525851
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -1877,6 +1880,64 @@ mod tests {
}
}
+ #[test]
+ fn test_i256_to_f64_midpoint_rounding() {
+ // Regression test for #11314: the value is one above the binary64
midpoint.
+ let integer = (1_i128 << 63) + 1024 + 1;
+ assert_eq!(
+ i256::from_i128(integer).to_f64().unwrap().to_bits(),
+ (integer as f64).to_bits()
+ );
+
+ fn pow2(exponent: u32) -> i256 {
+ if exponent < 128 {
+ i256::from_parts(1u128 << exponent, 0)
+ } else {
+ i256::from_parts(0, 1i128 << (exponent - 128))
+ }
+ }
+
+ for exponent in 53..=254 {
+ let midpoint = pow2(exponent) + (pow2(exponent - 52) >> 1_u8);
Review Comment:
could we have a brief comment here explaining why this calculates the
midpoint
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -677,7 +677,10 @@ impl i256 {
fn i256_to_f64(input: i256) -> f64 {
let k = i256::redundant_leading_sign_bits_i256(input);
let n = input << k; // left-justify (no redundant sign bits)
- let n = (n.high >> 64) as i64; // throw away the lower 192 bits
+ // Set the low bit if any discarded bit is set, so exact f64 midpoints
+ // round to the nearest value instead of to even.
Review Comment:
```suggestion
// In case we truncate to an exact midpoint, ensure lower bits
contribute
// to bumping just above midpoint to ensure correct rounding
// (Unless we are exactly on the midpoint, in which case lower bits
are zero)
```
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -1877,6 +1880,64 @@ mod tests {
}
}
+ #[test]
+ fn test_i256_to_f64_midpoint_rounding() {
+ // Regression test for #11314: the value is one above the binary64
midpoint.
+ let integer = (1_i128 << 63) + 1024 + 1;
+ assert_eq!(
+ i256::from_i128(integer).to_f64().unwrap().to_bits(),
+ (integer as f64).to_bits()
+ );
+
+ fn pow2(exponent: u32) -> i256 {
Review Comment:
we can probably just use existing checked_pow/wrapping_pow which is defined
in i256
https://github.com/apache/arrow-rs/blob/cac30c2d586bde9b3c13a3f1166ddba4fbf87cd1/arrow-buffer/src/bigint/mod.rs#L577-L607
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -1877,6 +1880,64 @@ mod tests {
}
}
+ #[test]
+ fn test_i256_to_f64_midpoint_rounding() {
+ // Regression test for #11314: the value is one above the binary64
midpoint.
+ let integer = (1_i128 << 63) + 1024 + 1;
+ assert_eq!(
+ i256::from_i128(integer).to_f64().unwrap().to_bits(),
+ (integer as f64).to_bits()
+ );
+
+ fn pow2(exponent: u32) -> i256 {
+ if exponent < 128 {
+ i256::from_parts(1u128 << exponent, 0)
+ } else {
+ i256::from_parts(0, 1i128 << (exponent - 128))
+ }
+ }
+
+ for exponent in 53..=254 {
+ let midpoint = pow2(exponent) + (pow2(exponent - 52) >> 1_u8);
+ for delta in -2_i64..=2 {
+ for value in [
+ midpoint + i256::from(delta),
+ -(midpoint + i256::from(delta)),
+ ] {
+ let expected: f64 = value.to_string().parse().unwrap();
+ assert_eq!(
+ value.to_f64().unwrap().to_bits(),
+ expected.to_bits(),
+ "i256 {value} should round to nearest"
+ );
+ }
+ }
+ }
+ }
+
+ #[test]
+ fn test_i256_to_f64_matches_decimal_oracle() {
+ for value in [i256::MIN, i256::MAX, i256::MINUS_ONE, i256::ZERO,
i256::ONE] {
+ let expected: f64 = value.to_string().parse().unwrap();
+ assert_eq!(value.to_f64().unwrap().to_bits(), expected.to_bits());
+ }
+
+ let mut state = 0x9E37_79B9_7F4A_7C15_u64;
+ let mut next = || {
+ state ^= state << 13;
+ state ^= state >> 7;
+ state ^= state << 17;
+ state
+ };
Review Comment:
why do we roll our own rng code here instead of using rng crate?
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -1877,6 +1880,64 @@ mod tests {
}
}
+ #[test]
+ fn test_i256_to_f64_midpoint_rounding() {
+ // Regression test for #11314: the value is one above the binary64
midpoint.
+ let integer = (1_i128 << 63) + 1024 + 1;
+ assert_eq!(
+ i256::from_i128(integer).to_f64().unwrap().to_bits(),
+ (integer as f64).to_bits()
+ );
+
+ fn pow2(exponent: u32) -> i256 {
+ if exponent < 128 {
+ i256::from_parts(1u128 << exponent, 0)
+ } else {
+ i256::from_parts(0, 1i128 << (exponent - 128))
+ }
+ }
+
+ for exponent in 53..=254 {
+ let midpoint = pow2(exponent) + (pow2(exponent - 52) >> 1_u8);
+ for delta in -2_i64..=2 {
+ for value in [
+ midpoint + i256::from(delta),
+ -(midpoint + i256::from(delta)),
+ ] {
+ let expected: f64 = value.to_string().parse().unwrap();
+ assert_eq!(
+ value.to_f64().unwrap().to_bits(),
+ expected.to_bits(),
+ "i256 {value} should round to nearest"
+ );
+ }
+ }
+ }
+ }
+
+ #[test]
+ fn test_i256_to_f64_matches_decimal_oracle() {
Review Comment:
im a little confused by this naming; why do we call it decimal oracle?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
