Re: [PR] Core: Fix thread conflict when deleting duplicate files in manifest [iceberg]

["amogh-jahagirdar (via GitHub)" <gi...@apache.org>]

amogh-jahagirdar commented on code in PR #16686:
URL: https://github.com/apache/iceberg/pull/16686#discussion_r3506909810


##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -533,16 +542,13 @@ private ManifestFile filterManifestWithDeletedFiles(
                     if (allRowsMatch) {
                       writer.delete(entry);
                       F fileCopy = file.copyWithoutStats();
-                      // add the file here in case it was deleted using an expression. The
-                      // DeleteManifestFilterManager will then remove its matching DV
-                      deleteFiles.add(fileCopy);
 
                       if (deletedFiles.contains(file)) {
                         LOG.warn(
                             "Deleting a duplicate path from manifest {}: {}",
                             manifest.path(),
                             file.location());
-                        duplicateDeleteCount += 1;
+                        duplicateDeleteCount.incrementAndGet();

Review Comment:
   After the most recent change, does this still need to be atomic int? It's only being updated in a single threaded manner as far as I can tell. 



##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -229,6 +228,15 @@ List<ManifestFile> filterManifests(Schema tableSchema, List<ManifestFile> manife
               filtered[index] = manifest;
             });
 
+    for (ManifestFile manifest : filtered) {

Review Comment:
   Minor: maybe an inline comment "collect all deleted files" or just a helper method which abstracts this `collectDeletedFiles(ManifestFile[] filter)`



-- 
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-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org