DanielLeens commented on PR #10958:
URL: https://github.com/apache/seatunnel/pull/10958#issuecomment-5369109533
Thanks @SEZ9 — checked these against `ArrowToSeatunnelRowReader.java` at
`35fb3f4a1b41` rather than taking them at face value.
Confirmed as real:
- Issue 1: correct. `close()` (`ArrowToSeatunnelRowReader.java:336-363`)
only catches `IOException` from `arrowStreamReader.close()`. If that call
throws an unchecked exception instead, it propagates straight out of the try
block; the `finally`'s inner catch still assigns any `rootAllocator.close()`
failure to `closeException`, but the `if (closeException != null) throw
closeException;` after the try/finally is unreachable once the original
unchecked exception is already unwinding the stack, so a genuine allocator
failure on that path is silently dropped instead of attached as suppressed.
Agree this should be broadened (e.g. `catch (Exception e)`, wrapped as needed)
so the aggregation always has a primary exception to attach to.
- Issue 6: also confirmed. The new package-private test constructor runs
`initFieldIndexMap(seaTunnelRowType)` before assigning
`this.arrowStreamReader`/`this.rootAllocator` — a schema failure there leaves
the caller-supplied resources unreachable to `close()`. Low real-world exposure
since it's package-private and only exercised by the mock tests today, but the
one-line fix (assign the resource fields first) removes the footgun for any
future caller.
- Issue 3: sound as a defensive point. I can't independently confirm from
this file alone whether this project's pulled-in Arrow version gates
`root.close()` behind `initialized` inside `ArrowReader.close(boolean)`, but
the concern holds regardless: `root` is a field this class still owns, and a
defensive `if (root != null) { root.close(); }` costs nothing given
`VectorSchemaRoot.close()` is idempotent.
I don't think any of these reopen my merge conclusion, though. They're all
secondary-failure/edge-path hardening around a `close()` that is already
correctly ordered for the #9863 symptom itself — the case with real Arrow
buffers on both the normal path and the reader-close-failure path, which the
existing `InOrder` mock tests do pin correctly (your Issue 2 is fair that they
can't reproduce the actual leak-detection exception, but they do prove the
ordering contract that fixes it). I'd fold Issues 1 and 6 into this PR since
they're one-line, low-risk, and clearly justified; 2/3/4/5/7 read as good
non-blocking follow-up material rather than blockers.
Net: still no source-level blocker for the #9863 fix itself from my side —
CI remains the only gate I'd insist on before formally approving.
--
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]
