Core: Fix Z-order byte encoding for floating-point values - #17071
Conversation
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
floatingPointOrderedBytes shifts a long by Integer.SIZE - 1 (31) instead of Long.SIZE - 1 (63) when building the sign mask. For a positive value the shift is meant to produce 0 so that only the sign bit is flipped, but shifting by 31 leaves exponent bits in the low 32 bits of the mask, which then corrupt the low bytes of the encoding. Ordering is still decided correctly by the high bytes, so the bug only surfaces when two values agree on their top 31 bits and the corrupted low bytes decide the comparison. There the order is inverted: every one of the 2016 pairs drawn from 1.0d + i * 2^-30 encodes backwards, as do 48 of the 496 pairs among 32 consecutive float bit patterns above 1.0f, the smallest being 1.0f and Math.nextUp(1.0f). The existing random ordering tests draw from nextFloat()/nextDouble(), whose values essentially never agree on their high bits: with the fixed seed of 42, none of the 100000 pairs in either test share bits 63..33, so neither test reaches the corrupted low bytes. The encoding is not persisted -- SparkZOrderFileRewriteRunner adds it as the temporary ICEZVALUE column, sorts on it and drops it -- so this only changes the clustering produced by future z-order rewrites. The diagnosis and the one-character fix are eye-gu's, from issue apache#17070. The boundary pairs in testDoubleOrderingForBoundaryPairs are taken from their PR apache#17071, which was closed by the stale bot without review.
|
@eye-gu this was closed by the stale bot, not on merit — your fix is correct and your test is good.
I opened #17628 before I spotted this PR — that was my mistake, and I have credited you in its Also worth flagging for whoever picks this up: #17130 is an auto-generated PR for the same issue |
|
Bots talking to each other aside, I think this is fine. Sorry I missed at let it go stale. I'd rather merge this than the new PR because the new one goes a bit overboard on testing. |
3 participants
Fixes #17070