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

Core: Store split offset for delete files #7011

Merged
merged 7 commits into from
Mar 24, 2023
Merged

Core: Store split offset for delete files #7011

merged 7 commits into from
Mar 24, 2023

Conversation

singhpk234
Copy link
Contributor

@singhpk234 singhpk234 commented Mar 4, 2023 •
edited by jackye1995
Loading

Fixes #6659

@github-actions github-actions bot added the core label Mar 4, 2023
@rdblue
Copy link
Contributor

rdblue commented Mar 5, 2023

Overall this looks fine, but I see CI failures.

@github-actions github-actions bot added the spark label Mar 6, 2023
@singhpk234 singhpk234 changed the title [WIP] Core: Store split offset for delete files Core: Store split offset for delete files Mar 6, 2023
@jackye1995 jackye1995 self-requested a review March 6, 2023 17:48
@singhpk234 singhpk234 force-pushed the feature/store-split-offset-delete-files branch from 5e4868d to 0d1f29b Compare March 6, 2023 19:46
jackye1995
jackye1995 approved these changes Mar 6, 2023
View reviewed changes
Copy link
Contributor

@jackye1995 jackye1995 left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, I checked all existing Util classes, did not find a good one to put this method either, so I am good with adding a KryoUtil class as well, unless there are other better suggestions.

szehon-ho
szehon-ho approved these changes Mar 7, 2023
View reviewed changes
Copy link
Collaborator

@szehon-ho szehon-ho left a comment

Choose a reason for hiding this comment

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

Thanks @singhpk234 for this, it is great, great to see the KryoUtil refactoring as well, just one suggestion for test for consideration.

core/src/test/java/org/apache/iceberg/TestSplitPlanning.java Outdated Show resolved Hide resolved
amogh-jahagirdar
amogh-jahagirdar reviewed Mar 7, 2023
View reviewed changes
Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar left a comment

Choose a reason for hiding this comment

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

Thanks @singhpk234 this is great, minor comments and a question.

core/src/main/java/org/apache/iceberg/FileMetadata.java Show resolved Hide resolved
core/src/main/java/org/apache/iceberg/FileMetadata.java Outdated Show resolved Hide resolved
spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestPositionDeletesTable.java Show resolved Hide resolved
@rdblue
Copy link
Contributor

rdblue commented Mar 8, 2023

This looks fine to me overall, but I'd prefer to use ImmutableList or long[] for split offsets in the builder to avoid an unnecessary new util class and copy method.

@singhpk234 singhpk234 force-pushed the feature/store-split-offset-delete-files branch from 40ee7ed to 19b35b3 Compare March 8, 2023 17:32
@singhpk234
Copy link
Contributor Author

This looks fine to me overall, but I'd prefer to use ImmutableList or long[] for split offsets in the builder to avoid an unnecessary new util class and copy method.

Ack made the changes

amogh-jahagirdar
amogh-jahagirdar approved these changes Mar 11, 2023
View reviewed changes
Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar left a comment

Choose a reason for hiding this comment

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

Overall looks good to me @singhpk234 !

@singhpk234 singhpk234 force-pushed the feature/store-split-offset-delete-files branch from 19b35b3 to 733a8c1 Compare March 23, 2023 22:00
@singhpk234 singhpk234 force-pushed the feature/store-split-offset-delete-files branch from 733a8c1 to 3fd1781 Compare March 23, 2023 22:24
szehon-ho
szehon-ho approved these changes Mar 23, 2023
View reviewed changes
Copy link
Collaborator

@szehon-ho szehon-ho left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @singhpk234

@szehon-ho
Copy link
Collaborator

Approved for test run.

@szehon-ho szehon-ho merged commit faf974e into apache:master Mar 24, 2023
@szehon-ho
Copy link
Collaborator

Thanks @singhpk234 for change, and @rdblue @jackye1995 @amogh-jahagirdar for extra review!

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

@rdblue rdblue rdblue left review comments

@szehon-ho szehon-ho szehon-ho approved these changes

@jackye1995 jackye1995 jackye1995 approved these changes

@amogh-jahagirdar amogh-jahagirdar amogh-jahagirdar approved these changes

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

Successfully merging this pull request may close these issues.

Store split offsets for delete files
5 participants
@singhpk234 @rdblue @szehon-ho @jackye1995 @amogh-jahagirdar