mbutrovich commented on code in PR #3302:
URL: https://github.com/apache/iceberg-rust/pull/3302#discussion_r4188559901
##########
crates/iceberg/src/arrow/reader/predicate_visitor.rs:
##########
@@ -195,6 +197,238 @@ 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 field_ids = HashSet::new();
+ let row: Struct = schema
+ .as_struct()
+ .fields()
+ .iter()
+ .map(|field| {
+ if !predicate_field_ids.contains(&field.id) ||
field_id_map.contains_key(&field.id) {
+ return None;
+ }
+ let value = match partition_constants.get(&field.id) {
+ Some(datum) =>
Some(Literal::Primitive(datum.literal().clone())),
+ None => field.initial_default.clone(),
+ };
+ if value.is_some() {
Review Comment:
I don't think they're intended. I checked on this branch with an optional
`b` that has no default. For a file that doesn't store `b`, `b < 5` and `b <=
5` keep all 3 rows, and `b > 5`, `b >= 5`, `b = 5`, and `b != 5` keep none. For
a file that stores `b` as 3 nulls, `b < 5` and `b <= 5` keep none. So the
result depends on whether the nulls are stored or come from a missing column.
The branches date back to #295, and I don't know why `<` and `<=` differ there.
Java's row-level `Evaluator` compares with a [nulls-first
comparator](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/api/src/main/java/org/apache/iceberg/expressions/Literals.java#L160-L174),
so
[`lt`](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/api/src/main/java/org/apache/iceberg/expressions/Evaluator.java#L105-L114)
is true for a null value there. That could be the source, but
[`testColumnNotInFile`](https://github.com/apache/iceberg/blob/5e7169168db3d34e
29354c6f59ec4d6e420b8d2d/data/src/test/java/org/apache/iceberg/data/TestMetricsRowGroupFilter.java#L445-L473)
expects the Parquet row group filter to skip the file for both. I opened #3355
for it.
--
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]
