laskoviymishka commented on code in PR #3453:
URL: https://github.com/apache/iceberg-rust/pull/3453#discussion_r4399296638
##########
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:
Should this return an error when an equality id doesn't resolve in the scan
schema? As I read it, an unmatched id is filtered out silently, top-level ones
too, e.g. an eq-delete column dropped while the delete file is still live. If
every id drops, the batch has no columns and the delete becomes
[`AlwaysTrue`](https://github.com/apache/iceberg-rust/blob/1fc76289a6de43f2b96510305de925278c1fe090/crates/iceberg/src/arrow/caching_delete_file_loader.rs#L593-L595).
If only some drop, rows match on fewer columns and we over-delete.
`main` fails with [`field not
found`](https://github.com/apache/iceberg-rust/blob/d2679a029f50a25f00850731650d7fbab1b03ca8/crates/iceberg/src/arrow/record_batch_transformer.rs#L887-L891),
and Java's
[`DeleteFilter`](https://github.com/apache/iceberg/blob/apache-iceberg-1.9.0/data/src/main/java/org/apache/iceberg/data/DeleteFilter.java)
fails with `Cannot find required field for ID`. Could we keep the error and
add a missing-id test?
##########
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:
Does a delete file from our own `EqualityDeleteWriter` still load correctly
through this projection? For `s.f` the writer writes a flat top-level `f` with
id 3
([record_batch_projector.rs#L72-L75](https://github.com/apache/iceberg-rust/blob/1fc76289a6de43f2b96510305de925278c1fe090/crates/iceberg/src/arrow/record_batch_projector.rs#L72-L75)),
while this asks the file for id 2.
Could we add a `load_deletes`-level test that writes the delete with
`EqualityDeleteWriter`? The current
[test](https://github.com/apache/iceberg-rust/blob/1fc76289a6de43f2b96510305de925278c1fe090/crates/iceberg/src/arrow/caching_delete_file_loader.rs#L1857-L1872)
hardcodes `&[2]` and calls `evolve_schema` directly, so neither call site sees
a nested id. Checking the bound field id directly would also hold up better
than matching the `Debug` string.
--
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]
