laskoviymishka commented on code in PR #3144:
URL: https://github.com/apache/iceberg-rust/pull/3144#discussion_r4047013263
##########
crates/iceberg/src/expr/visitors/manifest_evaluator.rs:
##########
@@ -409,24 +409,18 @@ impl BoundPredicateVisitor for ManifestFilterVisitor<'_> {
return ROWS_MIGHT_MATCH;
}
- if let Some(lower_bound) = &field.lower_bound {
- let lower_bound = ManifestFilterVisitor::bytes_to_datum(
- lower_bound,
- *reference.field().clone().field_type,
- );
- if literals.iter().all(|datum| &lower_bound > datum) {
- return ROWS_CANNOT_MATCH;
- }
- }
-
- if let Some(upper_bound) = &field.upper_bound {
- let upper_bound = ManifestFilterVisitor::bytes_to_datum(
- upper_bound,
- *reference.field().clone().field_type,
- );
- if literals.iter().all(|datum| &upper_bound < datum) {
- return ROWS_CANNOT_MATCH;
- }
+ let field_type = *reference.field().field_type.clone();
Review Comment:
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.
##########
crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs:
##########
@@ -1551,6 +1540,45 @@ mod test {
);
}
+ #[test]
+ fn test_integer_in_straddling_bounds() {
+ let result = InclusiveMetricsEvaluator::eval(
+ &r#in_int("id", &[INT_MIN_VALUE - 25, INT_MAX_VALUE + 25]),
+ &get_test_file_1(),
+ true,
+ )
+ .unwrap();
+ assert!(!result, "Should skip: id in (5, 104), bounds are [30, 79]");
+ }
+
+ #[test]
+ fn test_float_in_nan_upper_bound_prunes_below_lower() {
+ let result = InclusiveMetricsEvaluator::eval(
+ &r#in_float("no_nans", &[2.0, 3.0]),
+ &get_test_file_float_nan_upper(),
+ true,
+ )
+ .unwrap();
+ assert!(
+ !result,
+ "Should skip: NaN upper is unbounded, both literals are below
lower 4.0"
+ );
+ }
+
+ #[test]
+ fn test_float_in_nan_lower_bound_prunes_above_upper() {
Review Comment:
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.
##########
crates/iceberg/src/expr/visitors/mod.rs:
##########
@@ -26,3 +30,101 @@ pub(crate) mod rewrite_not;
pub(crate) mod row_group_metrics_evaluator;
pub(crate) mod strict_metrics_evaluator;
pub(crate) mod strict_projection;
+
+/// 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.
+/// Manifest evaluation must not reach this helper when the partition summary
+/// has no lower bound: that case is all-null and `IN` prunes before calling
+/// here. Metrics evaluators use `(None, None)` for a missing min/max pair.
+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,
+ }
+}
+
+/// 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> {
Review Comment:
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?
##########
crates/iceberg/src/expr/visitors/mod.rs:
##########
@@ -26,3 +30,101 @@ pub(crate) mod rewrite_not;
pub(crate) mod row_group_metrics_evaluator;
pub(crate) mod strict_metrics_evaluator;
pub(crate) mod strict_projection;
+
+/// 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.
+/// Manifest evaluation must not reach this helper when the partition summary
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
