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: [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]
