mbutrovich commented on code in PR #3302:
URL: https://github.com/apache/iceberg-rust/pull/3302#discussion_r4298006612
##########
crates/iceberg/src/arrow/reader/predicate_visitor.rs:
##########
@@ -195,6 +197,236 @@ impl BoundPredicateVisitor for CollectFieldIdVisitor {
}
}
+/// Returns the residual of `predicate` for one data file: each leaf on a
top-level field that is
+/// missing from the file becomes `AlwaysTrue` or `AlwaysFalse`, based on the
value projection
+/// returns for that field. That value is the identity partition value,
otherwise the field's
+/// `initial-default`, and every row of the file holds it. Leaves on missing
fields with neither
+/// keep the null handling of the row filter and the page index evaluator.
+///
+/// Leaves on nested fields are kept, because a nested field also reads as
null in any row where
+/// an ancestor struct is null.
+pub(super) fn residual_for_missing_fields(
+ predicate: BoundPredicate,
+ predicate_field_ids: &HashSet<i32>,
+ field_id_map: &HashMap<i32, usize>,
+ schema: &Schema,
+ partition_spec: Option<&PartitionSpec>,
+ partition: Option<&Struct>,
+) -> Result<BoundPredicate> {
+ if predicate_field_ids
+ .iter()
+ .all(|id| field_id_map.contains_key(id))
+ {
+ return Ok(predicate);
+ }
+
+ let partition_constants = match (partition_spec, partition) {
+ (Some(spec), Some(data)) => constants_map(spec, data, schema)?,
+ _ => HashMap::new(),
+ };
+
+ let mut constant_field_ids = HashSet::new();
Review Comment:
> Nit: this set holds at most `predicate_field_ids.len()` entries, so
`HashSet::with_capacity(predicate_field_ids.len())` bounds it.
I'd like to keep `HashSet::new()`, because [it doesn't allocate until the
first
insert](https://doc.rust-lang.org/std/collections/struct.HashSet.html#method.new)
and `with_capacity` allocates up front. A common case here is a column added
without a default. In that case no missing field has a value, and we [return
early](https://github.com/apache/iceberg-rust/blob/006d4b9a648b49e7ffbed59a2d2da9c8f425bd70/crates/iceberg/src/arrow/reader/predicate_visitor.rs#L249-L251)
with the set still empty, so `with_capacity` would allocate for nothing. When
a field does have a value, the first allocation from `new()` holds three
entries, which covers most predicates.
##########
crates/iceberg/src/expr/visitors/expression_evaluator.rs:
##########
@@ -54,14 +54,14 @@ impl ExpressionEvaluator {
/// evaluation logic to different parts of a data structure,
/// specifically for data file partitions.
#[derive(Debug)]
-struct ExpressionEvaluatorVisitor<'a> {
+pub(crate) struct ExpressionEvaluatorVisitor<'a> {
/// Reference to a [`DataFile`]'s partition [`Struct`].
partition: &'a Struct,
Review Comment:
> Nit: the struct doc and this field name still say "partition", but the
residual now passes a row of top-level field values.
I renamed the field to `row` and rewrote the struct doc. The docs on
`ExpressionEvaluator` still say partition, since that is what it evaluates.
--
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]
