GH-3471: Fix ByteBuffer handling in VariantUtil and VariantBuilder#3472
GH-3471: Fix ByteBuffer handling in VariantUtil and VariantBuilder#3472rayokota wants to merge 1 commit intoapache:masterfrom
ByteBuffer handling in VariantUtil and VariantBuilder#3472Conversation
|
CC: @alamb to help kick off test workflows. |
steveloughran
left a comment
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
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."
3 participants
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