wgtmac commented on code in PR #18112:
URL: https://github.com/apache/iceberg/pull/18112#discussion_r4390183491
##########
api/src/main/java/org/apache/iceberg/util/TruncateUtil.java:
##########
@@ -45,15 +45,15 @@ public class TruncateUtil {
private TruncateUtil() {}
public static byte truncateByte(int width, byte value) {
- return (byte) (value - (((value % width) + width) % width));
+ return (byte) (value - Math.floorMod(value, width));
Review Comment:
https://github.com/apache/iceberg/blob/e5f8ab19c82fcf24be430cc6c0cecf9a23f5afe4/format/spec.md?plain=1#L656
This fixes the implementation, but the spec above still doesn’t say the
arithmetic must use unbounded integers. Could we clarify the spec as `r =
floorMod(v, W)` and `v - r` computed with unbounded arithmetic? Otherwise other
implementations can still produce different partition values.
##########
api/src/main/java/org/apache/iceberg/util/TruncateUtil.java:
##########
@@ -45,15 +45,15 @@ public class TruncateUtil {
private TruncateUtil() {}
public static byte truncateByte(int width, byte value) {
- return (byte) (value - (((value % width) + width) % width));
+ return (byte) (value - Math.floorMod(value, width));
Review Comment:
Another issue is that `truncateInt(10, Integer.MIN_VALUE)` still returns
`Integer.MAX_VALUE - 1` after this PR: the final mathematical result is outside
int, so wrapping breaks ordering and can cause incorrect partition pruning as
reported in https://github.com/apache/iceberg/issues/9767 and
https://github.com/apache/iceberg/issues/13105.
Do we need a spec change for clarification?
--
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]
