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: manifest list writer #76

Merged
merged 6 commits into from
Oct 26, 2023
Merged

feat: manifest list writer #76

merged 6 commits into from
Oct 26, 2023

Conversation

barronw
Copy link
Contributor

@barronw barronw commented Oct 11, 2023

This adds a manifest list writer based on the discussion in #72.

liurenjie1024
liurenjie1024 reviewed Oct 11, 2023
View reviewed changes
Copy link
Collaborator

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

Generally LGTM, thanks!

liurenjie1024
liurenjie1024 reviewed Oct 11, 2023
View reviewed changes
crates/iceberg/src/spec/manifest_list.rs Outdated Show resolved Hide resolved
crates/iceberg/src/spec/manifest_list.rs Outdated Show resolved Hide resolved
crates/iceberg/src/spec/manifest_list.rs Outdated Show resolved Hide resolved
liurenjie1024
liurenjie1024 approved these changes Oct 16, 2023
View reviewed changes
Copy link
Collaborator

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@liurenjie1024
Copy link
Collaborator

liurenjie1024 commented Oct 16, 2023 •
edited

CC @Xuanwo @JanKaul @Fokko PTAL

Fokko
Fokko approved these changes Oct 16, 2023
View reviewed changes
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Two minor comments, but looks good in general

crates/iceberg/src/spec/manifest_list.rs Show resolved Hide resolved
crates/iceberg/src/spec/manifest_list.rs Outdated Show resolved Hide resolved
ZENOTME
ZENOTME approved these changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@ZENOTME ZENOTME left a comment

Choose a reason for hiding this comment

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

Thanks!

liurenjie1024
liurenjie1024 approved these changes Oct 18, 2023
View reviewed changes
Copy link
Collaborator

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

Xuanwo
Xuanwo reviewed Oct 18, 2023
View reviewed changes
crates/iceberg/src/avro/schema.rs Outdated Show resolved Hide resolved
crates/iceberg/src/avro/schema.rs Show resolved Hide resolved
crates/iceberg/src/spec/manifest_list.rs Outdated Show resolved Hide resolved
crates/iceberg/src/spec/manifest_list.rs Outdated Show resolved Hide resolved
Fokko
Fokko reviewed Oct 18, 2023
View reviewed changes
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

@barronw This is great! One question, do we need the Avro file (the one in testdata/)? I don't see it being referenced, and we generally try to avoid binary data in the repository.

@barronw
Copy link
Contributor Author

barronw commented Oct 18, 2023

@barronw This is great! One question, do we need the Avro file (the one in testdata/)? I don't see it being referenced, and we generally try to avoid binary data in the repository.

The Avro files are used to test ManifestList::parse_with_version in test_parse_manifest_list_v1 and test_parse_manifest_list_v2.

ManifestList::parse_with_version is used to test ManifestListWriter, so I don't think we should also use ManifestListWriter to generate the Avro files to test ManifestList::parse_with_version.

@barronw barronw requested a review from Fokko October 18, 2023 12:11
@barronw
Copy link
Contributor Author

barronw commented Oct 21, 2023

@Fokko Are there any other changes that need to be made?

@Fokko
Copy link
Contributor

Fokko commented Oct 26, 2023

@barronw No, I think we're good 👍

@Fokko Fokko merged commit 94a1c5d into apache:main Oct 26, 2023
6 checks passed
@Fokko
Copy link
Contributor

Fokko commented Oct 26, 2023

Thanks for working on this @barronw and @liurenjie1024, @Xuanwo and @ZENOTME for the review 👍

@barronw barronw deleted the barronw/manifest-list-writer branch October 26, 2023 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liurenjie1024 liurenjie1024 liurenjie1024 approved these changes

@Xuanwo Xuanwo Xuanwo left review comments

@ZENOTME ZENOTME ZENOTME approved these changes

@Fokko Fokko Awaiting requested review from Fokko

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@barronw @liurenjie1024 @Fokko @Xuanwo @ZENOTME