jbonofre commented on code in PR #1425:
URL: https://github.com/apache/arrow-java/pull/1425#discussion_r4432891643
##########
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:
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:
```java
TransferPair pair = vector.getTransferPair(vector.getField(),
vector.getAllocator());
try (FieldVector rebased = (FieldVector) pair.getTo()) {
pair.splitAndTransfer(0, length);
appendNodes(rebased, nodes, buffers, variadicBufferCounts);
}
```
##########
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:
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.
##########
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:
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 #967 fixed for GH-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:
```java
// 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.
--
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]
