JeonDaehong commented on code in PR #3302:
URL: https://github.com/apache/iceberg-rust/pull/3302#discussion_r4300877441
##########
crates/iceberg/src/arrow/reader/predicate_visitor.rs:
##########
@@ -195,6 +197,251 @@ 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. The
resulting constants
+/// are folded out of `AND` and `OR`.
+///
+/// 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();
+ 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(),
Review Comment:
A null identity partition value falls through to `initial-default` here,
because `constants_map` skips nulls. From reading Java's code, Java keeps the
null: `PartitionUtil.constantsMap` puts it in the map, and
`ParquetValueReaders` checks `idToConstant.containsKey(id)` before anything
else, so the column reads as null. On this branch, with a null partition value
and `initial-default` 7, the column reads as 7 and `b = 7` keeps every row. The
residual matches the projection, so the two paths agree and the difference
comes from `constants_map`. Probably a separate issue, since it changes
projection too.
##########
crates/iceberg/src/arrow/reader/predicate_visitor.rs:
##########
@@ -195,6 +197,251 @@ 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. The
resulting constants
+/// are folded out of `AND` and `OR`.
+///
+/// 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)?,
Review Comment:
An identity partition value written before a type promotion doesn't match
here. The manifest is decoded with its own schema, so after `b` is promoted
from int to long, `task.partition()` holds `Literal::int(7)` and
`constants_map` builds `Datum::new(Long, Int(7))`. `Datum` compares type and
literal together, so `b = 7` and `b > 5` fold to `AlwaysFalse` while the
projected column shows 7, and an equality delete on `b = 7` keeps every row.
I checked this with a reader test like
`test_predicate_on_absent_identity_partition_column_uses_partition_value`,
using partition `Literal::int(7)`: `b = 7` returns 0 rows, and 3 rows remain
after the delete (0 with `Literal::long(7)`). `main` gives the same results.
Converting the value in `constants_map` with `Datum::to`, which handles int to
long and float to double, would fix both this and the projection path. Fine as
a follow-up if you'd rather keep this PR focused.
--
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]
