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 3 commits 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.
Adds the edge coverage uros-b asked for on the PR: the full Double.compare ladder for both double and float (signed zeros distinguished, infinities bounding the finite range, NaN sorting above everything), plus a test that distinct NaN payloads encode identically. The NaN case pins a determinism invariant rather than an ordering one. The encoding goes through Double.doubleToLongBits, which collapses every NaN to the canonical quiet NaN, so switching it to doubleToRawLongBits would make the z-order key depend on the incoming NaN payload. These special values order correctly under both the old and the new mask, so they complete coverage rather than witness the regression; the mantissa and boundary cases remain the tests that fail without the fix.
|
Thanks — added in 1831d1c. I checked your four encodings against the corrected mask before writing
What I added:
21 tests in the class, all passing; One caveat I would rather state than let you discover: as you noted, the corrected mask already Also, while you are here — please see my earlier |
Replaces the nested-lambda encode(Function<ByteBuffer, ByteBuffer>) plumbing with named encodeDouble/encodeFloat helpers used as method references, drops a redundant array clone (allocatePrimitiveBuffer already returns a fresh buffer), and moves the private helpers below the test methods. No change to what is asserted.
|
Correcting myself on the coverage claim above. I said the special-value ladder "has zero
The smallest-magnitude negative subnormal and negative zero share all their high bits, so they land Why I got it wrong: the throwaway ladder I checked before writing the test omitted Corrected tally, verified by reverting the one-character change and re-running: 4 of 7 new tests fail without the fix (21 tests in the class, 4 failed):
3 pass either way, which I have now actually confirmed rather than assumed:
Also pushed |
|
Closing in favour of #17071 — that was the right call. @eye-gu reported the bug and had the fix up Fair point on the testing, @RussellSpitzer — noted for next time. For the record, nothing is lost by closing this: I checked that the boundary pairs in the merged |
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.