feat(scan): [2/N] carry deletion-vector coordinates on FileScanTaskDeleteFile - #2868
feat(scan): [2/N] carry deletion-vector coordinates on FileScanTaskDeleteFile#2868mbutrovich wants to merge 13 commits into
Conversation
Add referenced_data_file, content_offset, and content_size_in_bytes to FileScanTaskDeleteFile, populated from the delete file's manifest entry. These locate a deletion-vector blob and scope it to its data file, which the delete loader needs to read and apply V3 deletion vectors. Refs apache#2792.
hsiang-c
left a comment
hsiang-c
left a comment
There was a problem hiding this comment.
Thanks @mbutrovich, this is a concise and self-contained PR.
CTTY
left a comment
CTTY
left a comment
There was a problem hiding this comment.
Thanks for adding this! I think the existing PR looks good but I'm more curious about the following work: how are these DV related fields used
Maybe you can post the draft for the following work to provide more context?
Thanks @CTTY! The epic generally tracks it: #2792. I have it working end-to-end in Comet in this branch: main...mbutrovich:iceberg-rust:dv-read. Task 3 would be the remaining PR (though we could break that up further if we need to). I can see about getting the task 3 PR up as a draft on top of this branch if that helps. |
# Conflicts: # crates/iceberg/src/delete_file_index.rs
The stacked final PR is at #2868. |
CTTY
left a comment
CTTY
left a comment
There was a problem hiding this comment.
Thanks for posting the draft follow up! Mostly LGTM.
I have some considerations that apply to the followup but I think would be better to address/clarify in this PR
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| #[builder(default)] | ||
| pub content_size_in_bytes: Option<i64>, | ||
|
|
There was a problem hiding this comment.
In #3035 I noticed we are also trying to add record_count for cardinality check. I'm leaning toward adding it here
| /// For a deletion vector, the length in bytes of the blob within its Puffin file. Set | ||
| /// whenever `content_offset` is. |
There was a problem hiding this comment.
this is a bit unclear, I'd recommend
| /// For a deletion vector, the length in bytes of the blob within its Puffin file. Set | |
| /// whenever `content_offset` is. | |
| /// For a deletion vector, the length in bytes of the blob within its Puffin file. | |
| /// Required together with `content_offset`; both are absent for non-DV delete files. |
also I noticed in #3035 , we are checking if a file is DV by checking if content_offset.is_some(). I believe instead of that, we should check isDv = (if type == PosDel && format == puffin). and we should throw error if (isDV && (content_offset.is_none() || content_size_in_bytes.is_none())) because both of them are required for DVs
4 participants
Which issue does this PR close?
deletion-vector-v1Puffin blobs) #2792.What changes are included in this PR?
Adds
referenced_data_file,content_offset, andcontent_size_in_bytestoFileScanTaskDeleteFile, populated from the delete file's manifest entry (DataFile) inFrom<&DeleteFileContext>.content_offset/content_size_in_byteslocate a deletion-vector blob within its Puffin file, andreferenced_data_filescopes it to the data file it applies to. The delete loader will use these to read and apply V3 deletion vectors (a later PR in this epic).This mirrors Iceberg-Java, where a deletion vector is a
DeleteFile(content=POSITION_DELETES,format=PUFFIN) carrying these fields, attached to a task viaFileScanTask.deletes().No behavior change on its own: the fields default to
Noneand are only populated, nothing reads them yet. This is independent of the codec PR #2866; both are prerequisites for the loader.Whether
referenced_data_file,content_offset, andcontent_size_in_bytesare actually present and consistent is not validated here. The spec requires all three together whencontent=POSITION_DELETESandformat=PUFFIN(a deletion vector), but that's an invariant of the manifest entry, not of this conversion, and nothing in this crate enforces it yet atDataFileconstruction or manifest-read time either. The delete loader is the first place a missing or inconsistent value actually matters, so it validates before use rather than this plumbing.Are these changes tested?
Unit test in
delete_file_index.rs: builds a deletion-vector-shapedDataFile(Puffin,PositionDeletes, withcontent_offset/content_size_in_bytes/referenced_data_file), converts it to aFileScanTaskDeleteFile, and asserts the three coordinates survive the conversion.This is also exercised end to end against deletion vectors written by Spark / Iceberg-Java in a draft DataFusion Comet PR (apache/datafusion-comet#4887), which reads real V3 merge-on-read tables through this path.
AI Disclosure
Developed with the help of Claude Code, but I understand and support these changes.