brgr-s commented on code in PR #2936:
URL: https://github.com/apache/iceberg-rust/pull/2936#discussion_r3812005757
##########
crates/iceberg/src/delete_file_index.rs:
##########
@@ -113,25 +115,74 @@ impl DeleteFileIndex {
}
}
+/// The single data file a position delete file applies to, `None` if it is
not tied
+/// to a single data file.
+fn position_delete_target(data_file: &DataFile) -> Option<String> {
+ // data files is named directly
+ if let Some(path) = data_file.referenced_data_file() {
+ return Some(path);
+ }
+
+ // lower and upper bound of reserved field are equal, so all rows
+ // have the same value
+ let lower = data_file
+ .lower_bounds()
+ .get(&RESERVED_FIELD_ID_DELETE_FILE_PATH)?;
+ let upper = data_file
+ .upper_bounds()
+ .get(&RESERVED_FIELD_ID_DELETE_FILE_PATH)?;
+ if lower != upper {
+ return None;
+ }
+
+ match lower.literal() {
+ PrimitiveLiteral::String(path) => Some(path.clone()),
+ _ => None,
+ }
+}
+
+/// Whether a position delete file's sequence number lets it apply to a data
file whose
+/// own sequence number is `data_file_seq_num`.
+fn position_delete_applies(delete_seq_num: Option<i64>, data_file_seq_num:
Option<i64>) -> bool {
Review Comment:
Fair point, but I'd like to argue that the function actually is just an
extraction of the current behaviour on main:
```rust
seq_num.map(|seq_num| delete.manifest_entry.sequence_number() >=
Some(seq_num))
.unwrap_or_else(|| true)
```
Regarding how this could even be a problem:
- `ManifestEntry::inherit_data` resolves the seq number and runs on every
read path via `ManifestFile::load_manifest`
- It is filled unconditionally when `status == ADDED ||
snapshot_entry.sequence_number == INITIAL_SEQUENCE_NUMBER`
- Via `process_data_manifest_entry`, only `ADDED` and `EXISTING` entries
reach delete index
I think there is only one case left: v2+ table with an `EXISTING` delete
file manifest whose writer omitted sequence number.
That is a spec violation in the manifest layer, and I'd argue this needs to
be resolved there. I don't think this layer should have a `ManifestEntry` just
for this.
This is the issue @xanderbailey offered to file. I think fixing this in this
PR would convolute it, because this is a performance PR.
If it is the agreement that it needs to be done here, I'd be happy to do 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]
