RussellSpitzer commented on a change in pull request #2506:
URL: https://github.com/apache/iceberg/pull/2506#discussion_r620391167
##########
File path: core/src/main/java/org/apache/iceberg/TableOperations.java
##########
@@ -117,7 +117,7 @@ default long newSnapshotId() {
UUID uuid = UUID.randomUUID();
long mostSignificantBits = uuid.getMostSignificantBits();
long leastSignificantBits = uuid.getLeastSignificantBits();
- return Math.abs(mostSignificantBits ^ leastSignificantBits);
+ return (mostSignificantBits ^ leastSignificantBits) & Long.MAX_VALUE;
Review comment:
There definitely are :),
There are at least 2 (same is with the ^ Long.MaxValue)
We have 64 random bits ^ 64 Random bits and we put it into 63 bits (Remember
the first bit is sign so we only have 63 bits of values after abs) BUT because
of the overflow we get 1 extra value in our output domain, so we actually get
63 Bits (All positive longs) + 1 (Long.minValue)
With the
^ & method
We actually do the same thing
64 random bits & 64 random bits ^ 63 bits (the first bit is 0 because
Long.MaxValue is positive) so we still can only get 63 bit's worth of values in
there. (No Extra 1 for long.min value)
I think the only difference is that because of the 0 point you end up with 3
values which end up matching Long.maxValue: Long.maxValue, Long.minValue,
-Long.maxValue
Now again this basically isn't an issue any more than using a UUID is for
repeated values since the chance of collision is really small so no big worry.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
