varun-lakhyani commented on PR #17284: URL: https://github.com/apache/iceberg/pull/17284#issuecomment-5010962858
While reviewing this PR, a concern from the discussion on a similar PR: https://github.com/apache/iceberg/pull/4518#discussion_r855667527 should also be revisited here too. But I think https://github.com/apache/iceberg/pull/16729 already does it correctly just a fresh look at this will be worth it. - **Unbounded byte[] allocation:** guarded in two places. `ManifestFiles.canEagerFetch` only wraps files with `0 < len <= EAGER_FETCH_THRESHOLD_BYTES` (currently a fixed 1 MB constant, intentionally not user-configurable), and `EagerInputFile` additionally validates that `0 <= fileSize <= Integer.MAX_VALUE` before allocating the buffer. The maximum allocation is therefore bounded at ~1 MB. - **Incorrect reported file size** (both values come from `manifest.length()`, so this is about robustness rather than an expected scenario): - **reported < actual:** eager fetching returns a truncated view. For Parquet, this fails the same way a truncated or corrupt file does today, since the footer is no longer where the reported file length says it should be. - **reported > actual:** `IOUtil.readFully` reaches EOF before filling the buffer, causing the read to fail immediately with an `EOFException`, which is surfaced as a `RuntimeIOException`. No partial data is returned. So I don't think eager fetching introduces any new failure modes here; it either behaves the same as the existing path or fails immediately when the metadata is inconsistent. Just need a confirmation on here. -- 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]
