c-thiel commented on code in PR #2936:
URL: https://github.com/apache/iceberg-rust/pull/2936#discussion_r3727441522
##########
crates/iceberg/src/delete_file_index.rs:
##########
@@ -144,23 +188,50 @@ impl PopulatedDeleteFileIndex {
}
let destination_map = match arc_ctx.manifest_entry.content_type() {
- DataContentType::PositionDeletes => &mut
pos_deletes_by_partition,
+ DataContentType::PositionDeletes => {
+ if let Some(path) =
position_delete_target(arc_ctx.manifest_entry.data_file()) {
+ pos_deletes_by_path
+ .entry(path)
+ .or_default()
+ .push(arc_ctx.clone());
+ return;
+ }
+ &mut pos_deletes_by_partition
+ }
DataContentType::EqualityDeletes => &mut
eq_deletes_by_partition,
_ => unreachable!(),
};
destination_map
- .entry(partition.clone())
+ .entry((arc_ctx.partition_spec_id, partition.clone()))
.and_modify(|entry| {
entry.push(arc_ctx.clone());
})
.or_insert(vec![arc_ctx.clone()]);
Review Comment:
`or_insert(vec![...])` allocates on every iteration
`or_insert`'s argument is eager, so `vec![arc_ctx.clone()]` allocates a
`Vec` and bumps the `Arc` refcount on every delete file, including the
overwhelmingly common case where the entry already exists and the vec is thrown
away. `.or_default().push(arc_ctx)` avoids both, and `arc_ctx` can be moved
rather than cloned since it's unused afterwards. Same applies at line 196
##########
crates/iceberg/src/delete_file_index.rs:
##########
@@ -122,16 +156,26 @@ impl PopulatedDeleteFileIndex {
/// 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.
Review Comment:
Stale doc comment - "the delete file is added to one of two hash maps based
on its content type" — there are three now, and pos deletes are routed by path
before content type is consulted. This PR is what invalidated it.
##########
crates/iceberg/src/delete_file_index.rs:
##########
@@ -42,11 +43,10 @@ enum DeleteFileIndexState {
#[derive(Debug)]
struct PopulatedDeleteFileIndex {
global_equality_deletes: Vec<Arc<DeleteFileContext>>,
- eq_deletes_by_partition: HashMap<Struct, Vec<Arc<DeleteFileContext>>>,
- pos_deletes_by_partition: HashMap<Struct, Vec<Arc<DeleteFileContext>>>,
- // TODO: do we need this?
- // pos_deletes_by_path: HashMap<String, Vec<Arc<DeleteFileContext>>>,
-
+ /// eq and pos deletes keyed by (partition spec id, partition value):
Review Comment:
the doc comment describes both partition maps but rustdoc attaches it only
to eq_deletes_by_partition. Use a plain // comment above both, or one /// each.
The trailing : reads as truncated.
##########
crates/iceberg/src/delete_file_index.rs:
##########
@@ -182,32 +253,34 @@ impl PopulatedDeleteFileIndex {
})
.for_each(|delete| results.push(delete.as_ref().into()));
- if let Some(deletes) =
self.eq_deletes_by_partition.get(data_file.partition()) {
+ let partition_key = (data_file.partition_spec_id,
data_file.partition().clone());
Review Comment:
This clone is a small performance regression. I tested this (same structure
as Java):
```rust
eq_deletes_by_partition: HashMap<i32, HashMap<Struct,
Vec<Arc<DeleteFileContext>>>>,
// lookup:
self.eq_deletes_by_partition
.get(&data_file.partition_spec_id)
.and_then(|by_partition| by_partition.get(data_file.partition()))
```
which was consistently slightly faster.
##########
crates/iceberg/src/delete_file_index.rs:
##########
@@ -122,16 +156,26 @@ impl PopulatedDeleteFileIndex {
/// 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.
fn new(files: Vec<DeleteFileContext>) -> PopulatedDeleteFileIndex {
- let mut eq_deletes_by_partition: HashMap<Struct,
Vec<Arc<DeleteFileContext>>> =
+ let mut eq_deletes_by_partition: HashMap<(i32, Struct),
Vec<Arc<DeleteFileContext>>> =
+ HashMap::default();
+ let mut pos_deletes_by_partition: HashMap<(i32, Struct),
Vec<Arc<DeleteFileContext>>> =
HashMap::default();
- let mut pos_deletes_by_partition: HashMap<Struct,
Vec<Arc<DeleteFileContext>>> =
+ 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!(
+ delete_file =
arc_ctx.manifest_entry.data_file().file_path(),
+ status = ?arc_ctx.manifest_entry.status(),
+ "delete file manifest entry has no data sequence number.
it will not be applied to any data file"
Review Comment:
"it will not be applied to any data file" is false when the data file's
sequence number is also None: both position_delete_applies(None, None) and the
eq-delete filter hit unwrap_or(true) and the delete is applied. Suggest "…will
be skipped for data files with a known sequence number".
```suggestion
"delete file manifest entry has no data sequence number.
It will be skipped for data files with a known sequence number"
```
##########
crates/iceberg/src/delete_file_index.rs:
##########
@@ -113,6 +113,40 @@ impl DeleteFileIndex {
}
}
+/// The single data file a position delete file applies to, `None` if it not
tied
Review Comment:
```suggestion
/// The single data file a position delete file applies to, `None` if it is
not tied
```
--
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]
