vbhanuchander-lang commented on PR #17628:
URL: https://github.com/apache/iceberg/pull/17628#issuecomment-5279380826
**Correcting myself on the coverage claim above.** I said the special-value
ladder "has zero
ordering violations under both `>> 31` and `>> 63`", so those tests complete
coverage without
witnessing the regression. That is wrong for the double ladder, and your
instinct was better than my
analysis.
`testDoubleOrderingAcrossSpecialValues` **does** fail without the production
fix, on one adjacent
pair:
```
-Double.MIN_VALUE < -0.0d
buggy (>> 31): 0x7fffffff00000001 >= 0x7fffffff00000000 inverted
fixed (>> 63): 0x8000000000000000 < 0x7fffffffffffffff … ordered
```
The smallest-magnitude negative subnormal and negative zero share all their
high bits, so they land
squarely in the corrupted low region — the same failure mode as my mantissa
sweeps, just reached from
the zero boundary instead.
Why I got it wrong: the throwaway ladder I checked before writing the test
omitted
`±Double.MIN_VALUE`, while the test I actually committed includes them. So I
validated a weaker input
set than the one in the code and then reported the weaker result. My
mistake, and worth flagging
because it changes the value of what you asked for: the signed-zero edge is
not just completeness, it
catches a real inversion my sweeps missed.
Corrected tally, verified by reverting the one-character change and
re-running:
**4 of 7 new tests fail without the fix** (21 tests in the class, 4 failed):
- `testFloatOrderingForConsecutiveMantissaValues`
- `testDoubleOrderingForValuesDifferingInLowMantissaBits`
- `testDoubleOrderingForBoundaryPairs`
- `testDoubleOrderingAcrossSpecialValues` ← the one I misreported
**3 pass either way**, which I have now actually confirmed rather than
assumed:
- `testFloatOrderingAcrossSpecialValues` — the float ladder has no violating
adjacent pair under
either mask, because widening a float to double leaves the low 29 bits
zero, so
`-Float.MIN_VALUE` and `-0.0f` do not collide in the corrupted region the
way the double
subnormal does
- `testNegativeDoubleOrderingForValuesDifferingInLowMantissaBits`
- `testDoubleOrderedBytesCanonicalizesNaN` — a determinism invariant, not an
ordering one
Also pushed `ef70925`, a readability pass on the tests only, no change to
what is asserted: named
`encodeDouble`/`encodeFloat` helpers used as method references instead of
the nested
`encode(Function<ByteBuffer, ByteBuffer>)` plumbing, a redundant array clone
removed since
`allocatePrimitiveBuffer()` already returns a fresh buffer, and the private
helpers moved below the
tests. 21 tests pass; `spotlessCheck`, `checkstyleMain` and `checkstyleTest`
are clean.
--
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]
