Skip to content

Core: harden puffin reader to malformed input#17253

Open
steveloughran wants to merge 2 commits into
apache:mainfrom
steveloughran:pr/harden-puffin-reader
Open

Core: harden puffin reader to malformed input#17253
steveloughran wants to merge 2 commits into
apache:mainfrom
steveloughran:pr/harden-puffin-reader

Conversation

@steveloughran

Copy link
Copy Markdown
Contributor

No serious issues found; the roaring bitmap overallocation is the big one in terms of memory consumption from small files. Fix

  • limit allocation to the length of the data
  • as the data is read, reduce that maximum size that can be allocated

Fix for #17214

Contains content generated by Claude Code

Change-Id: I8b6fcdbca42e1c19c322ef97809700e765d847f5
@steveloughran
steveloughran marked this pull request as draft July 16, 2026 12:57
@github-actions github-actions Bot added the core label Jul 16, 2026
@steveloughran
steveloughran marked this pull request as ready for review July 17, 2026 11:21
Comment thread core/src/main/java/org/apache/iceberg/puffin/PuffinReader.java
*
* @param dv deletion vector.
*/
private static void validateDV(DeleteFile dv) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validateDV duplicates the first three checks of BaseDeleteLoader.validateDV; consider hoisting the shared shape validation into ContentFileUtil next to dvDesc so the query and merge paths use one copy. Follow-up, not a blocker.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure about this, as I know that making changes to other bits of code don't always go down well, but here I will. I will also improve the error message when the content length is > 2GB to include the full dvdesc() info, (filename, offset, length) rather than just the requested length. Whoever reads the error logs will appreciate this.

+ more test coverage

Change-Id: I8bf825553de3271cf9d330ffbf7fa96a129b5d4d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants