-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support append data file and add e2e test #9
Conversation
b2fc5ff
to
4b3a6b9
Compare
Should we have another PR to rebase with the mainstream first? Or exclude them. |
Better to drop upstream commits. To rebase with upstream, we will need to force push or create a new branch |
4b3a6b9
to
dd9a186
Compare
crates/iceberg/src/transaction.rs
Outdated
.filter(|entry| { | ||
entry.has_added_files() | ||
|| entry.has_existing_files() | ||
|| entry.added_snapshot_id == snapshot_produce.snapshot_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which cases will this branch be triggered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I ask this in the community and get the answer:
We've copied this from the Java code: https://github.com/apache/iceberg/blob/307593ffd99752b2d62cc91f4928285fc0c62b75/co[…]e/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
I don't think this is used in PyIceberg today. In Java it is possible to add existing manifests to a commit, while in PyIceberg we only accept datafiles. This is on purpose since the ability to add manifests also comes with problems of its own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest LGTM
dd9a186
to
16c4987
Compare
86fd4ac
to
2cd6703
Compare
upstream pr: apache#349
This PR: