Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: support append data file and add e2e test #9

Open
wants to merge 15 commits into
base: dev-rebase-main-20241030
Choose a base branch
from
Open

feat: support append data file and add e2e test #9

wants to merge 15 commits into from

Conversation

ZENOTME
Copy link
Collaborator

@ZENOTME ZENOTME commented Nov 15, 2024

upstream pr: apache#349

This PR:

  1. It adds the FastAppendAction to commit the data file
  2. It init the e2e test for write data file

@xxchan xxchan changed the base branch from main to dev-rebase-main-20241030 November 15, 2024 03:26
@ZENOTME
Copy link
Collaborator Author

ZENOTME commented Nov 15, 2024

Should we have another PR to rebase with the mainstream first? Or exclude them.

@xxchan
Copy link
Member

xxchan commented Nov 15, 2024

Better to drop upstream commits.

To rebase with upstream, we will need to force push or create a new branch

chenzl25
chenzl25 reviewed Nov 15, 2024
View reviewed changes
crates/iceberg/src/transaction.rs
.filter(|entry| {
entry.has_added_files()
|| entry.has_existing_files()
|| entry.added_snapshot_id == snapshot_produce.snapshot_id
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

chenzl25
chenzl25 approved these changes Nov 15, 2024
View reviewed changes
Copy link
Collaborator

@chenzl25 chenzl25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chenzl25 chenzl25 chenzl25 approved these changes

@xxchan xxchan Awaiting requested review from xxchan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ZENOTME @xxchan @chenzl25 @c-thiel