JeonDaehong commented on code in PR #3453:
URL: https://github.com/apache/iceberg-rust/pull/3453#discussion_r4400374921
##########
crates/iceberg/src/arrow/caching_delete_file_loader.rs:
##########
@@ -302,8 +302,9 @@ impl CachingDeleteFileLoader {
del_filter.insert_equality_delete(task.file_path(), receiver);
// Per the Iceberg spec, evolve schema for equality deletes
but only for the
- // equality_ids columns, not all table columns.
+ // columns holding the equality_ids, not all table columns.
let equality_ids_vec = task.equality_ids().unwrap().to_vec();
+ let projection = equality_delete_projection(&schema,
&equality_ids_vec);
Review Comment:
good catch, it didn't. EqualityDeleteWriter writes s.f as a top-level f (id
3), so asking the file for id 2 found nothing and s came back null. the
predicate ended up as `s.f IS NOT NULL`, which deletes the rows where s.f is
null and keeps s.f = 7.
43d3169 reads the file as the scan schema pruned to the equality fields,
like Java's DeleteFilter, and moves a top-level column whose id is nested back
under its parent struct first, so both layouts load the same way.
test_nested_field_equality_delete_from_equality_delete_writer writes the file
with EqualityDeleteWriter and goes through load_deletes; without the re-nesting
it fails with that IS NOT NULL predicate. the tests now compare the bound
predicate and its field ids instead of the Debug string.
##########
crates/iceberg/src/arrow/delete_file_loader.rs:
##########
@@ -115,6 +115,30 @@ impl BasicDeleteFileLoader {
}
}
+/// Returns the top-level field ids to read for an equality delete file.
+///
+/// Equality ids may name fields nested in structs, but schema evolution of a
delete file works on
+/// top-level fields, so each id is replaced by the top-level field that
contains it.
+pub(crate) fn equality_delete_projection(schema: &Schema, equality_ids:
&[i32]) -> Vec<i32> {
+ use crate::spec::{NestedField, Type};
+
+ fn contains(field: &NestedField, id: i32) -> bool {
+ field.id == id
+ || match field.field_type.as_ref() {
+ Type::Struct(s) => s.fields().iter().any(|child|
contains(child, id)),
+ _ => false,
+ }
+ }
+
+ schema
+ .as_struct()
+ .fields()
+ .iter()
+ .filter(|field| equality_ids.iter().any(|&id| contains(field, id)))
Review Comment:
yes, an id missing from the scan schema now fails with DataInvalid naming
the id, before the file is read.
test_equality_delete_with_id_missing_from_schema_fails covers it through
load_deletes.
--
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]
