GH-343: Fix BaseVariableWidthVector and BaseLargeVariableWidthVector offset buffer serialization by Yicong-Huang · Pull Request #989 · apache/arrow-java · GitHub

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

cc @viirya @jbonofre

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

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

@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?

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

@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.

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

Closes #343

@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?

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

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).
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.

See my previous comment about the tests.

Thanks a lot, @jbonofre, for the new pass! I've reworked the PR based on your feedback. Here's a summary of the changes:

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).

You were right that the test expectations for empty vectors should stay at 0. my earlier approach of allocating in the constructor was causing cascading issues. I've reverted that entirely. The constructor now continues to use allocator.getEmpty() as before.

The actual fix is now just in setReaderAndWriterIndex(): instead of setting offsetBuffer.writerIndex(0) when valueCount == 0, we always set it to (valueCount + 1) * OFFSET_WIDTH. There's a capacity check before that — if the buffer is too small (e.g. loaded from IPC with an empty offset buffer), we allocate just enough space on demand.

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.

For the memory leaks in test you flagged, I fixed one failing case, the ResultSetUtilityTest: the empty vector now allocates a 4-byte offset buffer in setReaderAndWriterIndex(), and the test wasn't closing the iterator and root properly. I can also include the same fix (clean up after test) to make the other tests more correct, if needed.

@Yicong-Huang awesome ! Thanks ! I'm doing a new pass.

LGTM with the latest changes.

Thanks !

…columns `checkBinaryOffsetsBuffer` always required at least `sizeof(offset_type)` bytes (the `+1` sentinel entry), even when `chunk.length() == 0`. Apache Arrow Java < 19.0.0, as bundled with Apache Spark, emits a 0-byte offsets buffer for empty String/Binary children (e.g. when every map row is empty so the key child has zero elements). Every other Arrow implementation (arrow-cpp, pyarrow, arrow-rs, Arrow Java >= 19) accepts this; arrow-java fixed the non-compliance in apache/arrow-java#989. When `chunk.length() == 0`, every caller's iteration loop is skipped and no byte of the offsets buffer is ever accessed, so requiring any bytes was a spec-pedantry regression, not a security necessity (confirmed by thorough code review of all 8 call sites). The `checkValidityBitmap` call inside `checkBinaryOffsetsBuffer` is unconditional and continues to validate the null bitmap. Fix: gate the `+1` sentinel on `chunk.length() > 0`; when the column is empty, set `count_plus_one = 0` so any buffer size (including 0 bytes) passes the check. Regression tests in `04356_arrow_empty_string_offsets.sh` cover: standalone empty `String` column, `Map(String, Int32)` with all-empty rows, and `Array(String)` with all-empty rows — all using the 0-byte offsets buffer that Arrow Java < 19.0.0 produces. Closes: ClickHouse#107749 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Yicong-Huang requested review from jbonofre, laurentgo, lidavidm and wgtmac as code owners January 27, 2026 00:42

This comment has been minimized.

Sign in to view

lidavidm added the bug-fix PRs that fix a big. label Jan 27, 2026

jbonofre approved these changes Jan 27, 2026

View reviewed changes

jbonofre added this to the 19.0.0 milestone Jan 27, 2026

jbonofre requested changes Mar 11, 2026

View reviewed changes

Yicong-Huang force-pushed the fix/343-base-variable-width-vector-offset-buffer branch 5 times, most recently from cac75c2 to c76dbbb Compare March 12, 2026 01:15

fix: write offset buffer for empty variable-width vectors per Arrow spec

fcb011a

Yicong-Huang force-pushed the fix/343-base-variable-width-vector-offset-buffer branch from c76dbbb to fcb011a Compare March 12, 2026 02:15

chore: retrigger CI

b0a6a12

Yicong-Huang requested a review from jbonofre March 12, 2026 05:21

jbonofre approved these changes Mar 12, 2026

View reviewed changes

jbonofre merged commit 77df3ec into apache:main Mar 12, 2026
27 checks passed

Algunenano mentioned this pull request Jun 17, 2026

Fix regression: accept 0-byte offsets buffer for empty Arrow String/Binary columns ClickHouse/ClickHouse#107764

Merged

dmitry-chirkov-dremio mentioned this pull request Jun 18, 2026

GH-343: Fix ListVector offset buffer not properly serialized for nested empty arrays #967

Merged

lriggs mentioned this pull request Jun 18, 2026

Revert "GH-343: Fix BaseVariableWidthVector and BaseLargeVariableWidt… dremio/arrow-java#28

Closed

GH-343: Fix BaseVariableWidthVector and BaseLargeVariableWidthVector offset buffer serialization by Yicong-Huang · Pull Request #989 · apache/arrow-java · GitHub

Uh oh!

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

Conversation

Yicong-Huang commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's Changed

Problem

Fix

Testing

Uh oh!

This comment has been minimized.

Yicong-Huang commented Jan 27, 2026

Uh oh!

jbonofre left a comment

Choose a reason for hiding this comment

Uh oh!

jbonofre commented Jan 27, 2026

Uh oh!

Yicong-Huang commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbonofre commented Mar 11, 2026

Uh oh!

Yicong-Huang commented Mar 11, 2026

Uh oh!

jbonofre commented Mar 11, 2026

Uh oh!

jbonofre commented Mar 11, 2026

Uh oh!

Yicong-Huang commented Mar 11, 2026

Uh oh!

jbonofre commented Mar 11, 2026

Uh oh!

jbonofre left a comment

Choose a reason for hiding this comment

Uh oh!

Yicong-Huang commented Mar 12, 2026

Uh oh!

jbonofre commented Mar 12, 2026

Uh oh!

jbonofre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Yicong-Huang commented Jan 27, 2026 •
edited
Loading

Yicong-Huang commented Jan 27, 2026 •
edited
Loading