Core, REST: Fix delete file references for DVs in the same Puffin file - #17497
Core, REST: Fix delete file references for DVs in the same Puffin file#17497williamhyun wants to merge 4 commits into
Conversation
|
cc: @singhpk234 |
singhpk234
left a comment
singhpk234
left a comment
There was a problem hiding this comment.
LGTM, thanks @williamhyun !
singhpk234
changed the title
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Ok thank you @williamhyun I had to think through different solutions (mentioned in the comments) but I came to the conclusion that exposing this is probably the best thing to do. Let's give it another day or so for @nastra to review!
| List<FileScanTask> fileScanTasks, | ||
| List<DeleteFile> deleteFiles, |
There was a problem hiding this comment.
I was looking at this and in hindsight I think it would've been better to pass in Iterables for both of these. In the end this implementation takes the responsibility of tracking the positions, it's not like we need to rely on the caller to pass in an explicit List.
There was a problem hiding this comment.
@amogh-jahagirdar
Could we open this in a follow-up api change?
There was a problem hiding this comment.
Yeah that should be good, not needed in this PR
| JsonGenerator gen) | ||
| throws IOException { | ||
| Map<String, Integer> deleteFilePathToIndex = Maps.newHashMap(); | ||
| Map<DeleteFileWrapper, Integer> deleteFileToIndex = Maps.newHashMap(); |
There was a problem hiding this comment.
Normally, I think I'd have this map be defined to the interface DeleteFile (and the implementation just uses the DeleteFileWrapper when populating for correctness of the fix) but in this case I think it's actually better to define it to the concrete implementation as it's done here because it makes it clear that this really does need to be the wrapper for proper uniqueness comparison. It guarantees at compile tme that if someone changes the below logic to populate some other type of DeleteFile that we're doing something wrong.
There was a problem hiding this comment.
I would just add a 1 line comment maybe explaining DeleteFileWrapper is important here.
| * <p>Delete files are identified by location and content range rather than location alone, as a | ||
| * single Puffin file can hold a deletion vector for each of several data files. | ||
| */ | ||
| public class DeleteFileWrapper implements WrapperSet.Wrapper<DeleteFile> { |
There was a problem hiding this comment.
I looked at other approaches (like how difficult it would be to make DeleteFileSet an indexed structure since it does store in insertion order, but removals get complicated etc). So I came to the conclusion that fundamentally we do need to expose DeleteFile equality as is done here via the wrapper.
There was a problem hiding this comment.
And I think any other kind of implementation would basically be a O(n) lookup when trying to find the index to serialize (the mapping from delete file to position in the protocol) into the response which feels needlessly suboptimal to just exposing this.
| * Wrapper class to adapt DeleteFile for use in maps and sets. | ||
| * | ||
| * <p>Delete files are identified by location and content range rather than location alone, as a | ||
| * single Puffin file can hold a deletion vector for each of several data files. |
There was a problem hiding this comment.
"single Puffin file can hold multiple DVs"? a DV is already known to be 1:1 for a data file so I don't think we need the "each of several data files"
|
Thank you @singhpk234 and @amogh-jahagirdar for the comments! |
| public String toString() { | ||
| return file.location(); | ||
| } | ||
| } No newline at end of file |
| * <p>Delete files are identified by location and content range rather than location alone, as a | ||
| * single Puffin file can hold multiple deletion vectors. | ||
| */ | ||
| public class DeleteFileWrapper implements WrapperSet.Wrapper<DeleteFile> { |
There was a problem hiding this comment.
Now that we're exposing this class we should probably have a TestDeleteFileWrapper
4 participants
Motivation: Scan planning responses index delete files by position in the delete-files array.
The index map was keyed on the delete file location alone, but a single Puffin file can hold a deletion vector for each of several data files. Every DV in that file shares a location, so each new DV entry overwrote the previous one and every task referencing a DV in that Puffin file resolved to the same last index.
Changes: Extract DeleteFileSet's private wrapper into a public DeleteFileWrapper, mirroring CharSequenceWrapper, and key the index map on it so serialization uses the same delete file identity that builds the delete-files array.
Fail with a clear message when a task references a delete file that is not in that array.