vbhanuchander-lang opened a new pull request, #17628:
URL: https://github.com/apache/iceberg/pull/17628
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.floatingPointOrderedBytes` builds its sign mask by shifting
a `long` by
`Integer.SIZE - 1` (31) rather than `Long.SIZE - 1` (63):
```java
long lval = Double.doubleToLongBits(val);
lval ^= ((lval >> (Integer.SIZE - 1)) | Long.MIN_VALUE);
```
For a positive value the shift is supposed to yield `0`, so that `|
Long.MIN_VALUE` flips only the
sign 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`:
| | value |
|---|---|
| `doubleToLongBits(1.0)` | `0x3ff0000000000000` |
| `lval >> 31` | `0x000000007fe00000` ← leaks exponent bits into the low
word |
| mask actually used | `0x800000007fe00000` |
| `lval >> 63` | `0x0000000000000000` |
| mask intended | `0x8000000000000000` ← sign-bit flip only |
## Why 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:
- all **2016** pairs drawn from `1.0d + i * 0x1.0p-30` (i = 0..63) encode in
**reverse** order
- **48** of the 496 pairs among 32 consecutive `float` bit patterns above
`1.0f` are inverted, the
smallest being `1.0f` and `Math.nextUp(1.0f)`:
`1.0f` → `0xbff000007fe00000` vs `1.0000001f` → `0xbff000005fe00000`
`testFloatOrdering` and `testDoubleOrdering` draw from
`random.nextFloat()`/`nextDouble()`, whose
values essentially never agree on their high bits. I replayed the exact
`Random(42)` sequence those
tests 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 `main` today.
## Tests
Three deterministic cases that target the region the random tests cannot
reach — values differing
only in low mantissa bits:
| Test | Fails before fix |
|---|---|
| `testFloatOrderingForConsecutiveMantissaValues` — 64 consecutive `float`
bit patterns from `1.0f` | ✅ |
| `testDoubleOrderingForValuesDifferingInLowMantissaBits` — `1.0d + i *
0x1.0p-30` | ✅ |
| `testNegativeDoubleOrderingForValuesDifferingInLowMantissaBits` | ❌
(passes either way) |
The 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 the
change, all pass after. `spotlessApply`, `checkstyleMain` and
`checkstyleTest` are clean.
## Compatibility
None to worry about: the encoding is never persisted. Its only production
consumer is
`SparkZOrderUDF` via `SparkZOrderFileRewriteRunner`, which adds the bytes as
the temporary
`ICEZVALUE` column, sorts on it, and drops it again before writing. So this
changes only the
clustering 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`, `spark` and `flink` for the same `Integer.SIZE
- 1` shift pattern;
this is the only occurrence.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
