[andygrove]

What's Changed

The C Data Interface importer ignored the offset of an ArrowArray struct, and since #1263 rejected it outright. Exporting a sliced RecordBatch from C++ puts the slice offset on every child array, so such batches could not be imported into Java.

ArrayImporter now supports a non-zero offset. Java vectors cannot represent an offset, so the importer loads the rows up to the end of the slice into a temporary vector and transfers the slice into the target with splitAndTransfer. Leading rows that fill whole bytes of a bitmap are skipped by advancing the buffer pointers, so the temporary vector, and any validity bitmap synthesized for it, starts fewer than 8 rows before the slice. Children indexed by the rows of their parent (sparse union, fixed-size list) get the same skip and are trimmed to the slice. A struct is imported into the target itself instead, because transferring a struct matches its children by name, which loses children with the same name: its offset is pushed into its children, and only its own validity bitmap is sliced. Run-end encoded arrays are sliced by the split alone, so they fail with a clear error when the rows up to the end of the slice are more than a vector can hold. Fixed-width data buffers and byte-aligned validity bitmaps are shared with the C memory, while offset buffers, unaligned validity bitmaps and view data are copied.

The null count reported by a producer only covers the slice, so the temporary import passes an unknown null count (-1) when a validity bitmap is present and 0 when the producer omitted it.

The temporary vector is created from the imported field, so it has the extension types of vectors whose getField() reports only their storage type, and the top-level transfer happens between storage vectors. Importing into an existing vector fails with a clear error if an extension vector nested in a list, map or union does not report its type.

Bugs in the vectors this relies on are fixed:

• ExtensionTypeVector.makeTransferPair failed with a ClassCastException for an extension vector target, unless the subclass overrode it as UuidVector and JsonVector do and OpaqueVector does not.
• The DenseUnionVector split indexed its per-child state by type id, which failed for type ids other than 0 to n - 1. It also returned wrong values when the offsets of a child had gaps. Such values are now copied before anything is transferred, so a child that does not support copying, such as a run-end encoded vector, fails the split with an error that names it instead of leaving the target partly filled.
• DenseUnionVector.getField() listed the children in the order of their type ids rather than in their own order. Code that pairs the children of the field with those of the vector by position, such as an exported schema or the importer's temporary vector, mixed up the children of a union whose type ids are not ascending.

Tests import 20-row vectors at every offset for int, bit, decimal, decimal256, float16, timestamp with time zone, fixed-size binary, varchar, binary, large varchar, large binary, string and binary views, list, large list, list view, large list view, fixed-size list, map, struct (including a struct of a map and a list of structs), sparse and dense union (including type ids that are not ascending), run-end encoded, extension types (UUID, opaque, and one that overrides neither getField() nor makeTransferPair(), at the top level and nested) and dictionary encoded arrays, and compare the values and null counts. Others cover a list whose child has an offset of its own, sliced dictionary values, a VectorSchemaRoot with duplicate field names, a negative offset or length, and a slice whose offset is beyond the range of an int. They also cover the original report's scenario of a VectorSchemaRoot whose children carry the slice offset. Two tests check that importing 10 rows at an offset of 65,536 does not hold memory for the rows before the offset.

The documentation now describes offset support and its copying caveat instead of stating that offsets are unsupported.

Closes #88.

[jbonofre]

Producers commonly omit the validity bitmap for columns without nulls. With null_count = 0, BitVectorHelper.loadValidityBuffer then allocates and fills an all-ones bitmap for offset + length rows.
For byte-aligned offsets the target keeps a slice of that buffer, so the whole bitmap stays allocated as long as the target lives.

For example, importing 0 rows at offset 8,000,000 from a non-null int column allocates about 1 MB of bitmap and keeps it after the temporary vector is closed. That is the main use case of this PR, since a sliced RecordBatch puts the offset on every column.
When a large batch is streamed in slices, the cost of each import grows with its offset.

The synthesized bitmap should only cover the length rows that end up in the target.

[andygrove]

Fixed in e110114. The importer now advances the buffer pointers past the leading rows that fill whole bytes of a bitmap, so the temporary vector starts at most 7 rows before the slice. A bitmap synthesized for it covers at most length + 7 rows. The imported buffers shrink the same way, so the target no longer keeps a wrap of every row before the offset either.

Children whose rows line up with the parent's rows (struct, sparse union, fixed-size list) get the same skip. They are also trimmed to the slice, because a sliced struct exports them at full length. Run-end encoded arrays keep the split-only path, since their children are not indexed by row.

testImportSliceWithoutValidityBitmapAllocatesOnlyTheSlice and testImportStructSliceWithoutValidityBitmapsAllocatesOnlyTheSlice import 10 rows at offsets around 65,536 from arrays without bitmaps, and check that the importing allocator holds less than 1 KB. Before the change it held about 260 KB.

[jbonofre]

This relies on makeTransferPair(vector) + splitAndTransfer working for every vector type, and it doesn't for extension vectors. ExtensionTypeVector.makeTransferPair passes the extension target straight to the storage vector, which casts it to its own class:

java.lang.ClassCastException: class org.apache.arrow.vector.extension.OpaqueVector cannot be cast to class org.apache.arrow.vector.VarBinaryVector

UuidVector (the one in the tests) and JsonVector work only because they override makeTransferPair. OpaqueVector and any user-defined ExtensionTypeVector that doesn't override it fail, both at the top level and as a child of a sliced struct or list. A user-defined vector that keeps the default getField() fails as well: getField() then returns the storage field, so the temporary vector is created as the plain storage vector, and that vector can't transfer into the extension target either.

A sliced RecordBatch puts the offset on every column, so any batch with a custom extension column hits this.

Could we fix it in ExtensionTypeVector (unwrap an extension target to its underlying vector, and make sure the temporary vector has the extension type) and add tests with a custom extension type that overrides neither getField() nor makeTransferPair()?
The alternative is to keep rejecting offsets for these vectors with a clear error for now.

[andygrove]

Fixed in aead27c and e110114:

• ExtensionTypeVector.makeTransferPair now transfers into the underlying vector of an extension vector target, and getTo() still returns the extension vector. That covers OpaqueVector and any other vector that does not override it, at any depth.
• The importer creates the temporary vector from the imported field when it has one, which importVector, importVectorSchemaRoot and importIntoVectorSchemaRoot do. That field has the extension type even when the vector's getField() reports only its storage type, so the temporary vector has the same classes as the target.
• At the top level the importer transfers between the storage vectors, so importIntoVector into such a vector works as well.

One case is left: importIntoVector into a caller's vector that nests such an extension vector below the top level. No field reports that vector's type there, so the import now throws an UnsupportedOperationException that names the vector. Before, the struct's transfer pair silently replaced the child: the target's extension vector became a plain VarCharVector.

The tests use a PlainVector that overrides neither getField() nor makeTransferPair(). They cover it at the top level, in a struct, in a list, in a sliced record batch, and through importIntoVector. OpaqueVector is covered at the top level, in a struct and in a list. TestExtensionType tests the transfer pair itself with LocationVector.

[jbonofre]

Also, I believe dense unions have two problems in DenseUnionVector.TransferImpl.splitAndTransfer, which this PR now exposes to C Data imports:

1. typeCounts / typeStarts are sized by the number of children (nextTypeId) but indexed by the raw type code read from the type buffer. A dense union with type codes [5, 10] and offset 1 fails with ArrayIndexOutOfBoundException: Index 10 out of bounds for length 2. The same array imports fine with offset 0.
2. It assumes each child's offsets in the window are contiguous: it transfers count child values starting at the first offset it sees and renumbers them 0..count-1. The spec only requires the offsets to increase, so gaps are valid. With one child holding 100..107, row offsets [0, 2, 7] and a slice of 2 rows at offset 1, the result is 102, 103 instead of 102, 107, with no error.

The second one is wrong data with no error, where main rejects the same input today. The tests only use type codes 0 and 1 with contiguous offsets.

Could we either fix the split (map type code to child index, and rebuild the child offsets instead of assuming they are contiguous), or keep rejecting offsets for dense unions for now?

[andygrove]

Fixed both in c8083d5, in DenseUnionVector.TransferImpl:

1. The split maps each type id to the position of its child through typeMapFields. createTransferPairs() and transfer() had the same mix-up, which put the children of a new target under their positions rather than their type ids, so those are fixed as well.
2. The split checks whether the values a window selects from each child are contiguous. Contiguous values are still split without copying. Values with gaps are copied with copyValueSafe and renumbered from 0.

Copying keeps the value count of each child equal to the number of rows that select it. DenseUnionVector.setValueCount() enforces that, and a parent struct or list calls it after its own split, so keeping the gaps would have been undone there.

With your example the slice now returns 102 and 107. TestDenseUnionVector covers type ids [5, 10], gaps at the top level and inside a struct, and a transfer pair that creates its target. RoundtripTest imports both cases at every offset.

[parthchandra]

Many types now use the new offset code, but they have no test with an offset. These types are missing:

• ListView and LargeListView
• ViewVarBinary, VarBinary, LargeVarChar, and LargeVarBinary
• Decimal and Decimal256
• FixedSizeBinary, Float16, and timestamp with time zone

Can we run each of these through assertSlicedImportsMatchSource.

[andygrove]

Added in 0dcebc1, each through assertSlicedImportsMatchSource: ListView, LargeListView, ViewVarBinary, VarBinary, LargeVarChar, LargeVarBinary, Decimal, Decimal256, FixedSizeBinary, Float16, and timestamp with time zone (microseconds, Asia/Tokyo). They all passed without changes to the importer. The helper now compares binary values with assertArrayEquals, since getObject() returns a byte[]. validateFull supports neither list views nor float16, so the helper skips it for them, as it already did for maps and run-end encoded vectors.

[parthchandra]

There is no test for deeply nested types with an offset. Maybe add some tests with a struct that contains a map field and a list-of-struct field. Include both nullable and non-nullable children. Also add a list where both the list and its child array have their own offsets

[andygrove]

Added in 0dcebc1. testImportStructOfMapAndListOfStructsWithNonZeroOffset imports, at every offset, a struct with a nullable map and a non-nullable list of structs whose x is nullable and y is not. testImportListWithChildOffsetWithNonZeroOffset slices a list at offset 1 and its child at offset 2, and checks the lists against hand-computed values.

[parthchandra]

This helper does not compare the null count of the imported vector with the source. The importer ignores the producer's null count and works it out again. So the test should check that the result is correct

[andygrove]

Added in 0dcebc1: the helper now checks the imported vector's null count against the number of null rows in the slice of the source, for every test that uses it. Unions and run-end encoded vectors have no validity bitmap of their own and always report a null count of 0, so for them it expects 0.

[parthchandra]

The new error for a negative offset has no test. There is also no test for offset plus length being larger than an int can hold.

[andygrove]

Added in 0dcebc1. testImportArrayWithNegativeOffsetFails covers the negative offset, and the importer now rejects a negative length the same way (testImportArrayWithNegativeLengthFails).

Offset plus length beyond an int is fine for most types: the importer skips the whole bytes of rows before the slice by advancing the buffer pointers, so the temporary vector holds fewer than 8 rows before the slice. testImportSliceWithOffsetBeyondIntRange imports 5 rows at offset 2^31 + 3, with the buffer pointers moved back by the rows that are skipped. Run-end encoded arrays are the exception, because their run ends count from the start of the array, so their rows cannot be skipped. Such an import used to fail with an ArithmeticException from Math.addExact, and with bounds checking off checkedCastToInt could truncate the offset. It now checks the row count first and fails with a message that gives the offset, the length and the rows needed (testImportRunEndEncodedSliceBeyondIntRangeFails).

[parthchandra]

These tests only put an offset on the indices. Can we add a test where the dictionary values array also has an offset. Also add non-ASCII strings to setTestStrings and to the view test, such as "日本語", "é", and an emoji?

[andygrove]

Added in 0dcebc1. testWithDictionaryValuesWithNonZeroOffset slices the values of the dictionary at offsets 1 and 9, so that the indices refer to the values from the offset, and its values include é, 日本語 and 😀. setTestStrings and the view tests now have those characters too, in strings both within and beyond the 12 bytes that a view holds inline.

[jbonofre]

When field is null (the public Data.importIntoVector(allocator, array, vector, provider)), the temporary vector is built from vector.getField(). For a dense union, that doesn't reproduce the target. DenseUnionVector.getField() walks typeFields, which is indexed by type id, so it returns the children sorted by type id. The C array and the target keep the producer's order.

With type ids [10, 5] (children a, b), the temporary vector has children [b, a]. importUnsliced loads C child 0 (a) into the temporary type-5 vector, and createTransferPairs() then rewrites the target's typeMapFields in the temporary vector's order. If both children are IntVector, checkTransferable passes and rows read values from the wrong child, with no error. If the classes differ, it throws a misleading "its field create a ..." error.

The same array imports correctly with offset 0, and through Data.importVector, which passes the field from the C schema. So only the sliced import without a field is affected. All the dense union tests go through importVector, and testImportDenseUnionWithTypeIdsWithNonZeroOffset uses ascending ids with children of different types, so none of them catch this.

Field field = new Field("u",
    FieldType.nullable(new ArrowType.Union(UnionMode.Dense, new int[] {10, 5})),
    Arrays.asList(
        Field.nullable("a", MinorType.INT.getType()),
        Field.nullable("b", MinorType.INT.getType())));
// source rows: a=100, b=201, a=102, b=203
Data.exportVector(allocator, source, null, array);
slice(array, 1, 3);
try (FieldVector target = field.createVector(allocator)) {
  Data.importIntoVector(allocator, array, target, null);
  // expected [201, 102, 203]
}

Could we build the temporary field in the target's child order (children from getChildrenFromFields(), type ids from typeMapFields), or make DenseUnionVector.getField() keep the child order?

[andygrove]

Fixed in 90e7afe with the second option: DenseUnionVector.getField() now lists the children in their order, each with its type id, instead of sorting them by type id. Exports had the same mismatch: Data.exportVector with a schema takes the schema from getField() but the array's children from getChildrenFromFields(). createTransferPairs() now also sets the target's nextTypeId, which stayed at 0 for a target created by a transfer pair, so that target's getField() lists its children too.

testImportIntoDenseUnionWithDescendingTypeIdsWithNonZeroOffset (0dcebc1) is your example, imported with importIntoVector and no field. Before the fix its first row came back as 100. testImportDenseUnionWithDescendingTypeIdsWithNonZeroOffset imports every slice through a schema exported from getField(), and TestDenseUnionVector.testGetFieldKeepsChildOrder checks the field of the vector and of a transfer target.

[jbonofre]

Field.createVector builds a struct with the default CONFLICT_REPLACE policy, so duplicate child names collapse into one. importIntoVectorSchemaRoot uses StructVector.emptyWithDuplicates on purpose for this case (see testVectorSchemaRootWithDuplicatedFieldNames), but the sliced path loses that. With columns [a, a, b] and a non-zero offset, the temporary struct has 2 children, and importUnsliced fails with ArrowArray struct has 3 children (expected 2). The same array imports fine with offset 0.

Keeping the duplicates in the temporary vector is not enough on its own. StructTransferPair maps children by name (from.getChild(child)), so both a columns would be routed to the first one.

One option is to create the temporary struct with the target's conflict policy and transfer struct children by position. Another is to skip the temporary vector for structs: the importer already pushes the offset into struct children, so only the struct's own validity bitmap would need shifting.

[andygrove]

Fixed in 0dcebc1 with the second option: a struct with a non-zero offset is now imported into the target itself. Its offset is pushed into its children, each of which is imported into the child at the same position, and only the struct's own validity bitmap is sliced, through a temporary struct without children. A transfer pair into the target would have called the target's setValueCount(), which allocates and zeroes the buffers of every child.

testImportVectorSchemaRootWithDuplicatedFieldNamesWithNonZeroOffset imports columns [a, a, b] at offsets 3, 8 and 13, with different values in the two a columns. Before the fix it failed in checkTransferable, because the collapsed temporary struct [a, b] put a VarCharVector against the target's second a.

This also removes the case my earlier reply left unsupported: an extension vector that does not report its type, as a child of the caller's struct, is now imported like a top-level vector, so testImportIntoStructWithPlainExtensionChildWithNonZeroOffset checks the values instead of the exception. Below a list, map or union it still fails with the same error.

[jbonofre]

The fallback for children whose offsets have gaps copies values with copyValueSafe, which not every child type supports:

• RunEndEncodedVector doesn't override copyFrom, so it hits BaseValueVector.copyFrom and throws an UnsupportedOperationException with no message.
• LargeListVector.copyFrom throws "LargeListViewVector does not support copyFrom operation yet".

So splitAndTransfer fails for a dense union with a run-end encoded or large list view child whose offsets in the window are not consecutive, and so does a sliced C import of one. The exception is thrown after the contiguous children were already transferred, so the target is left partly filled.

setValueCount() recomputes the child value counts from the type buffer, so I agree the target can't keep the gaps and copying is the way to go.

Could we check up front that every child that needs copying supports it, and otherwise throw an error that names the child type?

[andygrove]

Fixed in 90e7afe. The values of children with gaps are now copied before any child is transferred. If a copy throws UnsupportedOperationException, the target is cleared and the split throws an UnsupportedOperationException that names the union, the child and its class, with the original as the cause. The copy itself is the check, rather than a list of classes: a check on the child's class would miss a struct with a run-end encoded field, whose copy fails the same way.

testSplitAndTransferWithOffsetGapsInRunEndEncodedChild and testSplitAndTransferWithOffsetGapsInLargeListViewChild check the message, and that the target has no rows and its int child no values. Before the fix the int child had already been transferred.

[parthchandra]

There is still no test (in c/src/test/python/integration_tests.py) that slices data in pyarrow and imports it into Java, and that is how the original bug happened. All the Java tests fake a slice by editing the offset on an array that Java exported.

[parthchandra]

A type id that does not belong to the union still crashes with ArrayIndexOutOfBoundsException at childCounts[-1]. This only happens when the producer sends bad data. It does not return wrong data or leak memory. It should still throw a clear error that names the bad type id, and it needs a test.