linhongyu510 commented on code in PR #3145:
URL: https://github.com/apache/iceberg-rust/pull/3145#discussion_r4054381287
##########
crates/iceberg/src/delete_file_index.rs:
##########
@@ -214,31 +214,9 @@ impl PopulatedDeleteFileIndex {
// A deletion vector is a position delete stored as a
Puffin blob. The file
// format is what distinguishes it from a position delete
parquet file.
if data_file.file_format() == DataFileFormat::Puffin {
- // The spec requires referenced_data_file,
content_offset and
- // content_size_in_bytes on a deletion vector, so a
missing one is a
- // malformed manifest entry, not an ordinary position
delete to fall back
- // on.
- let Some(path) = data_file.referenced_data_file() else
{
- return Err(Error::new(
- ErrorKind::DataInvalid,
- format!(
- "deletion vector {} is missing
referenced_data_file",
- arc_ctx.manifest_entry.file_path()
- ),
- ));
- };
-
- if data_file.content_offset().is_none()
- || data_file.content_size_in_bytes().is_none()
- {
- return Err(Error::new(
- ErrorKind::DataInvalid,
- format!(
- "deletion vector {} is missing
content_offset or content_size_in_bytes",
- arc_ctx.manifest_entry.file_path()
- ),
- ));
- }
+ let path = data_file
+ .referenced_data_file()
+ .expect("validated deletion vector must have
referenced_data_file");
Review Comment:
@mbutrovich You read the ordering correctly, and the `expect` was genuinely
reachable — `PopulatedDeleteFileIndex::new` consumes `DeleteFileContext`
straight from manifest scanning, while `build()` only runs later in
`get_deletes_for_data_file`. A Puffin entry missing `referenced_data_file`
would have panicked where `main` returned `DataInvalid`. That was a regression,
not a safe tightening.
Both points are resolved, and I took the option you suggested rather than
defending the `expect`:
**The index keeps its own check and returns `DataInvalid`.** As you noted,
it needs the value as a map key and so cannot defer it:
```rust
let path = data_file.referenced_data_file().ok_or_else(|| {
Error::new(
ErrorKind::DataInvalid,
format!("deletion vector {} is missing referenced_data_file",
data_file.file_path()),
)
})?;
```
**Removing `test_deletion_vector_missing_referenced_data_file_is_rejected`
was fallout, not intent** — it went with the check it covered. It is back as
`test_deletion_vector_index_rejects_missing_referenced_data_file`, alongside
`test_deletion_vector_builder_rejects_missing_referenced_data_file`, so both
entry points are pinned independently rather than one standing in for the other.
No `expect` on validation state remains in either `delete_file_index.rs` or
`caching_delete_file_loader.rs`; every missing coordinate degrades to
`DataInvalid`.
`cargo test -p iceberg --lib`: **1683 passed, 0 failed**.
--
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]
