Repository navigation
Clean up CodeQL alerts in library and test code - #792
Merged
Merged
Conversation
Library code, no behavior change: - Util.convertToLongArray: walk each 8-byte group forward with an explicit exclusive end, so the index bounds are visible. The output is unchanged, and UtilTest.checkConvertToLongArray still passes. - ReservoirItemsUnion and ReservoirLongsUnion toByteArray(): derive "empty" from gadgetBytes itself, so the null check guards the dereference directly. Tests: - UtilTest.checkCeilingMultiple2expK used (1 << 36) - 1L, an int shift that evaluates to 15, so the test only ever checked n = 15. It now uses 1L << 36 as intended. - PositionalSegmentTest: compute byte offsets in long arithmetic. - Remove locals that were assigned but never read. Where the call is what is tested, the call stays as a statement. CpcWrapperTest now asserts the empty-sketch estimate and bounds it previously discarded, and ArrayOfDoublesCompactSketchTest asserts the type its cast checked. Left as they are: the println(String s) debug helpers whose bodies are commented out (the unused-parameter alerts), locals used by commented-out debug println lines, and the deliberate bad cast in HeapIntersectionTest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The flag was named littleEndian, but true produced Big-Endian longs:
bytes {1..8} became 0x0102030405060708. Renaming it to bigEndian makes
the name match the behavior, so the code itself is unchanged. The
method is package-private and only UtilTest calls it.
The javadoc documents the byte order, and that a final group of fewer
than 8 bytes is flush right in its long in either byte order.
UtilTest.checkConvertToLongArray keeps its expectations and adds checks
of full groups against ByteBuffer in both byte orders, using bytes with
the high bit set, plus an empty array.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The debug toString(MemorySegment, SketchType, boolean, serDe) already required a non-null serDe for an items sketch, but the check was at the top of the method, and CodeQL could not connect it to the later "else //ITEMS_SKETCH" branches. Each items branch now takes its serDe from Objects.requireNonNull(serDe, ITEMS_SERDE_MSG), so the guard is at the dereference. No behavior change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
freakyzoidberg
approved these changes
Nov 2, 2026
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.
Cleans up most of the open CodeQL alerts. The library-code changes don't change behavior; one test fix corrects a real test bug.
Library code
Util.convertToLongArrayend, so the bounds are visible. The output is identical (UtilTest.checkConvertToLongArraystill passes).ReservoirItemsUnion/ReservoirLongsUniontoByteArray()emptyfromgadgetBytes == null, so the guard covers the dereference directly.KllPreambleUtil.toString()(debug helper)Objects.requireNonNull(serDe, ...), so the guard sits at the dereference. The method already required it at the top, but CodeQL could not connect that check to the laterelse //ITEMS_SKETCHbranches.Util.convertToLongArrayalso gets its flag renamed fromlittleEndiantobigEndian.truealways produced Big-Endian longs (bytes {1..8} become0x0102030405060708), so the name now matches the behavior and the code is unchanged. It is package-private and onlyUtilTestcalls it. It also gets a javadoc: a final group of fewer than 8 bytes is flush right in its long in either byte order. The test now also checks full groups againstByteBufferin both byte orders.Tests
UtilTest.checkCeilingMultiple2expKused(1 << 36) - 1L. That's an int shift, which Java masks to1 << 4, so the test only ever checked n = 15. It now uses1L << 36.PositionalSegmentTest: compute offsets as(long) i * X.BYTES(21 integer-multiplication alerts).CpcWrapperTestnow asserts the empty-sketch estimate and bounds it used to discard, andArrayOfDoublesCompactSketchTestasserts the type its unused cast was checking.Deliberately not changed
These are better dismissed in the code-scanning UI:
println(String s) { //System.out.println(s); }debug helpers (unused-parameter alerts): the commented-out body is the intended toggle.printlns (IsomorphicTest), and the deliberate bad cast inHeapIntersectionTest.checkFamilyID.deprecated-callin tests, anduncaught-number-format-exceptioninFrequentLongsSketch.getInstance(String), which other open work already touches. ANumberFormatExceptionis anIllegalArgumentExceptionin any case.Testing
mvn verify: 2320 tests, 0 failures. The CodeQL run on this PR will show which alerts close.🤖 Generated with Claude Code