feat(parquet): Consolidate bloom filter reads with a push decoder by etseidl · Pull Request #11975 · apache/arrow-rs

etseidl · 2026-10-28T19:04:56Z

Which issue does this PR close?

Closes Consolidate Bloom filter parsing #11974
Follow up to feat(parquet): Add async Sbbf::read_from_column_chunk_async Bloom filter API #11953

Rationale for this change

Reduce code duplication and centralize Bloom filter parsing and validation using a push decoder.

What changes are included in this PR?

Add SbbfPushDecoder to decode Bloom filters from caller-provided byte ranges without I/O.
Share decoding between synchronous and asynchronous reads, including Arrow builder APIs.
Apply the same header and bitset validation to Sbbf::from_bytes.
Consolidate parsing tests around the push decoder, retaining sync/async I/O tests..

Are these changes tested?

Yes, new tests are added

Are there any significant user-facing changes?

Adds the public SbbfPushDecoder API. Existing read APIs are unchanged, but synchronous reads and Sbbf::from_bytes now reject invalid bitset sizes and inconsistent declared lengths, matching the asynchronous behavior introduced in #11953.

AI disclosure

Implementation and regression tests were prepared with coding-agent assistance, but were reviewed by me.

etseidl · 2026-10-28T19:08:02Z

Note this is stacked on #11953.

etseidl · 2026-11-02T15:33:16Z

@alamb I think this is ready...added the push API you requested.

cc @adriangb who also has Sbbf changes in flight.

alamb · 2026-11-02T21:59:32Z

👀

adriangb

This looks really nice @etseidl !

adriangb · 2026-11-02T22:04:01Z

+#[derive(Debug)]
+enum DecodeState {
+    /// Finding the first Bloom filter range
+    FindingRange,
+    // Reading the SBBF header
+    ReadingSbbfHeader(Range<u64>),
+    // Reading the SBBF bitset bytes
+    ReadingSbbfBitset(Range<u64>),
+    // Decoding is complete
+    Finished,
+    /// State left during the `try_decode` method so something valid is present.
+    /// This state should never be observed.
+    Intermediate,
+}


adriangb · 2026-11-02T22:08:04Z

+    /// Attempts to decode the filter, returning the file ranges needed if data
+    /// is missing. Repeated calls before supplying data return the same ranges.
+    ///
+    /// Without prefetched data, requests one range when `bloom_filter_length` is
+    /// known, or two otherwise (a 20-byte header estimate, then the bitset).
+    /// Headers larger than the estimate require an advertised length.
+    /// After returning [`DecodeResult::Data`], subsequent calls return
+    /// [`DecodeResult::Finished`].
+    pub fn try_decode(&mut self) -> Result<DecodeResult<Sbbf>> {


Would it be helpful to have an agent generate a state transition diagram so readers can easily visualize the code?

I'll see what it comes up with, although I think @alamb enjoys making ascii art 😂

Ok, it's there, @adriangb. That's the best we could come up with 😅

I do enjoy making ascii art. Though recently I had a 🤯 experience of having my agent reverse engineer the monodraw file format so I can have it draft ascii art diagrams and then tweak them visually ❤️

adriangb · 2026-11-02T22:11:39Z

+    /// Headers larger than the estimate require an advertised length.
+    /// After returning [`DecodeResult::Data`], subsequent calls return
+    /// [`DecodeResult::Finished`].


Do we know under what circumstances this happens? Does this error if no length is provided or does it request more data?

This is a guard for any changes that may be made to the thrift header, either via a new version of the spec, or proprietary changes that would otherwise be ignored by a streaming thrift decoder. One would hope that the bloom filter length field would be populated in such a scenario, which is what the comment is driving at. Were an expanded header not be accompanied by a valid length, then read_bloom_filter_header_and_length would hit an EoF and return an error, and the state machine would be left in an unusable state.

Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>

Replace hand-written `match ... other => panic!` blocks and `assert!(matches!(...))` checks with the `expect_data`, `expect_needs_data` and `expect_finished` helpers used by the other push decoder tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ter_reads

alamb

Thanks @etseidl and @adriangb -- this looks great to me

I did take the liberty of tweaking some tests to use the same pattern as the other push decoders in 4d4935e, just for consistency (e.g.

arrow-rs/parquet/src/file/metadata/push_decoder.rs

Lines 1079 to 1094 in b231cf1

    
               fn expect_data<T: Debug>(result: Result<DecodeResult<T>>) -> T { 
        
                   match result.expect("Expected Ok(DecodeResult::Data(T))") { 
        
                       DecodeResult::Data(data) => data, 
        
                       result => panic!("Expected DecodeResult::Data, got {result:?}"), 
        
                   } 
        
               } 
        
               /// Expect that the [`DecodeResult`] is a [`DecodeResult::NeedsData`] and return the corresponding ranges 
        
               fn expect_needs_data<T: Debug>(result: Result<DecodeResult<T>>) -> Vec<Range<u64>> { 
        
                   match result.expect("Expected Ok(DecodeResult::NeedsData(ranges))") { 
        
                       DecodeResult::NeedsData(ranges) => ranges, 
        
                       result => panic!("Expected DecodeResult::NeedsData, got {result:?}"), 
        
                   } 
        
               } 
        
               fn expect_finished<T: Debug>(result: Result<DecodeResult<T>>) {

)

alamb · 2026-11-03T16:31:22Z

    );
 }

-#[derive(Default)]


alamb · 2026-11-03T16:34:04Z

-                    "Bloom filter length does not match its header"
-                ));
+        loop {
+            let result = decoder.try_decode()?;


this is so nice

alamb · 2026-11-03T16:36:46Z

+    /// Attempts to decode the filter, returning the file ranges needed if data
+    /// is missing. Repeated calls before supplying data return the same ranges.
+    ///
+    /// Without prefetched data, requests one range when `bloom_filter_length` is
+    /// known, or two otherwise (a 20-byte header estimate, then the bitset).
+    /// Headers larger than the estimate require an advertised length.
+    /// After returning [`DecodeResult::Data`], subsequent calls return
+    /// [`DecodeResult::Finished`].
+    pub fn try_decode(&mut self) -> Result<DecodeResult<Sbbf>> {


I do enjoy making ascii art. Though recently I had a 🤯 experience of having my agent reverse engineer the monodraw file format so I can have it draft ascii art diagrams and then tweak them visually ❤️

alamb · 2026-11-03T16:39:20Z

-                .contains("Bloom filter read range overflow")
-        );
-        assert_eq!(reader.position(), 0);
+    #[test]


it isn't clear to me why these tests are not with the others in parquet/tests/arrow_reader/bloom_filter/push.rs -- as they all seem to be testing the public APIs

I recommend consolidating to parquet/tests/arrow_reader/bloom_filter/push.rs

alamb · 2026-11-03T16:43:11Z

-#[inline]
-fn hash_as_bytes<A: AsBytes + ?Sized>(value: &A) -> u64 {
-    XxHash64::oneshot(SEED, value.as_bytes())
+/// Decoding state machine


as a minor nit I might move this into its own mod like bloom_filter/push.rs or something, but this is totally fine too

github-actions Bot added parquet Changes to the parquet crate enhancement New feature/addition labels Oct 28, 2026

This was referenced Oct 28, 2026

Make bloom filters / ArrowSbbf it's own crate #11681

Open

feat(parquet): Add async Sbbf::read_from_column_chunk_async Bloom filter API #11953

Merged

etseidl force-pushed the consolidate_bloom_filter_reads branch from ffa993a to 4bd0911 Compare November 1, 2026 17:50

etseidl and others added 2 commits November 1, 2026 23:05

refactor(parquet): consolidate Bloom filter reading and validation

bf5226e

add SbbfPushDecoder

8a73701

etseidl force-pushed the consolidate_bloom_filter_reads branch from e86ec10 to 8a73701 Compare November 2, 2026 07:05

clean up push API and add tests/docs

4dbecb9

etseidl changed the title ~~feat(parquet): Consolidate bloom filter reads~~ feat(parquet): Consolidate bloom filter reads with a push decoder Nov 2, 2026

etseidl marked this pull request as ready for review November 2, 2026 15:30

adriangb approved these changes Nov 2, 2026

View reviewed changes

adriangb reviewed Nov 2, 2026

View reviewed changes

add ascii art

d9ea8af

adriangb reviewed Nov 3, 2026

View reviewed changes

Comment thread parquet/src/bloom_filter/mod.rs Resolved

etseidl and others added 3 commits November 2, 2026 18:30

Update ASCII art

a198a1f

Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>

Merge remote-tracking branch 'apache/main' into consolidate_bloom_fil…

55ad027

…ter_reads

alamb approved these changes Nov 3, 2026

View reviewed changes

	fn expect_data<T: Debug>(result: Result<DecodeResult<T>>) -> T {
	match result.expect("Expected Ok(DecodeResult::Data(T))") {
	DecodeResult::Data(data) => data,
	result => panic!("Expected DecodeResult::Data, got {result:?}"),
	}
	}

	/// Expect that the [`DecodeResult`] is a [`DecodeResult::NeedsData`] and return the corresponding ranges
	fn expect_needs_data<T: Debug>(result: Result<DecodeResult<T>>) -> Vec<Range<u64>> {
	match result.expect("Expected Ok(DecodeResult::NeedsData(ranges))") {
	DecodeResult::NeedsData(ranges) => ranges,
	result => panic!("Expected DecodeResult::NeedsData, got {result:?}"),
	}
	}

	fn expect_finished<T: Debug>(result: Result<DecodeResult<T>>) {

Conversation

etseidl commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any significant user-facing changes?

AI disclosure

Uh oh!

etseidl commented Oct 28, 2026

Uh oh!

etseidl commented Nov 2, 2026

Uh oh!

alamb commented Nov 2, 2026

Uh oh!

adriangb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

etseidl commented Oct 28, 2026 •

edited

Loading

alamb left a comment •

edited

Loading