sdd commented on code in PR #367:
URL: https://github.com/apache/iceberg-rust/pull/367#discussion_r1597946439
##########
crates/iceberg/src/expr/visitors/manifest_evaluator.rs:
##########
@@ -103,98 +106,245 @@ impl BoundPredicateVisitor for ManifestFilterVisitor<'_>
{
reference: &BoundReference,
_predicate: &BoundPredicate,
) -> crate::Result<bool> {
- todo!()
+ let field: &FieldSummary = self.field_summary_for_reference(reference);
+
+ // contains_null encodes whether at least one partition value is null,
+ // lowerBound is null if all partition values are null
+ if self.are_all_null(field, &reference.field().field_type) {
+ ROWS_CANNOT_MATCH
+ } else {
+ ROWS_MIGHT_MATCH
+ }
}
+ #[allow(clippy::bool_comparison)]
fn is_nan(
&mut self,
reference: &BoundReference,
_predicate: &BoundPredicate,
) -> crate::Result<bool> {
- Ok(self
- .field_summary_for_reference(reference)
- .contains_nan
- .is_some())
+ let field: &FieldSummary = self.field_summary_for_reference(reference);
+ if field.contains_nan.is_some_and(|x| x == false) {
+ return ROWS_CANNOT_MATCH;
+ }
+
+ if self.are_all_null(field, &reference.field().field_type) {
+ return ROWS_CANNOT_MATCH;
+ }
+
+ ROWS_MIGHT_MATCH
}
+ #[allow(clippy::bool_comparison)]
fn not_nan(
&mut self,
reference: &BoundReference,
_predicate: &BoundPredicate,
) -> crate::Result<bool> {
- todo!()
+ let field: &FieldSummary = self.field_summary_for_reference(reference);
+ if field.contains_nan.is_some_and(|x| x == true)
+ && !field.contains_null
+ && field.lower_bound.is_none()
+ {
+ ROWS_CANNOT_MATCH
+ } else {
+ ROWS_MIGHT_MATCH
+ }
}
fn less_than(
&mut self,
reference: &BoundReference,
- literal: &Datum,
+ datum: &Datum,
_predicate: &BoundPredicate,
) -> crate::Result<bool> {
- todo!()
+ let field: &FieldSummary = self.field_summary_for_reference(reference);
+ if let Some(Literal::Primitive(lower_bound)) = &field.lower_bound {
+ Ok(lower_bound < datum.literal())
Review Comment:
It's a bit inconsistent to return `ROWS_MIGHT_MATCH` / `ROWS_CANNOT_MATCH`
in most places, but then in some places to return in this style. To improve
readability and consistency, could you refactor this, the other inequality
methods, and `eq` to return `ROWS_MIGHT_MATCH` / `ROWS_CANNOT_MATCH` rather
than `Ok(...)` please? I know that adds a few extra lines but it improves
readability.
--
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]
