Repository navigation
Conversation
The C Data Interface importer ignored, and later rejected, the offset of an ArrowArray struct, so a sliced record batch exported from C++ could not be imported. Java vectors cannot represent an offset, so the importer now loads the leading offset + length values into a temporary vector and transfers the requested window into the target with splitAndTransfer. Buffers that can be sliced are shared; offset buffers and unaligned validity bitmaps are copied. Closes apache#88
This comment has been minimized.
This comment has been minimized.
| // The producer's null count only covers the slice. Without a validity bitmap the null count | ||
| // must be 0 for the buffers to load, otherwise it is unknown. | ||
| boolean hasBitmap = snapshot.n_buffers > 0 && MemoryUtil.getLong(snapshot.buffers) != NULL; | ||
| snapshot.null_count = hasBitmap ? -1 : 0; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| unslicedImporter.recursionLevel = recursionLevel; | ||
| unslicedImporter.underlyingAllocation = underlyingAllocation; | ||
| unslicedImporter.importUnsliced(snapshot); | ||
| unslicedVector.makeTransferPair(vector).splitAndTransfer(offset, length); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ExtensionTypeVector.makeTransferPairnow transfers into the underlying vector of an extension vector target, andgetTo()still returns the extension vector. That coversOpaqueVectorand 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,importVectorSchemaRootandimportIntoVectorSchemaRootdo. That field has the extension type even when the vector'sgetField()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
importIntoVectorinto 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.
| unslicedImporter.recursionLevel = recursionLevel; | ||
| unslicedImporter.underlyingAllocation = underlyingAllocation; | ||
| unslicedImporter.importUnsliced(snapshot); | ||
| unslicedVector.makeTransferPair(vector).splitAndTransfer(offset, length); |
There was a problem hiding this comment.
Also, I believe dense unions have two problems in DenseUnionVector.TransferImpl.splitAndTransfer, which this PR now exposes to C Data imports:
typeCounts/typeStartsare 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 withArrayIndexOutOfBoundException: Index 10 out of bounds for length 2. The same array imports fine with offset 0.- It assumes each child's offsets in the window are contiguous: it transfers
countchild values starting at the first offset it sees and renumbers them0..count-1. The spec only requires the offsets to increase, so gaps are valid. With one child holding100..107, row offsets[0, 2, 7]and a slice of 2 rows at offset 1, the result is102, 103instead of102, 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?
There was a problem hiding this comment.
Fixed both in c8083d5, in DenseUnionVector.TransferImpl:
- The split maps each type id to the position of its child through
typeMapFields.createTransferPairs()andtransfer()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. - 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
copyValueSafeand 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.
Co-authored-by: JB Onofré <jbonofre@apache.org>
ExtensionTypeVector.makeTransferPair passed an extension vector target to the transfer pair of its underlying vector, which casts the target to its own class. Transfers between extension vectors that do not override makeTransferPair, such as OpaqueVector, failed with a ClassCastException. Transfer into the underlying vector of an extension vector target instead.
DenseUnionVector's transfer pair indexed its per-child state by type id, although it is sized and ordered by child. Splitting a dense union whose type ids are not 0 to n - 1 failed with an ArrayIndexOutOfBoundsException, and transfers placed the target's children under the wrong type ids. The split also assumed that the values a window selects from a child are contiguous. The offsets of a child only have to increase, so for offsets with gaps the split returned the wrong values. Values with gaps are now copied, while contiguous values are still split.
A sliced import loaded every row before the slice into a temporary vector, so the validity bitmap synthesized for an array without one, and the imported buffers, grew with the offset and stayed alive with the target. Skip the leading rows that fill whole bytes of a bitmap by advancing the buffer pointers, and import only the rows of the slice from the children of structs, sparse unions and fixed-size lists. Create the temporary vector from the imported field, which has the extension types of vectors whose getField() reports their storage type, and transfer between storage vectors at the top level. Importing into an existing vector whose nested extension vectors do not report their type fails with a clear error instead of replacing them.
What's Changed
The C Data Interface importer ignored the
offsetof anArrowArraystruct, and since #1263 rejected it outright. Exporting a slicedRecordBatchfrom C++ puts the slice offset on every child array, so such batches could not be imported into Java.ArrayImporternow 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 withsplitAndTransfer. 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 (struct, sparse union, fixed-size list) get the same skip and are trimmed to the slice. Run-end encoded arrays are sliced by the split alone. 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 whose nested extension vectors do not report their type fails with a clear error.Two bugs in the transfers this relies on are fixed:
ExtensionTypeVector.makeTransferPairfailed with aClassCastExceptionfor an extension vector target, unless the subclass overrode it asUuidVectorandJsonVectordo andOpaqueVectordoes not.DenseUnionVectorsplit 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.Tests import 20-row vectors at every offset for int, bit, varchar, string view, list, large list, fixed-size list, map, struct, sparse and dense union, run-end encoded, extension types (UUID, opaque, and one that overrides neither
getField()normakeTransferPair(), at the top level and nested) and dictionary encoded arrays. They also cover the original report's scenario of aVectorSchemaRootwhose 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.