brgr-s commented on code in PR #2936:
URL: https://github.com/apache/iceberg-rust/pull/2936#discussion_r3764758982
##########
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 {
+ data_file_seq_num
+ .map(|seq| delete_seq_num >= Some(seq))
+ .unwrap_or(true)
+}
+
impl PopulatedDeleteFileIndex {
/// Creates a new populated delete file index from a list of delete file
contexts, which
/// allows for fast lookup when determining which delete files apply to a
given data file.
///
/// 1. The partition information is extracted from each delete file's
manifest entry.
/// 2. If the partition is empty and the delete file is not a positional
delete,
/// it is added to the `global_equality_deletes` vector
- /// 3. Otherwise, the delete file is added to one of two hash maps based
on its content type.
+ /// 3. A positional delete that names a single data file is keyed by that
path.
+ /// 4. Any other delete file is keyed by partition, in the map for its
content type.
fn new(files: Vec<DeleteFileContext>) -> PopulatedDeleteFileIndex {
- let mut eq_deletes_by_partition: HashMap<Struct,
Vec<Arc<DeleteFileContext>>> =
- HashMap::default();
- let mut pos_deletes_by_partition: HashMap<Struct,
Vec<Arc<DeleteFileContext>>> =
+ let mut eq_deletes_by_partition: HashMap<
+ i32,
+ HashMap<Struct, Vec<Arc<DeleteFileContext>>>,
+ > = HashMap::default();
+ let mut pos_deletes_by_partition: HashMap<
+ i32,
+ HashMap<Struct, Vec<Arc<DeleteFileContext>>>,
+ > = HashMap::default();
+ let mut pos_deletes_by_path: HashMap<String,
Vec<Arc<DeleteFileContext>>> =
HashMap::default();
let mut global_equality_deletes: Vec<Arc<DeleteFileContext>> = vec![];
files.into_iter().for_each(|ctx| {
let arc_ctx = Arc::new(ctx);
+ if arc_ctx.manifest_entry.sequence_number().is_none() {
+ tracing::warn!(
Review Comment:
This line is basically the pre-existing behaviour, but with an added
warning. I added that behaviour back in my last commit, the original proposal
was different:
```
match (delete_seq_num, data_file_seq_num) {
(Some(delete_seq_num), Some(data_file_seq_num)) => delete_seq_num >=
data_file_seq_num,
_ => true,
}
```
So no failure or warning, the pos delete file just gets an "applicable"
state. In my opinion this is safe: the entries in a delete file key each line
to a file and a row. If that row exists in that file, it probably should be
deleted. If it doesn't exist, nothing happens.
However, a missing sequence number is a spec violation, I think, so failing
hard is an option. It boils down to an implementation problem: `new` is called
in a spawned thread with no error path, so we'd have to wire the failure state
through. Considering this, a seperate issue is called for, but the question
remains which behaviour is wanted for this PR:
- old, but risk ressurecting rows
- new, probably "more correct", but it is a behaviour change that might be
uncalled for in a "performance PR"
--
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]
