laskoviymishka commented on code in PR #3144:
URL: https://github.com/apache/iceberg-rust/pull/3144#discussion_r3966648546
##########
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:
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?
##########
crates/iceberg/src/expr/visitors/row_group_metrics_evaluator.rs:
##########
@@ -1808,6 +1798,41 @@ mod tests {
Ok(())
}
+ #[test]
Review Comment:
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.
##########
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 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.
##########
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:
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.
##########
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:
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?
--
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]
