laskoviymishka commented on code in PR #3144:
URL: https://github.com/apache/iceberg-rust/pull/3144#discussion_r4064152243
##########
crates/iceberg/src/expr/visitors/manifest_evaluator.rs:
##########
@@ -409,20 +412,15 @@ 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().field_type);
- if literals.iter().all(|datum| &lower_bound > datum) {
- return ROWS_CANNOT_MATCH;
- }
- }
+ let lower_bound = field.lower_bound.as_ref().map(|bound| {
+ ManifestFilterVisitor::bytes_to_datum(bound,
reference.field().field_type.as_ref())
+ });
+ let upper_bound = field.upper_bound.as_ref().map(|bound| {
+ ManifestFilterVisitor::bytes_to_datum(bound,
reference.field().field_type.as_ref())
+ });
- if let Some(upper_bound) = &field.upper_bound {
- let upper_bound =
- ManifestFilterVisitor::bytes_to_datum(upper_bound,
&reference.field().field_type);
- if literals.iter().all(|datum| &upper_bound < datum) {
- return ROWS_CANNOT_MATCH;
- }
+ if !super::any_literal_in_bounds(lower_bound.as_ref(),
upper_bound.as_ref(), literals) {
Review Comment:
Optional, non-blocking: this is the one of the three evaluators that doesn't
wrap its bounds in `finite_bound`, and that's correct — partition-summary
bounds are the min/max of non-NaN values per spec, so there's no NaN to drop
here.
Sitting right next to the other two call sites though, the omission reads
like an oversight, and the next person is going to either file a bug or add a
spurious `finite_bound`. A one-liner — something like `// summary bounds are
finite per spec; NaN is tracked separately via contains_nan` — closes that off.
wdyt?
##########
crates/iceberg/src/expr/visitors/mod.rs:
##########
@@ -27,3 +31,102 @@ 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.
+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.
+/// Inclusive evaluators in Java and PyIceberg bail to might-match when
+/// either bound is NaN. Dropping only the NaN side is a deliberate
+/// divergence: `total_cmp` treats NaN as the maximum, so the remaining
Review Comment:
Optional doc polish — the `total_cmp` line explains why NaN sorts to the
max, but the load-bearing assumption is really that the *retained* bound is
trustworthy. Worth a sentence naming that: we drop the NaN side and trust the
finite one, which is sound as long as the writer's non-NaN bound is itself
reliable. Keeps a future reader from seeing the NaN min and assuming the whole
stat block is poisoned. Purely a comment tweak, not a gate.
##########
crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs:
##########
@@ -2093,6 +2145,85 @@ mod test {
content_size_in_bytes: None,
}
}
+
+ fn get_test_file_float_nan_upper() -> DataFile {
Review Comment:
Two optional test follow-ups, neither blocking. These NaN-bound fixtures
pair a NaN bound with `nan_value_counts: 0` — a deliberately impossible "broken
writer" state, which is exactly the case worth exercising, but a one-line
comment saying so would stop a future reader from treating it as a valid
fixture. And the NaN coverage here is all `f32`/Float; since `finite_bound`
also handles Double, a double-typed case (plus a both-bounds-NaN case) at the
evaluator level would round it out — the helper unit tests cover those arms,
the evaluators don't.
--
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]
