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, RowGroupMetricsEvaluator::in, and ManifestEvaluator::in used to test each 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.

The three evaluators now share any_literal_in_bounds, which requires some literal to satisfy both bounds together. Scan results were already correct (the plan was a superset); this only avoids opening files and manifests that cannot contain a match.

A NaN min or max is treated as unbounded on that side, so a valid bound can still prune. Manifest IN still prunes all-null summaries before the helper; (None, None) in the helper means no bound is available.

Are these changes tested?

Straddling IN against [30, 79] / [4.0, 6.0].
NaN upper with all literals below a valid lower (4.0 / IN (2.0, 3.0)), and NaN lower with all literals above a valid upper.
Helper coverage for (Some, None), (None, Some), and (None, None).

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

AI Disclosure

Assisted draft of the bound check, NaN handling, and regression tests. The straddling check matches Iceberg Java, PyIceberg, and StrictMetricsEvaluator::not_in. Reviewed and verified with the checks above.

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.

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.

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?

You are right, and thank you for walking through the old versus new control flow. I agree with treating a NaN bound as unbounded on that side rather than restoring the sequential bail.

eq still returns might-match when the first bound is NaN, even if the other bound would prune. For in that is the wrong tradeoff: a reliable lower of 4.0 with a NaN upper should still reject IN (2.0, 3.0). I drop the NaN bound to None and let any_literal_in_bounds handle (Some, None) / (None, Some). If you would rather keep in identical to eq's early exit, say so and I will switch it.

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.

Same change here. Inclusive now runs each bound through finite_bound before any_literal_in_bounds, so a valid bound still prunes when the other side is NaN.

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.

Agreed. I added that pin (lower = 4.0, upper = NaN, IN (2.0, 3.0) must prune), the inclusive counterpart, and helper tests for the one-sided arms. The old might-match NaN test really did pass under both implementations.

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.

Good catch. Switched to *reference.field().field_type.clone() so we only clone the boxed type.

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?

Yes: if field.lower_bound is missing, ManifestEvaluator::in already returns cannot-match (all-null summary) and never reaches the helper. (None, None) is only a "no stats, cannot prune" signal for the metrics evaluators. I put that precondition on the helper so a later refactor cannot swap the two meanings.

M-Tesla · 2026-09-09T22:23:57Z

Thank you for taking the time on this, @laskoviymishka. This was a careful read, and you are right that the NaN reorg introduced a pruning regression the existing tests could not catch. I am sorry the first reply was just a changelog.

On treating a NaN bound as unbounded versus restoring the per-bound checks: I prefer unbounded on that side. Restoring the sequential bail would keep in identical to eq (might-match as soon as the first bound is NaN), but it would also throw away a valid bound that can still prove the literal set is outside the range. Your example is exactly that case: lower = 4.0, upper = NaN, IN (2.0, 3.0). Dropping the NaN bound to None and letting any_literal_in_bounds see (Some(4.0), None) is the version that still prunes, and that is what I implemented in both evaluators.

On the helper's (None, None): yes, the manifest path already returns cannot-match when lower_bound is missing, before it calls the helper. That is the all-null summary case. I added a note on that precondition so a later refactor cannot flip it silently, and I added direct tests for the (Some, None) and (None, Some) arms.

The NestedField clone is now *reference.field().field_type.clone().

If you would rather in keep eq's NaN short-circuit instead, I can change it. Otherwise I think this is the right tradeoff.

laskoviymishka

This is basically there, the NaN rework I asked for landed cleanly and it's correct. One thing gates it, and it's mechanical.

The manifest evaluator won't build against current main: #3214 changed bytes_to_datum to take &Type instead of an owned Type, so the field_type.clone() in visit_in no longer typechecks. A rebase fixes it — and nicely, on the new signature the whole field_type local plus both clones just disappear, which resolves the manifest clone I flagged last round for free.

Everything else from round-1 is in:

NaN-as-unbounded via the new finite_bound() helper, applied in both the inclusive and row-group evaluators
the pinning tests — test_float_in_nan_upper_bound_prunes_below_lower, test_float_in_nan_lower_bound_prunes_above_upper, and the flipped eval_false_..._all_literals_above_upper_is_in
the (None, None) doc note on the helper

I left a couple of non-blocking notes inline — the helper doc's "all-null" precondition is a touch narrower than the real guarantee, and the NaN tightening is inclusive-only (worth marking as a deliberate divergence from Java/PyIceberg, with a strict-not_in follow-up). None of those need to hold up merge.

Rebase and this is good to land.

laskoviymishka · 2026-09-18T13:11:07Z

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


This won't build against current main — worth a rebase before anything else.

#3214 changed ManifestFilterVisitor::bytes_to_datum to take &Type instead of an owned Type, so field_type.clone() here won't typecheck once you're on top of it.

The happy part: rebasing also dissolves the clone I flagged last round. With the &Type signature you can drop the field_type local entirely and pass reference.field().field_type.as_ref() straight into both calls — no clone on either side. Rebase and this whole block gets simpler.

Merged main and switched both calls to reference.field().field_type.as_ref(). The local and the clones are gone.

laskoviymishka · 2026-09-18T13:11:07Z

+/// Missing bounds are treated as unbounded on that side.
+///
+/// `(None, None)` returns true because no bound is available to prune against.
+/// Manifest evaluation must not reach this helper when the partition summary


Small doc thing. The "that case is all-null" reason is narrower than the actual guarantee — per the spec a missing lower_bound also covers all-NaN and mixed null+NaN partitions. The manifest early-return handles all three, so the code is fine; the sentence just undersells why.

More to the point, this precondition is specific to the manifest caller but it's sitting on a generic three-caller helper. I'd move it down to the manifest call site next to the field.lower_bound.is_none() guard and leave the helper doc with just the neutral (None, None) note. wdyt?

Moved. The helper now only says (None, None) cannot prune. The spec cases (all-null, all-NaN, mixed null+NaN) are documented on the manifest lower_bound.is_none() guard.

laskoviymishka · 2026-09-18T13:11:07Z

+/// Drops a NaN bound so that side is treated as unbounded.
+///
+/// A NaN min or max is unreliable, but the other bound may still prune.
+pub(crate) fn finite_bound(bound: Option<&Datum>) -> Option<&Datum> {


This is the NaN-as-unbounded behavior I asked for last round, and it's right.

One thing worth a note while we're here: finite_bound now prunes in a spot Java and PyIceberg's inclusive evaluators don't — both bail to might-match the moment either bound is NaN, so for lower = NaN, upper = 1.0, IN (2.0, 3.0) we skip where they'd read. It's a legitimate improvement (total_cmp makes NaN the max, so dropping it and trusting the finite side is sound), just more aggressive than the reference clients. A one-line comment marking it as a deliberate divergence would save the next reader the double-take.

Related, and not for this PR: the straddling duality I mentioned last round still holds, but this new NaN tightening is inclusive-only — StrictMetricsEvaluator::not_in still bails on NaN, so the pair isn't symmetric on that edge. It's safe (strict stays conservative), so a follow-up to give not_in the same finite_bound treatment is plenty. wdyt?

Added that note on finite_bound. On strict::not_in, I would rather leave the NaN tightening as a follow-up. Strict staying conservative is safe, and that path deserves the same kind of pinning tests we added here.

laskoviymishka · 2026-09-18T13:11:07Z

+    }
+
+    #[test]
+    fn test_float_in_nan_lower_bound_prunes_above_upper() {


These pin the skip direction, which is exactly what I asked for last round — thanks.

One small gap: every NaN test here asserts a prune, so a bug that dropped the valid bound instead of the NaN one would sail through — keeping a NaN lower makes ge(NaN) false for every literal, which also prunes. A companion that should not prune — NaN lower, upper = 3.0, IN (2.0, 4.0) → might-match — would pin the wrong-side case. Low priority, but cheap.

Added. Inclusive now has test_float_in_nan_lower_bound_does_not_prune_when_a_literal_is_inside_upper (NaN lower, upper = 3.0, IN (2.0, 4.0) must read), and the row-group evaluator has the matching case.

M-Tesla · 2026-09-18T21:33:39Z

Thank you for coming back to this, @laskoviymishka. I merged current main (no rebase) so the branch builds again after #3214.

visit_in now passes reference.field().field_type.as_ref() into both bytes_to_datum calls, so the field_type local and the clones are gone.

I also took the non-blocking notes: the missing-lower-bound comment sits next to the manifest guard, finite_bound now calls out the Java/PyIceberg divergence, and there is a companion might-match test (NaN lower, upper = 3.0, IN (2.0, 4.0)).

On giving StrictMetricsEvaluator::not_in the same finite_bound treatment: I would keep that as a follow-up. Strict staying conservative is the right default until we pin it the same way.

laskoviymishka

The rebase landed — bytes_to_datum takes field_type.as_ref() now and the old field_type local plus both clones are gone, which was the only thing gating this last round. Both non-blocking notes from round 2 are in too: the helper precondition now reads "all-null, all-NaN, and mixed null+NaN summaries", and finite_bound spells out the deliberate divergence from Java/PyIceberg.

That was the whole list, so this is an approve from me. The inline notes I left are all optional follow-ups a maintainer can fold in or punt — none of them gate: the manifest evaluator is the one call site that skips finite_bound (correct per spec, just reads like an oversight next to the other two), a doc-completeness tweak on finite_bound, and a bit more NaN test coverage at the evaluator level (double-typed and both-bounds-NaN). A couple of cosmetic nits in there too (a test name or two) genuinely take-or-leave.

Nice work seeing it through three rounds. This is good to land!

laskoviymishka · 2026-09-21T16:23:11Z

-            if literals.iter().all(|datum| &upper_bound < datum) {
-                return ROWS_CANNOT_MATCH;
-            }
+        if !super::any_literal_in_bounds(lower_bound.as_ref(), upper_bound.as_ref(), literals) {


Optional, non-blocking: this is the one of the three evaluators that doesn't wrap its bounds in finite_bound, and that's correct — partition-summary bounds are the min/max of non-NaN values per spec, so there's no NaN to drop here.

Sitting right next to the other two call sites though, the omission reads like an oversight, and the next person is going to either file a bug or add a spurious finite_bound. A one-liner — something like // summary bounds are finite per spec; NaN is tracked separately via contains_nan — closes that off. wdyt?

laskoviymishka · 2026-09-21T16:23:11Z

+/// A NaN min or max is unreliable, but the other bound may still prune.
+/// Inclusive evaluators in Java and PyIceberg bail to might-match when
+/// either bound is NaN. Dropping only the NaN side is a deliberate
+/// divergence: `total_cmp` treats NaN as the maximum, so the remaining


Optional doc polish — the total_cmp line explains why NaN sorts to the max, but the load-bearing assumption is really that the retained bound is trustworthy. Worth a sentence naming that: we drop the NaN side and trust the finite one, which is sound as long as the writer's non-NaN bound is itself reliable. Keeps a future reader from seeing the NaN min and assuming the whole stat block is poisoned. Purely a comment tweak, not a gate.

laskoviymishka · 2026-09-21T16:23:11Z

        }
    }
+
+    fn get_test_file_float_nan_upper() -> DataFile {


Two optional test follow-ups, neither blocking. These NaN-bound fixtures pair a NaN bound with nan_value_counts: 0 — a deliberately impossible "broken writer" state, which is exactly the case worth exercising, but a one-line comment saying so would stop a future reader from treating it as a valid fixture. And the NaN coverage here is all f32/Float; since finite_bound also handles Double, a double-typed case (plus a both-bounds-NaN case) at the evaluator level would round it out — the helper unit tests cover those arms, the evaluators don't.

github-actions · 2026-10-22T00:22:23Z

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.

M-Tesla · 2026-10-22T00:51:37Z

Still relevant. This is approved and waiting on a committer. I merged current main into the branch so it applies cleanly again.

laskoviymishka · 2026-10-23T18:19:52Z

@M-Tesla can you rebase one more time? I'll try to take some time this weekend to finish this one

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.

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

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

A valid min or max still prunes when the other bound is NaN, instead of bailing on both sides.

M-Tesla · 2026-10-23T21:43:55Z

Thanks. Rebased onto current main. The conflict in inclusive_metrics_evaluator kept any_literal_in_bounds.

The security audit fails on ctutils 0.4.2 and warns on tokio 1.53.1. Both versions are already on main. 0.4.5 and 1.53.3 clear the two advisories.

dannycjones

Thanks @M-Tesla. I have been stumbling across a few issues with the pruning in the library when working on new data types.

I had a note on one of the changes here. I'm hoping its a quick fix, and maybe we get this merged very soon.

dannycjones · 2026-10-24T18:26:49Z

+/// Returns true if any literal could match the inclusive `[lower, upper]` range.
+/// Missing bounds are treated as unbounded on that side.
+///
+/// `(None, None)` returns true because no bound is available to prune against.
+pub(crate) fn any_literal_in_bounds(
+    lower: Option<&Datum>,
+    upper: Option<&Datum>,
+    literals: &FnvHashSet<Datum>,
+) -> bool {
+    match (lower, upper) {
+        (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,
+    }
+}


I think we need to be careful here. This function doesn't handle where comparison isn't defined.

If we take the first arm: we have both an upper and lower bound defined, but we say that it is maybe in bounds if and ONLY IF the datum >= Some(lower bound) AND datum <= Some(upper bound), which is a problem if the values aren't comparable. When not comparable, those fns return false because they can't confirm that they hold.

I think instead we should have something like this, flipping the callers.

Suggested change

/// Returns true if any literal could match the inclusive `[lower, upper]` range.

/// Missing bounds are treated as unbounded on that side.

///

/// `(None, None)` returns true because no bound is available to prune against.

pub(crate) fn any_literal_in_bounds(

lower: Option<&Datum>,

upper: Option<&Datum>,

literals: &FnvHashSet<Datum>,

) -> bool {

match (lower, upper) {

(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,

}

}

/// Returns true if every literal is provably outside the inclusive `[lower, upper]`

/// range, so rows cannot match. Missing bounds are treated as unbounded on that side.

///

/// Returns `false` (cannot prune) when there are no bounds, or when a literal

/// does not compare with a bound.

pub(crate) fn all_literals_outside_bounds(

lower: Option<&Datum>,

upper: Option<&Datum>,

literals: &FnvHashSet<Datum>,

) -> bool {

match (lower, upper) {

(Some(lower), Some(upper)) => literals

.iter()

.all(|datum| datum.lt(lower) || datum.gt(upper)),

(Some(lower), None) => literals.iter().all(|datum| datum.lt(lower)),

(None, Some(upper)) => literals.iter().all(|datum| datum.gt(upper)),

(None, None) => false,

}

}

I've been encountering pain here with new data type changes which don't necessarily have comparison defined, leading to incorrect pruning. For example, where we don't currently define date to timestamp promotion (funnily enough, just opened: #3522), so the data isn't comparable. I've also had issues with geo development, although we should start blocking that before reaching here. (I like defense in depth though.)

Thanks @dannycjones. I flipped the helper to all_literals_outside_bounds, so a literal that does not compare stays on the might-match path. Ordered values still prune the same way, including the straddling case.

ge and le are false when Datum::partial_cmp is None, so any_literal_in_bounds treated those literals as outside the range. Prune only when every literal is provably below the lower bound or above the upper bound.

dannycjones

I haven't took a look at the whole PR, it was the new bound checker I was most interested in. The diff since last review LGTM: https://github.com/apache/iceberg-rust/pull/3144/changes/f188567f023489b173ae9bc5a9298cf6ad133dc2..007a8f2b565e92280e87897ec234333d75c22bab

I'll leave to Andrei to merge if he's already taken a look at the rest of the PR.

laskoviymishka · 2026-10-27T07:14:10Z

thanks @dannycjones!

I'll take another look today.

laskoviymishka

Thanks @dannycjones for the incomparable-literal catch.

With all_literals_outside_bounds, a literal that doesn't compare with a bound can't prune anymore. That also fixes a false prune main has today for a date bound against a timestamp literal.

There's one optional suggestion inline. I'll merge once it's in, or once you tell me you'd rather leave it.

laskoviymishka · 2026-10-27T14:03:01Z

+        let lower_bound = self.min_value(field_id)?;
+        let upper_bound = self.max_value(field_id)?;


Optional: could this keep the single stats_and_type_for_field_id lookup from #3254? min_value and max_value each resolve the stats again, so in now does it twice while eq does it once. Results don't change. The suggestion compiles and passes the expr::visitors tests.

Suggested change

let lower_bound = self.min_value(field_id)?;

let upper_bound = self.max_value(field_id)?;

let Some((stats, primitive_type)) = self.stats_and_type_for_field_id(field_id)? else {

return ROW_GROUP_MIGHT_MATCH;

};

let lower_bound = get_parquet_stat_min_as_datum(&primitive_type, stats)?;

let upper_bound = get_parquet_stat_max_as_datum(&primitive_type, stats)?;

Thanks :D. In now uses one stats_and_type_for_field_id lookup, then the same min and max helpers. The NaN bound handling is unchanged.

min_value and max_value each resolved the same statistics. The In path now uses one stats_and_type_for_field_id lookup and still drops a NaN bound before the prune.

laskoviymishka · 2026-10-28T09:55:19Z

Thanks @M-Tesla !

Welcome to contributors gang!

dhruvarya-db reviewed Sep 4, 2026

View reviewed changes

dhruvarya-db approved these changes Sep 5, 2026

View reviewed changes

laskoviymishka requested changes Sep 9, 2026

View reviewed changes

laskoviymishka requested changes Sep 18, 2026

View reviewed changes

laskoviymishka approved these changes Sep 21, 2026

View reviewed changes

github-actions Bot added the stale label Oct 22, 2026

github-actions Bot removed the stale label Oct 23, 2026

M-Tesla added 4 commits October 23, 2026 18:23

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

e1f8960

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

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

985feb7

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

fix(expr): treat NaN metrics bounds as unbounded for In pruning

d7cef0b

A valid min or max still prunes when the other bound is NaN, instead of bailing on both sides.

M-Tesla force-pushed the fix-3118-in-predicate-straddling-bounds branch from b9fe555 to d7cef0b Compare October 23, 2026 21:43

chore(deps): bump ctutils and tokio past rustsec advisories

f188567

The security audit fails on ctutils 0.4.2 and warns on tokio 1.53.1. Both versions are already on main. 0.4.5 and 1.53.3 clear the two advisories.

dannycjones reviewed Oct 24, 2026

View reviewed changes

fix(expr): keep incomparable In literals from pruning

007a8f2

ge and le are false when Datum::partial_cmp is None, so any_literal_in_bounds treated those literals as outside the range. Prune only when every literal is provably below the lower bound or above the upper bound.

dannycjones reviewed Oct 24, 2026

View reviewed changes

laskoviymishka approved these changes Oct 27, 2026

View reviewed changes

fix(expr): look up row group stats once for In

daeba12

min_value and max_value each resolved the same statistics. The In path now uses one stats_and_type_for_field_id lookup and still drops a NaN bound before the prune.

laskoviymishka added this pull request to the merge queue Oct 28, 2026

Merged via the queue into apache:main with commit 2135b37 Oct 28, 2026
24 checks passed

                       Ok(())
                   }
+                  #[test]

		let lower_bound = self.min_value(field_id)?;
		let upper_bound = self.max_value(field_id)?;

-        let lower_bound = self.min_value(field_id)?;
-        let upper_bound = self.max_value(field_id)?;
+        let Some((stats, primitive_type)) = self.stats_and_type_for_field_id(field_id)? else {
+            return ROW_GROUP_MIGHT_MATCH;
+        };
+        let lower_bound = get_parquet_stat_min_as_datum(&primitive_type, stats)?;
+        let upper_bound = get_parquet_stat_max_as_datum(&primitive_type, stats)?;

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!

M-Tesla Sep 4, 2026 • 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!

M-Tesla Sep 4, 2026 • 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!

M-Tesla commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

M-Tesla commented Sep 18, 2026

M-Tesla commented Sep 4, 2026 •

edited

Loading

M-Tesla Sep 4, 2026 •

edited

Loading

M-Tesla Sep 4, 2026 •

edited

Loading

M-Tesla commented Sep 4, 2026 •

edited

Loading

M-Tesla commented Sep 9, 2026 •

edited

Loading

M-Tesla Oct 27, 2026 •

edited

Loading