API: Access deleted and added delete files in Snapshot #5105
Conversation
| * @return all data files added to the table in this snapshot. | ||
| */ | ||
| Iterable<DataFile> addedFiles(FileIO io); | ||
| Iterable<DataFile> addedDataFiles(FileIO io); |
There was a problem hiding this comment.
Renamed methods haven't been released yet so we won't break anyone.
| manifest -> Objects.equal(manifest.snapshotId(), snapshotId)); | ||
|
|
||
| for (ManifestFile manifest : changedManifests) { | ||
| try (ManifestReader<DeleteFile> reader = ManifestFiles.readDeleteManifest(manifest, fileIO, null)) { |
There was a problem hiding this comment.
Warning: the spec map is null.
If I remember correctly, ManifestReader will use the Avro header metadata to parse the schema and spec. While it is generally not reliable as the schema may be old, I think it should be fine as long as we don't do any binding or filtering (like in this case).
aokolnychyi
force-pushed
the
snapshot-delete-files
branch
2 times, most recently
from
e6e3669 to
3c4fc53
Compare
| * @param io a {@link FileIO} instance used for reading files from storage | ||
| * @return all delete files deleted from the table in this snapshot | ||
| */ | ||
| default Iterable<DeleteFile> deletedDeleteFiles(FileIO io) { |
There was a problem hiding this comment.
Should we use removed instead of deleted so we don't have deleted-delete-...?
There was a problem hiding this comment.
This question came up a few times. I went with deleted for consistency cause we use that for data files in this class and other places like deletedPositionDeleteFilesCount. We do use removed in snapshot summary but that seems to be the only place.
If we decide to use removed, I think we should update the naming for data file methods too. They haven't been released yet. I'd say let's either keep things as they are or switch to this:
removedDataFiles(FileIO io);
removedDeleteFiles(FileIO io);
There was a problem hiding this comment.
Sounds good, I'll update both.
rdblue
left a comment
rdblue
left a comment
There was a problem hiding this comment.
+1. Merge when you're ready, but I had a question on a method name.
aokolnychyi
force-pushed
the
snapshot-delete-files
branch
from
6e11433 to
c262b5a
Compare
|
Thanks, @rdblue! |
2 participants
This PR uses the work in #4873 as an opportunity to support accessing delete files in
Snapshot. That PR added new methods for accessing data files with an explicitFileIO, which haven't been released yet. I propose to rename the newly added methods toaddedDataFilesanddeletedDataFilesand also add similar methods for delete files. I feel it is important to do before 1.0.0.