GH-44088: [Java] Fix copyFrom in BaseVariableWidthViewVector #44078

ViggoC · 2024-09-12T10:53:15Z

Fix bugs in copyFromSafe and handleSafe

GitHub Issue: [Java] Fix BaseVariableWidthViewVector #44088

github-actions · 2024-09-12T10:53:39Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

github-actions · 2024-09-12T14:52:21Z

⚠️ GitHub issue #44088 has been automatically assigned in GitHub to PR creator.

lidavidm · 2024-09-14T02:11:41Z

CC @vibhatha

vibhatha · 2024-09-14T02:51:02Z

On it @lidavidm

ViggoC · 2024-09-14T09:52:21Z

java/vector/src/test/java/org/apache/arrow/vector/TestVarCharViewVector.java

@@ -261,6 +261,15 @@ public void testDataBufferBasedAllocationInOtherBuffer() {
    }
  }

+  @Test
+  public void testSetSafe() {


@vibhatha I wrote a test case for setSafe.

Can we add a test case with long string as well? and a mix of short and long?

Of course, I can do that. But let's fix this one first. What do you think about the modification I mentioned in comment of reallocViewBuffer.

Right, I am still checking it. Please give an hour to verify this. I am thinking what I have already missed and what this is doing.

I think the issue here is that the getValueCapacity depends on the capacity of the viewBuffer and validityBuffer, so in the while loop, when just one is changed, and the other value is kept the same, the minimum will always be the unchanged value. So I think the main issue is the error in that function. Thank you for this test case, I don't think the existing test cases covered this case.

fix in 15f78da

ViggoC · 2024-09-14T10:24:47Z

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

@@ -1478,7 +1479,7 @@ protected final void handleSafe(int index, int dataLength) {
      reallocViewBuffer(Math.max(writePosition, targetCapacity));
    }

-    while (index >= getValueCapacity()) {
+    while (index >= getValidityBufferValueCapacity()) {


@vibhatha I try to fix testSetSafe, but the logic here makes me confused. The writePosition can only be larger than targetCapacity when lastSet < 0. But I don't think there is a need for special treatment in this situation. Is there a problem if I change it to

final long targetCapacity = roundUpToMultipleOf16((long) index * ELEMENT_SIZE + dataLength); if (viewBuffer.capacity() < targetCapacity) { reallocViewBuffer(targetCapacity); }

@ViggoC I have only skimmed through the PR. Please give me some time. I will verify.

I think changing to this won't hurt much. But the actual issue is just changing the validity buffer capacity, it must be changed along with the viewBuffer capacity as the getCapacity is a function of both changes. So just changing one is the issue in the original code. What do you think? Am I missing your point?

java/vector/src/test/java/org/apache/arrow/vector/TestVarCharViewVector.java

vibhatha · 2024-09-18T02:20:15Z

java/vector/src/test/java/org/apache/arrow/vector/TestVarCharViewVector.java

+        }
+      }
+
+      // test target vector with different initialCapacity


this also failed prior to this change?

Yes, prior to this change, the dataBufferIndex and offset is just copied to viewBuffer by

from.getDataBuffer().getBytes(start, viewBuffer, copyStart, ELEMENT_SIZE);

But they might be difference from fromVector's.

vibhatha · 2024-09-18T03:46:43Z

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

+      viewBuffer.setBytes(start, from.getDataBuffer(), copyStart, LENGTH_WIDTH + PREFIX_WIDTH);
+      int writePosition = start + LENGTH_WIDTH + PREFIX_WIDTH;
+      // set buf id
+      viewBuffer.setInt(writePosition, dataBuffers.size() - 1);
+      writePosition += BUF_INDEX_WIDTH;
+      // set offset
+      viewBuffer.setInt(writePosition, (int) thisDataBuf.writerIndex());


why do we have to do this, why not copy the 16 bytes from the from.getDataBuffer() from the copyStart till it reads 16 bytes. And I think the existing logic covers that.

the buf id and offset will be different if the lengths of dataBuffers is different from the from vector.

Oh! I have missed that. Good catch. Can we add a test case for that as well. I mean create vector 1 some content and then vector 2 with a different content. Copy content from vector 1 to vector 2, and validate this?

vibhatha · 2024-09-18T05:48:01Z

@ViggoC what do you think about the following diff on top of your changes

diff --git a/java/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java b/java/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java
index 327548e0bc..0434024dec 100644
--- a/java/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java
+++ b/java/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java
@@ -1252,6 +1252,7 @@ public abstract class BaseVariableWidthViewVector extends BaseValueVector
     // We need to check and reallocate the validity buffer
     while (index >= getValueCapacity()) {
       reallocValidityBuffer();
+      reallocViewBuffer();
     }
     BitVectorHelper.unsetBit(validityBuffer, index);
   }
@@ -1461,22 +1462,9 @@ public abstract class BaseVariableWidthViewVector extends BaseValueVector
   }
 
   protected final void handleSafe(int index, int dataLength) {
-    final long lastSetCapacity = lastSet < 0 ? 0 : (long) index * ELEMENT_SIZE;
-    final long targetCapacity = roundUpToMultipleOf16(lastSetCapacity + dataLength);
-    // for views, we need each buffer with 16 byte alignment, so we need to check the last written
-    // index
-    // in the viewBuffer and allocate a new buffer which has 16 byte alignment for adding new
-    // values.
-    long writePosition = (long) index * ELEMENT_SIZE;
-    if (viewBuffer.capacity() <= writePosition || viewBuffer.capacity() < targetCapacity) {
-      /*
-       * Everytime we want to increase the capacity of the viewBuffer, we need to make sure that the new capacity
-       * meets 16 byte alignment.
-       * If the targetCapacity is larger than the writePosition, we may not necessarily
-       * want to allocate the targetCapacity to viewBuffer since when it is >={@link #INLINE_SIZE} either way
-       * we are writing to the dataBuffer.
-       */
-      reallocViewBuffer(Math.max(writePosition, targetCapacity));
+    final long targetCapacity = roundUpToMultipleOf16((long) index * ELEMENT_SIZE + dataLength);
+    if (viewBuffer.capacity() < targetCapacity) {
+      reallocViewBuffer(targetCapacity);
     }
 
     while (index >= getValidityBufferValueCapacity()) {

vibhatha · 2024-09-18T05:48:41Z

@danepitkin could you please help us to run the workflows?

ViggoC · 2024-09-18T08:33:25Z

@vibhatha It LGTM, I'll apply this patch and add more test cases you commented.

vibhatha

Thanks @ViggoC for working on this. PR LGTM.

vibhatha · 2024-09-19T04:57:05Z

@lidavidm could you please take a look as well?

danepitkin

Nice work!

danepitkin · 2024-09-20T17:58:34Z

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

@@ -1251,6 +1252,7 @@ public void setNull(int index) {
    // We need to check and reallocate the validity buffer
    while (index >= getValueCapacity()) {
      reallocValidityBuffer();
+      reallocViewBuffer();


Out of curiosity, why do we need to call reallocViewBuffer here? Could it be possible to have null values not backed by a view buffer and only realloc when we add non-null values?

fix org.apache.arrow.vector.BaseVariableWidthViewVector.copyFromSafe

c6eba05

github-actions bot added Component: Java awaiting review Awaiting review labels Sep 12, 2024

ViggoC added 2 commits September 12, 2024 18:58

update lastValueCapacity in reallocViewBuffer

f63dbcd

test different initialCapacity

e10f5f7

ViggoC mentioned this pull request Sep 12, 2024

[Java] Fix BaseVariableWidthViewVector #44088

Open

extract copyFromNotNull

98e183d

ViggoC changed the title ~~[Java] Fix BaseVariableWidthViewVector~~ GH-44088: [Java] Fix copyFrom in BaseVariableWidthViewVector Sep 12, 2024

ViggoC marked this pull request as ready for review September 12, 2024 14:52

ViggoC requested a review from lidavidm as a code owner September 12, 2024 14:52

fix

45f89bf

ViggoC added 2 commits September 14, 2024 14:47

setBytes is more intuitive

a9c9aac

testSetSafe

a1791c5

ViggoC commented Sep 14, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 14, 2024

vibhatha reviewed Sep 18, 2024

View reviewed changes

java/vector/src/test/java/org/apache/arrow/vector/TestVarCharViewVector.java Show resolved Hide resolved

vibhatha reviewed Sep 18, 2024

View reviewed changes

ViggoC added 2 commits September 18, 2024 16:37

fix testSetSafe

15f78da

test overwrite by copy

ae4cdd6

vibhatha approved these changes Sep 19, 2024

View reviewed changes

danepitkin reviewed Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-44088: [Java] Fix copyFrom in BaseVariableWidthViewVector #44078

GH-44088: [Java] Fix copyFrom in BaseVariableWidthViewVector #44078

ViggoC commented Sep 12, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Sep 12, 2024

github-actions bot commented Sep 12, 2024

lidavidm commented Sep 14, 2024

vibhatha commented Sep 14, 2024

ViggoC Sep 14, 2024

vibhatha Sep 18, 2024

ViggoC Sep 18, 2024

vibhatha Sep 18, 2024

vibhatha Sep 18, 2024

ViggoC Sep 18, 2024

ViggoC Sep 14, 2024

vibhatha Sep 14, 2024

vibhatha Sep 18, 2024

vibhatha Sep 18, 2024

ViggoC Sep 18, 2024

vibhatha Sep 18, 2024

vibhatha Sep 18, 2024

ViggoC Sep 18, 2024

vibhatha Sep 18, 2024

vibhatha commented Sep 18, 2024

vibhatha commented Sep 18, 2024

ViggoC commented Sep 18, 2024

vibhatha left a comment

vibhatha commented Sep 19, 2024

danepitkin left a comment

danepitkin Sep 20, 2024

GH-44088: [Java] Fix copyFrom in BaseVariableWidthViewVector #44078

Are you sure you want to change the base?

GH-44088: [Java] Fix copyFrom in BaseVariableWidthViewVector #44078

Conversation

ViggoC commented Sep 12, 2024 • edited by github-actions bot Loading

github-actions bot commented Sep 12, 2024

github-actions bot commented Sep 12, 2024

lidavidm commented Sep 14, 2024

vibhatha commented Sep 14, 2024

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

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

Choose a reason for hiding this comment

vibhatha commented Sep 18, 2024

vibhatha commented Sep 18, 2024

ViggoC commented Sep 18, 2024

vibhatha left a comment

Choose a reason for hiding this comment

vibhatha commented Sep 19, 2024

danepitkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViggoC commented Sep 12, 2024 •

edited by github-actions bot

Loading