Spark: Fix Fast forward procedure output for non-main branches #8854

amogh-jahagirdar · 2023-10-17T11:42:51Z

Currently the output of the Spark fast forward procedure always outputs currentSnapshot before and after the fast forward operation. However, this is not correct in case the branch being fast forwarded is not main. This change updates the output to always output the before and after of the actual branch being fast forwarded.

amogh-jahagirdar · 2023-10-17T11:47:07Z

...v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/FastForwardBranchProcedure.java

        table -> {
-          long currentRef = table.currentSnapshot().snapshotId();
+          long currentRef = table.snapshot(source).snapshotId();
          table.manageSnapshots().fastForwardBranch(source, target).commit();


Another thing to fix....I think when I originally implemented the fastForward operation I switched the definitions of source/target in my head so the API is confusing (in the API, target means the branch that is actually being fast forwarded, source is where target will be moved). That should probably be reversed so target is actually the target to which source will be moved.

I think we should be able to safely just rename the parameters in the API and update the javadoc. Luckily both are String parameters.

cc @rdblue

I too got confused today when I checked replace branch API vs this procedure variables. Source and target is reversed.

The procedure's named arguments of 'branch' and 'to' is proper. It is like fast forward branch x to y.
Only thing is these internal variables in this procedure is reversed. I think we can rename it in this PR.

ManageSnapshots.replaceBranch and ManageSnapshots.fastForwardBranch seems to have a correct naming IMO

I also agree it is confusing. It would be nice to rename in a follow-up PR or here. It seems like source represents a branch name in this case.

I put up a separate PR here #9134

amogh-jahagirdar · 2023-10-17T11:55:11Z

also cc @rakesh-das08 let me know your thoughts on this fix!

rakesh-das08 · 2023-10-17T12:30:29Z

@amogh-jahagirdar the fix LGTM. Thanks for fixing this.

...nsions/src/test/java/org/apache/iceberg/spark/extensions/TestFastForwardBranchProcedure.java

ajantha-bhat · 2023-10-17T12:38:16Z

...v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/FastForwardBranchProcedure.java

        table -> {
-          long currentRef = table.currentSnapshot().snapshotId();
+          long currentRef = table.snapshot(source).snapshotId();
          table.manageSnapshots().fastForwardBranch(source, target).commit();


I too got confused today when I checked replace branch API vs this procedure variables. Source and target is reversed.

The procedure's named arguments of 'branch' and 'to' is proper. It is like fast forward branch x to y.
Only thing is these internal variables in this procedure is reversed. I think we can rename it in this PR.

ManageSnapshots.replaceBranch and ManageSnapshots.fastForwardBranch seems to have a correct naming IMO

ajantha-bhat · 2023-10-17T12:42:03Z

...v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/FastForwardBranchProcedure.java

        tableIdent,
        table -> {
-          long currentRef = table.currentSnapshot().snapshotId();
+          long currentRef = table.snapshot(source).snapshotId();


There is also a recent issue reported for fast forward on an empty branch
#8849.

I have analyzed but it looks to be clumsy if we try to support dummy snapshot. Let me know what you guys think.

Right, actually I was investigating that issue and when going through the procedure code I just noticed this issue :) On the dummy snapshot idea, I need to think more. I think that idea has been floated around a few times and it logically makes sense I just don't know all the implications of that change.

Yeah, I also couldn't conclude on the implications. All these days, if the snapshot id is -1, we assume that it is an empty table or just create table happened.

We also need a dummy snapshot id for ancestor check to be passed for fast forward operations.
Nessie uses a constant default hash for on empty branch for handling this kind of ancestor problems. Maybe we need to introduce something like that.

I had thought of this case where we could introduce a dummy snapshot, but as you mentioned, it did not look like an elegant solution. And with this PR : #7652 , i just basically fall back to the underlying replace operation to throw an appropriate exception.

ajantha-bhat · 2023-10-17T13:23:02Z

logged the flaky test: #8855

...v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/FastForwardBranchProcedure.java

nastra

overall LGTM once checkstyle/CI is happy. I would probably also use fast-forward in the error msg

…nches

amogh-jahagirdar · 2023-11-23T18:11:02Z

Thanks for the reviews @nastra @rakesh-das08 @ajantha-bhat @aokolnychyi ! I'm going to merge this to keep the fix focused on the procedure output for non-main branches. There are still 2 pending related items:

1.) Fixing the naming in the replace/fastForward APIs: That PR is here https://github.com/apache/iceberg/pull/9134/files
2.) The original issue for fast forwarding a branch which does not exist yet (this PR still preserves the original behavior, just with an explicit null check beforehand).#8849

For 2, I do agree it's an awkward experience that should be fixed (after all many folks would write to audit first before the existence of main). I noticed that recently there was a create added which does add the dummy append. We could maybe stitch together the APIs to achieve the "create" if it does not already exist or we could add a new API. The first is preferable IMO but I'd need to check further on the feasibility.

…nches (apache#8854)

amogh-jahagirdar requested a review from ajantha-bhat October 17, 2023 11:43

github-actions bot added the spark label Oct 17, 2023

amogh-jahagirdar requested a review from nastra October 17, 2023 11:43

amogh-jahagirdar commented Oct 17, 2023

View reviewed changes

amogh-jahagirdar requested a review from rdblue October 17, 2023 11:47

amogh-jahagirdar changed the title ~~Spark: Fix Fast forward before/after snapshot output for non-main branches~~ Spark: Fix Fast forward procedure output for non-main branches Oct 17, 2023

ajantha-bhat reviewed Oct 17, 2023

View reviewed changes

amogh-jahagirdar closed this Oct 17, 2023

amogh-jahagirdar reopened this Oct 17, 2023

ajantha-bhat approved these changes Oct 18, 2023

View reviewed changes

nastra reviewed Oct 18, 2023

View reviewed changes

...v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/FastForwardBranchProcedure.java Outdated Show resolved Hide resolved

amogh-jahagirdar mentioned this pull request Nov 22, 2023

API, Core: Fix naming in fastForwardBranch/replaceBranch APIs #9134

Merged

amogh-jahagirdar force-pushed the fix-fastforward-output branch 2 times, most recently from 8fb30d0 to bcd7722 Compare November 23, 2023 08:25

amogh-jahagirdar requested review from aokolnychyi and nastra November 23, 2023 08:32

nastra reviewed Nov 23, 2023

View reviewed changes

...v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/FastForwardBranchProcedure.java Outdated Show resolved Hide resolved

nastra reviewed Nov 23, 2023

View reviewed changes

...v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/FastForwardBranchProcedure.java Outdated Show resolved Hide resolved

nastra approved these changes Nov 23, 2023

View reviewed changes

Spark: Fix Fast forward before/after snapshot output for non-main bra…

cb6a841

…nches

amogh-jahagirdar force-pushed the fix-fastforward-output branch from bcd7722 to cb6a841 Compare November 23, 2023 16:29

amogh-jahagirdar merged commit 13fcf62 into apache:main Nov 23, 2023

devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024

Spark: Fix Fast forward before/after snapshot output for non-main bra…

65a4434

…nches (apache#8854)

Spark: Fix Fast forward procedure output for non-main branches #8854

Spark: Fix Fast forward procedure output for non-main branches #8854

Uh oh!

Conversation

amogh-jahagirdar commented Oct 17, 2023

Uh oh!

amogh-jahagirdar Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar commented Oct 17, 2023

Uh oh!

rakesh-das08 commented Oct 17, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajantha-bhat commented Oct 17, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar commented Nov 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

amogh-jahagirdar Oct 17, 2023 •

edited

Loading