feat: bloom filter pushdown by xanderbailey · Pull Request #2398 · apache/iceberg-rust

xanderbailey · 2026-05-02T13:21:10Z

Which issue does this PR close?

Closes Support Parquet bloom filter pruning on read #2841

What changes are included in this PR?

Adds bloom filter pushdown for equality predicates during Parquet reads. When enabled, the reader loads bloom filters from row group column chunks and uses them to skip row groups that definitely don't contain the queried values.

Key points:

New bloom_filter_enabled option on TableScanBuilder and ArrowReaderBuilder (off by default since it requires extra I/O per column per row group)
Only loads bloom filters for columns referenced in eq or in predicates — range predicates and other operators are ignored

Are these changes tested?

Unit tests covering the bloom filter evaluator: eq/in present/absent, AND/OR/NOT logic, all decimal physical types (INT32, INT64, FIXED_LEN_BYTE_ARRAY), negative values, missing bloom filters etc
Integration tests writing multi-row-group Parquet files with bloom filters enabled and verifying end-to-end row group pruning

xanderbailey · 2026-05-02T13:21:57Z

+    /// against them to filter out row groups that definitely don't match.
+    async fn filter_row_groups_by_bloom_filter(
+        predicate: &crate::expr::BoundPredicate,
+        builder: &mut ParquetRecordBatchStreamBuilder<ArrowFileReader>,


mut reference is because get_row_group_column_bloom_filter requires it.

…-endian (#2397) ## Which issue does this PR close?  - Closes #. Found this whilst working on #2398 ## What changes are included in this PR? [Spec](https://iceberg.apache.org/spec/#binary-single-value-serialization) says `Int128` and `UInt128` are big-endian not little-endian and indeed we are using big-endian [here](https://github.com/apache/iceberg-rust/blob/c1538de36dd53e491299b62ad89286f2db496bc7/crates/iceberg/src/arrow/schema.rs#L761) for example. I think it's just the doc string which needs correcting.  ## Are these changes tested?

github-actions · 2026-06-02T00:48:58Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

github-actions · 2026-06-10T00:48:29Z

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

amitgilad3 · 2026-06-28T07:26:27Z

is this still relevant ? would love to see this pr merged

xanderbailey · 2026-06-28T15:30:15Z

Just never got the review unfortunately. To my knowledge it’s still missing functionality

dannycjones · 2026-06-29T08:36:52Z

Let's open a feature request for this one: https://github.com/apache/iceberg-rust/issues/new/choose

dannycjones · 2026-06-29T08:37:01Z

Just never got the review unfortunately. To my knowledge it’s still missing functionality

@xanderbailey do you still want to own this one? we can reopen if so

xanderbailey · 2026-06-29T09:16:23Z

For sure, happy to continue with this. I don’t have permission to reopen however.

xanderbailey · 2026-08-05T16:46:16Z

@CTTY are you maybe able to reopen this PR?

mbutrovich

First pass, thanks for updating this PR @xanderbailey!

mbutrovich · 2026-09-09T19:50:30Z

+            },
+            _ => true,
+        },
+        PrimitiveLiteral::Float(v) => match physical_type {


Not a bug, but worth a test, for a non-obvious reason.

sbbf.check(&v.0) hashes raw IEEE bytes, so it treats -0.0 and +0.0 as distinct. That matches the reader today, but only because arrow_ord::cmp::eq, which the row filter uses, also treats them as distinct. That is not IEEE behavior:

arrow eq([-0.0, 0.0, 1.0], 0.0) = [false, true, false] raw rust: -0.0f32 == 0.0f32 -> true

So this bloom check and the row filter agree that a query for 0.0 shouldn't match a stored -0.0, which also lines up with the Iceberg spec: its Sorting section puts -0 strictly before 0 ("-NaN < -Infinity < -value < -0 < 0 < value < Infinity < NaN", aligned with Java's float comparison).

But they agree for unrelated reasons, and one of them is an upstream implementation detail rather than anything this repo controls. If arrow-rs ever made that kernel IEEE-conformant, the row filter would start matching -0.0 while this bloom check kept pruning it, and results would diverge silently. A case covering float positive and negative zero, asserting the same rows come back with with_bloom_filter_enabled(true) and (false), seems worth pinning down.

mbutrovich · 2026-09-09T19:50:55Z

+        Ok(())
+    }
+
+    fn not(&mut self, _inner: ()) -> Result<()> {


not() is a pass-through here, so eq and in sitting under a NOT still register their field ids. But the evaluator's not() at L316 always returns might-match, so those filters can never prune anything. Does that mean a bloom filter gets fetched per such column per row group without ever being able to help?

Yes, this is a good catch, I have a fix coming

mbutrovich · 2026-09-09T19:51:16Z

+    field_ids: HashSet<i32>,
+}
+
+impl BoundPredicateVisitor for BloomFilterFieldIdCollector {


These no-op methods overlap a fair bit with CollectFieldIdVisitor in arrow/reader/projection.rs. Would parameterizing that one (collect-all vs eq/in-only) be preferable to a second visitor, or is the trait boilerplate unavoidable enough that it isn't worth it?

mbutrovich · 2026-09-09T19:54:45Z

+                let type_length = col_meta.column_descr().type_length();
+
+                match builder
+                    .get_row_group_column_bloom_filter(rg_idx, col_idx)


These fetches look sequential, one awaited get_row_group_column_bloom_filter at a time, nested inside the per-row-group loop. A file with 100 row groups and 3 predicate columns would be up to 300 serialized round trips. On high-latency object storage, could that cost more than the pruning saves? try_buffer_unordered against a concurrency limit is used in this file at L100.

Separately, is there a way for a user to tell whether the option helped? Nothing appears to record row groups pruned, and ScanMetrics only exposes bytes_read, so that may be better as a follow-up than something for this PR.

Answered the first part #2398 (comment)

Happy to track metrics in a follow-up!

mbutrovich · 2026-09-09T19:54:55Z

+                let col_meta = builder.metadata().row_group(rg_idx).column(col_idx);
+
+                // Only attempt to load if this column chunk actually has a bloom filter
+                if col_meta.bloom_filter_offset().is_none() {


Is this bloom_filter_offset().is_none() pre-check needed? get_row_group_column_bloom_filter appears to make the same check and return Ok(None) before any I/O, which the Ok(None) arm at L771 already handles.

mbutrovich · 2026-09-09T19:55:41Z

+
+    /// Tests that bloom filter pushdown correctly prunes row groups.
+    #[tokio::test]
+    async fn test_bloom_filter_pushdown_prunes_row_groups() {


This test passes with pushdown disabled, so it doesn't demonstrate pruning. Running the same case both ways:

int32 id=150: bloom_on=1 bloom_off=1

The assertion at L1366 holds either way, since the row filter alone produces one row with id == 150. Would asserting on ScanResult::metrics(), checking bytes read is lower with pushdown on, test the pruning more directly? The same applies to test_bloom_filter_pushdown_value_absent at L1376, where a zero-row result is also what plain filtering gives.

More broadly, I think there's a gap here that no single test covers: nothing asserts that results are identical with the option on and off. That property is what catches encoding bugs in the bloom path, since those show up as rows that pushdown drops and the row filter keeps, which is exactly the failure mode the decimal arms are guarding against. A helper that reads a file both ways and asserts the batches match, plus one #[tokio::test] per case, would cover a lot cheaply: Int32 eq, Int32 in, string eq, decimal at each physical width including a widened-precision file, float positive and negative zero, and a file with no bloom filters. Coverage here is currently Int32-only.

laskoviymishka

Really glad to see bloom filter pushdown here, the safety model is sound (conservative fallbacks everywhere, no false negatives in the probe logic) and the decimal/promotion handling is more careful than the Java reference in a couple of spots.

I'd hold this before merging, since the whole point of the feature is to be faster when it's on, and right now two things push the other way. The field-ID collector walks into NOT subtrees, so an equality-delete scan — (scan_pred) AND NOT(del_col IN {...}) — loads del_col's bloom filter on every candidate row group only for not() to throw the result away. That's wasted I/O in precisely the case bloom filters are most likely to be combined with. Java avoids it by running rewriteNot before binding so the visitor never sees a NOT; doing the same here fixes it and buys back double-negation pruning for free.

The other one is that the filters are fetched sequentially, one row-group-column at a time, so a scan over many row groups pays a long series of round trips before it can skip anything — enough on object storage to make enabling the feature a net loss.

A few other things I'd want to settle before merge:

don't collect field IDs from eq/in under a NOT (rewrite_not, or a NOT-depth guard)
fetch the per-row-group bloom filters concurrently rather than in series
make the integration tests actually assert pruning happened (bytes read / a read counter), not just the final row count
align the Int128 -> INT32/INT64 casts with the try_from arm next to them, and fix the backwards "cannot be present" comment
log the swallowed bloom-filter read/eval errors so a silent fallback to full scan is visible

The decimal BYTE_ARRAY gap and the BOOLEAN arm are fine as follow-ups if noted. Once the two performance issues and the tests are addressed, happy to take another pass and approve.

laskoviymishka · 2026-09-09T19:45:18Z

+
+/// Collects field IDs that appear in `eq` or `in` predicates — the only
+/// predicate types that benefit from bloom filter checks.
+pub(crate) fn collect_bloom_filter_field_ids(predicate: &BoundPredicate) -> Result<HashSet<i32>> {


This collects field IDs from eq/in nodes even when they sit under a NOT, so NOT(id = 5) and NOT(id IN {...}) both end up loading a bloom filter that can never prune anything — the evaluator's not() always returns might-match and throws the inner result away.

That's not just a missed optimization: the equality-delete path builds (scan_pred) AND NOT(del_col IN {del_vals}), so any scan with equality deletes fetches del_col's bloom filter on every candidate row group and discards it. With bloom filters on, that's slower than with them off, in exactly the case they'd most often be combined.

Java sidesteps this by running rewriteNot before binding, so its visitor never sees a NOT (the not() handler throws). I'd do the same here — normalize with rewrite_not before collect_bloom_filter_field_ids/eval — or, if we'd rather not, track NOT-depth in the collector so eq/in under a NOT don't insert. The rewrite path also buys back NOT(NOT(x = v)) pruning for free. wdyt?

Have pruned them out 8adcd72

I chased this down and a couple of things came out differently than expected.

rewrite_not() already runs upstream in TableScanBuilder::with_filter (scan/mod.rs:184), and binding preserves structure, so no Not node reaches the reader on the scan path. A second rewrite here would allocate a fresh predicate tree per task to strip nodes that aren't there.

The NOT-depth guard isn't implementable as written: BoundPredicateVisitor is post-order, so by the time not() runs the inner eq/in have already inserted. I changed Self::T to HashSet<i32> instead - each node returns its subtree's field ids, not returns empty. That makes the collector mirror the evaluator by construction, and it holds for hand-built FileScanTask predicates that bypass with_filter. Added five tests, including one that a pass-through not fails.

Rewriting is still the better normalization in principle, but since with_filter already applies it, a reader-side rewrite would only add pruning for predicates built directly onto a FileScanTask — recovering NOT(a != v) → a = v, which the collector guard deliberately forgoes. Happy to file a follow-up if you think that path is worth optimising.

laskoviymishka · 2026-09-09T19:45:18Z

+
+        let mut result = Vec::with_capacity(candidate_row_groups.len());
+
+        for &rg_idx in candidate_row_groups {


Every (row_group, column) pair here ends in a separate sequential .await on get_row_group_column_bloom_filter. For 100 row groups and 3 predicate columns that's 300 round trips in series before a single group is skipped — on object storage that latency can easily swamp whatever I/O the pruning saves, which undercuts the reason to enable this at all.

I'd fetch the filters concurrently — join_all over the (rg, col) pairs, or at least per-column within a row group — so the round trips overlap. Thoughts?

join_all won't work here since get_row_group_column_bloom_filter requires a mutable reference to self. Same reason a buffered stream won't work.

For what it's worth DataFusion does the same as us https://github.com/apache/datafusion/blob/55.0.0/datafusion/datasource-parquet/src/opener/mod.rs#L1266-L1292

So I'd rather do this as a follow up optimization rather than block this PR? WDYT?

Ah, https://github.com/apache/arrow-rs/pull/8462/changes is what we need. I'll create an issue to track a follow-up

Not a new comment, a response to your question about deferring.

Deferring seems right to me. get_row_group_column_bloom_filter takes &mut self, so this isn't a case of picking the wrong combinator, and arrow-rs#8462 is the actual unblock. The DataFusion reference is fair, and since the option is off by default, a user who opts in is accepting the current cost rather than silently paying it.

The thing that would make me comfortable is the doc on with_bloom_filter_enabled being explicit that the reads are serialized per row group per column, so the cost model is visible at the call site instead of only in the tracking issue. Right now it says extra I/O per column per row group, which reads as a volume cost rather than a latency one, and latency is what dominates on object storage.

laskoviymishka · 2026-09-09T19:45:18Z

+        // Only row group 1 (ids 100..200) should be read. The row filter
+        // then further filters to just id=150.
+        let total_rows: usize = result.iter().map(|b| b.num_rows()).sum();
+        assert_eq!(total_rows, 1, "Should find exactly one row matching id=150");


Both integration tests only assert the final row count, which the existing row filter already guarantees — a pruner that read every row group and skipped nothing would pass both identically. So they don't actually prove bloom filter pruning happened.

I'd assert on something that only changes when a group is skipped: metrics().bytes_read() against a with_bloom_filter_enabled(false) baseline, or a per-row-group read counter. Once there's a metric that observes pruning, it'd also be worth reaching past eq-on-INT32 to cover the IN path and a string column end to end, since those are only unit-tested today.

laskoviymishka · 2026-09-09T19:45:18Z

+            // Decimal: dispatch based on the actual Parquet physical type
+            // from the file, not inferred from precision.
+            match physical_type {
+                PhysicalType::INT32 => sbbf.check(&(*v as i32)),


These *v as i32 / *v as i64 casts truncate silently, while the Long -> INT32 arm just above uses i32::try_from with a conservative fallback. It's safe here only because an i128 that doesn't fit can't be in the narrower column — but that reasoning isn't obvious, and the mismatch with the arm above is the kind of thing someone later "cleans up" into a real false negative.

I'd match the Long arm and use try_from with Err(_) => true for both INT32 and INT64 so the intent is self-documenting.

Nice suggestion 6c67826

laskoviymishka · 2026-09-09T19:45:18Z

+            PhysicalType::INT64 => sbbf.check(v),
+            PhysicalType::INT32 => match i32::try_from(*v) {
+                Ok(narrowed) => sbbf.check(&narrowed),
+                // Out of range for the column, so it cannot be present.


The comment says the value "cannot be present," which reads like we should prune (return false), but the arm returns true (might-match). The behavior is right — conservatively keeping the group — it's just the comment that's backwards, and it's exactly the sort of thing that could talk a future reader into flipping the arm.

I'd reword to something like "value can't fit in an INT32 column, so we can't probe it — conservatively keep the row group."

laskoviymishka · 2026-09-09T19:45:18Z

+                        None => true,
+                    }
+                }
+                _ => true, // Unexpected physical type — conservatively might match


Decimals encoded as BYTE_ARRAY fall into this _ => true and skip the probe entirely. It's valid per the Parquet spec (and some older Spark writers do emit it), and Java handles BINARY alongside FIXED_LEN_BYTE_ARRAY for decimals.

No correctness risk since we just keep the group, but it's a silent parity gap — decimal predicates get no pruning against those files. Fine as a follow-up if we note it, but I'd at least leave a comment here so it reads as a known gap rather than an oversight.

laskoviymishka · 2026-09-09T19:45:18Z

+    let physical_type = *physical_type;
+
+    match datum.literal() {
+        PrimitiveLiteral::Boolean(v) => sbbf.check(v),


Parquet doesn't define bloom filter semantics for BOOLEAN — Java's hash only covers int/long/float/double/binary — so this arm probes something the spec says nothing about. In practice no writer builds a bloom filter for a bool column, so bloom_filter_offset().is_none() short-circuits it and the code is dead.

Since it's dead and off-spec, I'd either drop the arm and let it fall through to a conservative default, or leave a one-line comment noting it's undefined. Minor, but no reason to carry a probe the spec doesn't back.

Nothing in arrow-rs actually stops you from writing bloom filters for booleans... Ah but min/max statistics are strictly at least as informative as a bloom filter... Okay I'll remove this with a comment

xanderbailey · 2026-09-10T11:59:20Z

Thank you both for your reviews, I think this should be good for another look now!

…/bloom_filter_pushdown

mbutrovich

Minor testing and comment feedback, otherwise this is looking really good. Thanks for sticking with this, @xanderbailey!

mbutrovich · 2026-09-10T18:58:48Z

+        // Row group 0 holds only -0.0; row group 1 only +0.0; row group 2 neither.
+        write_value_fixture(
+            &path,
+            DataType::Float32,
+            |g| {
+                let fill = match g {
+                    0 => -0.0f32,
+                    1 => 0.0f32,
+                    _ => 7.5f32,
+                };
+                Arc::new(Float32Array::from(vec![fill; ROWS_PER_GROUP as usize]))
+            },
+            true,
+        );


Thanks for adding this one. I think the fixture layout keeps it from testing what it describes, though: row-group statistics prune the interesting group before the bloom filter is consulted, so the test passes no matter how either layer treats signed zero.

row_group_filtering_enabled defaults to true and no test here turns it off, so RowGroupMetricsEvaluator runs first. Each group is filled with a single value, so min == max, and Iceberg's float comparison is a total order where -0.0 < 0.0. Probing 0.0 puts group 0 (min = max = -0.0) out of range, so it's pruned on statistics alone; probing -0.0 does the same to group 1. In both iterations the group holding the other zero disappears before any probe happens.

So the divergence this test is meant to catch cannot make it fail. If the bloom check started treating -0.0 and +0.0 as equal, group 0 is already gone. If arrow_ord::cmp::eq became IEEE-conformant, group 0 is still already gone, so the extra rows never surface.

For the test to be able to fail, a row group needs to hold one zero but not the other, and have a min/max range wide enough that statistics keep it. A group of -0.0 plus a larger filler value, probed with +0.0, would do it:

statistics: 0.0 falls inside [-0.0, 5.0], so the group survives

bloom: +0.0 was never inserted, so the group is pruned and no rows come back

pushdown off: arrow_ord::cmp::eq also rejects -0.0, so no rows come back either, and the two agree today

if that kernel ever became IEEE-conformant, the off path would return the -0.0 rows while the bloom path still pruned them, and assert_pushdown_agrees would fail

Asserting the row count explicitly would also help, so the test records which equality semantics is expected rather than only that the two paths match.

I went digging on this because it sounded right, but I don't think the min == max case
actually happens, parquet-rs widens the bounds for single-value zero groups.

There's a spec-mandated special case in the writer: if the computed min is any zero it's
written as -0.0, and if the max is any zero it's written as +0.0 —
replace_zero in column/writer/encoder.rs.
So the all--0.0 group lands with [-0.0, +0.0], and so does the all-+0.0 group. Both
directions are pinned by round-trip tests:
test_float_statistics_zero_only
and test_float_statistics_neg_zero_only.

Which would mean either probe sits inside both zero groups' ranges, so neither is pruned on
statistics. I didn't want to trust my reading of that, so Claude put a temporary eprintln! in
filter_row_groups_by_bloom_filter, both zero groups do reach the bloom check, and only
group 2 (the 7.5 filler) is pruned by stats:

PROBE positive_zero: BLOOM rg=0 PRUNE BLOOM rg=1 KEEP PROBE negative_zero: BLOOM rg=0 KEEP BLOOM rg=1 PRUNE

That said, I prefer what you're suggesting, so I've
switched to your version. Two things came out of actually mutating the code to check:

The direction you describe in your last bullet is caught. I made the bloom prune a group that
does hold the probed zero, and the test fails with bloom filter pushdown changed the rows returned (left: None, right: Some(RecordBatch…)). That's the one that matters, since it's
the bloom dropping rows the filter would have kept.

The other direction isn't, and can't be at the row level: I made the probe canonicalize the
sign (sbbf.check(&v.0) || sbbf.check(&-v.0)) and the test still passed. The bloom filter is
only a row-group prefilter and the row filter still matches exactly afterwards, so a probe that
over-keeps costs pruning rather than correctness. I couldn't pin it with bytes either — on this
fixture bytes_on=1209 vs bytes_off=1163, because the bloom metadata reads outweigh the data
saved on 20k very compressible floats. assert_prunes_and_agrees covers "the probe never
prunes" for the other types, so I've left this one asserting the exact row count and reworded
the comment to say only what holds.

The reason your fixture is better, I think, is the one neither of us started with: the only
thing making the old one reach the bloom layer was that writer-side widening, which is
incidental to what's under test. If arrow-rs ever stopped doing it, the test would keep passing
while quietly exercising nothing. Pairing each zero with a larger filler gets the range from
the data, so it doesn't depend on that at all. Row count assertion added as well — rows(&on) > 0
didn't record which equality semantics we expect, which is probably part of why the fixture read
the way it did.

mbutrovich · 2026-09-10T18:59:21Z

+
+        let mut result = Vec::with_capacity(candidate_row_groups.len());
+
+        for &rg_idx in candidate_row_groups {


Not a new comment, a response to your question about deferring.

Deferring seems right to me. get_row_group_column_bloom_filter takes &mut self, so this isn't a case of picking the wrong combinator, and arrow-rs#8462 is the actual unblock. The DataFusion reference is fair, and since the option is off by default, a user who opts in is accepting the current cost rather than silently paying it.

The thing that would make me comfortable is the doc on with_bloom_filter_enabled being explicit that the reads are serialized per row group per column, so the cost model is visible at the call site instead of only in the tracking issue. Right now it says extra I/O per column per row group, which reads as a volume cost rather than a latency one, and latency is what dominates on object storage.

mbutrovich

Thanks @xanderbailey!

laskoviymishka

This is close now. The sequential fetch was my load-bearing concern last round, and while it's still serial, I'm good with where it landed — it's called out in the with_bloom_filter_enabled docstring, tracked as TODO(#3191), genuinely constrained by the &mut self on get_row_group_column_bloom_filter, and off by default. That's the right shape for a follow-up, so I won't hold on it.

The one thing I'd like to confirm before we merge is the signed-zero case. The Float arm probes sbbf.check(&v.0) on raw IEEE bits, so -0.0 and +0.0 hash to different slots, while arrow's row filter treats them as equal. If a row group holds only -0.0 and someone probes = 0.0, I want to be sure we're not pruning rows the row filter would have kept. test_bloom_pushdown_float_signed_zero passing suggests arrow normalizes the sign somewhere on the read path, but the test comment claims arrow's float is_eq is bitwise, which I don't think is right — so I'd like to nail down which it actually is. If it does diverge, the fix is cheap: probe both zero bit patterns, or just return might-match for any ±0.0 probe.

Everything else I asked for last round is in:

the NOT-subtree guard — collect_bloom_filter_field_ids no longer collects eq/in under a not
the Int128 -> INT32/INT64 casts aligned on try_from, and the backwards "cannot be present" comment fixed
swallowed bloom read/eval errors now surfaced via tracing::debug!
integration tests assert bytes_on < bytes_off, so they actually prove pruning rather than just row counts

Confirm the signed-zero behavior and I'm happy to approve. Will wait some time before merging for others to look on it.

laskoviymishka · 2026-09-11T12:54:20Z

+            _ => true,
+        },
+        PrimitiveLiteral::Float(v) => match physical_type {
+            PhysicalType::FLOAT => sbbf.check(&v.0),


I'd want to confirm this arm doesn't diverge from the row filter on signed zero. sbbf.check(&v.0) hashes raw IEEE bits, so a file holding only -0.0 won't match a = 0.0 probe here — but arrow's cmp::eq treats -0.0 == 0.0 as true, which would mean we prune a row group the row filter would have returned rows from.

test_bloom_pushdown_float_signed_zero passing hints that arrow normalizes the sign somewhere on the read path, but the test comment says arrow's float eq is bitwise "for the same reason," and I don't think that's right. Could we pin down which it actually is? If it does diverge, probing both zero bit patterns (or returning might-match for any ±0.0 probe) closes it cheaply.

Okay bare with me, I have never looked this closely at IEEE float equality but here's my reading...

https://github.com/apache/arrow-rs/blob/59.2.0/arrow-array/src/arithmetic.rs#L397-L402

fn is_eq(self, rhs: Self) -> bool { // Equivalent to `self.total_cmp(&rhs).is_eq()` // but LLVM isn't able to realise this is bitwise equality // https://rust.godbolt.org/z/347nWGxoW self.to_bits() == rhs.to_bits() }

So arrow's float equality is total-order, not IEEE: -0.0 != 0.0, and NaN == NaN. The probe and the row filter agree, and the pruning is sound.

test_bloom_pushdown_float_signed_zero is also a controlled experiment for this rather than just a hint. With ROWS_PER_GROUP = 20_000, row group 0 holds 19,999 × -0.0 plus one 5.0 filler, so its stats range is [-0.0, 5.0] — it spans the probe, and nothing prunes it on the bloom-off read. That read therefore does hand 19,999 -0.0 values to cmp::eq against a 0.0 literal. IEEE-lenient eq returns 39,998 rows there; the test asserts 19,999. The bloom-off arm is the control that pins arrow's semantics on the exact path in question, and assert_pushdown_agrees compares full RecordBatch contents rather than row counts, so if arrow ever flipped is_eq this test fails instead of silently over-pruning.

You're right that the comment's justification was sloppy, though — "for the same reason" is wrong. Sbbf is bitwise because it hashes raw bytes; arrow is bitwise because it implements total_cmp semantics. Two independent reasons that happen to agree, and conflating them is probably what made the claim look shaky. Reworded to cite ArrowNativeTypeOp::is_eq directly so the invariant is traceable.

laskoviymishka · 2026-09-11T12:54:20Z

+            PhysicalType::INT32 => match i32::try_from(*v) {
+                Ok(narrowed) => sbbf.check(&narrowed),
+                // Too wide for an INT32 column to hold, so there is nothing
+                // meaningful to probe — keep the row group.


The comment has it backwards — a value too wide for INT32 provably can't be in an INT32 column, so the filter can give a definitive answer here; we're just choosing not to act on it. "Nothing meaningful to probe" reads like the filter is helpless, when we're really returning a conservative might-match rather than pruning.

If we ever wanted the extra pruning, returning false here would actually beat the Java reference (which truncates and can't prune) — but the tests deliberately assert the conservative path, so that's a genuine follow-up, not something to change now. Just the comment wording, wdyt?

laskoviymishka · 2026-09-11T12:54:20Z

+        PrimitiveLiteral::UInt128(v) => {
+            // UUID: stored as FIXED_LEN_BYTE_ARRAY(16), big-endian
+            let bytes = v.to_be_bytes();
+            sbbf.check(&ByteArray::from(bytes.to_vec()))


bytes is already a [u8; 16] on the stack, so we can hand the slice straight to check and skip the Vec + ByteArray allocation — the Binary arm just above already does this with v.as_slice().

Suggested change

sbbf.check(&ByteArray::from(bytes.to_vec()))

sbbf.check(bytes.as_ref())

Same bytes hashed, just no per-probe alloc, which adds up for IN over many row groups.

laskoviymishka

That's is good to land for me, nice work! Thanks!

xanderbailey · 2026-09-15T11:46:58Z

Are we okay to merge this @laskoviymishka or would you want eyes from another committer?

laskoviymishka · 2026-09-15T15:28:01Z

@xanderbailey i think we are good to merge

xanderbailey commented May 2, 2026

View reviewed changes

xanderbailey mentioned this pull request May 2, 2026

docs: Correct docs for Int128 and UInt128 being big-endian not little-endian #2397

Merged

github-actions Bot added the stale label Jun 2, 2026

github-actions Bot closed this Jun 10, 2026

led-d mentioned this pull request Jul 17, 2026

Support Parquet bloom filter pruning on read #2841

Closed

CTTY added not-stale and removed stale labels Aug 6, 2026

CTTY reopened this Aug 6, 2026

mbutrovich self-requested a review September 8, 2026 20:16

xanderbailey added 6 commits September 9, 2026 08:44

feat: bloom filter pushdown

1c013de

test

0671bf1

avoid copy

87146af

new name

73c4fdd

fmt

1559571

doc string

58ad776

xanderbailey force-pushed the xb/bloom_filter_pushdown branch from 86c50b7 to 58ad776 Compare September 9, 2026 07:49

xanderbailey added 2 commits September 9, 2026 09:30

handle type widening

e23e733

widen decimals

29072aa

mbutrovich reviewed Sep 9, 2026

View reviewed changes

laskoviymishka requested changes Sep 9, 2026

View reviewed changes

pub crate

1c4a8c1

xanderbailey added 6 commits September 10, 2026 09:18

add not supported comment

eed3e03

make it more safe

6c67826

Add tests

db5daf8

fix

a1c6b92

fmt

3d80060

don't read not predicates

8adcd72

xanderbailey mentioned this pull request Sep 10, 2026

Call get_row_group_column_bloom_filter in parallel #3191

Open

xanderbailey added 2 commits September 10, 2026 12:21

tests

cfbf233

remove boolean

351f1dd

xanderbailey requested a review from laskoviymishka September 10, 2026 11:59

Merge branch 'main' of https://github.com/apache/iceberg-rust into xb…

1bf11bd

…/bloom_filter_pushdown

mbutrovich self-requested a review September 10, 2026 18:57

mbutrovich reviewed Sep 10, 2026

View reviewed changes

doc

631461d

mbutrovich approved these changes Sep 10, 2026

View reviewed changes

update 0 test

053ddb3

laskoviymishka approved these changes Sep 11, 2026

View reviewed changes

xanderbailey added 3 commits September 11, 2026 14:54

UUID

1a1540e

update comment

76aca80

update test doc

1f54c40

xanderbailey requested a review from laskoviymishka September 11, 2026 14:12

laskoviymishka approved these changes Sep 14, 2026

View reviewed changes

laskoviymishka enabled auto-merge September 15, 2026 15:28

Merge branch 'main' into xb/bloom_filter_pushdown

f3b179c

laskoviymishka added this pull request to the merge queue Sep 15, 2026

Merged via the queue into apache:main with commit 8193cce Sep 15, 2026
23 checks passed


		let mut result = Vec::with_capacity(candidate_row_groups.len());

		for &rg_idx in candidate_row_groups {

	sbbf.check(&ByteArray::from(bytes.to_vec()))
	sbbf.check(bytes.as_ref())

Uh oh!

Conversation

xanderbailey commented May 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 2, 2026

Uh oh!

github-actions Bot commented Jun 10, 2026

Uh oh!

amitgilad3 commented Jun 28, 2026

Uh oh!

xanderbailey commented Jun 28, 2026

Uh oh!

dannycjones commented Jun 29, 2026

Uh oh!

dannycjones commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xanderbailey commented Jun 29, 2026

Uh oh!

xanderbailey commented Aug 5, 2026

Uh oh!

mbutrovich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laskoviymishka 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!

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!

Choose a reason for hiding this comment

Uh oh!

xanderbailey commented May 2, 2026 •

edited

Loading

dannycjones commented Jun 29, 2026 •

edited

Loading