xanderbailey commented on code in PR #2936:
URL: https://github.com/apache/iceberg-rust/pull/2936#discussion_r3768689141
##########
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:
Yeah I would typically lean on the side of Postel’s Law: “Be conservative in
what you send, be liberal in what you accept" BUT in this case I do find the
spec violation a little scary and susceptible to correctness issues.
+1 that this is existing behaviour and can be made as a follow up. Would you
be able to create an issue so we can track? I'm happy to take it if you're busy
or review if you want to give it a shot.
--
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]
