Skip to content

GH-343: Fix BaseVariableWidthVector and BaseLargeVariableWidthVector offset buffer serialization#989

Open
Yicong-Huang wants to merge 9 commits intoapache:mainfrom
Yicong-Huang:fix/343-base-variable-width-vector-offset-buffer
Open

GH-343: Fix BaseVariableWidthVector and BaseLargeVariableWidthVector offset buffer serialization#989
Yicong-Huang wants to merge 9 commits intoapache:mainfrom
Yicong-Huang:fix/343-base-variable-width-vector-offset-buffer

Conversation

@Yicong-Huang
Copy link
Contributor

@Yicong-Huang Yicong-Huang commented Jan 27, 2026 •
edited
Loading

What's Changed

Fix BaseVariableWidthVector/BaseLargeVariableWidthVector IPC serialization when valueCount is 0.

Problem

When valueCount == 0, setReaderAndWriterIndex() was setting offsetBuffer.writerIndex(0), which means readableBytes() == 0. IPC serializer uses readableBytes() to determine buffer size, so 0 bytes were written to the IPC stream. This crashes IPC readers in other libraries because Arrow spec requires offset buffer to have at least one entry [0].

This is a follow-up to #967 which fixed the same issue in ListVector/LargeListVector.

Fix

Modify setReaderAndWriterIndex() to always use (valueCount + 1) * OFFSET_WIDTH for the offset buffer's writerIndex, moved outside the if/else branch. When the offset buffer capacity is insufficient (e.g., empty buffer from constructor or loaded via loadFieldBuffers()), it reallocates a properly sized buffer on demand.

Testing

Added tests for empty VarCharVector and LargeVarCharVector verifying offset buffer has correct readableBytes() after setValueCount(0).

Closes #343

@github-actions

This comment has been minimized.

Sign in to view
@Yicong-Huang
Copy link
Contributor Author

Yicong-Huang commented Jan 27, 2026

cc @viirya @jbonofre

@lidavidm lidavidm added the bug-fix PRs that fix a big. label Jan 27, 2026
jbonofre
jbonofre approved these changes Jan 27, 2026
View reviewed changes
Copy link
Member

@jbonofre jbonofre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the similar fix as for valueBuffer afair. It sounds good to me.

@jbonofre jbonofre added this to the 19.0.0 milestone Jan 27, 2026
@jbonofre
Copy link
Member

jbonofre commented Jan 27, 2026

@Yicong-Huang sorry to be a pain, can you please rebase ? Thanks !

@Yicong-Huang
Copy link
Contributor Author

Yicong-Huang commented Jan 27, 2026 •
edited
Loading

@Yicong-Huang sorry to be a pain, can you please rebase ? Thanks !

Thanks @jbonofre . I did rebase but I think the root error in this change is BaseVariableWidthVector's offsetBuffer is allocated to be 0 capacity when it is empty. Writing the offset 0 to it would be invalid. It is also not easy to increase the allocation as all tests are written against that. Could you advise on this case?

@jbonofre
Copy link
Member

jbonofre commented Mar 11, 2026

@Yicong-Huang can you please rebase again ? Sorry about that. Else I can do the rebase for you.

@Yicong-Huang
Copy link
Contributor Author

Yicong-Huang commented Mar 11, 2026

@Yicong-Huang can you please rebase again ? Sorry about that. Else I can do the rebase for you.

Thanks. Just merged the latest master back in.

@jbonofre
Copy link
Member

jbonofre commented Mar 11, 2026

@Yicong-Huang thanks a lot ! I would like to include this fix on Arrow Java 19.0.0 release 😄

@jbonofre
Copy link
Member

jbonofre commented Mar 11, 2026

Closes #343

@Yicong-Huang
Copy link
Contributor Author

Yicong-Huang commented Mar 11, 2026

@Yicong-Huang thanks a lot ! I would like to include this fix on Arrow Java 19.0.0 release 😄

Thanks that'd be nice! I can work with you closely on fixing this. However I see it is still failing CI, I think the original issue is still persist

I did rebase but I think the root error in this change is BaseVariableWidthVector's offsetBuffer is allocated to be 0 capacity when it is empty. Writing the offset 0 to it would be invalid. It is also not easy to increase the allocation as all tests are written against that.

Do you have any suggestions?

@jbonofre
Copy link
Member

jbonofre commented Mar 11, 2026

I did a new pass on the PR and I don't think it's correct.

  1. TestValueVector tests are failing. The PR changes offset buffer behavior for empty vectors, now always writes OFFSET_WIDTH bytes. In the case of empty vectors, I would expect 0 (as before).
  2. There are some memory leaks (TestDenseUnionVector.testGetFieldTypeInfo, TestStructVector.testAddChildVectorsWithDuplicatedFieldNamesForConflictPolicyReplace, TestSplitAndTransfer.testWithEmptyVector, TestUnionVector.testGetFieldTypeInfo). This is a consequence of this change as the offset buffer now allocating extra bytes that aren't being properly freed in the test cleanup.

I think the problem is that the PR changes setReaderAndWriterIndex() to always use (valueCount + 1) + OFFSET_WIDTH for the offset buffer. The text expectations need to be adjusted by +4 bytes (one 32-bit offset) for the affected cases.

jbonofre
jbonofre requested changes Mar 11, 2026
View reviewed changes
Copy link
Member

@jbonofre jbonofre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my previous comment about the tests.

Yicong Huang and others added 4 commits March 11, 2026 18:57
@claude
fix: revert constructor offset buffer allocation to avoid memory leaks
5f12320 
Keep constructor using allocator.getEmpty() instead of
allocateOffsetBuffer(OFFSET_WIDTH) to avoid memory leaks in tests
where vectors are created without being closed. The
setReaderAndWriterIndex() reallocation logic handles the offset
buffer sizing on demand.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude
fix: ensure offset buffer has at least one entry for empty variable-w…
f008eba 
…idth vectors

Per Arrow spec, offset buffers must contain at least (valueCount + 1) entries.
For empty vectors (valueCount=0), the offset buffer must still have one entry [0].

Changes:
- BaseVariableWidthVector/BaseLargeVariableWidthVector: allocate offset buffer with
  OFFSET_WIDTH in constructor instead of using empty buffer
- BaseLargeVariableWidthVector: set offset writerIndex in setReaderAndWriterIndex
- NonNullableStructVector: close replaced vectors in initializeChildrenFromFields
  to prevent memory leaks when CONFLICT_REPLACE policy removes old vectors
- Update test assertions to account for the additional 4 bytes from offset buffer
- Fix memory leaks in tests by adding proper try-with-resources

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude
fix: wrap long comment lines to pass spotless check
8a2a7d1 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude
fix: apply spotless formatting to ResultSetUtilityTest
7488bb5 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jbonofre jbonofre jbonofre requested changes

@lidavidm lidavidm Awaiting requested review from lidavidm lidavidm is a code owner

@laurentgo laurentgo Awaiting requested review from laurentgo laurentgo is a code owner

@wgtmac wgtmac Awaiting requested review from wgtmac wgtmac is a code owner

Assignees

No one assigned

Labels

bug-fix PRs that fix a big.

Projects

None yet

Milestone

19.0.0

Development

Successfully merging this pull request may close these issues.

[C++/Java] Error when reading inner lists within a struct in empty outer lists from C++/Python in Java

3 participants

@Yicong-Huang @jbonofre @lidavidm