Skip to content

fix(parquet): read full bloom filter buffers#892

Open
fallintoplace wants to merge 1 commit into
apache:mainfrom
fallintoplace:fix-bloom-filter-reader-full-read
Open

fix(parquet): read full bloom filter buffers#892
fallintoplace wants to merge 1 commit into
apache:mainfrom
fallintoplace:fix-bloom-filter-reader-full-read

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

What changed

The unencrypted Bloom filter reader now uses io.ReadFull when reading both the serialized header buffer and any remaining bitset bytes.

The regression test uses a reader that returns short successful chunks and covers both metadata cases: when bloom_filter_length is present and when the reader falls back to the legacy 256-byte header probe path.

Why

Read is allowed to return fewer bytes than requested without an error. Treating that as complete could deserialize a partial Bloom filter header or build a Bloom filter from a partially read bitset.

Validation

  • go test ./parquet/metadata -run 'TestBloomFilterReaderHandlesShortReads|TestBloomFilterRoundTrip'
  • PARQUET_TEST_DATA=/Users/hoangvu/Code/OSS/arrow-go/parquet-testing/data PARQUET_TEST_BAD_DATA=/Users/hoangvu/Code/OSS/arrow-go/parquet-testing/bad_data go test ./parquet/metadata
  • go test -tags assert ./parquet/metadata -run 'TestBloomFilterReaderHandlesShortReads|TestBloomFilterRoundTrip'
  • PARQUET_TEST_DATA=/Users/hoangvu/Code/OSS/arrow-go/parquet-testing/data PARQUET_TEST_BAD_DATA=/Users/hoangvu/Code/OSS/arrow-go/parquet-testing/bad_data go test ./parquet/...

@fallintoplace fallintoplace requested a review from zeroshade as a code owner July 2, 2026 22:01

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @fallintoplace — good catch on the short-read semantics; Read returning a partial buffer without an error is a real trap here.

The bitset read (2nd hunk) is an unambiguous fix — 👍. My one concern is the header probe (1st hunk): io.ReadFull over-reads the fixed 256-byte fallback and can regress reads of legacy/foreign files that omit BloomFilterLength. Details and a suggested clamp are inline. Marking request-changes for that one line — happy to approve once the probe is bounded to the section size.

}()

if _, err = sectionRdr.Read(headerBuf.Bytes()); err != nil {
if _, err = io.ReadFull(sectionRdr, headerBuf.Bytes()); err != nil {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

io.ReadFull fills the whole buffer, but in the fallback path (BloomFilterLength absent) this buffer is the fixed 256-byte probe, not the exact filter size. If fewer than 256 bytes remain between the filter offset and EOF — e.g. a legacy/foreign file with a small trailing Bloom filter — this now returns io.ErrUnexpectedEOF, whereas the old Read tolerated a short probe. Our own writer always sets BloomFilterLength (line 620), so we never hit this, but the 256 fallback exists precisely for files that don't.

Clamping the probe to the section length keeps the short-read fix while never demanding bytes past EOF (io.SectionReader.Size() == sourceFileSize - offset). ResizeNoShrink sets the length down to the clamp, so the downstream len(headerBuf.Bytes()) checks stay correct:

Suggested change
if _, err = io.ReadFull(sectionRdr, headerBuf.Bytes()); err != nil {
if sz := int(sectionRdr.Size()); sz < len(headerBuf.Bytes()) {
headerBuf.ResizeNoShrink(sz)
}
if _, err = io.ReadFull(sectionRdr, headerBuf.Bytes()); err != nil {

Reproducible in this PR's own harness: shrink the filter in makeBloomFilterReaderTestData to the minimum size and drop BloomFilterLength — the section is then ~50 bytes and io.ReadFull(256) fails with ErrUnexpectedEOF on the unpatched line.

}

if _, err = sectionRdr.Read(buf.Bytes()[filterBytesInHeader:]); err != nil {
if _, err = io.ReadFull(sectionRdr, buf.Bytes()[filterBytesInHeader:]); err != nil {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This one is spot-on and the real prize: the exact remaining bitset length is known here, so a short Read previously built a filter from a partially-read bitset with no error (silent false negatives on CheckHash). io.ReadFull is exactly right — no change needed.

filterMap := make(map[string]metadata.BloomFilterBuilder)
bldr.AppendRowGroup(rgMeta, filterMap)

bf := metadata.NewBloomFilter(1024, 1024, memory.NewGoAllocator())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice regression test. One gap: this 1024-byte filter (with sourceFileSize == buf.Len()) keeps the section ≥ 256 bytes, so the "without bloom filter length" subtest can't reach the section < 256 case that the header-read change actually affects. A second case with a minimum-size filter and no BloomFilterLength would exercise (and guard) that fallback — it fails on the current diff and passes with the clamp suggested in bloom_filter.go.

Minor, non-blocking: shortReadAtSeeker.Read (and the offset field it maintains) is never exercised — Input is parquet.ReaderAtSeeker (= io.ReaderAt + io.Seeker), and the read path goes through io.SectionReader.Read → ReadAt.

@fallintoplace fallintoplace force-pushed the fix-bloom-filter-reader-full-read branch from 75c3a68 to 5b29575 Compare July 3, 2026 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants