fix(expr): prune In predicates that straddle metrics bounds by M-Tesla · Pull Request #3144 · apache/iceberg-rust

M-Tesla · 2026-09-04T01:54:27Z

Which issue does this PR close?

Closes In predicate is not pruned when its literals straddle the bounds (InclusiveMetricsEvaluator and ManifestEvaluator) #3118.

What changes are included in this PR?

InclusiveMetricsEvaluator::in and ManifestEvaluator::in tested the lower bound and the upper bound against the full literal set independently. An In list whose values sit entirely outside [lower, upper] but straddle it — for example bounds [30, 79] and id IN (5, 104) — was therefore not pruned.

Both evaluators now narrow the literal set against each bound in turn, matching Iceberg Java, PyIceberg, and the existing StrictMetricsEvaluator::not_in implementation in this crate. Scan results were already correct (the plan was a superset); this only avoids opening files and manifests that cannot contain a match.

Are these changes tested?

Unit tests in inclusive_metrics_evaluator and manifest_evaluator for the straddling case (id IN (5, 104) against bounds [30, 79]). Existing In tests in those modules still pass.

Locally: cargo fmt --all -- --check, cargo clippy -p iceberg --all-targets --all-features -- -D warnings, and cargo test -p iceberg --lib expr::visitors.

AI Disclosure

Assisted draft of the bound-narrowing change and regression tests. The algorithm matches Iceberg Java, PyIceberg, and StrictMetricsEvaluator::not_in. Reviewed and verified with the checks above.

InclusiveMetricsEvaluator and ManifestEvaluator tested each bound against the full literal set, so IN lists with values both below lower and above upper were not pruned. Narrow the set the way Java, PyIceberg, and StrictMetricsEvaluator::not_in already do.

dhruvarya-db · 2026-09-04T22:34:36Z

+        // Narrow the set against each bound, matching Java / PyIceberg.
+        let mut filtered_literals = literals.clone();
+
        if let Some(lower_bound) = self.lower_bound(field_id) {


Looks like RowGroupMetricsEvaluator::in also has the same issue?

Thanks — it did. RowGroupMetricsEvaluator::in now uses the same both-bounds check, with a regression test for float bounds [4.0, 6.0] and IN (2.0, 8.0).

dhruvarya-db · 2026-09-04T22:36:22Z

        }

+        // Narrow the set against each bound, matching Java / PyIceberg.
+        let mut filtered_literals = literals.clone();


I wonder if something like might be more readable. We avoid creating a mutable filtered_literals as well this way

let lower_bound = self.lower_bound(field_id); let upper_bound = self.upper_bound(field_id); if lower_bound.is_some_and(Datum::is_nan) || upper_bound.is_some_and(Datum::is_nan) { return ROWS_MIGHT_MATCH; } let any_literal_in_bounds = match (lower_bound, upper_bound) { (Some(lower), Some(upper)) => { literals.iter().any(|datum| datum.ge(lower) && datum.le(upper)) } (Some(lower), None) => literals.iter().any(|datum| datum.ge(lower)), (None, Some(upper)) => literals.iter().any(|datum| datum.le(upper)), (None, None) => true, }; if !any_literal_in_bounds { return ROWS_CANNOT_MATCH; } ROWS_MIGHT_MATCH

Agreed, that's cleaner. Switched to any() over the original set instead of clone + retain.

dhruvarya-db · 2026-09-04T22:36:53Z

        }

+        // Narrow the set against each bound, matching InclusiveMetricsEvaluator.
+        let mut filtered_literals = literals.clone();


Same applies here https://github.com/apache/iceberg-rust/pull/3144/changes#r3938316479

Done here as well — ManifestEvaluator::in now uses the same any() form.

Apply review feedback: use any() instead of clone/retain, and prune straddling In predicates in RowGroupMetricsEvaluator too.

M-Tesla · 2026-09-04T23:26:45Z

Thanks @dhruvarya-db — applied your suggestions in b75d924: any() instead of clone/retain, and the same straddling-bounds prune in RowGroupMetricsEvaluator.

dhruvarya-db

LGTM (I am not a maintainer though)

dhruvarya-db · 2026-09-05T00:42:59Z

-                // if all values are less than lower bound, rows cannot match.
-                return ROWS_CANNOT_MATCH;
-            }
+        if lower_bound.is_some_and(|d| d.is_nan()) || upper_bound.is_some_and(|d| d.is_nan()) {


I wonder if it is worth it to factor out this pattern into a function and reuse it across the three callsites?

Good call, the three in evaluators share that match. Pulled it into a small crate-private helper and left bound loading / NaN handling at each callsite.

The three In evaluators used the same match. Pull it into a crate-private helper.

laskoviymishka

Nice fix — the straddling IN bug was real (two independent bound checks letting a literal set that brackets the range through), and unifying all three evaluators behind a single any_literal_in_bounds is the right shape. The strict::not_in duality holds up too.

I'd hold this before merging though. Collapsing the per-bound NaN checks into one up-front lower.is_nan() || upper.is_nan() bail changed more than intended: when one bound is valid and the other is NaN, the old code could still prune on the valid bound, and this version returns might-match instead. So for lower = 4.0, upper = NaN, IN (2.0, 3.0), we now read a row group the old code correctly skipped. It's a pruning regression rather than a correctness one, but it quietly reverses work we were already doing, and the existing NaN in tests pass under both old and new code so there's no signal.

A few things I'd want before merge:

treat a NaN bound as unbounded on that side (or restore the per-bound checks) so a valid bound still prunes — in both row_group_metrics_evaluator and inclusive_metrics_evaluator
add a test that pins it: lower = 4.0, upper = NaN, all literals below the lower bound should prune (the current NaN in test passes either way)
the NestedField clone in the manifest evaluator and a doc note on the helper's (None, None) precondition are minor, details inline

No maintainer has signed off yet, so treat this as a first pass. Once the NaN path is sorted, happy to take another look and approve.

laskoviymishka · 2026-09-09T09:13:36Z

-                // if all values are less than lower bound, rows cannot match.
-                return ROW_GROUP_CANT_MATCH;
-            }
+        if lower_bound.as_ref().is_some_and(|d| d.is_nan())


I think there's a subtle regression hiding in this reorg — flagging it because the existing NaN tests won't surface it.

The old code checked each bound in sequence, so it could prune on a valid lower bound before ever looking at the upper. This version bails to ROW_GROUP_MIGHT_MATCH the moment either bound is NaN, so a valid bound that would have pruned gets skipped. Concretely: lower = 4.0, upper = NaN, IN (2.0, 3.0) — old code returns ROW_GROUP_CANT_MATCH (both literals are below 4.0), new code sees the NaN upper and returns ROW_GROUP_MIGHT_MATCH. Results stay correct, but we read a row group the old code correctly skipped, and it leaves in inconsistent with eq, which still short-circuits per bound.

I'd treat a NaN bound as unbounded on that side rather than bailing on both — drop the NaN'd bound to None and let any_literal_in_bounds handle it, so the reliable bound still prunes. Or, if we'd rather not change the NaN semantics at all, keep the per-bound checks the old code had. Either way the same fix applies in inclusive_metrics_evaluator.rs. wdyt?

laskoviymishka · 2026-09-09T09:13:36Z

-                // if all values are less than lower bound, rows cannot match.
-                return ROWS_CANNOT_MATCH;
-            }
+        if lower_bound.is_some_and(|d| d.is_nan()) || upper_bound.is_some_and(|d| d.is_nan()) {


Same NaN-reordering regression as in row_group_metrics_evaluator.rs — a valid lower/upper bound that would prune gets skipped as soon as the other bound is NaN. Whatever we settle on there should apply here too.

laskoviymishka · 2026-09-09T09:13:36Z

        Ok(())
    }

+    #[test]


I don't think the existing in NaN tests cover the regression above — the ones asserting might-match use literals that already clear the valid bound, so the lower-bound path returns first and they pass identically under old and new code.

The case that actually pins the NaN behavior is lower = 4.0, upper = NaN, IN (2.0, 3.0) (all below the valid lower) — that should prune. Worth adding here, and in inclusive_metrics_evaluator, which has no in NaN-bound test at all. While we're in the helper's tests, the (Some, None) and (None, Some) arms are also only reached through the (Some, Some) cases so far.

laskoviymishka · 2026-09-09T09:13:36Z

-            if literals.iter().all(|datum| &upper_bound < datum) {
-                return ROWS_CANNOT_MATCH;
-            }
+        let field_type = *reference.field().clone().field_type;


Small thing while we're here: *reference.field().clone().field_type clones the whole NestedField (name, doc, defaults) just to move out the boxed type. Elsewhere in this file it's written *reference.field().field_type.clone(), which only clones the box. Hoisting it into one field_type binding is a nice cleanup though.

laskoviymishka · 2026-09-09T09:13:36Z

+            .any(|datum| datum.ge(lower) && datum.le(upper)),
+        (Some(lower), None) => literals.iter().any(|datum| datum.ge(lower)),
+        (None, Some(upper)) => literals.iter().any(|datum| datum.le(upper)),
+        (None, None) => true,


The (None, None) => true arm is right for the metrics evaluators — no bounds means we can't prune. For the manifest path a missing lower bound means the summary is all-null and IN should prune, which is the opposite. I'm assuming that case is already caught upstream before we reach the helper?

If so, a one-line note on that precondition here would keep a future refactor from silently flipping manifest pruning without any test catching it. wdyt?

dhruvarya-db reviewed Sep 4, 2026

View reviewed changes

fix(expr): check In literals against both bounds together

b75d924

Apply review feedback: use any() instead of clone/retain, and prune straddling In predicates in RowGroupMetricsEvaluator too.

dhruvarya-db approved these changes Sep 5, 2026

View reviewed changes

refactor(expr): share In bound check across metrics evaluators

52a92db

The three In evaluators used the same match. Pull it into a crate-private helper.

laskoviymishka requested changes Sep 9, 2026

View reviewed changes

                       Ok(())
                   }
+                  #[test]

Uh oh!

Conversation

M-Tesla commented Sep 4, 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?

AI Disclosure

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!

M-Tesla commented Sep 4, 2026

Uh oh!

dhruvarya-db 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

M-Tesla commented Sep 4, 2026 •

edited

Loading