laskoviymishka commented on code in PR #3573:
URL: https://github.com/apache/iceberg-rust/pull/3573#discussion_r4472935662
##########
crates/iceberg/src/transform/truncate.rs:
##########
@@ -50,17 +50,22 @@ 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.
Review Comment:
Could this also say the wrapped value isn't order-preserving? At width 1000,
`i32::MIN` lands on `2_147_483_296`, and truncate projection assumes order
([`adjust_boundary`](https://github.com/apache/iceberg-rust/blob/81fe86b97006a47b0d73b0740cab004905698722/crates/iceberg/src/spec/transform.rs#L775-L778)).
As I read it, `value < i32::MIN + 700` projects to `name <= -2147483000` and
skips that partition. Release builds on `main` wrap the same way, so this is
only about the comment.
```suggestion
// 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.
```
##########
crates/iceberg/src/transform/truncate.rs:
##########
@@ -50,17 +50,22 @@ 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.
+ 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)
+ // See `truncate_i32`. Only mantissas outside the 38-digit decimal
range can wrap.
Review Comment:
Could this comment say the wrap here only avoids a panic? "See
`truncate_i32`" brings along "wrap as Java's `TruncateUtil` does", but Java's
`truncateDecimal` works on `BigInteger` and never wraps
([`TruncateUtil`](https://github.com/apache/iceberg/blob/f5704ee94b62839d79d9caf38045a3275fd3b4f0/api/src/main/java/org/apache/iceberg/util/TruncateUtil.java#L63-L74)).
The comment on `test_truncate_decimal_i128_min_does_not_panic` already says
this.
```suggestion
// 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.
```
##########
crates/iceberg/src/transform/truncate.rs:
##########
@@ -1010,6 +1081,14 @@ 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. This only pins that debug builds do not panic.
+ super::Truncate::truncate_decimal_i128(i128::MIN, 1000);
Review Comment:
Could this assert the value too? As written it only catches a debug panic.
`i128::MIN.rem_euclid(1000)` is 272, so the result wraps to `i128::MAX - 271`.
The "only pins that debug builds do not panic" sentence above can go with it.
```suggestion
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
);
```
##########
crates/iceberg/src/transform/truncate.rs:
##########
@@ -968,6 +1012,33 @@ 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),
+ // 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)
Review Comment:
Could the loop asserts say which case failed? The test at
[L993](https://github.com/apache/iceberg-rust/blob/81fe86b97006a47b0d73b0740cab004905698722/crates/iceberg/src/transform/truncate.rs#L993)
passes `"width: {width}, input: {input:?}"`. The same goes for the
`transform_literal` assert below and both asserts in
`test_truncate_long_min_matches_java`.
```suggestion
Some(&expected_array),
"width: {width}, value: {value}"
```
##########
crates/iceberg/src/transform/truncate.rs:
##########
@@ -968,6 +1012,33 @@ 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),
Review Comment:
Could the wrapped rows carry their derivation, so checking them doesn't mean
rerunning Java? `i32::MIN.rem_euclid(1000)` is 352, so the result wraps to
`i32::MAX - 351`. The `i64::MIN` rows in `test_truncate_long_min_matches_java`
work the same way: `i64::MAX - 191` and `i64::MAX - 2_147_483_644`.
```suggestion
(1000, i32::MIN, 2_147_483_296), // i32::MAX - 351: rem_euclid
is 352
```
--
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]
