xiaoxuandev commented on code in PR #17764:
URL: https://github.com/apache/iceberg/pull/17764#discussion_r3909883719
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -528,87 +528,117 @@ private void validateNoNewDeletesForDataFiles(
return;
}
- DeleteFileIndex deletes = addedDeleteFiles(base, startingSnapshotId,
dataFilter, null, parent);
+ List<DeleteFileIndex> deleteIndexes =
+ addedDeleteFileIndexes(base, startingSnapshotId, dataFilter, null,
parent);
long startingSequenceNumber = startingSequenceNumber(base,
startingSnapshotId);
for (DataFile dataFile : dataFiles) {
- // if any delete is found that applies to files written in or before the
starting snapshot,
- // fail
- DeleteFile[] deleteFiles = deletes.forDataFile(startingSequenceNumber,
dataFile);
- if (ignoreEqualityDeletes) {
- ValidationException.check(
- Arrays.stream(deleteFiles)
- .noneMatch(deleteFile -> deleteFile.content() ==
FileContent.POSITION_DELETES),
- "Cannot commit, found new position delete for replaced data file:
%s",
- dataFile);
- } else {
- ValidationException.check(
- deleteFiles.length == 0,
- "Cannot commit, found new delete for replaced data file: %s",
- dataFile);
+ for (DeleteFileIndex deletes : deleteIndexes) {
+ // if any delete is found that applies to files written in or before
the starting snapshot,
+ // fail
+ DeleteFile[] deleteFiles = deletes.forDataFile(startingSequenceNumber,
dataFile);
+ if (ignoreEqualityDeletes) {
+ ValidationException.check(
+ !containsPositionDeletes(deleteFiles),
+ "Cannot commit, found new position delete for replaced data
file: %s",
+ dataFile);
+ } else {
+ ValidationException.check(
+ deleteFiles.length == 0,
+ "Cannot commit, found new delete for replaced data file: %s",
+ dataFile);
+ }
+ }
+ }
+ }
+
+ private static boolean containsPositionDeletes(DeleteFile[] deleteFiles) {
Review Comment:
Mostly readability, and allocation on the default compaction path; the loop
is now nested (D×N), and compaction defaults into the `ignoreEqualityDeletes`
branch. Since `forDataFile` returns an empty array on no conflict, the inline
Arrays.stream(...) allocated a pipeline every success-path iteration; the plain
loop avoids that and reads clearer. Escape analysis might elide it, but isn't
guaranteed to. Small either way.
--
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]
