M-Tesla commented on code in PR #3144:
URL: https://github.com/apache/iceberg-rust/pull/3144#discussion_r4050846530
##########
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:
Merged main and switched both calls to
`reference.field().field_type.as_ref()`. The local and the clones are gone.
##########
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:
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.
##########
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:
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.
##########
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:
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.
--
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]
