[common] Run the z-order sign flip at the width of the bit pattern - #9628
Open
LuciferYang wants to merge 1 commit into
Open
[common] Run the z-order sign flip at the width of the bit pattern#9628LuciferYang wants to merge 1 commit into
LuciferYang wants to merge 1 commit into
Conversation
doubleToOrderedBytes built its sign-magnitude mask with (lval >> (Integer.SIZE - 1)), a 31-bit shift of a 64-bit value, so for a negative double only part of the magnitude was inverted and adjacent negatives compared out of order. floatToOrderedBytes widens to double and ran the same line. Shift by Long.SIZE - 1 in both. SortUtil already does this correctly for its own float and double normalized keys.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
close #9627
ZOrderByteUtils.doubleToOrderedBytesbuilt its sign-magnitude mask from a 31-bit shift of a 64-bit value:The shift is meant to be a sign extension, all ones for a negative pattern and all zeros for a positive one. At 31 it instead copies bits 31 through 61 into the low half of the mask, so any value bit lining up with a set bit there gets inverted. Negatives lose most of their ordering:
-2.5encodes to0x3ffbffff80080000while the strictly smaller-2.5000000000000004encodes to0x3ffbffff80080001. Positives are affected wherever two values differ at one of those positions:2.5encodes to0xc004000080080000and the larger2.5000000002328306to0xc004000080000000.floatToOrderedByteswidens to double and ran the same line, so both types were wrong.Both now shift by
Long.SIZE - 1. The same transform is written correctly inSortUtil, whereputFloatNormalizedKeyshifts anintbyInteger.SIZE - 1andputDoubleNormalizedKeyshifts alongbyLong.SIZE - 1; the z-order copy had kept the int-sized constant on a long.The float path keeps widening to double instead of encoding the 32-bit pattern into half the buffer. Widening is exact and order-preserving, and it keeps all eight bytes carrying information, which matters because
interleaveBitsconsumes bytes most-significant first and Spark truncates the interleaved output to a maximum size.Nothing on disk changes, since a z-value only exists while sorting.
Tests
TestZOrderByteUtil.testFloatDoubleNegativeOrderingwalks an ascending array from-MAX_VALUEtoMAX_VALUEfor both types and asserts the encodings ascend with it. It then walks a thousand adjacent bit patterns descending from-3.0fand from just below-2.5d, asserting each encoding is strictly lower than the one before, which is where the old mask fails: the arrays on their own are too sparse to catch it.The existing
testFloatOrderingandtestDoubleOrderingonly usedrandom.nextFloat()andrandom.nextDouble(), both in [0, 1), which is why this survived.mvn -pl paimon-common -Dtest=TestZOrderByteUtil teston JDK 8: 15 tests, 0 failures.spotless:checkandcheckstyle:checkon paimon-common are clean.