From 692dd929c061afcdc49fa58844b790fb9dcbad4e Mon Sep 17 00:00:00 2001 From: Marcelo Tesla <9055877+M-Tesla@users.noreply.github.com> Date: Thu, 3 Sep 2026 22:54:01 -0300 Subject: [PATCH 1/4] fix(expr): prune In predicates that straddle metrics bounds 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. --- .../visitors/inclusive_metrics_evaluator.rs | 22 ++++++++++--- .../src/expr/visitors/manifest_evaluator.rs | 33 +++++++++++++++++-- 2 files changed, 49 insertions(+), 6 deletions(-) diff --git a/crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs b/crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs index 06c92ab3e8..b0779d1abb 100644 --- a/crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs @@ -437,14 +437,17 @@ impl BoundPredicateVisitor for InclusiveMetricsEvaluator<'_> { return ROWS_MIGHT_MATCH; } + // 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) { if lower_bound.is_nan() { // NaN indicates unreliable bounds. See the InclusiveMetricsEvaluator docs for more. return ROWS_MIGHT_MATCH; } - if !literals.iter().any(|datum| datum.ge(lower_bound)) { - // if all values are less than lower bound, rows cannot match. + filtered_literals.retain(|datum| datum.ge(lower_bound)); + if filtered_literals.is_empty() { return ROWS_CANNOT_MATCH; } } @@ -455,8 +458,8 @@ impl BoundPredicateVisitor for InclusiveMetricsEvaluator<'_> { return ROWS_MIGHT_MATCH; } - if !literals.iter().any(|datum| datum.le(upper_bound)) { - // if all values are greater than upper bound, rows cannot match. + filtered_literals.retain(|datum| datum.le(upper_bound)); + if filtered_literals.is_empty() { return ROWS_CANNOT_MATCH; } } @@ -1551,6 +1554,17 @@ 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_integer_not_in() { let result = InclusiveMetricsEvaluator::eval( diff --git a/crates/iceberg/src/expr/visitors/manifest_evaluator.rs b/crates/iceberg/src/expr/visitors/manifest_evaluator.rs index 53dbafc065..e78ca5e687 100644 --- a/crates/iceberg/src/expr/visitors/manifest_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/manifest_evaluator.rs @@ -409,12 +409,16 @@ impl BoundPredicateVisitor for ManifestFilterVisitor<'_> { return ROWS_MIGHT_MATCH; } + // Narrow the set against each bound, matching InclusiveMetricsEvaluator. + let mut filtered_literals = literals.clone(); + 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) { + filtered_literals.retain(|datum| datum >= &lower_bound); + if filtered_literals.is_empty() { return ROWS_CANNOT_MATCH; } } @@ -424,7 +428,8 @@ impl BoundPredicateVisitor for ManifestFilterVisitor<'_> { upper_bound, *reference.field().clone().field_type, ); - if literals.iter().all(|datum| &upper_bound < datum) { + filtered_literals.retain(|datum| datum <= &upper_bound); + if filtered_literals.is_empty() { return ROWS_CANNOT_MATCH; } } @@ -1375,6 +1380,30 @@ mod test { Ok(()) } + #[test] + fn test_in_straddling_bounds() -> Result<()> { + let case_sensitive = true; + let schema = create_schema()?; + let manifest_file = create_manifest_file(create_partitions()); + + let filter = Predicate::Set(SetExpression::new( + PredicateOperator::In, + Reference::new("id"), + FnvHashSet::from_iter(vec![ + Datum::int(INT_MIN_VALUE - 25), + Datum::int(INT_MAX_VALUE + 25), + ]), + )) + .bind(schema.clone(), case_sensitive)?; + assert!( + !ManifestEvaluator::builder(filter) + .build() + .eval(&manifest_file)?, + "Should not read: id in (5, 104), summary is [30, 79]" + ); + Ok(()) + } + #[test] fn test_not_in() -> Result<()> { let case_sensitive = true; From b75d924101840dd1e7fe08ebcc7137a99ebf2de7 Mon Sep 17 00:00:00 2001 From: Marcelo Tesla <9055877+M-Tesla@users.noreply.github.com> Date: Fri, 4 Sep 2026 20:26:16 -0300 Subject: [PATCH 2/4] fix(expr): check In literals against both bounds together Apply review feedback: use any() instead of clone/retain, and prune straddling In predicates in RowGroupMetricsEvaluator too. --- .../visitors/inclusive_metrics_evaluator.rs | 36 ++++------ .../src/expr/visitors/manifest_evaluator.rs | 42 ++++++----- .../visitors/row_group_metrics_evaluator.rs | 70 ++++++++++++++----- 3 files changed, 87 insertions(+), 61 deletions(-) diff --git a/crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs b/crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs index b0779d1abb..cd01461277 100644 --- a/crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs @@ -437,31 +437,25 @@ impl BoundPredicateVisitor for InclusiveMetricsEvaluator<'_> { return ROWS_MIGHT_MATCH; } - // Narrow the set against each bound, matching Java / PyIceberg. - let mut filtered_literals = literals.clone(); + let lower_bound = self.lower_bound(field_id); + let upper_bound = self.upper_bound(field_id); - if let Some(lower_bound) = self.lower_bound(field_id) { - if lower_bound.is_nan() { - // NaN indicates unreliable bounds. See the InclusiveMetricsEvaluator docs for more. - return ROWS_MIGHT_MATCH; - } - - filtered_literals.retain(|datum| datum.ge(lower_bound)); - if filtered_literals.is_empty() { - return ROWS_CANNOT_MATCH; - } + if lower_bound.is_some_and(|d| d.is_nan()) || upper_bound.is_some_and(|d| d.is_nan()) { + // NaN indicates unreliable bounds. See the InclusiveMetricsEvaluator docs for more. + return ROWS_MIGHT_MATCH; } - if let Some(upper_bound) = self.upper_bound(field_id) { - if upper_bound.is_nan() { - // NaN indicates unreliable bounds. See the InclusiveMetricsEvaluator docs for more. - 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, + }; - filtered_literals.retain(|datum| datum.le(upper_bound)); - if filtered_literals.is_empty() { - return ROWS_CANNOT_MATCH; - } + if !any_literal_in_bounds { + return ROWS_CANNOT_MATCH; } ROWS_MIGHT_MATCH diff --git a/crates/iceberg/src/expr/visitors/manifest_evaluator.rs b/crates/iceberg/src/expr/visitors/manifest_evaluator.rs index e78ca5e687..edbd89f1c4 100644 --- a/crates/iceberg/src/expr/visitors/manifest_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/manifest_evaluator.rs @@ -409,29 +409,27 @@ impl BoundPredicateVisitor for ManifestFilterVisitor<'_> { return ROWS_MIGHT_MATCH; } - // Narrow the set against each bound, matching InclusiveMetricsEvaluator. - let mut filtered_literals = literals.clone(); - - if let Some(lower_bound) = &field.lower_bound { - let lower_bound = ManifestFilterVisitor::bytes_to_datum( - lower_bound, - *reference.field().clone().field_type, - ); - filtered_literals.retain(|datum| datum >= &lower_bound); - if filtered_literals.is_empty() { - return ROWS_CANNOT_MATCH; - } - } + let field_type = *reference.field().clone().field_type; + let lower_bound = field + .lower_bound + .as_ref() + .map(|bound| ManifestFilterVisitor::bytes_to_datum(bound, field_type.clone())); + let upper_bound = field + .upper_bound + .as_ref() + .map(|bound| ManifestFilterVisitor::bytes_to_datum(bound, field_type)); + + let any_literal_in_bounds = match (&lower_bound, &upper_bound) { + (Some(lower), Some(upper)) => literals + .iter() + .any(|datum| datum >= lower && datum <= upper), + (Some(lower), None) => literals.iter().any(|datum| datum >= lower), + (None, Some(upper)) => literals.iter().any(|datum| datum <= upper), + (None, None) => true, + }; - if let Some(upper_bound) = &field.upper_bound { - let upper_bound = ManifestFilterVisitor::bytes_to_datum( - upper_bound, - *reference.field().clone().field_type, - ); - filtered_literals.retain(|datum| datum <= &upper_bound); - if filtered_literals.is_empty() { - return ROWS_CANNOT_MATCH; - } + if !any_literal_in_bounds { + return ROWS_CANNOT_MATCH; } ROWS_MIGHT_MATCH diff --git a/crates/iceberg/src/expr/visitors/row_group_metrics_evaluator.rs b/crates/iceberg/src/expr/visitors/row_group_metrics_evaluator.rs index 3a5a406e78..6b57372a34 100644 --- a/crates/iceberg/src/expr/visitors/row_group_metrics_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/row_group_metrics_evaluator.rs @@ -476,28 +476,27 @@ impl BoundPredicateVisitor for RowGroupMetricsEvaluator<'_> { return ROW_GROUP_MIGHT_MATCH; } - if let Some(lower_bound) = self.min_value(field_id)? { - if lower_bound.is_nan() { - // NaN indicates unreliable bounds. See the InclusiveMetricsEvaluator docs for more. - return ROW_GROUP_MIGHT_MATCH; - } + let lower_bound = self.min_value(field_id)?; + let upper_bound = self.max_value(field_id)?; - if !literals.iter().any(|datum| datum.ge(&lower_bound)) { - // 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()) + || upper_bound.as_ref().is_some_and(|d| d.is_nan()) + { + // NaN indicates unreliable bounds. See the InclusiveMetricsEvaluator docs for more. + return ROW_GROUP_MIGHT_MATCH; } - if let Some(upper_bound) = self.max_value(field_id)? { - if upper_bound.is_nan() { - // NaN indicates unreliable bounds. See the InclusiveMetricsEvaluator docs for more. - return ROW_GROUP_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 !literals.iter().any(|datum| datum.le(&upper_bound)) { - // if all values are greater than upper bound, rows cannot match. - return ROW_GROUP_CANT_MATCH; - } + if !any_literal_in_bounds { + return ROW_GROUP_CANT_MATCH; } ROW_GROUP_MIGHT_MATCH @@ -1808,6 +1807,41 @@ mod tests { Ok(()) } + #[test] + fn eval_false_for_literals_straddling_bounds_is_in() -> Result<()> { + // Bounds are [4.0, 6.0]; IN (2.0, 8.0) straddles the range with no + // literal inside it and must be pruned. + let row_group_metadata = create_row_group_metadata( + 1, + 1, + Some(Statistics::float( + Some(4.0), + Some(6.0), + None, + Some(0), + false, + )), + 1, + None, + )?; + + let (iceberg_schema_ref, field_id_map) = build_iceberg_schema_and_field_map()?; + + let filter = Reference::new("col_float") + .is_in([Datum::float(2.0_f32), Datum::float(8.0_f32)]) + .bind(iceberg_schema_ref.clone(), false)?; + + let result = RowGroupMetricsEvaluator::eval( + &filter, + &row_group_metadata, + &field_id_map, + iceberg_schema_ref.as_ref(), + )?; + + assert!(!result); + Ok(()) + } + #[test] fn eval_true_for_not_in() -> Result<()> { let row_group_metadata = create_row_group_metadata( From 52a92db3227f573a3a693e74df78c5880af68fad Mon Sep 17 00:00:00 2001 From: Marcelo Tesla <9055877+M-Tesla@users.noreply.github.com> Date: Fri, 4 Sep 2026 22:01:56 -0300 Subject: [PATCH 3/4] refactor(expr): share In bound check across metrics evaluators The three In evaluators used the same match. Pull it into a crate-private helper. --- .../visitors/inclusive_metrics_evaluator.rs | 11 +--------- .../src/expr/visitors/manifest_evaluator.rs | 11 +--------- crates/iceberg/src/expr/visitors/mod.rs | 21 +++++++++++++++++++ .../visitors/row_group_metrics_evaluator.rs | 11 +--------- 4 files changed, 24 insertions(+), 30 deletions(-) diff --git a/crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs b/crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs index cd01461277..1b91504638 100644 --- a/crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs @@ -445,16 +445,7 @@ impl BoundPredicateVisitor for InclusiveMetricsEvaluator<'_> { 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 { + if !super::any_literal_in_bounds(lower_bound, upper_bound, literals) { return ROWS_CANNOT_MATCH; } diff --git a/crates/iceberg/src/expr/visitors/manifest_evaluator.rs b/crates/iceberg/src/expr/visitors/manifest_evaluator.rs index edbd89f1c4..51dc4835f2 100644 --- a/crates/iceberg/src/expr/visitors/manifest_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/manifest_evaluator.rs @@ -419,16 +419,7 @@ impl BoundPredicateVisitor for ManifestFilterVisitor<'_> { .as_ref() .map(|bound| ManifestFilterVisitor::bytes_to_datum(bound, field_type)); - let any_literal_in_bounds = match (&lower_bound, &upper_bound) { - (Some(lower), Some(upper)) => literals - .iter() - .any(|datum| datum >= lower && datum <= upper), - (Some(lower), None) => literals.iter().any(|datum| datum >= lower), - (None, Some(upper)) => literals.iter().any(|datum| datum <= upper), - (None, None) => true, - }; - - if !any_literal_in_bounds { + if !super::any_literal_in_bounds(lower_bound.as_ref(), upper_bound.as_ref(), literals) { return ROWS_CANNOT_MATCH; } diff --git a/crates/iceberg/src/expr/visitors/mod.rs b/crates/iceberg/src/expr/visitors/mod.rs index 38332ea6a0..42e01554f3 100644 --- a/crates/iceberg/src/expr/visitors/mod.rs +++ b/crates/iceberg/src/expr/visitors/mod.rs @@ -15,6 +15,10 @@ // specific language governing permissions and limitations // under the License. +use fnv::FnvHashSet; + +use crate::spec::Datum; + pub(crate) mod bound_predicate_visitor; pub(crate) mod expression_evaluator; pub(crate) mod inclusive_metrics_evaluator; @@ -26,3 +30,20 @@ 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. +pub(crate) fn any_literal_in_bounds( + lower: Option<&Datum>, + upper: Option<&Datum>, + literals: &FnvHashSet, +) -> 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, + } +} diff --git a/crates/iceberg/src/expr/visitors/row_group_metrics_evaluator.rs b/crates/iceberg/src/expr/visitors/row_group_metrics_evaluator.rs index 6b57372a34..3dc170373c 100644 --- a/crates/iceberg/src/expr/visitors/row_group_metrics_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/row_group_metrics_evaluator.rs @@ -486,16 +486,7 @@ impl BoundPredicateVisitor for RowGroupMetricsEvaluator<'_> { return ROW_GROUP_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 { + if !super::any_literal_in_bounds(lower_bound.as_ref(), upper_bound.as_ref(), literals) { return ROW_GROUP_CANT_MATCH; } From d6a3b678e34b3a25a1f5bc63b3e6c287e600f719 Mon Sep 17 00:00:00 2001 From: Marcelo Tesla <9055877+M-Tesla@users.noreply.github.com> Date: Wed, 9 Sep 2026 19:23:33 -0300 Subject: [PATCH 4/4] fix(expr): treat NaN metrics bounds as unbounded for In pruning A valid min or max still prunes when the other bound is NaN, instead of bailing on both sides. --- .../visitors/inclusive_metrics_evaluator.rs | 104 +++++++++++++++++- .../src/expr/visitors/manifest_evaluator.rs | 2 +- crates/iceberg/src/expr/visitors/mod.rs | 81 ++++++++++++++ .../visitors/row_group_metrics_evaluator.rs | 58 ++++++++-- 4 files changed, 226 insertions(+), 19 deletions(-) diff --git a/crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs b/crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs index 1b91504638..a6568a00e9 100644 --- a/crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs @@ -440,12 +440,13 @@ impl BoundPredicateVisitor for InclusiveMetricsEvaluator<'_> { let lower_bound = self.lower_bound(field_id); let upper_bound = self.upper_bound(field_id); - if lower_bound.is_some_and(|d| d.is_nan()) || upper_bound.is_some_and(|d| d.is_nan()) { - // NaN indicates unreliable bounds. See the InclusiveMetricsEvaluator docs for more. - return ROWS_MIGHT_MATCH; - } - - if !super::any_literal_in_bounds(lower_bound, upper_bound, literals) { + // A NaN bound is unreliable on that side only. Drop it to unbounded + // so a valid bound can still prune. + if !super::any_literal_in_bounds( + super::finite_bound(lower_bound), + super::finite_bound(upper_bound), + literals, + ) { return ROWS_CANNOT_MATCH; } @@ -1550,6 +1551,34 @@ mod test { 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() { + let result = InclusiveMetricsEvaluator::eval( + &r#in_float("no_nans", &[2.0, 3.0]), + &get_test_file_float_nan_lower(), + true, + ) + .unwrap(); + assert!( + !result, + "Should skip: NaN lower is unbounded, both literals are above upper 1.0" + ); + } + #[test] fn test_integer_not_in() { let result = InclusiveMetricsEvaluator::eval( @@ -1879,6 +1908,16 @@ mod test { filter.bind(schema.clone(), true).unwrap() } + fn in_float(reference: &str, float_literals: &[f32]) -> BoundPredicate { + let schema = create_test_schema(); + let filter = Predicate::Set(SetExpression::new( + In, + Reference::new(reference), + FnvHashSet::from_iter(float_literals.iter().copied().map(Datum::float)), + )); + filter.bind(schema.clone(), true).unwrap() + } + fn not_in_int(reference: &str, int_literals: &[i32]) -> BoundPredicate { let schema = create_test_schema(); let filter = Predicate::Set(SetExpression::new( @@ -2092,6 +2131,59 @@ mod test { content_size_in_bytes: None, } } + + fn get_test_file_float_nan_upper() -> DataFile { + DataFile { + content: DataContentType::Data, + file_path: "/test/path".to_string(), + file_format: DataFileFormat::Parquet, + partition: Struct::empty(), + record_count: 10, + file_size_in_bytes: 10, + column_sizes: Default::default(), + value_counts: HashMap::from([(9, 10)]), + null_value_counts: HashMap::from([(9, 0)]), + nan_value_counts: HashMap::from([(9, 0)]), + lower_bounds: HashMap::from([(9, Datum::float(4.0_f32))]), + upper_bounds: HashMap::from([(9, Datum::float(f32::NAN))]), + key_metadata: None, + split_offsets: None, + equality_ids: None, + sort_order_id: None, + partition_spec_id: 0, + first_row_id: None, + referenced_data_file: None, + content_offset: None, + content_size_in_bytes: None, + } + } + + fn get_test_file_float_nan_lower() -> DataFile { + DataFile { + content: DataContentType::Data, + file_path: "/test/path".to_string(), + file_format: DataFileFormat::Parquet, + partition: Struct::empty(), + record_count: 10, + file_size_in_bytes: 10, + column_sizes: Default::default(), + value_counts: HashMap::from([(9, 10)]), + null_value_counts: HashMap::from([(9, 0)]), + nan_value_counts: HashMap::from([(9, 0)]), + lower_bounds: HashMap::from([(9, Datum::float(f32::NAN))]), + upper_bounds: HashMap::from([(9, Datum::float(1.0_f32))]), + key_metadata: None, + split_offsets: None, + equality_ids: None, + sort_order_id: None, + partition_spec_id: 0, + first_row_id: None, + referenced_data_file: None, + content_offset: None, + content_size_in_bytes: None, + } + } + fn get_test_file_2() -> DataFile { DataFile { content: DataContentType::Data, diff --git a/crates/iceberg/src/expr/visitors/manifest_evaluator.rs b/crates/iceberg/src/expr/visitors/manifest_evaluator.rs index 51dc4835f2..93cc60df83 100644 --- a/crates/iceberg/src/expr/visitors/manifest_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/manifest_evaluator.rs @@ -409,7 +409,7 @@ impl BoundPredicateVisitor for ManifestFilterVisitor<'_> { return ROWS_MIGHT_MATCH; } - let field_type = *reference.field().clone().field_type; + let field_type = *reference.field().field_type.clone(); let lower_bound = field .lower_bound .as_ref() diff --git a/crates/iceberg/src/expr/visitors/mod.rs b/crates/iceberg/src/expr/visitors/mod.rs index 42e01554f3..b9117f127b 100644 --- a/crates/iceberg/src/expr/visitors/mod.rs +++ b/crates/iceberg/src/expr/visitors/mod.rs @@ -33,6 +33,11 @@ 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>, @@ -47,3 +52,79 @@ pub(crate) fn any_literal_in_bounds( (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> { + bound.filter(|datum| !datum.is_nan()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn floats(vals: &[f32]) -> FnvHashSet { + vals.iter().copied().map(Datum::float).collect() + } + + #[test] + fn both_bounds_require_a_literal_inside_the_range() { + let lower = Datum::float(4.0_f32); + let upper = Datum::float(6.0_f32); + assert!(!any_literal_in_bounds( + Some(&lower), + Some(&upper), + &floats(&[2.0, 8.0]) + )); + assert!(any_literal_in_bounds( + Some(&lower), + Some(&upper), + &floats(&[2.0, 5.0]) + )); + } + + #[test] + fn lower_only_prunes_literals_below_the_bound() { + let lower = Datum::float(4.0_f32); + assert!(!any_literal_in_bounds( + Some(&lower), + None, + &floats(&[2.0, 3.0]) + )); + assert!(any_literal_in_bounds( + Some(&lower), + None, + &floats(&[2.0, 4.0]) + )); + } + + #[test] + fn upper_only_prunes_literals_above_the_bound() { + let upper = Datum::float(1.0_f32); + assert!(!any_literal_in_bounds( + None, + Some(&upper), + &floats(&[2.0, 3.0]) + )); + assert!(any_literal_in_bounds( + None, + Some(&upper), + &floats(&[0.5, 3.0]) + )); + } + + #[test] + fn neither_bound_cannot_prune() { + assert!(any_literal_in_bounds(None, None, &floats(&[2.0, 3.0]))); + } + + #[test] + fn finite_bound_drops_nan() { + let nan = Datum::float(f32::NAN); + let finite = Datum::float(4.0_f32); + assert!(finite_bound(Some(&nan)).is_none()); + assert_eq!(finite_bound(Some(&finite)), Some(&finite)); + assert!(finite_bound(None).is_none()); + } +} diff --git a/crates/iceberg/src/expr/visitors/row_group_metrics_evaluator.rs b/crates/iceberg/src/expr/visitors/row_group_metrics_evaluator.rs index 3dc170373c..fdf438f8d1 100644 --- a/crates/iceberg/src/expr/visitors/row_group_metrics_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/row_group_metrics_evaluator.rs @@ -479,14 +479,13 @@ impl BoundPredicateVisitor for RowGroupMetricsEvaluator<'_> { let lower_bound = self.min_value(field_id)?; let upper_bound = self.max_value(field_id)?; - if lower_bound.as_ref().is_some_and(|d| d.is_nan()) - || upper_bound.as_ref().is_some_and(|d| d.is_nan()) - { - // NaN indicates unreliable bounds. See the InclusiveMetricsEvaluator docs for more. - return ROW_GROUP_MIGHT_MATCH; - } - - if !super::any_literal_in_bounds(lower_bound.as_ref(), upper_bound.as_ref(), literals) { + // A NaN bound is unreliable on that side only. Drop it to unbounded + // so a valid bound can still prune. See InclusiveMetricsEvaluator. + if !super::any_literal_in_bounds( + super::finite_bound(lower_bound.as_ref()), + super::finite_bound(upper_bound.as_ref()), + literals, + ) { return ROW_GROUP_CANT_MATCH; } @@ -1671,9 +1670,9 @@ mod tests { } #[test] - fn eval_true_for_lower_bound_is_nan_filter_is_in() -> Result<()> { - // TODO: should this be false, since the max stat - // is lower than the min val in the set? + fn eval_false_for_lower_bound_is_nan_all_literals_above_upper_is_in() -> Result<()> { + // NaN lower is treated as unbounded. The valid upper bound (1.0) + // still prunes IN (2.0, 3.0). let row_group_metadata = create_row_group_metadata( 1, 1, @@ -1701,7 +1700,7 @@ mod tests { iceberg_schema_ref.as_ref(), )?; - assert!(result); + assert!(!result); Ok(()) } @@ -1765,6 +1764,41 @@ mod tests { Ok(()) } + #[test] + fn eval_false_for_nan_upper_bound_all_literals_below_lower_is_in() -> Result<()> { + // NaN upper is treated as unbounded. The valid lower bound (4.0) + // still prunes IN (2.0, 3.0). + let row_group_metadata = create_row_group_metadata( + 1, + 1, + Some(Statistics::float( + Some(4.0), + Some(f32::NAN), + None, + Some(0), + false, + )), + 1, + None, + )?; + + let (iceberg_schema_ref, field_id_map) = build_iceberg_schema_and_field_map()?; + + let filter = Reference::new("col_float") + .is_in([Datum::float(2.0_f32), Datum::float(3.0_f32)]) + .bind(iceberg_schema_ref.clone(), false)?; + + let result = RowGroupMetricsEvaluator::eval( + &filter, + &row_group_metadata, + &field_id_map, + iceberg_schema_ref.as_ref(), + )?; + + assert!(!result); + Ok(()) + } + #[test] fn eval_false_for_upper_bound_below_all_vals_is_in() -> Result<()> { let row_group_metadata = create_row_group_metadata(