M-Tesla commented on code in PR #3144:
URL: https://github.com/apache/iceberg-rust/pull/3144#discussion_r3973611244
##########
crates/iceberg/src/expr/visitors/row_group_metrics_evaluator.rs:
##########
@@ -476,28 +476,18 @@ 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())
Review Comment:
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.
##########
crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs:
##########
@@ -437,28 +437,16 @@ impl BoundPredicateVisitor for
InclusiveMetricsEvaluator<'_> {
return ROWS_MIGHT_MATCH;
}
- 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;
- }
+ let lower_bound = self.lower_bound(field_id);
+ let upper_bound = self.upper_bound(field_id);
- if !literals.iter().any(|datum| datum.ge(lower_bound)) {
- // 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()) {
Review Comment:
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.
##########
crates/iceberg/src/expr/visitors/row_group_metrics_evaluator.rs:
##########
@@ -1808,6 +1798,41 @@ mod tests {
Ok(())
}
+ #[test]
Review Comment:
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.
##########
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().clone().field_type;
Review Comment:
Good catch. Switched to `*reference.field().field_type.clone()` so we only
clone the boxed type.
##########
crates/iceberg/src/expr/visitors/mod.rs:
##########
@@ -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<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,
Review Comment:
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.
--
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]
