Fokko commented on code in PR #3559:
URL: https://github.com/apache/parquet-java/pull/3559#discussion_r3565870390
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java:
##########
@@ -2327,6 +2327,10 @@ public void readFromVectoredRange(ParquetFileRange
currRange, ChunkListBuilder b
LOG.error(error, e);
throw new IOException(error, e);
}
+ // Release the vectored-read buffer back to the allocator when the row
group is closed.
+ // Requires fs.file.checksum.verify=false so the returned buffer is the
allocator buffer
+ // rather than a sliced subset (see Hadoop's fs.file.checksum.verify
docs).
+ builder.addBuffersToRelease(Collections.singletonList(buffer));
Review Comment:
Good catch. You're right that registering the exact buffer returned by the
future is unsafe: with `fs.file.checksum.verify=true`, `ChecksumFileSystem`
returns a *sliced* view of the allocated buffer (and allocates extra checksum
buffers it never releases), so strict allocators like
`TrackingByteBufferAllocator`/`ReusingByteBufferAllocator` would fail on
release or report a leak. Disabling checksum verification globally in the test
`core-site.xml` just masked that.
I've reworked it to guard the path properly instead of relying on the config:
- Added a `RecordingByteBufferAllocator` decorator that wraps the real
allocator during `readVectored` and records the **actual** buffers Hadoop
allocates. We now release those exact buffers (data + checksum) rather than
whatever the futures hand back.
- **Removed** the global `core-site.xml` override, so the tests run with
checksum verification at Hadoop's default (enabled).
- `TestParquetFileWriter` already parameterizes over vectored/non-vectored
with the strict `TrackingByteBufferAllocator`; with checksum verification now
left on, the `[vectored : true]` case exercises exactly the sliced-buffer path.
I added a comment there documenting that intent.
Verified: `TestParquetFileWriter`, `TestParquetReader`,
`TestColumnIndexFiltering`, and `TestDataPageChecksums` all pass with checksum
verification on and strict allocators — no leaks, no double-releases.
--
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]
