nastra commented on code in PR #9196:
URL: https://github.com/apache/iceberg/pull/9196#discussion_r1459108230
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestFastForwardBranchProcedure.java:
##########
@@ -247,4 +234,39 @@ public void testFastForwardNonMain() {
assertThat(outputRow.get(1)).isEqualTo(branch1Snapshot.snapshotId());
assertThat(outputRow.get(2)).isEqualTo(branch2Snapshot.snapshotId());
}
+
+ @Test
+ public void testFastForwardNonExistingFromMainCreatesBranch() {
+ sql("CREATE TABLE %s (id int NOT NULL, data string) USING iceberg",
tableName);
+ String branch1 = "branch1";
+ sql("ALTER TABLE %s CREATE BRANCH %s", tableName, branch1);
+ String branchIdentifier = String.format("%s.branch_%s", tableName,
branch1);
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", branchIdentifier);
+ sql("INSERT INTO TABLE %s VALUES (2, 'b')", branchIdentifier);
+ Table table = validationCatalog.loadTable(tableIdent);
+ table.refresh();
+ Snapshot branch1Snapshot = table.snapshot(branch1);
+
+ List<Object[]> output =
+ sql(
+ "CALL %s.system.fast_forward('%s', '%s', '%s')",
+ catalogName, tableIdent, SnapshotRef.MAIN_BRANCH, branch1);
+ List<Object> outputRow =
Arrays.stream(output.get(0)).collect(Collectors.toList());
Review Comment:
can this be simplified to
`assertThat(sql(...)).containsExactly(row(SnapshotRef.MAIN_BRANCH, null,
branch1Snapshot.snapshotId()));`?
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/FastForwardBranchProcedure.java:
##########
@@ -73,19 +71,16 @@ public StructType outputType() {
@Override
public InternalRow[] call(InternalRow args) {
Identifier tableIdent = toIdentifier(args.getString(0),
PARAMETERS[0].name());
- String source = args.getString(1);
- String target = args.getString(2);
+ String from = args.getString(1);
+ String to = args.getString(2);
return modifyIcebergTable(
tableIdent,
table -> {
- Snapshot currentSnapshot = table.snapshot(source);
- Preconditions.checkArgument(
- currentSnapshot != null, "Branch to fast-forward does not exist:
%s", source);
- table.manageSnapshots().fastForwardBranch(source, target).commit();
- long latest = table.snapshot(source).snapshotId();
- InternalRow outputRow =
- newInternalRow(UTF8String.fromString(source),
currentSnapshot.snapshotId(), latest);
+ Long before = table.snapshot(from) != null ?
table.snapshot(from).snapshotId() : null;
Review Comment:
maybe this should have `snapshotId` in the name to make reading the code a
bit easier. Same for the `after`
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestFastForwardBranchProcedure.java:
##########
@@ -247,4 +234,39 @@ public void testFastForwardNonMain() {
assertThat(outputRow.get(1)).isEqualTo(branch1Snapshot.snapshotId());
assertThat(outputRow.get(2)).isEqualTo(branch2Snapshot.snapshotId());
}
+
+ @Test
+ public void testFastForwardNonExistingFromMainCreatesBranch() {
+ sql("CREATE TABLE %s (id int NOT NULL, data string) USING iceberg",
tableName);
+ String branch1 = "branch1";
+ sql("ALTER TABLE %s CREATE BRANCH %s", tableName, branch1);
+ String branchIdentifier = String.format("%s.branch_%s", tableName,
branch1);
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", branchIdentifier);
+ sql("INSERT INTO TABLE %s VALUES (2, 'b')", branchIdentifier);
+ Table table = validationCatalog.loadTable(tableIdent);
+ table.refresh();
+ Snapshot branch1Snapshot = table.snapshot(branch1);
+
+ List<Object[]> output =
+ sql(
+ "CALL %s.system.fast_forward('%s', '%s', '%s')",
+ catalogName, tableIdent, SnapshotRef.MAIN_BRANCH, branch1);
+ List<Object> outputRow =
Arrays.stream(output.get(0)).collect(Collectors.toList());
+
+ assertThat(outputRow.get(0)).isEqualTo(SnapshotRef.MAIN_BRANCH);
+ assertThat(outputRow.get(1)).isNull();
+ assertThat(outputRow.get(2)).isEqualTo(branch1Snapshot.snapshotId());
+
+ // Ensure the same behavior for non-main branches
+ String branch2 = "branch2";
+ output =
+ sql(
+ "CALL %s.system.fast_forward('%s', '%s', '%s')",
+ catalogName, tableIdent, branch2, branch1);
+ outputRow = Arrays.stream(output.get(0)).collect(Collectors.toList());
Review Comment:
same as above
##########
api/src/main/java/org/apache/iceberg/ManageSnapshots.java:
##########
@@ -175,7 +176,8 @@ default ManageSnapshots createBranch(String name) {
/**
* Performs a fast-forward of {@code from} up to the {@code to} snapshot if
{@code from} is an
* ancestor of {@code to}. The {@code to} will remain unchanged, and {@code
from} will retain its
- * retention properties.
+ * retention properties. If {@code from} does not exist, it will be created
with default retention
Review Comment:
```suggestion
* retention properties. If {@code from} branch does not exist, it will be
created with default retention
```
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
