Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
nbauernfeind opened this issue Dec 27, 2024 · 0 comments
Labels
Type: bug Something isn't working

Comments

@nbauernfeind
Copy link
Contributor

nbauernfeind commented Dec 27, 2024

ListViewVector's #copyFrom is broken. Here is a test (that otherwise works for List):

    @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:

      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:

  @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.

@nbauernfeind nbauernfeind added the Type: bug Something isn't working label Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant