GH-43577: [Java] getBuffers method needs correction on clear flag usage

[vibhatha]

Rationale for this change

getBuffers method provides the capability to clear the buffers in the vector, this has not been properly tested while clear flag is not properly used in the implementation across various types of vectors.

What changes are included in this PR?

Updating the vector getBuffers method to use clear flag as expected and adding corresponding test cases.

Are these changes tested?

Yes, via existing test cases and new test cases.

Are there any user-facing changes?

Yes

• GitHub Issue: [Java] getBuffers method needs correction on clear flag usage #43577

[lidavidm]

I think this should be considered a breaking change

[lidavidm]

Also, are we certain the original behavior wasn't intentional? How is clear currently used?

[vibhatha]

I think this should be considered a breaking change

Yes it is. Sorry I didn't make it.

[vibhatha]

Also, are we certain the original behavior wasn't intentional? How is clear currently used?

This came up in the LargeListVIewVector review. Up to that point, I also didn't think about it that much, I was actually using the existing logic from other vectors. Clear is just used to determine whether or not to clear the vector but not the FieldVector assoicated with that vector.

[vibhatha]

If we allow this, we need to document and remind the user that when clear is true, there is a responsibility in clearing the retrieved buffers.

[lidavidm]

Also, are we certain the original behavior wasn't intentional? How is clear currently used?

This came up in the LargeListVIewVector review. Up to that point, I also didn't think about it that much, I was actually using the existing logic from other vectors. Clear is just used to determine whether or not to clear the vector but not the FieldVector assoicated with that vector.

Right, I pointed it out since it didn't make immediate sense. But I'm asking if the codebase gives a reason why clear works this way. It might not be obvious.

[vibhatha]

Let me investigate this a little more.

[vibhatha]

Looking into this, I think, clearing the buffers of the child vector might release memory that is still in use elsewhere. And this might lead to unexpected behavior or errors as we are explicitly enforcing a buffer clear on the extracted ones. Also, I think the clear was only meant to be used to clear the vector's buffer not its child buffers. But I feel like this is not precisely documented, as it only says "Return the underlying buffers associated with this vector".

[vibhatha]

Reconsidering my position, I would like to propose a documentation update and reverting the change made for LargeListViewVector in addressing reviews in the previous PR.

[lidavidm]

Sounds good to me. Thanks for looking!

[vibhatha]

@lidavidm I didn't add the LargeList and LargeListView as LargeList wasn't there in the first place.

[vibhatha]

@lidavidm I updated the PR with the doc change.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 4d200dc.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.