mbutrovich commented on code in PR #3145:
URL: https://github.com/apache/iceberg-rust/pull/3145#discussion_r3969536371
##########
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:
I may be misreading the ordering here, but is the "validated" premise true
at this point? `PopulatedDeleteFileIndex::new` consumes `DeleteFileContext {
manifest_entry, .. }` fed straight from manifest scanning, and the conversion
to `FileScanTaskDeleteFile` (and so `build()`) happens later, in
`get_deletes_for_data_file`. If that's right, a manifest with a Puffin
position-delete entry missing `referenced_data_file` would panic here, where it
previously returned `DataInvalid`.
Relatedly, `test_deletion_vector_missing_referenced_data_file_is_rejected`
is removed in this PR. Was that deliberate, or fallout from the check moving?
One option: keep this one check returning `DataInvalid`, since the index
needs the value as its map key and so can't defer it, and move only
`content_offset` / `content_size_in_bytes` to the builder. Curious whether you
considered that and preferred the `expect`.
##########
crates/iceberg/src/arrow/caching_delete_file_loader.rs:
##########
@@ -347,46 +346,26 @@ impl CachingDeleteFileLoader {
fn validate_deletion_vector_task(
task: &FileScanTaskDeleteFile,
) -> Result<(u64, u64, String, u64)> {
- let content_offset = task.content_offset.ok_or_else(|| {
- Error::new(
- ErrorKind::DataInvalid,
- format!(
- "deletion vector {} is missing content_offset",
- task.file_path
- ),
- )
- })?;
- let content_size = task.content_size_in_bytes.ok_or_else(|| {
- Error::new(
- ErrorKind::DataInvalid,
- format!(
- "deletion vector {} is missing content_size_in_bytes",
- task.file_path
- ),
- )
- })?;
- let data_file_path = task.referenced_data_file.clone().ok_or_else(|| {
- Error::new(
- ErrorKind::DataInvalid,
- format!(
- "deletion vector {} is missing referenced_data_file",
- task.file_path
- ),
- )
- })?;
- let record_count = task.record_count.ok_or_else(|| {
- Error::new(
- ErrorKind::DataInvalid,
- format!("deletion vector {} is missing record_count",
task.file_path),
- )
- })?;
+ let content_offset = task
+ .content_offset()
+ .expect("validated deletion vector must have content_offset");
Review Comment:
Same question about the guarantee, for a different reason:
`FileScanTaskDeleteFile` derives plain `Deserialize` with no `#[serde(try_from
= ..)]`, unlike `FileScanTask`, which has one. So a task rebuilt from a
serialized scan plan, the planner to worker path the `key_metadata` doc comment
describes, never passes through `build()`. Does that leave these `expect`s
reachable on a malformed or hand-written plan?
Since the function already returns `Result`, keeping `ok_or_else(..)` here
would cost little and degrade to `DataInvalid` instead of a panic.
Alternatively, adding `#[serde(try_from = ..)]` to route deserialization
through the same validation would make the `expect`s sound, which seems like
the stronger fix if it's not too much scope for this 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]
