Yicong-Huang commented on PR #989:
URL: https://github.com/apache/arrow-java/pull/989#issuecomment-4044060509
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
