[andygrove]

What's Changed

VectorUnloader wrote a variable-width vector's data buffer from byte 0 up to its last offset, and a list's child vector from element 0. A vector imported through the C Data interface from a sliced array keeps the offsets of the slice, which start past zero, and the bytes or child elements before them, so every such batch was serialized with values no row references, and the k-th slice of an array carried the k-1 slices before it. The Rust and C++ IPC writers re-base offsets in this case; Java's VectorAppender was taught to in #1180.

VectorUnloader.appendNodes now re-bases a variable-width, list or map vector whose first offset is not zero through splitAndTransfer(0, valueCount) and writes the result, closing the temporary vector once the record batch has retained its buffers. The copy is limited to the offsets, and to the validity bits when the start is not byte-aligned; the data buffers are shared. A list or map whose offsets start at zero but whose child runs past the last offset, as the first slice of a list array does, keeps its own buffers and only has its child re-based to the covered length. Vectors whose offsets already start at zero are written with their own buffers as before, which a test pins by buffer address.

Only vectors whose split keeps every value in its slot are re-based or trimmed: fixed-width, variable-width and null vectors, and list, large list, fixed-size list, map and struct vectors whose children all qualify. A dense union's split assumes each type's offsets are contiguous, which the format does not require, so a vector with a union, view, run-end encoded or extension vector at or below it is written as on main.

Three things to be aware of:

• A list whose offsets point past the end of its child, which is malformed, now fails inside splitAndTransfer at unload time instead of producing a stream that fails to load.
• ListVector.getFieldBuffers() now reallocates an offsets buffer that cannot hold [0], as BaseVariableWidthVector has since GH-343: Fix BaseVariableWidthVector and BaseLargeVariableWidthVector offset buffer serialization #989. A zero-length split leaves a list with the allocator's shared empty buffer as its offsets, which was written with no bytes after getFieldBuffers() set the writer index of the shared buffer.
• The C Data module's StructVectorUnloader has a near-copy of appendNodes that is left as it is. It only feeds Data.importIntoVectorSchemaRoot, where the batch goes straight into a VectorLoader, and re-basing there would add an offsets copy to every sliced import whether or not it is ever serialized.

Tested with a new TestVectorUnloadNonZeroStartOffset, which builds the vectors a C Data import of a slice produces from raw buffers for VarChar, LargeVarChar, List, LargeList, Map, a list whose child runs past its last offset, and a VarChar with a non-zero start nested inside a list that starts at zero. Each case asserts the node lengths and buffer sizes written, that the written offsets start at zero, and that the batch loads back to the right values. It also covers an empty covered range over a nested list, a list over a dense union whose child skips an element, and a split that fails on malformed offsets, which must not leak. TestListVector and TestMapVector check that the target of a zero-length split writes one offset.

Closes #1424.

[jbonofre]

When the covered range is empty, this hands back a list without a valid offsets buffer.

If coveredChildLength is 0 and the child is a ListVector or MapVector, this calls splitAndTransfer(0, 0). That path only runs to.clear(), so the target's offsets stay as allocator.getEmpty(), the allocator's shared zero-capacity buffer. ListVector.getFieldBuffers() then calls writerIndex(4) on that shared buffer. The inner child's buffers are the same instance, so what gets written depends on which vector set the write index last:

• List<List<Int>>: the inner offsets buffer is written with 0 bytes, which is what GH-343: Fix ListVector offset buffer not properly serialized for nested empty arrays #967 fixed for [C++/Java] Error when reading inner lists within a struct in empty outer lists from C++/Python in Java #343.
• List<List<Null>>: ArrowStreamWriter.writeBatch() throws IndexOutOfBoundsException: index: 0, length: 4 (expected: range(0, 0)).

It also leaves the shared empty buffer with writeIndex() == 4 for everything else using that allocator.

This doesn't need a C Data import. A Java-built list hits it once the row count is lowered:

// outer: List<List<Int>> with rows [], [], [[1]], [[2]]
outer.setValueCount(4);
root.setRowCount(2); // outer now covers 0 inner lists, but the inner list still holds 2
new VectorUnloader(root).getRecordBatch(); // inner offsets buffer has 0 bytes

The List<List<Null>> case is outer offsets [0, 0, 0] over a 2-value inner list. main writes both correctly.

LargeListVector's split already allocates an offsets buffer when length == 0, and BaseVariableWidthVector.setReaderAndWriterIndex() reallocates the offsets buffer when it's too small for [0].

Doing either in ListVector would fix this, and so would handling length == 0 here without splitting.

Maybe we could add a test with an empty covered range over a nested list child.

[andygrove]

Fixed in d811f92 with the BaseVariableWidthVector approach: ListVector.getFieldBuffers() now reallocates an offsets buffer that can't hold [0], as #989 did for variable-width vectors, and MapVector inherits it. Allocating in the zero-length split instead made every zero-length split allocate, which TestSplitAndTransfer.testWithEmptyVector caught as a leak in code that doesn't close the target. Handling length == 0 in the unloader alone wouldn't cover a list nested in a re-based list either, since that empty child comes out of ListVector's own split. Your List<List<Int>> case is now emptyCoveredRangeOverNestedListKeepsItsFirstOffset (it wrote 0 bytes for the inner offsets before), and TestListVector and TestMapVector check that a zero-length split's target writes [0] without touching the shared empty buffer.

[jbonofre]

LargeListVector needs the same change. Otherwise the empty covered range case still writes a 0-byte offsets buffer when the nested list is a LargeList.

LargeListVector's own zero-length split allocates [0], but that doesn't help when the LargeListVector is the child of a ListVector split to zero length. ListVector's split only runs to.clear() and never splits its child, so the child keep allocator.getEmpty(). LargeListVector.setReaderAndWriterIndex() then calls writerIndex(8) on the shared buffer:

• List<List<LargeList<Int>>> with outer offsets [0, 0, 0] over a 2 value middle list: the int child sets the shared buffer back to 0, so the LargeList offsets are written with 0 bytes.
• List<List<LargeList<Null>>> nothing resets it, so the writer tries to read 8 bytes from a zero capacity buffer.

main writes both correctly, as it doesn't split the middle list.

I believe applying the same reallocation in LargeListVector.setReadersAndWriterIndex() fixes it:

final long requiredOffsetBufferSize = (long) (valueCount + 1) * OFFSET_WIDTH;
if (offsetBuffer.capacity() < requiredOffsetBufferSize) {
  // An empty vector may still hold the allocator's shared empty buffer
  ArrowBuf newOffsetBuffer = allocator.buffer(requiredOffsetBufferSize);
  newOffsetBuffer.setZero(0, newOffsetBuffer.capacity());
  if (offsetBuffer.capacity() > 0) {
    newOffsetBuffer.setBytes(0, offsetBuffer, 0, offsetBuffer.capacity());
  }
  offsetBuffer.getReferenceManager().release();
  offsetBuffer = newOffsetBuffer;
}
offsetBuffer.writerIndex(requiredOffsetBufferSize);

Maybe we could add a LargeList variant of emptyCoveredRangeOverNestedListKeepsItsFirstOffset (the LargeList offsets should be written as 8 bytes holding 0), and a check in TestLargeListVector that an empty vector's getFieldBuffers() leaves allocator.getEmpty().writerIndex() at 0, as testSplitAndTransferZeroLength does for ListVector.

I can do that for you 😄

[jbonofre]

Trimming a dense union child silently maps values to the wrong slots.

For a DenseUnionVector child, this goes through its splitAndTransfer, which takes the first offset it sees for each type id and assumes the rest of that type's values follow it contiguously (same as in the other PR 😄 ).
Again, the spec only requires each type's offsets to increase, so gaps are allowed. Producer that filter a dense union without compacting its children create them.

Fixing the dense union split would mean copying child values, which defeats the purpose here.

I would leave the union children untrimmed, as on main. More generally, I would only trim child types whose split is known to keep values intact.

[andygrove]

Done in d811f92. Only vectors whose split keeps every value in its slot are re-based or trimmed: fixed-width, variable-width and null vectors, and list, large list, fixed-size list, map and struct vectors whose children all qualify. Anything with a union, view, run-end encoded or extension vector at or below it is written as on main, both when trimming a child and when re-basing a vector whose offsets start past zero. Two new tests cover those paths with a dense union whose child skips an element; before the change, both split the union.

[jbonofre]

This leaks the target vector when the split throws.

splitAndTransfer runs before the try-with-resources that closes pair.getTo(), so if it throws after the target has allocated or taken buffers, nothing closes the target.
The description says malformed input now takes this path. A list with offsets [2, 4, 9] over an 8-element child makes getRecordBatch() throws IllegalArgumentException. 24 bytes stay allocated after the root is closed, and closing the allocator fails with Memory was leaked by query. Memory leaked: (24).

I believe moving the split inside the try fixes it:

 TransferPair pair = vector.getTransferPair(vector.getField(), vector.getAllocator());
 try (FieldVector rebased = (FieldVector) pair.getTo()) {
   pair.splitAndTransfer(0, length);
   appendNodes(rebased, nodes, buffers, variadicBufferCounts);
 }

[andygrove]

Applied in d811f92: the split now runs inside the try-with-resources. failedSplitDoesNotLeak uses your [2, 4, 9] list, which left the same 24 bytes allocated before the change, and failedSplitReleasesTheBuffersOfEarlierVectors puts a valid column first to check that the buffers getRecordBatch() already retained are released too.

[jbonofre]

@andygrove thanks, I'm doing a new pass 😄

[jbonofre]

Thanks @andygrove, all my comments are addressed 👍

I took the liberty to push with LargeListVector fix we discussed: getFieldBuffers() now reallocates an offsets buffer that can't hold [0], as ListVector does. It also adds a LargeList variant of emptyCoveredRangeOverNestedListKeepsItsFirstOffset, and a check in TestLargeListVector that an empty vector leaves allocator.getEmpty() untouched.

I will merge as soon as CI is green.

[andygrove]

Thanks @jbonofre, I appreciate it the patient review!