mbutrovich commented on code in PR #3573:
URL: https://github.com/apache/iceberg-rust/pull/3573#discussion_r4448234423
##########
crates/iceberg/src/transform/truncate.rs:
##########
@@ -50,17 +50,21 @@ impl Truncate {
#[inline]
fn truncate_i32(v: i32, width: i32) -> i32 {
- v - v.rem_euclid(width)
+ // Match Java TruncateUtil, including int overflow wrapping.
+ v.wrapping_sub((v % width).wrapping_add(width) % width)
}
#[inline]
fn truncate_i64(v: i64, width: i64) -> i64 {
- v - (((v % width) + width) % width)
+ // Match Java TruncateUtil, including long overflow wrapping.
+ v.wrapping_sub((v % width).wrapping_add(width) % width)
}
#[inline]
fn truncate_decimal_i128(v: i128, width: i128) -> i128 {
- v - (((v % width) + width) % width)
+ // Match Java TruncateUtil. Java uses BigInteger here, so wrapping
only keeps
+ // out-of-range i128 values from panicking.
+ v.wrapping_sub((v % width).wrapping_add(width) % width)
}
Review Comment:
Should `truncate_i32` keep `rem_euclid` for the remainder? The spec defines
`int` and `long` truncation as `v - (v % W)` and says "remainders must be
positive"
([spec](https://github.com/apache/iceberg/blob/f5704ee94b62839d79d9caf38045a3275fd3b4f0/format/spec.md?plain=1#L653-L661)).
Note 1 offers `v - (((v % W) + W) % W)` as a way to get that positive
remainder in languages where `%` can be negative. As I read it, the formula is
a means to the stated requirement, not a requirement to overflow in 32 bits.
For `W > 2^30`, Java's `(v % W) + W` overflows and the remainder comes out
negative, so Java's value breaks the rule the note is there to satisfy.
`rem_euclid` gave the spec value.
Java is treating this as a bug. apache/iceberg#18111 reports it, and
apache/iceberg#18112 changes `truncateInt` to `Math.floorMod(value, width)`,
which matches `rem_euclid`. apache/iceberg-cpp#1042 also follows `v - (v % W)`
here and returns 0 for `truncate[2147483646](2147483645)`. PyIceberg computes
[`v - v %
self._width`](https://github.com/apache/iceberg-python/blob/e3564a54c10960307147377c578680276865ed18/pyiceberg/transforms.py#L867)
on unbounded ints, so it gets 0 too.
The Java value also breaks scan planning in iceberg-rust. [Truncate
projection](https://github.com/apache/iceberg-rust/blob/9ca22683c3809cc5e6da1aecc05e489a72d587d1/crates/iceberg/src/spec/transform.rs#L615-L619)
relies on truncate preserving order. At the head commit,
`truncate[2000000000]` maps `1` to `0` and `1_000_000_000` to `-2_000_000_000`.
The predicate `value >= 1` projects to `name >= 0`, which prunes the partition
`-2_000_000_000` even though it holds `1_000_000_000`.
The integer minimum is a different case. There the spec result doesn't fit
in the type, the spec doesn't say what to do, and Java wraps (still true after
apache/iceberg#18112, see [this
thread](https://github.com/apache/iceberg/pull/18112#discussion_r4390228646)).
Keeping `wrapping_sub` for that case matches Java and fixes the debug panic.
Could all three helpers use the same shape? For `i64` and `i128` this gives
the same values as the head commit, because `(v % width) + width` can't
overflow with a `u32` width. The difference is only in `i32`.
```suggestion
fn truncate_i32(v: i32, width: i32) -> i32 {
// 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 {
// See `truncate_i32`.
v.wrapping_sub(v.rem_euclid(width))
}
#[inline]
fn truncate_decimal_i128(v: i128, width: i128) -> i128 {
// See `truncate_i32`. Only mantissas outside the 38-digit decimal
range can wrap.
v.wrapping_sub(v.rem_euclid(width))
}
```
##########
crates/iceberg/src/transform/truncate.rs:
##########
@@ -968,6 +995,34 @@ mod test {
assert_eq!(res, Datum::int(-10),);
}
+ #[test]
+ fn test_truncate_int_matches_java() {
+ // 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),
+ // Smallest width where `(v % w) + w` can overflow i32.
+ (1_073_741_825, 1_073_741_824, 2_147_483_646),
+ // Tiny value, max width: Java wraps to 2.
+ (i32::MAX as u32, 1, 2),
Review Comment:
With the helper following the spec, the large-width cases expect `0`. Could
the test take its expected values from the spec and keep the Java values only
for `i32::MIN`, where the spec is silent?
```suggestion
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),
```
Could we also add a projection case, since wrong pruning is what users would
see? `TestProjectionFixture` covers it in a few lines:
```rust
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"),
)?;
```
--
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]
