rdblue commented on code in PR #14509:
URL: https://github.com/apache/iceberg/pull/14509#discussion_r2496309974
##########
core/src/test/java/org/apache/iceberg/TestSnapshotProducer.java:
##########
@@ -74,4 +81,105 @@ private void assertManifestWriterCount(
int writerCount = SnapshotProducer.manifestWriterCount(workerPoolSize,
fileCount);
assertThat(writerCount).as(errMsg).isEqualTo(expectedManifestWriterCount);
}
+
+ @TestTemplate
+ public void testCommitValidationPreventingCommit() throws IOException {
+ // Commit the first file
+ table.newAppend().appendFile(FILE_A).commit();
+
+ // Create a file with no records for testing
+ DataFile fileNoRecords =
+ DataFiles.builder(SPEC)
+ .withPath("/path/to/data-no-records.parquet")
+ .withFileSizeInBytes(100)
+ .withRecordCount(0) // File with no records
+ .build();
+
+ // Create a CommitValidator that will reject commits based on snapshot
summary
+ SnapshotUpdateValidator validator =
+ (baseSnapshots, updatedSnapshots) -> {
+ long addedRecords =
+ PropertyUtil.propertyAsInt(
+ updatedSnapshots.iterator().next().summary(),
+ SnapshotSummary.ADDED_RECORDS_PROP,
+ 0);
+ long addedFiles =
+ PropertyUtil.propertyAsInt(
+ updatedSnapshots.iterator().next().summary(),
+ SnapshotSummary.ADDED_FILES_PROP,
+ 0);
+ // Reject if no records are added (empty file)
+ if (addedFiles >= 1 && addedRecords == 0) {
+ throw new CommitFailedException("Cannot add files with no
records");
+ }
+ };
+
+ // Test that the validator rejects commits with no records
+ AppendFiles append1 =
table.newAppend().validateWith(validator).appendFile(fileNoRecords);
+ assertThatThrownBy(append1::commit)
+ .isInstanceOf(CommitFailedException.class)
+ .hasMessage("Cannot add files with no records");
+
+ // Verify the file was not committed
+ assertThat(table.currentSnapshot().allManifests(table.io())).hasSize(1);
+
assertThat(table.currentSnapshot().summary().get(SnapshotSummary.TOTAL_DATA_FILES_PROP))
+ .isEqualTo("1");
+
+ // Verify files were not committed
+
assertThat(table.currentSnapshot().summary().get(SnapshotSummary.TOTAL_DATA_FILES_PROP))
+ .isEqualTo("1");
+
+ // Test that a valid commit passes the validator (FILE_B has only 1 record)
+ AppendFiles append2 =
table.newFastAppend().validateWith(validator).appendFile(FILE_B);
+ append2.commit();
+
+ // Verify the file was committed successfully
+
assertThat(table.currentSnapshot().summary().get(SnapshotSummary.TOTAL_DATA_FILES_PROP))
+ .isEqualTo("2");
+
assertThat(table.currentSnapshot().summary().get(SnapshotSummary.ADDED_FILES_PROP))
+ .isEqualTo("1");
+
assertThat(table.currentSnapshot().summary().get(SnapshotSummary.ADDED_RECORDS_PROP))
+ .isEqualTo("1");
+ }
+
+ @TestTemplate
+ public void testCommitValidationWithCustomSummaryProperties() throws
IOException {
+ // Create a validator that checks custom summary properties
+ SnapshotUpdateValidator customPropertyValidator =
Review Comment:
I think a better test case would be to check for a particular commit ID in
history. That's a real use case that we've seen, where two committers are
trying to cherry-pick the same WAP ID into the main branch. Both should have a
validation that the table doesn't already have a commit for that ID. That's
also fairly easy to test.
--
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]
