Skip to content

Commit

Permalink
apacheGH-43577: [Java] getBuffers method needs correction on clear fl…
Browse files Browse the repository at this point in the history
…ag usage (apache#43583)

### 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: apache#43577

Authored-by: Vibhatha Abeykoon <[email protected]>
Signed-off-by: David Li <[email protected]>
  • Loading branch information
vibhatha authored Aug 14, 2024
1 parent 69bce8f commit 4d200dc
Show file tree
Hide file tree
Showing 9 changed files with 65 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,17 @@ public VectorWithOrdinal getChildVectorWithOrdinal(String name) {
return new VectorWithOrdinal(vector, ordinal);
}

/**
* Return the underlying buffers associated with this vector. Note that this doesn't impact the
* reference counts for this buffer, so it only should be used for in-context access. Also note
* that this buffer changes regularly, thus external classes shouldn't hold a reference to it
* (unless they change it).
*
* @param clear Whether to clear vector before returning, the buffers will still be refcounted but
* the returned array will be the only reference to them. Also, this won't clear the child
* buffers.
* @return The underlying {@link ArrowBuf buffers} that is used by this vector instance.
*/
@Override
public ArrowBuf[] getBuffers(boolean clear) {
final List<ArrowBuf> buffers = new ArrayList<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,17 @@ public void reset() {
valueCount = 0;
}

/**
* Return the underlying buffers associated with this vector. Note that this doesn't impact the
* reference counts for this buffer, so it only should be used for in-context access. Also note
* that this buffer changes regularly, thus external classes shouldn't hold a reference to it
* (unless they change it).
*
* @param clear Whether to clear vector before returning, the buffers will still be refcounted but
* the returned array will be the only reference to them. Also, this won't clear the child
* buffers.
* @return The underlying {@link ArrowBuf buffers} that is used by this vector instance.
*/
@Override
public ArrowBuf[] getBuffers(boolean clear) {
final ArrowBuf[] buffers;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,17 @@ public void reset() {
valueCount = 0;
}

/**
* Return the underlying buffers associated with this vector. Note that this doesn't impact the
* reference counts for this buffer, so it only should be used for in-context access. Also note
* that this buffer changes regularly, thus external classes shouldn't hold a reference to it
* (unless they change it).
*
* @param clear Whether to clear vector before returning, the buffers will still be refcounted but
* the returned array will be the only reference to them. Also, this won't clear the child
* buffers.
* @return The underlying {@link ArrowBuf buffers} that is used by this vector instance.
*/
@Override
public ArrowBuf[] getBuffers(boolean clear) {
setReaderAndWriterIndex();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -882,12 +882,13 @@ public void reset() {

/**
* Return the underlying buffers associated with this vector. Note that this doesn't impact the
* reference counts for this buffer so it only should be used for in-context access. Also note
* that this buffer changes regularly thus external classes shouldn't hold a reference to it
* reference counts for this buffer, so it only should be used for in-context access. Also note
* that this buffer changes regularly, thus external classes shouldn't hold a reference to it
* (unless they change it).
*
* @param clear Whether to clear vector before returning; the buffers will still be refcounted but
* the returned array will be the only reference to them
* @param clear Whether to clear vector before returning, the buffers will still be refcounted but
* the returned array will be the only reference to them. Also, this won't clear the child
* buffers.
* @return The underlying {@link ArrowBuf buffers} that is used by this vector instance.
*/
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,8 @@ public void reset() {
* (unless they change it).
*
* @param clear Whether to clear vector before returning, the buffers will still be refcounted but
* the returned array will be the only reference to them
* the returned array will be the only reference to them. Also, this won't clear the child
* buffers.
* @return The underlying {@link ArrowBuf buffers} that is used by this vector instance.
*/
@Override
Expand All @@ -561,7 +562,7 @@ public ArrowBuf[] getBuffers(boolean clear) {
list.add(validityBuffer);
list.add(offsetBuffer);
list.add(sizeBuffer);
list.addAll(Arrays.asList(vector.getBuffers(clear)));
list.addAll(Arrays.asList(vector.getBuffers(false)));
buffers = list.toArray(new ArrowBuf[list.size()]);
}
if (clear) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -726,12 +726,13 @@ public void reset() {

/**
* Return the underlying buffers associated with this vector. Note that this doesn't impact the
* reference counts for this buffer so it only should be used for in-context access. Also note
* that this buffer changes regularly thus external classes shouldn't hold a reference to it
* reference counts for this buffer, so it only should be used for in-context access. Also note
* that this buffer changes regularly, thus external classes shouldn't hold a reference to it
* (unless they change it).
*
* @param clear Whether to clear vector before returning; the buffers will still be refcounted but
* the returned array will be the only reference to them
* @param clear Whether to clear vector before returning, the buffers will still be refcounted but
* the returned array will be the only reference to them. Also, this won't clear the child
* buffers.
* @return The underlying {@link ArrowBuf buffers} that is used by this vector instance.
*/
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,8 @@ public void reset() {
* (unless they change it).
*
* @param clear Whether to clear vector before returning, the buffers will still be refcounted but
* the returned array will be the only reference to them
* the returned array will be the only reference to them. Also, this won't clear the child
* buffers.
* @return The underlying {@link ArrowBuf buffers} that is used by this vector instance.
*/
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -396,12 +396,13 @@ public int getValueCapacity() {

/**
* Return the underlying buffers associated with this vector. Note that this doesn't impact the
* reference counts for this buffer so it only should be used for in-context access. Also note
* that this buffer changes regularly thus external classes shouldn't hold a reference to it
* reference counts for this buffer, so it only should be used for in-context access. Also note
* that this buffer changes regularly, thus external classes shouldn't hold a reference to it
* (unless they change it).
*
* @param clear Whether to clear vector before returning; the buffers will still be refcounted but
* the returned array will be the only reference to them
* @param clear Whether to clear vector before returning, the buffers will still be refcounted but
* the returned array will be the only reference to them. Also, this won't clear the child
* buffers.
* @return The underlying {@link ArrowBuf buffers} that is used by this vector instance.
*/
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.apache.arrow.memory.RootAllocator;
import org.apache.arrow.vector.complex.FixedSizeListVector;
import org.apache.arrow.vector.complex.ListVector;
import org.apache.arrow.vector.complex.ListViewVector;
import org.apache.arrow.vector.complex.NonNullableStructVector;
import org.apache.arrow.vector.complex.StructVector;
import org.apache.arrow.vector.complex.UnionVector;
Expand Down Expand Up @@ -122,7 +123,10 @@ public void testListTypeReset() {
"VarList", allocator, FieldType.nullable(MinorType.INT.getType()), null);
final FixedSizeListVector fixedList =
new FixedSizeListVector(
"FixedList", allocator, FieldType.nullable(new FixedSizeList(2)), null)) {
"FixedList", allocator, FieldType.nullable(new FixedSizeList(2)), null);
final ListViewVector variableViewList =
new ListViewVector(
"VarListView", allocator, FieldType.nullable(MinorType.INT.getType()), null)) {
// ListVector
variableList.allocateNewSafe();
variableList.startNewValue(0);
Expand All @@ -136,6 +140,13 @@ public void testListTypeReset() {
fixedList.setNull(0);
fixedList.setValueCount(1);
resetVectorAndVerify(fixedList, fixedList.getBuffers(false));

// ListViewVector
variableViewList.allocateNewSafe();
variableViewList.startNewValue(0);
variableViewList.endValue(0, 0);
variableViewList.setValueCount(1);
resetVectorAndVerify(variableViewList, variableViewList.getBuffers(false));
}
}

Expand Down

0 comments on commit 4d200dc

Please sign in to comment.