Repository navigation
Conversation
UInt64 widened its unsigned magnitude with `(double) (value >>> 1) * 2 + (value & 1L)`. Halving first puts the odd bit below the double grid wherever the spacing exceeds 2, so a residue just above a midpoint can only round down: 2^54 + 3 gave 1.8014398509481984E16 and 2^62 + 513 gave 4.6116860184273879E18, each one double too low. The class is one residue per two grid points in every binade from 2^53 up - 2,046 misses in the first 8,192 values of each binade, 6,157 in 5M random values - and it lands on the very accessor the class javadoc recommends "when the full unsigned magnitude must be exposed beyond the signed long range". floatValue() then narrowed that double again, which is wrong on its own account: a value just above a float midpoint can collapse onto that midpoint in the double step, where ties go to even mantissa instead of to the side the integer actually sits on. 2^53 + 2^29 + 1 came back as 9.0071993E15 rather than 9.0072003E15, and in [2^63, 2^64) the band around every float midpoint is 1,024 values wide, roughly 2^33 of the space. Both helpers now round once, straight to the target format. Non-negative longs already narrow correctly under JLS 5.1.3, and the top binade is a plain grid - 2^11 for double, 2^40 for float - whose tie is decided by the parity of the quotient. Evidence: UInt64 from the built module swept against Double/Float.parseFloat of each value's own unsigned decimal string, which the JLS requires to be correctly rounded. 524,288 consecutive values taken from every binade start: 2,046 wrong doubleValues before, 0 after. 2,389,824 values across the double and float midpoint bands: 352 wrong doubleValues and 1,050,624 wrong floatValues before, 0 after. 5,000,000 random unsigned values: 6,157 wrong doubleValues before, 0 after. The three new UnsignedTest cases keep two of those sweeps in-repo, walking the midpoint classes of every binade from 2^53 and comparing against the same decimal oracle: 640 and 108 failures before the fix, 0 after. No existing test called either accessor, and none of the parse/serialize round-trips go through them, so nothing in the suite could have seen this. Verified on JDK 17: UnsignedTest 95/95 (3 red before the fix); full fory-core suite `Tests run: 2349, Failures: 1`, the lone failure being the pre-existing ExpressionTest.testLiteralSourceRoundTrip, which also fails on main without this change; spotless:check and checkstyle:check clean, spotless run under JDK 21 as .agents/languages/java.md requires. No other Java module calls either accessor. Signed-off-by: Qingyuan Wang <162230229@nuaa.edu.cn>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why?
UInt64documentsdoubleValue()as the way to read a value that has left the signedlongrange, and that accessor was one ulp low for one residue class in every binade from 2^53 up: halving the unsigned value first put the odd low bit below the double grid, so it could only nudge the result down instead of breaking the tie.floatValue()was worse — it narrowed throughdoubleValue(), so a second rounding could land a value exactly on a float midpoint, and the tie then broke by mantissa parity instead of by which side of the midpoint the integer is on. Examples (unsigned inputs):doubleValue()2^54 + 31.8014398509481984E161.8014398509481988E162^62 + 5134.6116860184273879E184.6116860184273889E18floatValue()2^53 + 2^29 + 19.0071993E159.0072003E15In
[2^63, 2^64)the float midpoint band is 1,024–1,025 values wide per midpoint, so on the order of 2^33 values in the top binade alone are mis-reported.UInt8/UInt16/UInt32widen exactly, soUInt64is the only broken wrapper. Nothing inside the repository calls either accessor, which is why the existing suite cannot see it.What does this PR do?
Round once, straight to the target format. Non-negative payloads already narrow correctly under JLS 5.1.3, so they become a plain cast. The top unsigned binade
[2^63, 2^64)is handled on an integer grid — 2^11 for double, 2^40 for float — whose quotient fits the target mantissa exactly, so the remainder decides the rounding and the quotient's parity decides the tie; the result is the correctly-rounded value by construction, with no second rounding.floatValue()no longer delegates todoubleValue().Evidence
Differential census against a decimal-parse oracle (
Double.parseDouble/Float.parseFloatof the unsigned decimal string, which JLS requires to be correctly rounded), old formula vs new on identical populations:doubleValue()2,046 wrong, new 0doubleValue()6,157 wrong, new 0UnsignedTestgains three tests: literal spot checks (both mis-rounded classes above, plusMAX_VALUEand2^63, which stay correct), a double sweep comparing againstparseDoubleacross 64 grid points per binade 2^53…2^63 (640 disagreements before the fix), and a float sweep (108 disagreements before). All sweep loops are JDK-8 safe, so the JDK 8/11/17/21/25/26 CI matrix is safe.8e93633+ the new tests:Tests run: 95, Failures: 3→ green after the fix:Tests run: 95, Failures: 0fory-coremodule: 2349 tests / 1 failure (ExpressionTest.testLiteralSourceRoundTrip, pre-existing — reproduces on the base commit)spotless:check/checkstyle:checkpass; ASF license headers preserved; no new filesUser-facing change
UInt64.doubleValue()andfloatValue()may return a value one ulp different for inputs at or above 2^53; they now return the nearest representable float/double in every case. Nothing in the repository depends on the old results; downstream code comparing exact doubles could.AI Usage Disclosure
java/fory-core/src/main/java/org/apache/fory/type/unsigned/UInt64.java,java/fory-core/src/test/java/org/apache/fory/type/unsigned/UnsignedTest.javamvn -pl fory-core test -Dtest=UnsignedTestred (3 failures) → green (0); full module 2349 tests / 1 pre-existing failure; decimal-parse census 2,046/6,157/1,050,624 wrong → 0; spotless/checkstyle clean