Baunsgaard commented on code in PR #3573:
URL: https://github.com/apache/iceberg-rust/pull/3573#discussion_r4414088377
##########
crates/iceberg/src/transform/truncate.rs:
##########
@@ -50,17 +50,17 @@ impl Truncate {
#[inline]
fn truncate_i32(v: i32, width: i32) -> i32 {
- v - v.rem_euclid(width)
+ v.wrapping_sub((v % width).wrapping_add(width) % width)
}
#[inline]
fn truncate_i64(v: i64, width: i64) -> i64 {
- v - (((v % width) + width) % width)
+ v.wrapping_sub((v % width).wrapping_add(width) % width)
Review Comment:
add comment
##########
crates/iceberg/src/transform/truncate.rs:
##########
@@ -50,17 +50,17 @@ impl Truncate {
#[inline]
fn truncate_i32(v: i32, width: i32) -> i32 {
- v - v.rem_euclid(width)
+ v.wrapping_sub((v % width).wrapping_add(width) % width)
}
#[inline]
fn truncate_i64(v: i64, width: i64) -> i64 {
- v - (((v % width) + width) % width)
+ v.wrapping_sub((v % width).wrapping_add(width) % width)
}
#[inline]
fn truncate_decimal_i128(v: i128, width: i128) -> i128 {
- v - (((v % width) + width) % width)
+ v.wrapping_sub((v % width).wrapping_add(width) % width)
Review Comment:
add comment
##########
crates/iceberg/src/transform/truncate.rs:
##########
@@ -902,6 +902,19 @@ mod test {
);
}
+ #[test]
+ fn test_truncate_long_min_does_not_panic() {
Review Comment:
nit: the name `test_truncate_long_min_does_not_panic` sounds like the
decimal test, which only checks that debug does not crash and then throws the
result away. This test does more.
Something like `test_truncate_long_min_matches_java` matches what it asserts.
I would also extend it to do like the int test, to iterate a few cases:
```rust
for (width, value, expected) in [
(1000, i64::MIN, 9_223_372_036_854_775_616),
(i32::MAX as u32, i64::MIN, 9_223_372_034_707_292_163),
] {
let truncate = super::Truncate::new(width);
let input = Arc::new(Int64Array::from(vec![Some(value), None]));
let result = truncate.transform(input).unwrap();
let expected_array = Int64Array::from(vec![Some(expected), None]);
assert_eq!(
result.as_any().downcast_ref::<Int64Array>(),
Some(&expected_array)
);
}
```
##########
crates/iceberg/src/transform/truncate.rs:
##########
@@ -50,17 +50,17 @@ impl Truncate {
#[inline]
fn truncate_i32(v: i32, width: i32) -> i32 {
- v - v.rem_euclid(width)
+ v.wrapping_sub((v % width).wrapping_add(width) % width)
Review Comment:
nit: `v.wrapping_sub((v % width).wrapping_add(width) % width)` reads like a
bad `v - v.rem_euclid(width)`, which is exactly the bug this PR undoes.
One comment above would keep someone from "simplifying" it later:
```rust
// Match Java TruncateUtil, including int/long overflow wrapping.
```
##########
crates/iceberg/src/transform/truncate.rs:
##########
@@ -968,6 +981,30 @@ mod test {
assert_eq!(res, Datum::int(-10),);
}
+ #[test]
+ fn test_truncate_int_matches_java_wrapping_arithmetic() {
+ // Expected values computed with Iceberg Java's
`TruncateUtil.truncateInt`
+ // (iceberg 1.11.0, JDK 17).
+ for (width, value, expected) in [
+ (2_000_000_000, 1_500_000_000, -2_000_000_000),
+ (i32::MAX as u32, i32::MAX - 1, -2_147_483_647),
+ (1000, i32::MIN, 2_147_483_296),
Review Comment:
other fun ones:
// Smallest width where (v % w) + w can overflow i32.
(1_073_741_825, 1_073_741_824, 2_147_483_646), // 2^30+1 ; rem_euclid was 0
// Tiny value, max width: Java wraps to 2, rem_euclid was 0.
(i32::MAX as u32, 1, 2),
##########
crates/iceberg/src/transform/truncate.rs:
##########
@@ -968,6 +981,30 @@ mod test {
assert_eq!(res, Datum::int(-10),);
}
+ #[test]
+ fn test_truncate_int_matches_java_wrapping_arithmetic() {
Review Comment:
nit: i would consider removing the `wrapping_arithmetic` part of the name.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
