LuciferYang opened a new pull request, #7695: URL: https://github.com/apache/kyuubi/pull/7695
### Why are the changes needed? Closes #7694. `UuidUtilsTest` asserted `uuid.version()` and `uuid.variant()` and nothing else. Production writes those two from the literals `0x7000L` and `0x8000000000000000L`, so they hold whatever happens to `epochMs` and whatever happens to the 74 random bits. Setting the most significant half to `0L`, shifting the timestamp by 20 instead of 16, or deleting the `SECURE_RANDOM.nextBytes(randomBytes)` call all left the old suite fully green, and after that last one every UUID sharing a timestamp is byte-identical. Time ordering and uniqueness are what #7277 wanted from v7, and they were the two things unasserted. The test now decodes the top 48 bits and compares them with the input, brackets the no-arg overload's value between two clock readings, and asserts that the least significant half differs between two calls with the same timestamp. The two overloads move into one test each because the clock bracket has to close immediately after the no-arg call. Three coverage gaps stay open, each needing a decision rather than one more assertion. The accepting side of the 48-bit input check has no case. The rejecting side has two, negative and `1L << 48`; the largest legal value `(1L << 48) - 1` is untested, so an off-by-one on the upper bound would go unnoticed. Sign-adjacent mistakes are already covered, because the existing fixture has bit 47 set. rand_a, 12 bits wide, is still unasserted. A single pair of draws would collide once in 4096 runs, so covering it properly means drawing N times and asserting at least two distinct values, which is five lines of test for 12 bits of entropy. rand_b carries the collision risk and is now pinned. Sortability is unasserted, and `UUID.compareTo` is the wrong tool for asserting it. It compares each half as a signed long, and a v7 value whose timestamp has bit 47 set has a negative most significant half. The fixture used here is such a value: `0xFEDCBA987654L` is a millisecond count somewhere past the year 10000, and `generateUUIDv7(System.currentTimeMillis()).compareTo(generateUUIDv7(0xFEDCBA987654L))` returns 1, ordering the earlier timestamp above the later one. The string form and `Long.compareUnsigned` both give the right sign. An assertion has to pick one, which means first deciding which order this class promises. One flakiness note, not a gap: the clock bracket compares against `System.currentTimeMillis()`, which is not monotonic, so an NTP step backwards between the two readings can turn it red with production untouched. Removing that dependency means making the clock injectable, which is a production change. The failure message prints all three values so this case is distinguishable from a real unit-of-time regression. ### How was this patch tested? ``` build/mvn -o test -pl kyuubi-util -am -DwildcardSuites=none -Dtest=UuidUtilsTest build/mvn -o spotless:check -pl kyuubi-util ``` 4 of 4 pass. This is a test-only change, so a green run proves nothing by itself. Each new assertion was checked against a mutated `UuidUtils` compiled outside the repository, with the pre-patch suite as the control: - `long msb = 0L`, timestamp never written: control green, now `timestamp 0 outside [...]` and `expected: <280223976814164> but was: <0>` - `epochMs << 20` instead of `<< 16`: control green, now red on both timestamp assertions - no-arg overload switched to `System.currentTimeMillis() / 1000`: control green, now `timestamp 1788120386 outside [1788120386840, 1788120386853]`. This is the mutation that isolates the clock bracket - `SECURE_RANDOM.nextBytes(randomBytes)` deleted: control green, now `expected: not equal but was: <-9223372036854775808>` - `randLSB` forced to `0`, so rand_b is constant while rand_a still varies: control green, now red on the same assertion. This is why the assertion compares the least significant halves rather than the whole UUIDs, which the 12 bits of rand_a would satisfy on their own The explicit-timestamp fixture `0xFEDCBA987654L` has bit 47 set, so the most significant half is negative and the unsigned `>>> 16` decode is load-bearing; `>> 16` sign extends to `fffffedcba987654`. ### Was this patch assisted by generative AI tooling? Assisted-by: Claude Opus 5 -- 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]
