-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Core: Refactor actions results #7089
Conversation
cc: @nastra, @aokolnychyi, @rdblue, @jackye1995 |
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.
LGTM with one comment about the deprecation message
@@ -21,6 +21,8 @@ | |||
import org.apache.iceberg.StructLike; | |||
import org.apache.iceberg.relocated.com.google.common.base.MoreObjects; | |||
|
|||
/** @deprecated will be removed in 1.3.0. */ |
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.
I think the deprecation message should mention what should be used alternatively. Also the version might need to be updated (depending on when this gets merged)
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.
added the alternative and changed the version to 1.4.0 as 1.2.0 voting is already in progress.
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.
LGTM!
I noticed that there is another action result implementation may need to be deprecated in iceberg-delta-lake
:
iceberg/delta-lake/src/main/java/org/apache/iceberg/delta/SnapshotDeltaLakeTable.java
Lines 27 to 29 in 5ebb1aa
/** Snapshot an existing Delta Lake table to Iceberg in place. */ | |
public interface SnapshotDeltaLakeTable | |
extends Action<SnapshotDeltaLakeTable, SnapshotDeltaLakeTable.Result> { |
But since it is not in the core, I think it can be updated in a separate PR
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.
looks good to me!
Since. 1.2.0 release vote is passed. This can be merged now I guess. Thanks. |
Merging, thanks for the change! |
Each action has a separate boilerplate code with the public constructor. Which is hard to maintain. Also, the addition of new fields requires deprecating older constructors and adding a new one.
Recently two of the PRs (#6801, and #6091) also had to go through new constructors.
Changes:
This is a follow-up on #6091 (comment)
Note: only spark-3.3 is using the new builders. Other versions are still using the newly deprecated builders.
Will be replaced in a single follow-up PR after the review.