-
Notifications
You must be signed in to change notification settings - Fork 3k
Spark: support rewrite on specified target branch #12257
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
Conversation
| this( | ||
| table, | ||
| startingSnapshotId, | ||
| useStartingSequenceNumber, | ||
| ImmutableMap.of(), | ||
| SnapshotRef.MAIN_BRANCH); |
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.
Minor: these constructors read slightly off to me - the default branch being main is encoded in two constructors, and the one below doesn't chain.
My opinion: maybe keep this constructor as it was before, and replace the body of the one below with
this(table, startingSnapshotId, useStartingSequenceNumber, snapshotProperties, SnapshotRef.MAIN_BRANCH);to keep the constructor-chaining flow with each setting one default?
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.
thanks @smaheshwar-pltr - will look during the weekend
|
@pvary - so is this pr redundant ? and if so maybe we should close this |
|
@amitgilad3 I'm reading @pvary's comment as saying that this change will be refactored down the line as part of a general refactor of the codebase. I don't see that as superceding this work, just as a heads-up that it will get changed. @pvary let me know if that's not accurate. |
|
@amitgilad3: Sorry for the late response. I was OOO. I wanted to highlight that any changes done here will need to be updated when the refactoring is done |
|
Hi, we have some users waiting for this change, wondering if there's any work remaining on this PR? @amitgilad3 I'm happy to help on revisions if you don't have time to work on this. |
|
@lliangyu-lin - I have 1 fix todo, will work on it today and if all is good we can merge 👍 |
The refactoring PR will be merged later today. |
|
Thanks @pvary - i saw you merged the code , will take a look now that its merged |
|
Hello @amitgilad3, wondering if you are still actively working on this PR? If not, could I help on rebasing the refactored change based on your current changes to get this in? |
|
Hey @lliangyu-lin , i already started the work. Hope to finish it soon |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
This pr contains the fixes requested in the previous pr that was not worked on any more and closed - #8797