amogh-jahagirdar commented on code in PR #4926:
URL: https://github.com/apache/iceberg/pull/4926#discussion_r886365979
##########
api/src/main/java/org/apache/iceberg/SnapshotUpdate.java:
##########
@@ -60,4 +60,8 @@
* @return this for method chaining
*/
ThisT scanManifestsWith(ExecutorService executorService);
+ /**
Review Comment:
newline before this
##########
core/src/test/java/org/apache/iceberg/TestFastAppend.java:
##########
@@ -478,4 +478,27 @@ 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() {
+ table.newFastAppend()
+ .appendFile(FILE_C)
+ .commit();
+
+ table.newFastAppend()
+ .appendFile(FILE_B)
+ .commit();
+
+
table.manageSnapshots().createBranch("branch",table.currentSnapshot().snapshotId());
+
+ table.newFastAppend()
+ .appendFile(FILE_A)
+ .commit();
+
+ table.newOverwrite()
+ .deleteFile(FILE_A)
+ .branch("branch")
+ .commit();
+ //Check for refs having snapshot which depends on PR
https://github.com/apache/iceberg/pull/4428
Review Comment:
I'd remove the comment, and yeah I'll work on getting that PR in to unblock
these tests
##########
core/src/test/java/org/apache/iceberg/TestFastAppend.java:
##########
@@ -478,4 +478,27 @@ 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() {
+ table.newFastAppend()
+ .appendFile(FILE_C)
+ .commit();
+
+ table.newFastAppend()
+ .appendFile(FILE_B)
+ .commit();
+
+
table.manageSnapshots().createBranch("branch",table.currentSnapshot().snapshotId());
+
+ table.newFastAppend()
+ .appendFile(FILE_A)
Review Comment:
Assuming here in the test you want to add file A to the new branch?
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -298,11 +305,21 @@ public void commit() {
TableMetadata.Builder update = TableMetadata.buildFrom(base);
if (base.snapshot(newSnapshot.snapshotId()) != null) {
// this is a rollback operation
- update.setBranchSnapshot(newSnapshot.snapshotId(),
SnapshotRef.MAIN_BRANCH);
+ if (this.branchToUpdate != null) {
Review Comment:
When reading the field prefer not using "this", only when setting it.
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -86,8 +86,9 @@ public void accept(String file) {
private TableMetadata base;
private boolean stageOnly = false;
private Consumer<String> deleteFunc = defaultDelete;
-
private ExecutorService workerPool = ThreadPools.getWorkerPool();
+ private String branchToUpdate = null;
+
Review Comment:
Nit: remove newline 91
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -86,8 +86,9 @@ public void accept(String file) {
private TableMetadata base;
private boolean stageOnly = false;
private Consumer<String> deleteFunc = defaultDelete;
-
Review Comment:
Nit: Undo the removal of this line, ideally the useless newline wasn't there
in the first place but removing it adds more unnecessary changes which can
cause conflicts for others
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -298,11 +305,21 @@ public void commit() {
TableMetadata.Builder update = TableMetadata.buildFrom(base);
if (base.snapshot(newSnapshot.snapshotId()) != null) {
// this is a rollback operation
- update.setBranchSnapshot(newSnapshot.snapshotId(),
SnapshotRef.MAIN_BRANCH);
Review Comment:
I think we can have a 1 liner for the branch that way we don't need all the
if/else branching below.
`String branch = branchToUpdate == null ? SnapshotRef.MAIN_BRANCH :
branchToUpdate`
then just setBranchSnapshot(snaphotId, branch)
--
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]
