JingsongLi commented on code in PR #9626:
URL: https://github.com/apache/paimon/pull/9626#discussion_r3975920530
##########
paimon-common/src/main/java/org/apache/paimon/sort/zorder/ZIndexer.java:
##########
@@ -241,13 +248,23 @@ public ZProcessFunction visit(DecimalType decimalType) {
InternalRow.createFieldGetter(decimalType, fieldIndex);
return (row, reuse) -> {
Object o = fieldGetter.getFieldOrNull(row);
- return o == null
- ? NULL_BYTES
- : ZOrderByteUtils.byteTruncateOrFill(
- ((Decimal) o).toUnscaledBytes(),
- PRIMITIVE_BUFFER_SIZE,
- reuse)
- .array();
+ if (o == null) {
+ return NULL_BYTES;
+ }
+ Decimal decimal = (Decimal) o;
+ // toUnscaledBytes is a variable-length two's-complement
array, which is not
+ // order-preserving under the unsigned comparison a z-value
gets. Map the
+ // unscaled value through the same sign-flipped long transform
as BIGINT,
+ // clamped into the long range for the decimals that do not
fit it.
+ long unscaled =
+ decimal.isCompact()
+ ? decimal.toUnscaledLong()
+ : decimal.toBigDecimal()
+ .unscaledValue()
+ .max(MIN_UNSCALED)
+ .min(MAX_UNSCALED)
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
