Neuw84 commented on code in PR #17296:
URL: https://github.com/apache/iceberg/pull/17296#discussion_r3664004783
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorizedArrowReader.java:
##########
@@ -775,7 +776,7 @@ public VectorHolder read(VectorHolder reuse, int
numValsToRead) {
ArrowVectorAccessor<?, String, ?, ?> idsAccessor =
ids == null ? null :
ArrowVectorAccessors.getVectorAccessor(idsHolder);
- BigIntVector rowIds = allocateBigIntVector(ROW_ID_ARROW_FIELD,
numValsToRead);
+ BigIntVector rowIds = resultVector(numValsToRead);
Review Comment:
Good catch that the parameter is ignored.
I followed the base `VectorizedArrowReader.read` contract, where reuse is
only a signal. It never reads `reuse.vector()` either, it resets its own vec,
and both call sites (`ColumnarBatchReader, ArrowBatchReader`) pass back the
holder this same reader returned, so `reuse.vector()` is the vector we already
cache. Keeping the vector in the reader is also what lets `close() `release it,
which is the actual fix: previously nothing owned those vectors.
I've pushed a change that uses `reuse` as the same signal the base reader
uses (null → reallocate, otherwise reuse the cached vector if capacity
suffices), so the parameter is no longer ignored.
Happy to instead adopt `reuse.vector()` literally if you prefer, but then we
need guards for constant/dictionary holders and a decision on who closes a
vector the reader did not allocate, that ambiguity is what caused the leak.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
