fix: addressing reviews

vibhatha · May 22, 2024 · 4b71621 · 4b71621
1 parent e33deb6
commit 4b71621
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 32 deletions.
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
@@ -1323,24 +1323,21 @@ protected final void handleSafe(int index, int dataLength) {
   /**
    * Copy a cell value from a particular index in source vector to a particular position in this
    * vector.
-   * TODO: Improve functionality to support copying views.
-   * <a href="https://github.com/apache/arrow/issues/40933">Enhance CopyFrom</a>
-   *
    * @param fromIndex position to copy from in source vector
    * @param thisIndex position to copy to in this vector
    * @param from source vector
    */
   @Override
   public void copyFrom(int fromIndex, int thisIndex, ValueVector from) {
-    Preconditions.checkArgument(this.getMinorType() == from.getMinorType());
+    Preconditions.checkArgument(getMinorType() == from.getMinorType());
     if (from.isNull(fromIndex)) {
-      BitVectorHelper.unsetBit(this.validityBuffer, thisIndex);
+      BitVectorHelper.unsetBit(validityBuffer, thisIndex);
     } else {
       final int viewLength = from.getDataBuffer().getInt((long) fromIndex * ELEMENT_SIZE);
-      BitVectorHelper.setBit(this.validityBuffer, thisIndex);
+      BitVectorHelper.setBit(validityBuffer, thisIndex);
       final int start = thisIndex * ELEMENT_SIZE;
       final int copyStart = fromIndex * ELEMENT_SIZE;
-      from.getDataBuffer().getBytes(start, this.viewBuffer, copyStart, ELEMENT_SIZE);
+      from.getDataBuffer().getBytes(start, viewBuffer, copyStart, ELEMENT_SIZE);
       if (viewLength > INLINE_SIZE) {
         final int bufIndex = from.getDataBuffer().getInt(((long) fromIndex * ELEMENT_SIZE) +
             LENGTH_WIDTH + PREFIX_WIDTH);
@@ -1358,25 +1355,23 @@ public void copyFrom(int fromIndex, int thisIndex, ValueVector from) {
   /**
    * Same as {@link #copyFrom(int, int, ValueVector)} except that it handles the case when the
    * capacity of the vector needs to be expanded before copy.
-   * TODO: Improve functionality to support copying views.
-   * <a href="https://github.com/apache/arrow/issues/40933">Enhance CopyFrom</a>
    * @param fromIndex position to copy from in source vector
    * @param thisIndex position to copy to in this vector
    * @param from source vector
    */
   @Override
   public void copyFromSafe(int fromIndex, int thisIndex, ValueVector from) {
-    Preconditions.checkArgument(this.getMinorType() == from.getMinorType());
+    Preconditions.checkArgument(getMinorType() == from.getMinorType());
     if (from.isNull(fromIndex)) {
       handleSafe(thisIndex, 0);
-      BitVectorHelper.unsetBit(this.validityBuffer, thisIndex);
+      BitVectorHelper.unsetBit(validityBuffer, thisIndex);
     } else {
       final int viewLength = from.getDataBuffer().getInt((long) fromIndex * ELEMENT_SIZE);
       handleSafe(thisIndex, viewLength);
-      BitVectorHelper.setBit(this.validityBuffer, thisIndex);
+      BitVectorHelper.setBit(validityBuffer, thisIndex);
       final int start = thisIndex * ELEMENT_SIZE;
       final int copyStart = fromIndex * ELEMENT_SIZE;
-      from.getDataBuffer().getBytes(start, this.viewBuffer, copyStart, ELEMENT_SIZE);
+      from.getDataBuffer().getBytes(start, viewBuffer, copyStart, ELEMENT_SIZE);
       if (viewLength > INLINE_SIZE) {
         final int bufIndex = from.getDataBuffer().getInt(((long) fromIndex * ELEMENT_SIZE) +
             LENGTH_WIDTH + PREFIX_WIDTH);

diff --git a/java/vector/src/main/java/org/apache/arrow/vector/types/Types.java b/java/vector/src/main/java/org/apache/arrow/vector/types/Types.java
@@ -568,7 +568,7 @@ public FieldWriter getNewFieldWriter(ValueVector vector) {
         return new VarBinaryWriterImpl((VarBinaryVector) vector);
       }
     },
-    VIEWVARBINARY(Binary.INSTANCE) {
+    VIEWVARBINARY(BinaryView.INSTANCE) {
       @Override
       public FieldVector getNewVector(
               Field field,

diff --git a/java/vector/src/test/java/org/apache/arrow/vector/TestVarCharViewVector.java b/java/vector/src/test/java/org/apache/arrow/vector/TestVarCharViewVector.java
@@ -29,7 +29,6 @@
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
-import java.lang.reflect.InvocationTargetException;
 import java.nio.ByteBuffer;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
@@ -51,6 +50,7 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
 
 
@@ -1455,21 +1455,21 @@ public void testSafeOverwriteLongFromALongerLongString() {
     }
   }
 
-  static Stream<Class<? extends BaseVariableWidthViewVector>> vectorProvider() {
+  static Stream<Arguments> vectorTypeAndClassProvider() {
     return Stream.of(
-        ViewVarCharVector.class,
-        ViewVarBinaryVector.class
+        Arguments.of(Types.MinorType.VIEWVARBINARY, ViewVarBinaryVector.class),
+        Arguments.of(Types.MinorType.VIEWVARCHAR, ViewVarCharVector.class)
     );
   }
 
   @ParameterizedTest
-  @MethodSource("vectorProvider")
-  public void testCopyFromWithNulls(Class<? extends BaseVariableWidthViewVector> vectorClass)
-      throws NoSuchMethodException, InvocationTargetException, InstantiationException, IllegalAccessException {
-    try (final BaseVariableWidthViewVector vector = vectorClass.getConstructor(String.class, BufferAllocator.class)
-        .newInstance(EMPTY_SCHEMA_PATH, allocator);
-        final BaseVariableWidthViewVector vector2 = vectorClass.getConstructor(String.class, BufferAllocator.class)
-            .newInstance(EMPTY_SCHEMA_PATH, allocator)) {
+  @MethodSource({"vectorTypeAndClassProvider"})
+  public void testCopyFromWithNulls(Types.MinorType type,
+      Class<? extends BaseVariableWidthViewVector> vectorClass) {
+    try (final BaseVariableWidthViewVector vector = newVector(vectorClass, EMPTY_SCHEMA_PATH,
+        type, allocator);
+        final BaseVariableWidthViewVector vector2 = newVector(vectorClass, EMPTY_SCHEMA_PATH,
+            type, allocator)) {
       final int initialCapacity = 1024;
       vector.setInitialCapacity(initialCapacity);
       vector.allocateNew();
@@ -1556,13 +1556,13 @@ public void testCopyFromWithNulls(Class<? extends BaseVariableWidthViewVector> v
   }
 
   @ParameterizedTest
-  @MethodSource("vectorProvider")
-  public void testCopyFromSafeWithNulls(Class<? extends BaseVariableWidthViewVector> vectorClass)
-      throws NoSuchMethodException, InvocationTargetException, InstantiationException, IllegalAccessException {
-    try (final BaseVariableWidthViewVector vector = vectorClass.getConstructor(String.class, BufferAllocator.class)
-        .newInstance(EMPTY_SCHEMA_PATH, allocator);
-        final BaseVariableWidthViewVector vector2 = vectorClass.getConstructor(String.class, BufferAllocator.class)
-            .newInstance(EMPTY_SCHEMA_PATH, allocator)) {
+  @MethodSource("vectorTypeAndClassProvider")
+  public void testCopyFromSafeWithNulls(Types.MinorType type,
+      Class<? extends BaseVariableWidthViewVector> vectorClass) {
+    try (final BaseVariableWidthViewVector vector = newVector(vectorClass, EMPTY_SCHEMA_PATH,
+        type, allocator);
+        final BaseVariableWidthViewVector vector2 = newVector(vectorClass, EMPTY_SCHEMA_PATH,
+            type, allocator)) {
 
       final int initialCapacity = 4096;
       vector.setInitialCapacity(initialCapacity);