Jefffrey commented on code in PR #11315:
URL: https://github.com/apache/arrow-rs/pull/11315#discussion_r4180097854
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -1877,6 +1880,53 @@ 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()
+ );
+
+ let two = i256::from(2);
+ for exponent in 53..=254 {
+ // Midpoint between the two binary64 neighbours of 2^exponent.
Review Comment:
ill reiterate that it would be good to have a comment explaining why this
calculates midpoint, not what it does (code makes it clear from the name)
e.g. why are we doing `exponent - 53`; at a glance it doesnt seem obvious
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -1877,6 +1880,53 @@ 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()
+ );
+
+ let two = i256::from(2);
+ for exponent in 53..=254 {
+ // Midpoint between the two binary64 neighbours of 2^exponent.
+ let midpoint = two.wrapping_pow(exponent) +
two.wrapping_pow(exponent - 53);
+ 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_correctly_rounded_parse() {
+ // `to_string().parse::<f64>()` is a correctly rounded
decimal-to-binary 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 rng = StdRng::seed_from_u64(0x9E37_79B9_7F4A_7C15);
Review Comment:
```suggestion
let mut rng = StdRng::seed_from_u64(42);
```
if we're gonna choose an arbitrary seed we might as well choose something
less complicated
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -1877,6 +1880,53 @@ 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()
+ );
+
+ let two = i256::from(2);
+ for exponent in 53..=254 {
+ // Midpoint between the two binary64 neighbours of 2^exponent.
+ let midpoint = two.wrapping_pow(exponent) +
two.wrapping_pow(exponent - 53);
+ 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_correctly_rounded_parse() {
+ // `to_string().parse::<f64>()` is a correctly rounded
decimal-to-binary oracle.
Review Comment:
```suggestion
fn test_i256_to_f64_fuzz() {
```
more clear this is meant to be fuzzing
--
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]
