Core, API: Performing operations on a snapshot branch ref #4926

namrathamyske · 2022-06-01T05:39:02Z

This PR is related to the open issue #3896 to perform table operations on a branch ref. Right now throws UnsupportedOperationException. In laters PR's will include implementation for each operation.

amogh-jahagirdar

Thanks so much for contributing this!

api/src/main/java/org/apache/iceberg/SnapshotUpdate.java

core/src/main/java/org/apache/iceberg/SnapshotProducer.java

core/src/test/java/org/apache/iceberg/TestFastAppend.java

core/src/main/java/org/apache/iceberg/SnapshotProducer.java

namrathamyske · 2022-06-01T07:25:49Z

@amogh-jahagirdar Updated all minor changes. Have a question regarding writing the unit test for this. I would need a way to test the refs from table. How can i proceed with that?

amogh-jahagirdar

There is a more fundamental issue here for maintaining the correct snapshot lineage on a branch. The parent of the new snapshot needs to be set to the tip of the branch so that when the operation commits the lineage is correct.

Also there should be validation that the ref is actually a branch and not a tag since we cannot produce snapshots to a tag.

In apply https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/SnapshotProducer.java#L170

If the branch is main, the parent is the current snapshot

If the branch is not main, the parent is the tip of the branch which would be retrieved via table.ref(branch).snapshotId().

rdblue · 2022-06-03T19:51:43Z

api/src/main/java/org/apache/iceberg/SnapshotUpdate.java

  ThisT scanManifestsWith(ExecutorService executorService);
+
+  /**
+   * Perform operations a particular branch


This needs a @param to document branch. I think we also need to specify behavior:

What happens if the branch does not exist? I think we would create one from the main branch.

How does this change validation? What if the validation's from snapshot is not an ancestor of the branch?

@namrathamyske Each SnapshotProducer can implement their own validation logic https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/SnapshotProducer.java#L156

This gets called when trying to apply the operation as part of validating conflicts. For example rewriting data files needs to validate that there are no new row level deletes for data files which were replaced: https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/BaseRewriteFiles.java#L108

We should define the behavior of validation in the context of branching.

@amogh-jahagirdar @rdblue All validations must start from tip of branch, aka the current snapshot in the branch. So all the operations which override the protected void validate(TableMetadata currentMetadata) { } aka - BaseOverwriteFiles, BaseReplacePartitions, BaseRewriteFiles, BaseRowDelta need to reference the current snapshot from the branch tip.

Is any other validation needed here ?

api/src/main/java/org/apache/iceberg/SnapshotUpdate.java

core/src/main/java/org/apache/iceberg/SnapshotProducer.java

core/src/test/java/org/apache/iceberg/TestFastAppend.java

namrathamyske · 2022-06-06T23:23:26Z

There is a more fundamental issue here for maintaining the correct snapshot lineage on a branch. The parent of the new snapshot needs to be set to the tip of the branch so that when the operation commits the lineage is correct.

Also there should be validation that the ref is actually a branch and not a tag since we cannot produce snapshots to a tag.

In apply https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/SnapshotProducer.java#L170

If the branch is main, the parent is the current snapshot

If the branch is not main, the parent is the tip of the branch which would be retrieved via table.ref(branch).snapshotId().

@amogh-jahagirdar Not only the parent ID, even the manifests need to merged by taking the parent tip into account.

In line

iceberg/core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java

Line 686 in 9a8b83c

Snapshot current = base.currentSnapshot();

Making a change as following:

if(branchToUpdate != null){

  current = base.snapshot(base.ref(branchToUpdate).snapshotId());

}
else{

  current = base.currentSnapshot();

}

Append changes to snapshot branch - changes after review comments remove space Adding support methods for supporting performing append/delete on branch Removing spaces Removing spaces Removing unwanted vars Adding check for null and invalid branch Adding check for null and invalid branch

namrathamyske · 2022-06-13T16:28:32Z

@jackye1995 Is it possible for you to review this?

cc: @SreeramGarlapati

amogh-jahagirdar · 2022-06-21T09:30:43Z

Replied here #4926 (comment) , but yes as @rdblue pointed out I think we'll need to think of defining conflict detection validation behavior in the presence of branching and tagging (such as the case where validation starts at a snapshot which is not an ancestor of the branch). I'll need to think more on this.

namrathamyske · 2022-06-21T19:00:54Z

@amogh-jahagirdar @rdblue For operations like - BaseOverwriteFiles we call validationHistory method which is fetching ancestors between starting snapshot and current snapshot. After branching is introduced, this can cause inconsistencies. Is my understanding right or am I missing something wrt to validations?

For now if we just introduce branching for append and delete operations, is it going to be fine ? We can extend to overwrite later.

checkStyle errors

namrathamyske · 2022-07-09T22:18:09Z

@rdblue @amogh-jahagirdar can someone review this PR once again so that Implementation for child operations Append: #5010 , RowDelta #5181 can move forward ?

rdblue · 2022-07-31T22:30:09Z

core/src/test/java/org/apache/iceberg/TestFastAppend.java

    Assert.assertEquals("Should set changed partition count", "2", changedPartitions);
  }
+
+  @Test(expected = UnsupportedOperationException.class)


To test exceptions, we use either TestHelpers.assertThrows or Assertions. That's important so that you can add assertions about the error message, and so that you can run assertions to validate that state did not change. In this case, the the branch name should not exist and the snapshot list should be the same size.

rdblue · 2022-07-31T22:31:43Z

core/src/test/java/org/apache/iceberg/TestFastAppend.java

+            .appendFile(FILE_A)
+            .commit();
+    table.manageSnapshots().createBranch("ref", table.currentSnapshot().snapshotId()).commit();
+    table.newFastAppend().appendFile(FILE_B).toBranch("ref").commit();


Could this implement branch commits for FastAppend? That way we can see one operation that works from end-to-end when reviewing the changes to SnapshotUpdate.

@rdblue Have written the complete test-case in #5010.

rdblue · 2022-07-31T22:41:05Z

core/src/main/java/org/apache/iceberg/SnapshotProducer.java

+    Long parentSnapshotId = base.ref(targetBranch) != null ? base.ref(targetBranch).snapshotId() : null;
    long sequenceNumber = base.nextSequenceNumber();

    // run validations from the child operation


This needs to pass the parent snapshot ID into validate along with table metadata (base) so that validations can check changes between the starting snapshot ID and the parent snapshot, rather than between the starting snapshot ID and the current snapshot.

Same thing with apply.

@rdblue My thought process is to check the whether starting snapshot ID is an ancestor of parent snapshot. If not then throw invalid starting snapshot ID exception. Else we should go ahead with checking between starting snapshot ID & parent ID.

https://github.com/namrathamyske/iceberg/pull/1/files should take care of what @rdblue is talking about here which if I'm not mistaken is more about a cleaner abstraction for applying a change to a specific snapshot, and running a validation to a specific snapshot which will be needed when we implement branch support for the other producers.

amogh-jahagirdar · 2022-08-03T02:40:23Z

@namrathamyske Thanks for all the work on this! I've raised a PR https://github.com/namrathamyske/iceberg/pull/1/files to your fork, just addressing some of the comments @rdblue brought up, would love to get your thoughts on it. Also @rdblue would like to get your take as well.

master sync - PR apache#4926

…mplementation, append manifest lists of parent snapshot to the new manifests Add some more tests

Core: Pass in parent snapshot to apply and validate.

amogh-jahagirdar · 2022-08-04T16:52:04Z

Thanks for the review on the PR @namrathamyske. @rdblue could we get a review on the updated PR when you get a chance? Thanks!

amogh-jahagirdar · 2022-08-04T17:01:06Z

core/src/test/java/org/apache/iceberg/TestFastAppend.java

+  public void testAppendToExistingBranch() {
+    table.newFastAppend().appendFile(FILE_A).commit();
+    table.manageSnapshots().createBranch("branch", table.currentSnapshot().snapshotId()).commit();
+    table.newFastAppend().appendFile(FILE_B).toBranch("branch").commit();


Just realized I just hardcoded branch throughout the tests, probably cleaner to just define a variable in the tests and use that.

I think this is okay.

amogh-jahagirdar · 2022-08-04T19:23:10Z

core/src/main/java/org/apache/iceberg/SnapshotProducer.java

-        base.currentSnapshot() != null ? base.currentSnapshot().snapshotId() : null;
-    long sequenceNumber = base.nextSequenceNumber();
+    Snapshot parentSnapshot = base.currentSnapshot();
+    if (targetBranch != null) {


Also considering targetBranch will always be set (we have it default to Main, we don't need this check).

amogh-jahagirdar · 2022-08-04T22:41:13Z

core/src/test/java/org/apache/iceberg/TestOverwrite.java

+    AssertHelpers.assertThrows(
+        "Cannot commit to branch someBranch: org.apache.iceberg.BaseOverwriteFiles does not support branch commits",
+        UnsupportedOperationException.class,
+        () ->
+            table
+                .newOverwrite()
+                .overwriteByRowFilter(and(equal("date", "2018-06-09"), lessThan("id", 20)))
+                .addFile(FILE_10_TO_14)
+                .toBranch("someBranch"));


I should fix this assertion so it actually checks the expected error message; I think the assertThrows method with 3 params just surfaces this message in case the test fails, it doesn't actually check the expected error message. Like what's being done in TestReplacePartitions.

This is minor because it is going to be replaced very soon.

amogh-jahagirdar · 2022-08-07T17:29:51Z

core/src/main/java/org/apache/iceberg/SnapshotProducer.java

   *
   * @param metadataToUpdate the base table metadata to apply changes to
-   * @param snapshot
+   * @param snapshot parent snapshot passed to child operations.


Ah thanks for adding the commenting. Just a nit, I think this should say something like "snapshot to apply the changes to". Child operations doesn't make too much sense to me imo.

I think we just need to say "Apply the changes with the given metadata to the snapshot".

rdblue · 2022-08-07T17:40:41Z

api/src/main/java/org/apache/iceberg/SnapshotUpdate.java

+   *
+   * @param branch which is name of SnapshotRef of type branch.
+   */
+  ThisT toBranch(String branch);


This needs a default that throws UnsupportedOperationException.

rdblue · 2022-08-07T17:41:07Z

core/src/main/java/org/apache/iceberg/SnapshotProducer.java

+        String.format(
+            "Cannot commit to branch %s: %s does not support branch commits",
+            branch, this.getClass().getName()));
+  }


This should be in the interface, not here.

good point, fixed

rdblue · 2022-08-07T17:41:40Z

Looks like this is close, but we need to fix the toBranch definition and revapi checks.

amogh-jahagirdar · 2022-08-07T18:22:04Z

@namrathamyske raised a PR to your fork to address the revapi and moving the default implementation up to the interface level. https://github.com/namrathamyske/iceberg/pull/3/files

…s unsupported. Allow new API in revapi

Core, API: Default implementation of toBranch on SnapshotUpdate throws unsupported.

namrathamyske · 2022-08-07T23:29:46Z

@rdblue Merged PR with @amogh-jahagirdar's changes which had changes wrt to review comments.

amogh-jahagirdar · 2022-08-08T19:24:02Z

Thanks @jackye1995 for triggering the approval workflow. @rdblue when you get a chance, let us know how the change looks!

jackye1995

also looks good to me!

amogh-jahagirdar

Thanks for the contribution @namrathamyske !

jackye1995 · 2022-08-11T00:22:29Z

Looks like we are all in agreement, I will merge this to unblock next step implementations. thanks for the contribution @namrathamyske @amogh-jahagirdar ! and thanks for the review @rdblue !

…ef (apache#4926)

github-actions bot added API core labels Jun 1, 2022

namrathamyske mentioned this pull request Jun 1, 2022

Support Iceberg branching for all the snapshot producer operations #3896

Closed

amogh-jahagirdar requested changes Jun 1, 2022

View reviewed changes

namrathamyske changed the title ~~Append changes to snapshot branch~~ Core, API: Append changes to snapshot branch Jun 1, 2022

amogh-jahagirdar requested changes Jun 3, 2022

View reviewed changes

rdblue reviewed Jun 3, 2022

View reviewed changes

api/src/main/java/org/apache/iceberg/SnapshotUpdate.java Outdated Show resolved Hide resolved

rdblue reviewed Jun 3, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/SnapshotProducer.java Outdated Show resolved Hide resolved

rdblue reviewed Jun 3, 2022

View reviewed changes

core/src/test/java/org/apache/iceberg/TestFastAppend.java Outdated Show resolved Hide resolved

namrathamyske requested a review from rdblue June 8, 2022 16:36

namrathamyske force-pushed the master branch 2 times, most recently from e593035 to 3a1964d Compare June 9, 2022 20:20

namrathamyske force-pushed the master branch from 8c6280f to 9e43889 Compare June 9, 2022 20:22

namrathamyske mentioned this pull request Jun 9, 2022

Core, API: Append to branch Impl #5010

Closed

namrathamyske changed the title ~~Core, API: Append changes to snapshot branch~~ Core, API: Append/Delete changes to snapshot branch Jun 20, 2022

nkeshavaprakash added 2 commits June 24, 2022 17:09

checkStyle errors

0e2f690

checkStyle errors

Merge branch 'master' of https://github.com/apache/iceberg

bcfb7a8

namrathamyske mentioned this pull request Jul 2, 2022

Core, API: RowDelta changes to branch Impl #5181

Closed

Merge branch 'master' of https://github.com/apache/iceberg

8d80888

namrathamyske changed the title ~~Core, API: Append/Delete changes to snapshot branch~~ Core, API: Performing Table Operations to a snapshot branch Jul 9, 2022

namrathamyske changed the title ~~Core, API: Performing Table Operations to a snapshot branch~~ Core, API: Performing Table Operations to a snapshot branch ref Jul 9, 2022

namrathamyske changed the title ~~Core, API: Performing Table Operations to a snapshot branch ref~~ Core, API: Performing Operations on a snapshot branch ref Jul 9, 2022

rdblue reviewed Jul 31, 2022

View reviewed changes

conflicts

c87df9a

namrathamyske added a commit to SreeramGarlapati/iceberg that referenced this pull request Aug 3, 2022

Merge pull request #6 from namrathamyske/master

7bb2c49

master sync - PR apache#4926

amogh-jahagirdar and others added 2 commits August 3, 2022 23:47

Core: Pass in parent snapshot to apply and validate. For FastAppend i…

9e98792

…mplementation, append manifest lists of parent snapshot to the new manifests Add some more tests

Merge pull request #1 from amogh-jahagirdar/branch-snapshot-producing

9c1a2d2

Core: Pass in parent snapshot to apply and validate.

amogh-jahagirdar reviewed Aug 4, 2022

View reviewed changes

rdblue approved these changes Aug 5, 2022

View reviewed changes

checkstyle fix

4772047

amogh-jahagirdar reviewed Aug 7, 2022

View reviewed changes

rdblue reviewed Aug 7, 2022

View reviewed changes

amogh-jahagirdar and others added 3 commits August 7, 2022 11:22

Core, API: Default implementation of toBranch on SnapshotUpdate throw…

790613d

…s unsupported. Allow new API in revapi

Merge branch 'master' into branch-snapshot-producing

6948eac

Merge pull request #3 from amogh-jahagirdar/branch-snapshot-producing

3a60283

Core, API: Default implementation of toBranch on SnapshotUpdate throws unsupported.

jackye1995 approved these changes Aug 8, 2022

View reviewed changes

amogh-jahagirdar approved these changes Aug 11, 2022

View reviewed changes

jackye1995 merged commit 5a15efc into apache:master Aug 11, 2022

nastra mentioned this pull request Dec 1, 2022

Core|ORC|Spark: Remove deprecated functionality #6274

Merged

zhongyujiang pushed a commit to zhongyujiang/iceberg that referenced this pull request Apr 16, 2025

[Cherry-Pick] Core, API: Performing operations on a snapshot branch r…

4cd130f

…ef (apache#4926)

Core, API: Performing operations on a snapshot branch ref #4926

Core, API: Performing operations on a snapshot branch ref #4926

Uh oh!

Conversation

namrathamyske commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

namrathamyske commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amogh-jahagirdar left a comment • 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!

amogh-jahagirdar Jun 21, 2022 • 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!

namrathamyske Jun 21, 2022 • 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!

Uh oh!

Uh oh!

Uh oh!

namrathamyske commented Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

namrathamyske commented Jun 13, 2022

Uh oh!

amogh-jahagirdar commented Jun 21, 2022

Uh oh!

namrathamyske commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

namrathamyske commented Jul 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar commented Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amogh-jahagirdar commented Aug 4, 2022

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!

amogh-jahagirdar Aug 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

namrathamyske commented Jun 1, 2022 •

edited

Loading

namrathamyske commented Jun 1, 2022 •

edited

Loading

amogh-jahagirdar left a comment •

edited

Loading

amogh-jahagirdar Jun 21, 2022 •

edited

Loading

namrathamyske Jun 21, 2022 •

edited

Loading

namrathamyske commented Jun 6, 2022 •

edited

Loading

namrathamyske commented Jun 21, 2022 •

edited

Loading

namrathamyske commented Jul 9, 2022 •

edited

Loading

amogh-jahagirdar commented Aug 3, 2022 •

edited

Loading

amogh-jahagirdar Aug 7, 2022 •

edited

Loading