apache / apache/arrow-java

`ListViewVector#copyFrom` Throws `IndexOutOfBoundsException` on Non-Empty Elements

Open
#471 0 comments 0 reactions 0 assignees View on GitHub
help wanted Type: bug
Dominant language
Java
Stars
94
Forks
152
Avg merge
3d 16h
Merged PRs (30d)
11

Description

`ListViewVector`'s `#copyFrom` is broken. Here is a test (that otherwise works for `List`):
```java
@Test
public void testListViewCopy() {
final Field childField = new Field("testChild",
new FieldType(false, new ArrowType.Int(32, true), null), null);
final Field listField = new Field("test",
new FieldType(false, ArrowType.ListView.INSTANCE, null), Collections.singletonList(childField));
try (final ListViewVector src = (ListViewVector) listField.createVector(allocator);
final ListViewVector dst = (ListViewVector) listField.createVector(allocator)) {
// init child vector
final int numValues = 10;
final IntVector childSrc = (IntVector) src.getDataVector();
childSrc.setValueCount(numValues);
for (int ii = 0; ii < numValues; ++ii) {
childSrc.set(ii, ii);
}

// init source vector
src.setValueCount(1);
src.startNewValue(0);
src.endValue(0, numValues);

assertEquals(List.of(0, 1, 2, 3, 4, 5, 6, 7, 8, 9), src.getObject(0));

dst.setValueCount(src.getValueCount());
dst.getDataVector().setValueCount(numValues);
dst.copyFrom(0, 0, src);
assertEquals(src.getObject(0), dst.getObject(0));
}
}
```

`ComplexCopier#writeValue` has impl:
```java
case LIST:
case LISTVIEW:
case LARGELIST:
case LARGELISTVIEW:
case FIXED_SIZE_LIST:
if (reader.isSet()) {
writer.startList();
while (reader.next()) {
FieldReader childReader = reader.reader();
FieldWriter childWriter = getListWriterForReader(childReader, writer);
if (childReader.isSet()) {
writeValue(childReader, childWriter);
} else {
childWriter.writeNull();
}
}
writer.endList();
} else {
writer.writeNull();
}
break;
```

Note that the implementation of `UnionListViewReader#next` will never ever return false:
```java
@Override
public boolean next() {
// Here, the currentOffSet keeps track of the current position in the vector inside the list at
// set position.
// And, size keeps track of the elements count in the list, so to make sure we traverse
// the full list, we need to check if the currentOffset is less than the currentOffset + size
if (currentOffset < currentOffset + size) {
data.getReader().setPosition(currentOffset++);
return true;
} else {
return false;
}
}
```

Notice how `currentOffset < currentOffset + size` can only ever be false if `size <= 0` -- but `size` is never modified.

I suspect the desired conditional is:
```
if (currentOffset < size) {
```

Please note that the embedded comment is also nonsense. It's not clear why the approach differs from `UnionListReader`, keeping a consistent approach would have prevented introducing a bug.

This issue exists in main as of [480e1be](https://github.com/apache/arrow-java/commit/480e1be6b7c5fa7bb0aab67c6f700059e4fcd3ea).

Contributor guide

Open the contributing guide

Research direction

Start with ListViewVector#copyFrom and the supplied test, then inspect UnionListViewReader#next alongside UnionListReader and ComplexCopier#writeValue. Verify iteration terminates for a non-empty list, run the test, and confirm the copied vector returns the same elements without an IndexOutOfBoundsException.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
data-engineering
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.