andygrove commented on code in PR #1425:
URL: https://github.com/apache/arrow-java/pull/1425#discussion_r4436759208
##########
vector/src/main/java/org/apache/arrow/vector/VectorUnloader.java:
##########
@@ -137,8 +149,65 @@ private void appendNodes(
buf.getReferenceManager().retain();
buffers.add(codec.compress(vector.getAllocator(), buf));
}
+ long coveredChildLength = coveredChildLength(storageVector);
for (FieldVector child : vector.getChildrenFromFields()) {
- appendNodes(child, nodes, buffers, variadicBufferCounts);
+ if (coveredChildLength >= 0 && child.getValueCount() >
coveredChildLength) {
+ appendRebased(child, (int) coveredChildLength, nodes, buffers,
variadicBufferCounts);
Review Comment:
Fixed in d811f92a1 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.
##########
vector/src/main/java/org/apache/arrow/vector/VectorUnloader.java:
##########
@@ -137,8 +149,65 @@ private void appendNodes(
buf.getReferenceManager().retain();
buffers.add(codec.compress(vector.getAllocator(), buf));
}
+ long coveredChildLength = coveredChildLength(storageVector);
for (FieldVector child : vector.getChildrenFromFields()) {
- appendNodes(child, nodes, buffers, variadicBufferCounts);
+ if (coveredChildLength >= 0 && child.getValueCount() >
coveredChildLength) {
Review Comment:
Done in d811f92a1. 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.
##########
vector/src/main/java/org/apache/arrow/vector/VectorUnloader.java:
##########
@@ -137,8 +149,65 @@ private void appendNodes(
buf.getReferenceManager().retain();
buffers.add(codec.compress(vector.getAllocator(), buf));
}
+ long coveredChildLength = coveredChildLength(storageVector);
for (FieldVector child : vector.getChildrenFromFields()) {
- appendNodes(child, nodes, buffers, variadicBufferCounts);
+ if (coveredChildLength >= 0 && child.getValueCount() >
coveredChildLength) {
+ appendRebased(child, (int) coveredChildLength, nodes, buffers,
variadicBufferCounts);
+ } else {
+ appendNodes(child, nodes, buffers, variadicBufferCounts);
+ }
+ }
+ }
+
+ /** Appends the first {@code length} values of the vector, re-based to start
at offset zero. */
+ private void appendRebased(
+ FieldVector vector,
+ int length,
+ List<ArrowFieldNode> nodes,
+ List<ArrowBuf> buffers,
+ List<Long> variadicBufferCounts) {
+ TransferPair pair = vector.getTransferPair(vector.getField(),
vector.getAllocator());
+ pair.splitAndTransfer(0, length);
Review Comment:
Applied in d811f92a1: 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
