Uh oh!
There was an error while loading. Please reload this page.
feat: add RollingManifestWriter - #650
Conversation
4f45b47 to
f558e34Comparegeruh
commented
Apr 23, 2024
I believe in the Java implementation we have a concept of a PositionOutputStream which is used to keep track of bytes written to each file with a position/counter. What we can do here is extend the current For instance, the ManifestWriter can use the |
felixscherz
commented
Apr 24, 2024
Sounds good, I will have a look at the implementation and make a suggestion. Thank you! |
f558e34 to
ee63925Comparefelixscherz
commented
May 4, 2024
Hi, I finally had some time to continue working on this. Based on your suggestions @geruh I added a I initially tried to extend If we wanted to go with What do you think? |
42285e3 to
bdb8d2dComparebdb8d2d to
da96cedCompareda96ced to
f34d9f9Compare| self._current_file_rows = 0 | ||
| def to_manifest_files(self) -> list[ManifestFile]: | ||
| self._close_current_writer() |
There was a problem hiding this comment.
I like the same pattern as in Java, where the to_manifest_files call expects the writer to be closed.
There was a problem hiding this comment.
Changed it to raise a RuntimeError if the writer is not closed, similar to how trying to add an entry to a closed writer raises a RuntimeError.
| traceback: Optional[TracebackType], | ||
| ) -> None: | ||
| self.closed = True | ||
| if self._current_writer: |
There was a problem hiding this comment.
Why not re-use _close_current_writer here?
There was a problem hiding this comment.
Good point! I changed it to use _close_current_writer
Fokko
left a comment
There was a problem hiding this comment.
@felixscherz Sorry for the late reply here. It looks like the formatting is a bit off, could you check that one?
felixscherz
commented
Jul 9, 2024
@Fokko Thanks for taking a look! Sorry about the formatting, should be fixed now:) |
e1893a0 to
869ea57Compare…th `__len__` method
869ea57 to
9f01e5aCompare# Rationale for this change Currently, PyIceberg writes one manifest per snapshot operation regardless of manifest size. In order to eventually support this we need to be able to track written bytes without closing the file, so that we can roll to a new file once we hit target size. We had some of this work done in #650, but we can keep this simple and add writers as a follow up. The nice thing is that the underlying streams we support already have a tell() method and we just need to expose it. With this change in the follow up we can do: ``` with write_manifest(...) as writer: writer.add_entry(entry) if writer.tell() >= target_file_size: # roll to new file ``` ## Are these changes tested? Yes, added a test :) ## Are there any user-facing changes? No
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
Hi, this is in regards to #596 and still WIP.
The
RollingManifestWriterimplementation closely follows the java implementation.It takes in a generator that produces
ManifestWriterobjects and rolls over to a new one once either the number of rows appended or the file size in bytes exceeds the target value.It's not finished as of yet, I am still trying to find a good way to access the current file from the underlying reader. I tried to obtain that information from the
ManifestWriter._writer.output_streamobject, but that is write-only.Any pointers on how to access the current file size of the manifest writer would help me a lot:)