GH-38254: [Java] Add reusable buffer getters to char/binary vectors #38266

jduo · 2023-10-14T00:52:26Z

Rationale for this change

Provide a way for a user to reuse a buffer when iterating over byte-array-based ValueVectors to avoid excessive
reallocations.

What changes are included in this PR?

Add a reusable buffer interface that can be populated by character and binary vectors to avoid allocations when consuming vector content.

Optimize getObject() on VarCharVector/LargeVarCharVector to avoid an extra allocation of a byte array (copy from ArrowBuf directly to the resulting Text).

Are these changes tested?

Are there any user-facing changes?

Yes.

This PR includes breaking changes to public APIs.

Closes: [Java] Allow passing pre-allocated buffers to VarCharVector #38254

java/memory/memory-core/src/main/java/org/apache/arrow/memory/ReusableBuffer.java

java/vector/src/main/java/org/apache/arrow/vector/FixedSizeBinaryVector.java

adamkennedy · 2023-10-16T19:12:51Z

java/vector/src/main/java/org/apache/arrow/vector/LargeVarBinaryVector.java

+   * @param index position of element.
+   * @param outputBuffer the buffer to write into.
+   */
+  public void readBytes(int index, ReusableBuffer<?> outputBuffer) {


Would we allow any reusable buffer here, when the reusable buffers may be specialized to byte vs string type

The idea is that users could implement ReusableBuffer on top of other byte-container types such as ByteBuffer or Netty ByteBuf objects.

adamkennedy · 2023-10-16T19:14:40Z

java/vector/src/main/java/org/apache/arrow/vector/LargeVarBinaryVector.java

+   */
+  public void readBytes(int index, ReusableBuffer<?> outputBuffer) {
+    final long startOffset = getStartOffset(index);
+    final int dataLength =


Use getEndOffset instead? Do we need to worry about lengths exceeding max integer?

Oddly, getEndOffset() is part of BaseVariableWidthVector and not part of BaseLargeVariableWidthVector, and BLVW doesn't extend VLW.

The existing getters on this class do the narrowing cast from long to int as well, so if this is a problem the scope is larger than this.

I suppose they don't inherit because of the different integer types.

The long vs int problem is baked heavily into the library (and I guess ByteBuffer), so I don't think it's a concern here, but new interfaces should probably use long to be forward-looking (e.g., MemorySegment uses long now)

And I think that it's not possible for the current implementation to exceed int. However a Java 21+ memory module should actually be able to.

@danepitkin we should probably file some follow-up work for after your PR lands to shake out all the problems that are about to crop up.

adamkennedy · 2023-10-16T19:17:19Z

java/vector/src/main/java/org/apache/arrow/vector/LargeVarCharVector.java

+   */
+  public void readBytes(int index, ReusableBuffer<?> outputBuffer) {
+    final long startOffset = getStartOffset(index);
+    final int dataLength =


Similar to above comment, I thought main purpose of the Large variants was to allow for values larger than max integer.

java/vector/src/main/java/org/apache/arrow/vector/util/ReusableArrowBuf.java

adamkennedy · 2023-10-16T19:22:52Z

There's no unit tests written for these new classes, and we probably should have some.

jduo · 2023-10-16T23:10:08Z

There's no unit tests written for these new classes, and we probably should have some.

Added new tests.

lidavidm · 2023-10-20T13:50:35Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/ReusableBuffer.java

+   *
+   * @return the number of valid bytes in the data
+   */
+  int getLength();


Should we use long for future compatibility?

java/memory/memory-core/src/main/java/org/apache/arrow/memory/ReusableBuffer.java

lidavidm · 2023-10-20T13:59:07Z

java/vector/src/main/java/org/apache/arrow/vector/LargeVarBinaryVector.java

+   */
+  public void readBytes(int index, ReusableBuffer<?> outputBuffer) {
+    final long startOffset = getStartOffset(index);
+    final int dataLength =


I suppose they don't inherit because of the different integer types.

The long vs int problem is baked heavily into the library (and I guess ByteBuffer), so I don't think it's a concern here, but new interfaces should probably use long to be forward-looking (e.g., MemorySegment uses long now)

lidavidm · 2023-10-20T14:00:16Z

java/vector/src/main/java/org/apache/arrow/vector/LargeVarBinaryVector.java

+   */
+  public void readBytes(int index, ReusableBuffer<?> outputBuffer) {
+    final long startOffset = getStartOffset(index);
+    final int dataLength =


And I think that it's not possible for the current implementation to exceed int. However a Java 21+ memory module should actually be able to.

@danepitkin we should probably file some follow-up work for after your PR lands to shake out all the problems that are about to crop up.

java/vector/src/main/java/org/apache/arrow/vector/util/ReusableByteArray.java

lidavidm · 2023-10-20T18:48:22Z

I'm going to mark this a "breaking change" since Text#getLength now returns long

adamkennedy · 2023-10-20T21:09:46Z

java/vector/src/main/java/org/apache/arrow/vector/util/ReusableByteArray.java

+   * @param len      the number of bytes we need
+   * @param keepData should the old data be kept
+   */
+  protected void setCapacity(int len, boolean keepData) {


Under what situation would we need to keep the existing data?

This method is ported from Text. It gets set to true when used by the append() method there.

java/vector/src/main/java/org/apache/arrow/vector/FixedSizeBinaryVector.java

adamkennedy · 2023-10-20T21:12:05Z

java/vector/src/main/java/org/apache/arrow/vector/LargeVarBinaryVector.java

+  public void read(int index, ReusableBuffer<?> outputBuffer) {
+    final long startOffset = getStartOffset(index);
+    final long dataLength =
+        offsetBuffer.getLong((long) (index + 1) * OFFSET_WIDTH) - startOffset;


Use getEndOffset instead here rather than reimplement?

getEndOffset() is not implemented for LargeVectors. I can use it for VarVector and write an implementation for Large*Vectors.

lidavidm · 2023-10-23T13:45:38Z

Can you rebase to pick up the compilation fix from this weekend?

lidavidm · 2023-10-23T14:52:22Z

java/vector/src/main/java/org/apache/arrow/vector/util/ReusableByteArray.java

+  }
+
+  /**
+   * Copied from Arrays.hashCode so we don't have to copy the byte array.


nit: remove this comment? Unless you really did copy it from the JDK (in which case don't)

…tors Add a reusable buffer interface that can be populated by character and binary vectors to avoid allocations when consuming vector content. Optimize getObject() on VarCharVector/LargeVarCharVector to avoid an extra allocation of a byte array (copy from ArrowBuf directly to the resulting Text).

- Add getEndOffset() to BaseLargeVariableWidthVector. - Change BaseVariableWidthVector, BaseLargeVariableWidthVector, VarBinaryVector, VarCharVector, LargeVarBinaryVector, and LargeVarCharVector to use getStartOffset() and getEndOffset() when possible. - Add tests for ReusableByteArray equals(), hashCode(), and toString() functions. - Rename the outputBuffer parameter in read() functions to buffer.

conbench-apache-arrow · 2023-10-24T02:26:46Z

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

…tors (apache#38266) ### Rationale for this change Provide a way for a user to reuse a buffer when iterating over byte-array-based ValueVectors to avoid excessive reallocations. ### What changes are included in this PR? Add a reusable buffer interface that can be populated by character and binary vectors to avoid allocations when consuming vector content. Optimize getObject() on VarCharVector/LargeVarCharVector to avoid an extra allocation of a byte array (copy from ArrowBuf directly to the resulting Text). ### Are these changes tested? ### Are there any user-facing changes? Yes. **This PR includes breaking changes to public APIs.** * Closes: apache#38254 Authored-by: James Duong <[email protected]> Signed-off-by: David Li <[email protected]>

github-actions bot added Component: Java awaiting review Awaiting review labels Oct 14, 2023

jduo force-pushed the 38254-reusable-buffer-varcharvector branch from fcdf8cf to 0561808 Compare October 16, 2023 17:27

jduo marked this pull request as ready for review October 16, 2023 17:28

jduo requested a review from lidavidm as a code owner October 16, 2023 17:28

adamkennedy reviewed Oct 16, 2023

View reviewed changes

java/memory/memory-core/src/main/java/org/apache/arrow/memory/ReusableBuffer.java Outdated Show resolved Hide resolved

adamkennedy reviewed Oct 16, 2023

View reviewed changes

java/vector/src/main/java/org/apache/arrow/vector/FixedSizeBinaryVector.java Outdated Show resolved Hide resolved

adamkennedy reviewed Oct 16, 2023

View reviewed changes

java/vector/src/main/java/org/apache/arrow/vector/util/ReusableArrowBuf.java Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 16, 2023

jduo force-pushed the 38254-reusable-buffer-varcharvector branch from 0561808 to 519258f Compare October 16, 2023 23:09

lidavidm reviewed Oct 20, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Oct 20, 2023

jduo force-pushed the 38254-reusable-buffer-varcharvector branch from 519258f to cc90fb1 Compare October 20, 2023 18:28

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 20, 2023

lidavidm approved these changes Oct 20, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Oct 20, 2023

github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Oct 20, 2023

jduo force-pushed the 38254-reusable-buffer-varcharvector branch from cc90fb1 to 6540883 Compare October 20, 2023 20:02

github-actions bot removed the awaiting changes Awaiting changes label Oct 20, 2023

github-actions bot added awaiting committer review Awaiting committer review awaiting changes Awaiting changes and removed awaiting review Awaiting review awaiting changes Awaiting changes awaiting committer review Awaiting committer review labels Oct 20, 2023

adamkennedy reviewed Oct 20, 2023

View reviewed changes

java/vector/src/main/java/org/apache/arrow/vector/FixedSizeBinaryVector.java Outdated Show resolved Hide resolved

adamkennedy reviewed Oct 20, 2023

View reviewed changes

jduo force-pushed the 38254-reusable-buffer-varcharvector branch from d1471b4 to 63e8cf4 Compare October 20, 2023 23:03

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 20, 2023

lidavidm reviewed Oct 23, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Oct 23, 2023

jduo added 2 commits October 23, 2023 08:21

jduo force-pushed the 38254-reusable-buffer-varcharvector branch from 63e8cf4 to ae9d6a2 Compare October 23, 2023 15:23

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 23, 2023

lidavidm approved these changes Oct 23, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Oct 23, 2023

lidavidm merged commit 6f8f34b into apache:main Oct 23, 2023
15 checks passed

lidavidm added Breaking Change Includes a breaking change to the API and removed awaiting merge Awaiting merge labels Oct 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-38254: [Java] Add reusable buffer getters to char/binary vectors #38266

GH-38254: [Java] Add reusable buffer getters to char/binary vectors #38266

jduo commented Oct 14, 2023 •

edited by lidavidm

Loading

adamkennedy Oct 16, 2023

jduo Oct 16, 2023

adamkennedy Oct 16, 2023

jduo Oct 16, 2023

lidavidm Oct 20, 2023

lidavidm Oct 20, 2023

adamkennedy Oct 16, 2023

adamkennedy commented Oct 16, 2023

jduo commented Oct 16, 2023

lidavidm Oct 20, 2023

lidavidm Oct 20, 2023

lidavidm Oct 20, 2023

lidavidm commented Oct 20, 2023

adamkennedy Oct 20, 2023

jduo Oct 20, 2023

adamkennedy Oct 20, 2023

jduo Oct 20, 2023

lidavidm commented Oct 23, 2023

lidavidm Oct 23, 2023

conbench-apache-arrow bot commented Oct 24, 2023

GH-38254: [Java] Add reusable buffer getters to char/binary vectors #38266

GH-38254: [Java] Add reusable buffer getters to char/binary vectors #38266

Conversation

jduo commented Oct 14, 2023 • edited by lidavidm Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamkennedy commented Oct 16, 2023

jduo commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidavidm commented Oct 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidavidm commented Oct 23, 2023

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Oct 24, 2023

jduo commented Oct 14, 2023 •

edited by lidavidm

Loading