jbonofre commented on code in PR #1263:
URL: https://github.com/apache/arrow-java/pull/1263#discussion_r3873719696
##########
c/src/main/java/org/apache/arrow/c/ArrayImporter.java:
##########
Review Comment:
...and re-add it here instead:
```suggestion
private void doImport(ArrowArray.Snapshot snapshot) {
checkState(
snapshot.offset == 0 || snapshot.length == 0,
"ArrowArray struct has non-zero offset (%s), which is not supported",
snapshot.offset);
// First import children (required for reconstituting parent array data)
```
Three things this position buys us:
1. **The array is released on rejection.** `doImport` runs inside the `try {
... } finally { underlyingAllocation.release(); }` block in `importArray`, so
the move has already happened and the producer's release callback fires. That
fixes the CI leak, and fixes it for real callers too:
`ArrowArrayStreamReader.loadNextBatch` and
`NativeScanner.NativeReader.loadNextBatch` both do `try (ArrowArray a = ...) {
Data.importIntoVectorSchemaRoot(...); }`, so as written an offset rejection
would strand the producer's entire record batch on the native side.
2. **Children and dictionaries get covered.** `importChild` calls `doImport`
as well. This is the case that matters: `ExportRecordBatch` goes through
`RecordBatch::ToStructArray()`, which gives the struct offset 0 while each
column keeps its own offset. So a sliced pyarrow/Acero `RecordBatch` passes a
top-level-only check and still imports from the wrong position: exactly the
corruption we're trying to reject. A throw from a child still propagates up to
the parent's `finally`, so the release path holds.
3. **`|| snapshot.length == 0`** keeps empty tail slices working.
`pa.array([1,2,3]).slice(3, 0)` exports `offset=3, length=0`, and
`BufferImportTypeVisitor` sizes every buffer from `fieldNode.getLength()` alone
(so those import correctly today, and rejecting them would be a regression on
valid input).
##########
c/src/main/java/org/apache/arrow/c/ArrayImporter.java:
##########
@@ -53,6 +53,10 @@ final class ArrayImporter {
void importArray(ArrowArray src) {
ArrowArray.Snapshot snapshot = src.snapshot();
checkState(snapshot.release != NULL, "Cannot import released ArrowArray");
+ checkState(
+ snapshot.offset == 0,
Review Comment:
The check throws before the array is moved, so the producer's release
callback never fires and the exported data leaks. That's what's turning the six
JNI jobs red:
```
[ERROR] RoundtripTest.tearDown:125 ยป IllegalState Memory was leaked by
query. Memory leaked: (48)
```
`ArrowArray.close()` only frees the Java-side struct (it doesn't invoke the
release callback) so the buffers `exportCDataBuffers` retained stay retained.
The check also only covers the outermost array, which misses the case that
matters most in practice. Suggest dropping it here and re-adding it in
`doImport` (see my next comment):
```suggestion
checkState(snapshot.release != NULL, "Cannot import released
ArrowArray");
```
--
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]
