amogh-jahagirdar commented on code in PR #5010:
URL: https://github.com/apache/iceberg/pull/5010#discussion_r902363638
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -116,6 +118,19 @@ public ThisT scanManifestsWith(ExecutorService
executorService) {
return self();
}
+ @Override
+ public ThisT toBranch(String branch){
+ Preconditions.checkArgument(branch != null,"branch cannot be null");
+ if (ops.current().ref(branch) == null) {
+ SnapshotRef branchRef =
SnapshotRef.branchBuilder(ops.current().currentSnapshot().snapshotId()).build();
+ TableMetadata.Builder updatedBuilder = TableMetadata.buildFrom(base);
+ updatedBuilder.setRef(branch, branchRef).build();
+ }
Review Comment:
Newline after the if block
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -89,6 +89,8 @@ public void accept(String file) {
private ExecutorService workerPool = ThreadPools.getWorkerPool();
+ protected String toBranch = null;
Review Comment:
Imo, I think the name targetBranch is more clear. it's used in a few other
places as well. Also if we're assigning it, should we just assign it to main
instead of null?
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -282,6 +299,7 @@ protected TableMetadata refresh() {
@Override
public void commit() {
// this is always set to the latest commit attempt's snapshot id.
+ String branch = toBranch == null ? SnapshotRef.MAIN_BRANCH : toBranch;
Review Comment:
If we set the default value of targetBranch as main, then the branch to
produce snapshots to just becomes targetBranch. Then we don't need to declare a
separate variable, and so we can remove this line
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -167,8 +182,10 @@ protected void validate(TableMetadata currentMetadata) {
@Override
public Snapshot apply() {
refresh();
- Long parentSnapshotId = base.currentSnapshot() != null ?
- base.currentSnapshot().snapshotId() : null;
+ Long parentSnapshotId = base.currentSnapshot() != null ?
base.currentSnapshot().snapshotId() : null;
+ if(toBranch != null){
+ parentSnapshotId = base.ref(toBranch).snapshotId();
+ }
Review Comment:
Same, newline after the if block
##########
core/src/test/java/org/apache/iceberg/TestFastAppend.java:
##########
@@ -478,4 +478,37 @@ public void testIncludedPartitionSummaryLimit() {
String changedPartitions =
table.currentSnapshot().summary().get(SnapshotSummary.CHANGED_PARTITION_COUNT_PROP);
Assert.assertEquals("Should set changed partition count", "2",
changedPartitions);
}
+
+ @Test
+ public void testAppendToBranch() throws UnsupportedOperationException {
+ table.newFastAppend()
+ .appendFile(FILE_A)
+ .commit();
+
+ Long currSnapshot = table.currentSnapshot().snapshotId();
+ table.manageSnapshots().createBranch("ref",
table.currentSnapshot().snapshotId()).commit();
+ table.newFastAppend().toBranch("ref").appendFile(FILE_B).commit();
+ Snapshot branch =
table.snapshot(table.ops().current().ref("ref").snapshotId());
+ Assert.assertEquals(currSnapshot, branch.parentId());
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void testAppendToNullBranch() {
+ table.newFastAppend()
+ .appendFile(FILE_A)
+ .commit();
Review Comment:
Ditto on indentation
##########
core/src/test/java/org/apache/iceberg/TestFastAppend.java:
##########
@@ -478,4 +478,37 @@ public void testIncludedPartitionSummaryLimit() {
String changedPartitions =
table.currentSnapshot().summary().get(SnapshotSummary.CHANGED_PARTITION_COUNT_PROP);
Assert.assertEquals("Should set changed partition count", "2",
changedPartitions);
}
+
+ @Test
+ public void testAppendToBranch() throws UnsupportedOperationException {
+ table.newFastAppend()
+ .appendFile(FILE_A)
+ .commit();
Review Comment:
Indentation, the continuing indentation should be 4
##########
core/src/test/java/org/apache/iceberg/TestFastAppend.java:
##########
@@ -478,4 +478,37 @@ public void testIncludedPartitionSummaryLimit() {
String changedPartitions =
table.currentSnapshot().summary().get(SnapshotSummary.CHANGED_PARTITION_COUNT_PROP);
Assert.assertEquals("Should set changed partition count", "2",
changedPartitions);
}
+
+ @Test
+ public void testAppendToBranch() throws UnsupportedOperationException {
+ table.newFastAppend()
+ .appendFile(FILE_A)
+ .commit();
+
+ Long currSnapshot = table.currentSnapshot().snapshotId();
+ table.manageSnapshots().createBranch("ref",
table.currentSnapshot().snapshotId()).commit();
+ table.newFastAppend().toBranch("ref").appendFile(FILE_B).commit();
+ Snapshot branch =
table.snapshot(table.ops().current().ref("ref").snapshotId());
+ Assert.assertEquals(currSnapshot, branch.parentId());
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void testAppendToNullBranch() {
+ table.newFastAppend()
+ .appendFile(FILE_A)
+ .commit();
+
+ table.manageSnapshots().createBranch("ref",
table.currentSnapshot().snapshotId()).commit();
+ table.newDelete().toBranch(null).deleteFile(FILE_A);
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void testAppendToInValidBranch() {
+ table.newFastAppend()
+ .appendFile(FILE_A)
+ .commit();
Review Comment:
Ditto on indentation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
