Core: Fix Z-order byte encoding for floating point values - #17628
Core: Fix Z-order byte encoding for floating point values#17628vbhanuchander-lang wants to merge 1 commit into
Conversation
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.
db962b3 to
da7ee9f
Compare
|
Correction to my own description above, and full prior art. I wrote that "six weeks had
My Gate-A check missed both: I searched for So the credit is @eye-gu's — the diagnosis and the fix are theirs, and I have said so in the commit How I would like to see this resolved, in order of preference:
Whichever a committer prefers, the substantive point is that the same one-character fix has now |
| .isNegative(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Please note an IEEE-754 edge-coverage gap here. No test pins {-0.0d, +0.0d} or {Double.NEGATIVE_INFINITY, Double.POSITIVE_INFINITY}. The corrected mask orders all of them correctly (-0.0d -> 0x7FFFFFFFFFFFFFFF, +0.0d -> 0x8000000000000000; -Inf -> 0x000FFFFFFFFFFFFF, +Inf -> 0xFFF0000000000000), but for a change whose whole purpose is to close a systematic ordering gap, pinning these edges would complete coverage. Also, it would be nice to add some NaN tests too.
2 participants
Closes #17070.
Credit to @eye-gu, who reported this and identified the one-character fix. They ticked that they
could contribute it independently, so if a PR is already in flight I am happy to close this in
favour of it — six weeks had passed with no linked PR, so I verified the report and wrote it up
with tests.
The bug
ZOrderByteUtils.floatingPointOrderedBytesbuilds its sign mask by shifting alongbyInteger.SIZE - 1(31) rather thanLong.SIZE - 1(63):For a positive value the shift is supposed to yield
0, so that| Long.MIN_VALUEflips only thesign bit. Shifting by 31 instead leaves exponent bits sitting in the low 32 bits of the mask, and
those corrupt the low bytes of the output. For
1.0d:doubleToLongBits(1.0)0x3ff0000000000000lval >> 310x000000007fe00000← leaks exponent bits into the low word0x800000007fe00000lval >> 630x00000000000000000x8000000000000000← sign-bit flip onlyWhy it has gone unnoticed
The high bytes are still encoded correctly, so ordering is only wrong when two values agree on
their top 31 bits and the corrupted low bytes are what decide the comparison. That is rare for
arbitrary values but systematic for values that differ only in low mantissa bits:
1.0d + i * 0x1.0p-30(i = 0..63) encode in reverse orderfloatbit patterns above1.0fare inverted, thesmallest being
1.0fandMath.nextUp(1.0f):1.0f→0xbff000007fe00000vs1.0000001f→0xbff000005fe00000testFloatOrderingandtestDoubleOrderingdraw fromrandom.nextFloat()/nextDouble(), whosevalues essentially never agree on their high bits. I replayed the exact
Random(42)sequence thosetests use: none of the 100 000 pairs in either test share bits 63..33, so neither test ever
reaches the corrupted low bytes. That is why 17 tests pass on
maintoday.Tests
Three deterministic cases that target the region the random tests cannot reach — values differing
only in low mantissa bits:
testFloatOrderingForConsecutiveMantissaValues— 64 consecutivefloatbit patterns from1.0ftestDoubleOrderingForValuesDifferingInLowMantissaBits—1.0d + i * 0x1.0p-30testNegativeDoubleOrderingForValuesDifferingInLowMantissaBitsThe negative case passes before and after — for negative values the leaked bits flip the whole low
region and happen to preserve order. I kept it because the guarantee is worth pinning against a
future change to the mask, and I would rather state that it is not a regression witness than imply
all three are.
./gradlew :iceberg-core:test --tests "*TestZOrderByteUtil"→ 17 tests, 2 failed before thechange, all pass after.
spotlessApply,checkstyleMainandcheckstyleTestare clean.Compatibility
None to worry about: the encoding is never persisted. Its only production consumer is
SparkZOrderUDFviaSparkZOrderFileRewriteRunner, which adds the bytes as the temporaryICEZVALUEcolumn, sorts on it, and drops it again before writing. So this changes only theclustering produced by future z-order rewrites — no existing file or manifest encodes these bytes,
and no reader path parses them.
I also grepped
core,api,sparkandflinkfor the sameInteger.SIZE - 1shift pattern;this is the only occurrence.