amogh-jahagirdar commented on code in PR #5234:
URL: https://github.com/apache/iceberg/pull/5234#discussion_r952679192
##########
core/src/main/java/org/apache/iceberg/BaseRowDelta.java:
##########
@@ -95,24 +97,46 @@ public RowDelta validateNoConflictingDeleteFiles() {
return this;
}
+ @Override
+ public RowDelta toBranch(String branch) {
+ targetBranch(branch);
+ return this;
+ }
+
+ private void checkIfSnapshotIsAnAncestor(Snapshot snapshot, TableMetadata
base) {
+ if (this.startingSnapshotId == null || snapshot == null) {
+ return;
+ }
+
+ for (Snapshot ancestor : SnapshotUtil.ancestorsOf(snapshot.snapshotId(),
base::snapshot)) {
+ if (ancestor.snapshotId() == this.startingSnapshotId) {
+ return;
+ }
+ }
+ throw new ValidationException(
+ "Snapshot %s is not an ancestor of %s", startingSnapshotId,
snapshot.snapshotId());
+ }
Review Comment:
We can use SnapshotUtil.isAncestorOf, and I think we can extract the helper
to a protected method on the SnapshotProducer class because it would be valid
for every operation.
##########
core/src/test/java/org/apache/iceberg/TestRowDelta.java:
##########
@@ -1414,18 +1415,69 @@ public void testRowDeltaCaseSensitivity() {
}
@Test
- public void testRowDeltaToBranchUnsupported() {
- AssertHelpers.assertThrows(
- "Should reject committing row delta to branch",
- UnsupportedOperationException.class,
- "Cannot commit to branch someBranch: org.apache.iceberg.BaseRowDelta
does not support branch commits",
- () ->
+ public void testBranchConflictingDeletes() {
+ table.newAppend().appendFile(FILE_A).commit();
+ Long ancestorSnapshot = table.currentSnapshot().snapshotId();
+ table.manageSnapshots().createBranch("branch", ancestorSnapshot).commit();
+
+ // This commit not result in validation exception as we start validation
from a snapshot which
+ // is an actual ancestor of the branch
+ table
+ .newRowDelta()
+ .toBranch("branch")
+ .addDeletes(FILE_A_DELETES)
+ .validateFromSnapshot(ancestorSnapshot)
+ .conflictDetectionFilter(Expressions.equal("data", "a"))
+ .validateNoConflictingDeleteFiles()
+ .commit();
Review Comment:
Let's double check the style, I think we just need to run spotless here.
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -304,9 +304,12 @@ protected void validateAddedDataFiles(
* @param conflictDetectionFilter an expression used to find new conflicting
data files
*/
protected void validateAddedDataFiles(
- TableMetadata base, Long startingSnapshotId, Expression
conflictDetectionFilter) {
+ TableMetadata base,
+ Long startingSnapshotId,
+ Expression conflictDetectionFilter,
+ Snapshot snapshot) {
Review Comment:
Would it make sense to pass in just the snapshot id, and call it
endingSnapshotId? It also feels more readable to put the parameter next to
startingSnapshotId. For reference:
https://github.com/apache/iceberg/pull/5595/files#diff-f2d4265e007483f23a69703b88348743aecf99cc98395cd1730de3900a609eb6R339
##########
core/src/test/java/org/apache/iceberg/TestRowDelta.java:
##########
@@ -1414,18 +1415,69 @@ public void testRowDeltaCaseSensitivity() {
}
@Test
- public void testRowDeltaToBranchUnsupported() {
- AssertHelpers.assertThrows(
- "Should reject committing row delta to branch",
- UnsupportedOperationException.class,
- "Cannot commit to branch someBranch: org.apache.iceberg.BaseRowDelta
does not support branch commits",
- () ->
+ public void testBranchConflictingDeletes() {
Review Comment:
This test looks like we're testing a case where there aren't conflicting
deletes right?
##########
core/src/test/java/org/apache/iceberg/TestRowDelta.java:
##########
@@ -1414,18 +1415,69 @@ public void testRowDeltaCaseSensitivity() {
}
@Test
- public void testRowDeltaToBranchUnsupported() {
- AssertHelpers.assertThrows(
- "Should reject committing row delta to branch",
- UnsupportedOperationException.class,
- "Cannot commit to branch someBranch: org.apache.iceberg.BaseRowDelta
does not support branch commits",
- () ->
+ public void testBranchConflictingDeletes() {
+ table.newAppend().appendFile(FILE_A).commit();
+ Long ancestorSnapshot = table.currentSnapshot().snapshotId();
+ table.manageSnapshots().createBranch("branch", ancestorSnapshot).commit();
+
+ // This commit not result in validation exception as we start validation
from a snapshot which
+ // is an actual ancestor of the branch
+ table
+ .newRowDelta()
+ .toBranch("branch")
+ .addDeletes(FILE_A_DELETES)
+ .validateFromSnapshot(ancestorSnapshot)
+ .conflictDetectionFilter(Expressions.equal("data", "a"))
+ .validateNoConflictingDeleteFiles()
+ .commit();
+
+ int branchSnapshot = 2;
+
+ Assert.assertEquals(table.currentSnapshot().snapshotId(), 1);
+ Assert.assertEquals(table.ops().current().ref("branch").snapshotId(),
branchSnapshot);
+ }
+
+ @Test
+ public void testBranchValidationsNotValidAncestor() {
+ table.newAppend().appendFile(FILE_A).commit();
+ table.manageSnapshots().createBranch("branch",
table.currentSnapshot().snapshotId()).commit();
+ table.newAppend().appendFile(FILE_B).commit();
+
+ // This commit will result in validation exception as we start validation
from a snapshot which
+ // is not an ancestor of the branch
+ RowDelta rowDelta =
table
- .newRowDelta()
- .caseSensitive(false)
- .addRows(FILE_B)
- .addDeletes(FILE_A2_DELETES)
- .toBranch("someBranch")
- .commit());
+ .newRowDelta()
+ .toBranch("branch")
+ .addDeletes(FILE_A_DELETES)
+ .validateFromSnapshot(table.currentSnapshot().snapshotId())
+ .conflictDetectionFilter(Expressions.alwaysTrue())
+ .validateNoConflictingDeleteFiles();
+
+ AssertHelpers.assertThrows(
+ "No matching ancestor found", ValidationException.class, () ->
rowDelta.commit());
+ }
+
+ @Test
+ public void testBranchValidationsValidAncestor() {
+ table.newAppend().appendFile(FILE_A).commit();
+ Long ancestorSnapshot = table.currentSnapshot().snapshotId();
+ table.manageSnapshots().createBranch("branch", ancestorSnapshot).commit();
+
+ // This commit not result in validation exception as we start validation
from a snapshot which
+ // is an actual ancestor of the branch
+ table
+ .newRowDelta()
+ .toBranch("branch")
+ .addDeletes(FILE_A_DELETES)
+ .validateFromSnapshot(ancestorSnapshot)
+ .conflictDetectionFilter(Expressions.alwaysTrue())
+ .validateNoConflictingDeleteFiles()
+ .commit();
Review Comment:
Same, spotless
--
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]
