Uh oh!
There was an error while loading. Please reload this page.
GH-3471: Fix ByteBuffer handling in VariantUtil and VariantBuilder - #3472
Conversation
nssalian
commented
Apr 15, 2026
CC: @alamb to help kick off test workflows. |
steveloughran
left a comment
There was a problem hiding this comment.
this is good. does relate to the work in #3481 which needs to pick up the same fix.
getting your patch in first adds the regression tests we need
| int remainingBefore = buf.remaining(); | ||
| VariantBuilder vb = new VariantBuilder(); | ||
| vb.appendBinary(buf); | ||
| Assert.assertEquals(positionBefore, buf.position()); |
There was a problem hiding this comment.
nit add a "position" and "remaining" description for better diagnostics on CI runs
nssalian
left a comment
There was a problem hiding this comment.
tests pass and #3481 can follow this PR like @steveloughran mentioned.
Left a single comment.
| // getMetadataMap builds a key->id dictionary from the metadata buffer. | ||
| // With a non-zero position and read-only buffer, the else-branch is taken, | ||
| // which previously used the wrong offset. | ||
| ImmutableMetadata immutableMetadata = new ImmutableMetadata(offsetMetadata); |
There was a problem hiding this comment.
nit: The test comment reads more like a bug description than a test description. Consider reframing to describe what's being verified, e.g. "Verify metadata dictionary parsing works correctly when the ByteBuffer has a non-zero position and is read-only."
Fokko
left a comment
There was a problem hiding this comment.
Let's get this in to unblock #3481
Thanks @rayokota for adding this and thanks @nssalian and @steveloughran for the review 👍
Uh oh!
There was an error while loading. Please reload this page.
Rationale for this change
Fix
ByteBufferhandling inVariantUtilandVariantBuilder.What changes are included in this PR?
VariantUtil.getMetadataMapreading string bytes from the wrong positionwhen the metadata
ByteBufferhas a non-zeroposition(). Changedslice(metadata, stringStart + offset)toslice(metadata, pos + stringStart + offset) to match the array-backed branch.VariantBuilder.appendBinarymutating the caller'sByteBufferby usingbinary.duplicate(), consistent withappendEncodedValue.Are these changes tested?
TestVariantObject.testMetadataWithNonZeroPositionReadOnly: constructs ametadata
ByteBufferwith a non-zero position and read-only flag, then verifiesImmutableMetadata(which callsgetMetadataMap) correctly parses thedictionary. Fails without the fix.
TestVariantScalarBuilder.testBinaryBuilderDoesNotMutateCallerBuffer:verifies that appendBinary does not change the caller's
ByteBufferpositionor remaining count.
Are there any user-facing changes?
Closes#3471