GH-343: Fix BaseVariableWidthVector and BaseLargeVariableWidthVector offset buffer serialization

[Yicong-Huang]

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

[Yicong-Huang]

cc @viirya @jbonofre

[jbonofre]

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

[jbonofre]

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

[Yicong-Huang]

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

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

[Yicong-Huang]

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

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

[jbonofre]

Closes #343

[Yicong-Huang]

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

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]

See my previous comment about the tests.

[Yicong-Huang]

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

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

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.

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.

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.

[jbonofre]

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

[jbonofre]

LGTM with the latest changes.

Thanks !