This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-3573-72addd94670db273cdb03f4efce23913991a049e in repository https://gitbox.apache.org/repos/asf/iceberg-rust.git
commit 6f6d68bb29d00cd34a93a4d598f882b97020f676 Author: 龙骑士k <[email protected]> AuthorDate: Fri Nov 6 07:45:07 2026 +0000 fix(transform): avoid overflow panic in truncate at integer minimum (#3573) * fix(transform): match Java truncate wrapping arithmetic ## Which issue does this PR close? - Closes issue 3141. ## What changes are included in this PR? `truncate_i32` used `rem_euclid`, while `truncate_i64` and `truncate_decimal_i128` used `(v % w + w) % w`. Both shapes differ from Iceberg Java's `TruncateUtil`, which relies on `int`/`long` overflow wrapping: - For `truncate_i32`, `rem_euclid` never overflows, so any width above `2^30` produced a different partition value than Java. - For the other two, the final subtraction overflowed at the integer minimum, panicking in debug builds and wrapping to the Java value only in release builds. All three helpers now use `wrapping_sub` and `wrapping_add`, so every width reproduces the Java result in both debug and release builds. Each helper has a comment noting that the formula intentionally matches Java, so it is not "simplified" back to `rem_euclid` later. The function signatures, the public API, and the call sites are unchanged. ## Are these changes tested? Three unit tests in `crates/iceberg/src/transform/truncate.rs`. All expected values were computed with Iceberg Java's `TruncateUtil` (1.11.0, JDK 17): - `test_truncate_int_matches_java` covers the three `i32` cases from the issue, plus the smallest width where `(v % w) + w` can overflow (`2^30 + 1`) and a tiny value at the maximum width. Each case is checked through both `transform` on an array and `transform_literal`. - `test_truncate_long_min_matches_java` covers `i64::MIN` with widths 1000 and `i32::MAX`, through the same two paths. - `test_truncate_decimal_i128_min_does_not_panic` pins that the decimal path does not panic at `i128::MIN`. That value is outside the range of a 38-digit Iceberg decimal, and Java's `truncateDecimal` works on `BigInteger`, so there is no wrapping result to compare against. Reverting the helpers to their previous bodies makes all three tests fail. For the `i64` cases the old formula happens to give the Java values once wrapped, so it is caught by the debug-build overflow panic. ## AI Disclosure Human-led contribution. Codex and Grok Bot, working for dragonknightk, assisted with this change. All submitted content was reviewed by a human before submission. * fix(transform): follow the spec for truncate remainders * test(transform): document truncate wrap results and assert the i128 value --- crates/iceberg/src/transform/truncate.rs | 95 +++++++++++++++++++++++++++++++- 1 file changed, 92 insertions(+), 3 deletions(-) diff --git a/crates/iceberg/src/transform/truncate.rs b/crates/iceberg/src/transform/truncate.rs index 923650298..3efb09258 100644 --- a/crates/iceberg/src/transform/truncate.rs +++ b/crates/iceberg/src/transform/truncate.rs @@ -50,17 +50,24 @@ impl Truncate { #[inline] fn truncate_i32(v: i32, width: i32) -> i32 { - v - v.rem_euclid(width) + // The spec requires a non-negative remainder, which `rem_euclid` computes without + // overflow. Only the subtraction can overflow, when the result is below `i32::MIN`. + // The spec doesn't cover that case, so wrap as Java's `TruncateUtil` does. + // The wrapped result isn't order-preserving, which truncate projection assumes. + v.wrapping_sub(v.rem_euclid(width)) } #[inline] fn truncate_i64(v: i64, width: i64) -> i64 { - v - (((v % width) + width) % width) + // See `truncate_i32`. + v.wrapping_sub(v.rem_euclid(width)) } #[inline] fn truncate_decimal_i128(v: i128, width: i128) -> i128 { - v - (((v % width) + width) % width) + // Only mantissas outside the 38-digit decimal range can wrap. Java's `truncateDecimal` + // uses `BigInteger`, so there is no Java result to match and the wrap only avoids a panic. + v.wrapping_sub(v.rem_euclid(width)) } } @@ -826,6 +833,22 @@ mod test { Ok(()) } + #[test] + fn test_projection_truncate_integer_large_width() -> Result<()> { + let fixture = TestProjectionFixture::new( + Transform::Truncate(2_000_000_000), + "name", + NestedField::required(1, "value", Primitive(Int)), + ); + + fixture.assert_projection( + &fixture.binary_predicate(PredicateOperator::Eq, Datum::int(1_500_000_000)), + Some("name = 0"), + )?; + + Ok(()) + } + // Test case ref from: https://iceberg.apache.org/spec/#truncate-transform-details #[test] fn test_truncate_simple() { @@ -902,6 +925,32 @@ mod test { ); } + #[test] + fn test_truncate_long_min_matches_java() { + // Expected values computed with Iceberg Java's `TruncateUtil.truncateLong` + // (iceberg 1.11.0, JDK 17). + for (width, value, expected) in [ + (1000, i64::MIN, 9_223_372_036_854_775_616), // i64::MAX - 191: rem_euclid is 192 + // i64::MAX - 2_147_483_644: rem_euclid is 2_147_483_645 + (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), + "width: {width}, value: {value}" + ); + assert_eq!( + truncate.transform_literal(&Datum::long(value)).unwrap(), + Some(Datum::long(expected)), + "width: {width}, value: {value}" + ); + } + } + #[test] fn test_string_truncate() { let test1 = "イロハニホヘト"; @@ -968,6 +1017,35 @@ mod test { assert_eq!(res, Datum::int(-10),); } + #[test] + fn test_truncate_int_large_width() { + // Expected values follow the spec: `v - (v % W)` with a non-negative remainder. + // Below `i32::MIN` the spec is silent, and the result wraps as Java's `TruncateUtil` does. + for (width, value, expected) in [ + (2_000_000_000, 1_500_000_000, 0), + (i32::MAX as u32, i32::MAX - 1, 0), + (1000, i32::MIN, 2_147_483_296), // i32::MAX - 351: rem_euclid is 352 + // Smallest width where `(v % w) + w` can overflow i32. + (1_073_741_825, 1_073_741_824, 0), + (i32::MAX as u32, 1, 0), + ] { + let truncate = super::Truncate::new(width); + let input = Arc::new(Int32Array::from(vec![Some(value), None])); + let result = truncate.transform(input).unwrap(); + let expected_array = Int32Array::from(vec![Some(expected), None]); + assert_eq!( + result.as_any().downcast_ref::<Int32Array>(), + Some(&expected_array), + "width: {width}, value: {value}" + ); + assert_eq!( + truncate.transform_literal(&Datum::int(value)).unwrap(), + Some(Datum::int(expected)), + "width: {width}, value: {value}" + ); + } + } + #[test] fn test_literal_long() { let input = Datum::long(1); @@ -1010,6 +1088,17 @@ mod test { } } + #[test] + fn test_truncate_decimal_i128_min_does_not_panic() { + // `i128::MIN` is outside the range of a 38-digit Iceberg decimal, and Java's + // `TruncateUtil.truncateDecimal` works on `BigInteger`, so there is no wrapping + // result to match. + assert_eq!( + super::Truncate::truncate_decimal_i128(i128::MIN, 1000), + 170_141_183_460_469_231_731_687_303_715_884_105_456, // i128::MAX - 271 + ); + } + #[test] fn test_binary_literal() { for (value, width, expected) in [
