rdblue commented on a change in pull request #2925:
URL: https://github.com/apache/iceberg/pull/2925#discussion_r698069378
##########
File path: core/src/test/java/org/apache/iceberg/TestReplacePartitions.java
##########
@@ -253,6 +253,189 @@ public void testValidationSuccess() {
statuses(Status.ADDED, Status.ADDED));
}
+ @Test
+ public void testReplaceConflictPartitionedTable() {
+ table.newFastAppend()
+ .appendFile(FILE_A)
+ .appendFile(FILE_B)
+ .commit();
+
+ TableMetadata base = readMetadata();
+ long baseId = base.currentSnapshot().snapshotId();
+
+ // Concurrent Replace
+ table.newReplacePartitions()
+ .addFile(FILE_A)
+ .commit();
+
+ ReplacePartitions replace = table.newReplacePartitions()
+ .validateFromSnapshot(baseId)
+ .validateNoConflictingAppends()
+ .addFile(FILE_A)
+ .addFile(FILE_B);
+
+ AssertHelpers.assertThrows("Should reject commit with file matching
partitions replaced",
+ ValidationException.class,
+ "Found conflicting files that can contain records matching true:
[/path/to/data-a.parquet]",
+ replace::commit);
+ }
+
+ @Test
+ public void testAppendConflictPartitionedTable() {
+ table.newFastAppend()
+ .appendFile(FILE_A)
+ .commit();
+
+ TableMetadata base = readMetadata();
+ long baseId = base.currentSnapshot().snapshotId();
+
+ // Concurrent Insert
+ table.newFastAppend()
+ .appendFile(FILE_B)
+ .commit();
+
+ ReplacePartitions replace = table.newReplacePartitions()
+ .validateFromSnapshot(baseId)
+ .validateNoConflictingAppends()
+ .addFile(FILE_A)
+ .addFile(FILE_B);
+
+ AssertHelpers.assertThrows("Should reject commit with file matching
partitions replaced",
+ ValidationException.class,
+ "Found conflicting files that can contain records matching true:
[/path/to/data-b.parquet]",
+ replace::commit);
+ }
+
+ @Test
+ public void testNoReplaceConflictPartitionedTable() {
+ table.newFastAppend()
+ .appendFile(FILE_A)
+ .commit();
+
+ TableMetadata base = readMetadata();
+ long baseId = base.currentSnapshot().snapshotId();
+
+ // Concurrent Insert
+ table.newReplacePartitions()
+ .addFile(FILE_E)
+ .commit();
+
+ table.newReplacePartitions()
+ .validateFromSnapshot(baseId)
+ .addFile(FILE_A)
+ .addFile(FILE_B)
+ .commit();
+
+ long replaceId = readMetadata().currentSnapshot().snapshotId();
+ Assert.assertEquals("Table should have 2 manifest",
+ 2, table.currentSnapshot().allManifests().size());
+ validateManifestEntries(table.currentSnapshot().allManifests().get(0),
+ ids(replaceId, replaceId),
+ files(FILE_A, FILE_B),
+ statuses(Status.ADDED, Status.ADDED));
+
+ validateManifestEntries(table.currentSnapshot().allManifests().get(1),
+ ids(replaceId),
+ files(FILE_E),
+ statuses(Status.DELETED));
+ }
+
+ @Test
+ public void testNoAppendConflictPartitionedTable() {
+ table.newFastAppend()
+ .appendFile(FILE_A)
+ .commit();
+
+ TableMetadata base = readMetadata();
+ long baseId = base.currentSnapshot().snapshotId();
+
+ // Concurrent Insert
+ table.newFastAppend()
+ .appendFile(FILE_B)
+ .commit();
+
+ table.newReplacePartitions()
+ .validateFromSnapshot(baseId)
+ .addFile(FILE_E)
+ .addFile(FILE_F)
+ .commit();
+
+ long replaceId = readMetadata().currentSnapshot().snapshotId();
+ Assert.assertEquals("Table should have 3 manifest",
+ 3, table.currentSnapshot().allManifests().size());
+
+ validateManifestEntries(table.currentSnapshot().allManifests().get(0),
+ ids(replaceId, replaceId),
+ files(FILE_E, FILE_F),
+ statuses(Status.ADDED, Status.ADDED));
+
+ validateManifestEntries(table.currentSnapshot().allManifests().get(1),
+ ids(replaceId),
+ files(FILE_B),
+ statuses(Status.DELETED));
+
+ validateManifestEntries(table.currentSnapshot().allManifests().get(2),
+ ids(replaceId),
+ files(FILE_A),
+ statuses(Status.DELETED));
+ }
+
+ @Test
+ public void testReplaceConflictNonPartitionedTable() {
+ Table unpartitioned = TestTables.create(
+ tableDir, "unpartitioned", SCHEMA, PartitionSpec.unpartitioned(),
formatVersion);
+ unpartitioned.newAppend()
+ .appendFile(FILE_A)
+ .commit();
+
+ TableMetadata replaceMetadata = TestTables.readMetadata("unpartitioned");
+ long replaceBaseId = replaceMetadata.currentSnapshot().snapshotId();
+
+ // Intermediate commit
+ unpartitioned.newReplacePartitions()
+ .addFile(FILE_A)
+ .commit();
+
+ ReplacePartitions replace = unpartitioned.newReplacePartitions()
+ .validateFromSnapshot(replaceBaseId)
+ .validateNoConflictingAppends()
+ .addFile(FILE_A)
+ .addFile(FILE_B);
+
+ AssertHelpers.assertThrows("Should reject commit with file matching
partitions replaced",
+ ValidationException.class,
+ "Found conflicting files that can contain records matching true:
[/path/to/data-a.parquet]",
+ replace::commit);
Review comment:
I think it is usually easier to read tests if you put the whole
`replace` operation here, like this:
```java
() ->
unpartitioned.newReplacePartitions()
...
.commit());
```
--
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]
