[common] Order decimal z-values through the signed long transform - #9626
[common] Order decimal z-values through the signed long transform#9626LuciferYang wants to merge 3 commits into
Conversation
The DECIMAL branch wrote Decimal.toUnscaledBytes(), a variable-length two's-complement array, into the fixed 8-byte z-value buffer, which is compared unsigned. On DECIMAL(20,2) that put -1.00 above 1.00, put 100.00 below 1.00 because it is longer, and made 0.00 encode to the all-zero null sentinel. Encode the unscaled value with longToOrderedBytes, like the other numeric types. Unscaled values wider than a long are clamped rather than narrowed, so the mapping stays non-decreasing; the lower bound stops one above Long.MIN_VALUE, which would encode to the sentinel.
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
The decimal ordering problem has end-to-end value, but the proposed saturation introduces a clustering regression for supported high-precision decimal columns. The existing ordering test passes with this patch yet misses collisions between ordinary separated values.
| : decimal.toBigDecimal() | ||
| .unscaledValue() | ||
| .max(MIN_UNSCALED) | ||
| .min(MAX_UNSCALED) |
There was a problem hiding this comment.
[P2] Preserve high-precision decimal clustering instead of saturating at long bounds. For DECIMAL(38,18), ordinary values 10, 20 and 90 already exceed Long.MAX_VALUE after unscaling. With the second clustering column fixed at 0, an isolated base/head probe produces three distinct z-keys on the base but exactly the same key with this change. ZorderSorter uses the key directly, so the decimal column stops contributing clustering information throughout this range. Use a sortable fixed-width decimal encoding or a monotonic projection retaining high-order range information, and test separated values beyond both long bounds, rather than only their order relative to zero/null.
There was a problem hiding this comment.
Duplicate submission caused by an ambiguous API response. Please use the original review: #9626 (review)
Duplicate of review 5163156610; the original changes-requested review and its substantive comment remain active.
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED. Implementation: FINDINGS.
Replacing saturation fixes separation across some large magnitudes, but the fixed precision-based shift still removes clustering information for ordinary wide-decimal values. The remaining P2 is inline.
An exact base/head JDK 8 probe checked actual ZIndexer keys and the core ZorderSorter. For DECIMAL(38,18), 10,001 values in [0,10] at .001 increments went from 9,962 distinct keys to one. A 4,000-row repeated 1/2/3/10 workload changed from four value groups to mixed sorter output. These are real key/sorter checks, not a measured table-query performance multiplier. The PR description also still describes the earlier saturation implementation and should be updated with the final approach.
| // a long without reaching Long.MIN_VALUE (whose z-value is the null sentinel). Compact | ||
| // decimals (p <= 18) always fit, so the shift is 0. | ||
| final int unscaledShift = | ||
| Math.max(0, BigInteger.TEN.pow(decimalType.getPrecision()).bitLength() - 63); |
There was a problem hiding this comment.
[P2] Preserve decimal clustering resolution in ordinary value ranges
For DECIMAL(38,18), this shift is 64, so every nonnegative value below 18.446744073709551616 maps to long 0: 0/1/2/3/10 all have the same Z-key when the other column is fixed. Decimal.isCompact() depends on declared precision, so none escape via the compact branch. An exact base/head probe reduced 10,001 values in [0,10] from 9,962 keys to one, and the actual core ZorderSorter changed a repeated 1/2/3/10 input from four groups into mixed output because it compares only this key. Price/rate columns using a wide decimal schema therefore lose useful clustering while still paying the rewrite cost. Please use a monotonic representation with useful relative resolution (or a wider sortable decimal encoding), and test small separated values and negative counterparts as well as values beyond long bounds. The 10/20/90 test crosses the new coarse buckets and misses this regression.
2 participants
Purpose
close #9625
ZIndexer's DECIMAL branch fedDecimal.toUnscaledBytes(), a minimal two's-complement array, into the fixed 8-byte z-value buffer throughbyteTruncateOrFill. That call left-aligns and zero-pads, and z-values are compared unsigned, so the encoding was not order-preserving. On DECIMAL(20,2),-1.00sorted above1.00, the longer100.00sorted below1.00, and0.00became eight zero bytes, which is the all-zero null sentinel.Decimals now get a fixed-width, sign-flipped big-endian two's-complement encoding of the unscaled value. The width is sized to the column's precision (8 bytes for precision up to 18, up to 16 for DECIMAL(38, x)), enough to hold any value the precision allows. That makes the key order-preserving under the unsigned comparison a z-value gets, and it keeps full resolution: small separated values, their negatives, and values past the long range all get distinct keys. A decimal column reserves that width in the z-value instead of a flat 8 bytes.
This only changes the transient sort key used while clustering, so nothing on disk changes format.
An earlier revision narrowed the unscaled value into a single long, first by clamping to the long bounds and then by a precision-based right shift. Both preserved order but lost resolution: a shift wide enough to fit the DECIMAL(38, 18) range maps every value below about 18.44 to one key, so a price or rate column on that schema stops contributing clustering information while still paying the rewrite cost. The fixed-width encoding avoids that.
Tests
TestZOrderByteUtil.testZIndexerDecimalOrderingbuilds a two-column DECIMAL(20, 2)ZIndexerand checks ordering by value, separation from the null sentinel, and a large magnitude against small positives and negatives.TestZOrderByteUtil.testZIndexerHighPrecisionDecimalClusteringwalks a strictly ascending DECIMAL(38, 18) sequence covering negatives, sub-unit fractions, small integers, and a value pastLong.MAX_VALUE, and asserts each z-key is strictly greater than the previous one, with null below all. It fails against the earlier shift revision, where the small values shared a single key.mvn -pl paimon-common -Dtest=TestZOrderByteUtil teston JDK 11: 16 tests, 0 failures.