vbhanuchander-lang commented on PR #17628:
URL: https://github.com/apache/iceberg/pull/17628#issuecomment-5279308926
Thanks — added in 1831d1c. I checked your four encodings against the
corrected mask before writing
the tests and they are exactly right:
| value | `doubleToLongBits` | encoded |
|---|---|---|
| `-0.0d` | `0x8000000000000000` | `0x7fffffffffffffff` |
| `+0.0d` | `0x0000000000000000` | `0x8000000000000000` |
| `-Inf` | `0xfff0000000000000` | `0x000fffffffffffff` |
| `+Inf` | `0x7ff0000000000000` | `0xfff0000000000000` |
What I added:
- **`testDoubleOrderingAcrossSpecialValues`** — the full `Double.compare`
ladder in one sequence:
`-Inf, -MAX_VALUE, -1.0, -MIN_VALUE, -0.0, +0.0, MIN_VALUE, 1.0,
MAX_VALUE, +Inf, NaN`. This covers
both of your pairs and also pins that NaN sorts above `+Inf`, which is
where `Double.compare` puts
it (encoded `0xfff8000000000000` vs `0xfff0000000000000`).
- **`testFloatOrderingAcrossSpecialValues`** — the same ladder for `float`,
since
`floatToOrderedBytes` delegates to the same method and the widening is
worth pinning.
- **`testDoubleOrderedBytesCanonicalizeNaN`** — on your NaN suggestion. This
one 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 four distinct
raw payloads (non-zero payload, all payload bits set, sign bit set, and a
signalling NaN) must all
encode identically. If someone swapped it for `doubleToRawLongBits` the
z-order key would start
depending on the incoming NaN payload, and nothing would currently catch
that.
21 tests in the class, all passing; `checkstyleTest` clean.
One caveat I would rather state than let you discover: as you noted, the
corrected mask already
orders all of these correctly — and so does the *old* mask. I verified that
the whole ladder has
zero ordering violations under both `>> 31` and `>> 63`, so these three
tests complete coverage but
do not witness the regression. The tests that fail without the production
change remain
`testFloatOrderingForConsecutiveMantissaValues`,
`testDoubleOrderingForValuesDifferingInLowMantissaBits` and
`testDoubleOrderingForBoundaryPairs`
(3 of 7 new tests).
Also, while you are here — please see [my earlier
comment](https://github.com/apache/iceberg/pull/17628#issuecomment-5274568517)
on prior art. #17071
is @eye-gu's PR with the same fix, opened the day after they reported
#17070; it got no human review
and the stale bot closed it two days ago. Their work came first and I would
genuinely rather it were
revived and merged than this one — I have credited them in the commit
message and taken their
boundary pairs. There is also #17130, still open, which lands the production
fix with no test.
Happy to follow whichever route you prefer.
--
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]
