Repository navigation
Conversation
Execution engines that build scan tasks themselves do not always carry a delete file's size. Tasks planned from manifest entries keep the recorded size; externally constructed tasks may now set FileScanTaskDeleteFile::file_size_in_bytes to 0 when the size is unavailable, and the Parquet delete-file loader resolves it with one metadata request when the file is first loaded. The caching loader loads each delete file once per reader, so the request is made once per delete file even when several data files reference it. A failed metadata request is reported with the delete-file path while keeping the storage error's kind and retryability. A size below the Parquet footer minimum, whether resolved or supplied by the task, fails the read rather than dropping the file's deletes. Deletion vectors do not use this size and are unchanged.
JeonDaehong
left a comment
JeonDaehong
left a comment
There was a problem hiding this comment.
Heads-up @unikdahal
#3145 (approved) makes the FileScanTaskDeleteFile fields private and routes both the builder and deserialization through validate(). Merging it with this PR causes conflicts in delete_file_loader.rs and scan/task.rs, and the two tests that set delete.file_size_in_bytes = 0 will no longer compile once the field is private.
Since #3145 is already modifying this type for 0.11, would it make sense to model the unknown size as an Option<u64> (with the getter returning Option<u64>) instead of using 0 as a sentinel? That would make "unknown" explicit for external callers, and validate() could still check the known case.
I checked that the deletion vector path doesn't use file_size_in_bytes, so the DV note in your description holds.
Which issue does this PR close?
What changes are included in this PR?
Execution engines that build
FileScanTasks themselves do not always carry a delete file's size, and a delete file withfile_size_in_bytes == 0cannot currently be opened.0when the size is unavailable (documented onFileScanTaskDeleteFile::file_size_in_bytes).BasicDeleteFileLoaderresolves a zero size with one metadata request when the Parquet delete file is first loaded.CachingDeleteFileLoaderalready loads each delete file once per reader, so this happens once per delete file even when several data files reference it.Failed to stat delete file '…';BasicDeleteFileLoaderandCachingDeleteFileLoader::load_deleteskeep the storage error's kind and retryability.DataInvalidrather than dropping the file's deletes.No public type or method signatures change;
FileScanTaskDeleteFile::file_size_in_bytesadditionally accepts0for externally constructed tasks where the size is unavailable.Not in this PR
ArrowReaderwith a data-file concurrency of 1 wraps task errors asUnexpected; that pre-existing wrapping is left to a separate change.Loading: later positional-delete waiters for the same file are never notified, and the spawned equality-delete task panics on the dropped channel. This is pre-existing for open failures with known sizes; a failed metadata request is one more way to reach it. Fixing it needs failure propagation to waiters (they must not proceed as if there were no deletes), so it is left to a separate change.Are these changes tested?
Yes:
CachingDeleteFileLoaderfor two data files issue one metadata request and produce a bound predicate equal to the known-size one; parsing an unknown-size equality delete file yields an equal predicate.Failed to stat delete filewith exactly the storage error's kind and retryability, from the basic loader and throughCachingDeleteFileLoader::load_deletes; a retryable storage failure stays retryable.AI Disclosure
This change was developed with assistance from Claude Code and reviewed by me. Tests and CI results come from this branch.