You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2026/03/03 14:35:04 UTC
[PR] Core, Spark: Unify bulk deletion [iceberg]
nastra opened a new pull request, #15501:
URL: https://github.com/apache/iceberg/pull/15501
Noticed that there are a few places that do the same pattern of deletes:
1) check if IO supports `SupportsBulkOperations` and use its deletion functionality
2) fall back to concurrent deletes using worker thread pool
We already have defined the same functionality in `CatalogUtil`, so figured we can unify some of those places into calling the same functionality
--
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
Re: [PR] Core, Spark: Unify bulk deletion [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #15501:
URL: https://github.com/apache/iceberg/pull/15501#discussion_r2882673729
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -202,6 +203,18 @@ private static void deleteFiles(FileIO io, Set<ManifestFile> allManifests) {
});
}
+ /**
+ * Helper to delete files. Bulk deletion is used if possible, otherwise deletions are done
+ * concurrently for non-bulk FileIO.
+ *
+ * @param io FileIO for deletes
+ * @param files files to delete
+ * @param type type of files being deleted
+ */
+ public static void deleteFiles(FileIO io, Iterable<String> files, String type) {
Review Comment:
minor: do we need a **new** function which always pass true in concurrent ? we do have an API which takes all 4 args
--
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
Re: [PR] Core, Spark: Unify bulk deletion [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #15501:
URL: https://github.com/apache/iceberg/pull/15501#discussion_r2883151499
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -202,6 +203,18 @@ private static void deleteFiles(FileIO io, Set<ManifestFile> allManifests) {
});
}
+ /**
+ * Helper to delete files. Bulk deletion is used if possible, otherwise deletions are done
+ * concurrently for non-bulk FileIO.
+ *
+ * @param io FileIO for deletes
+ * @param files files to delete
+ * @param type type of files being deleted
+ */
+ public static void deleteFiles(FileIO io, Iterable<String> files, String type) {
Review Comment:
it's a bit inconvenient to always have to pass `true` in all call sites, given that in pretty much all cases you'd want to do concurrent deletes anyway. Also I think `deleteFiles(io, files, type, concurrent)` should have been private, since there's really only a single place that passes `false` there.
I've cleaned up all of the call sites and also backported the changes to earlier Spark versions
--
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
Re: [PR] Core, Spark: Unify bulk deletion [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #15501:
URL: https://github.com/apache/iceberg/pull/15501#discussion_r2897523611
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -202,6 +203,18 @@ private static void deleteFiles(FileIO io, Set<ManifestFile> allManifests) {
});
}
+ /**
+ * Helper to delete files. Bulk deletion is used if possible, otherwise deletions are done
+ * concurrently for non-bulk FileIO.
+ *
+ * @param io FileIO for deletes
+ * @param files files to delete
+ * @param type type of files being deleted
+ */
+ public static void deleteFiles(FileIO io, Iterable<String> files, String type) {
Review Comment:
nit: It looks like the 4-arg `deleteFiles(io, files, type, concurrent)` is still public in this PR: was making it private intentionally left out, or just missed?
--
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
Re: [PR] Core, Spark: Unify bulk deletion [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #15501:
URL: https://github.com/apache/iceberg/pull/15501#discussion_r2903933552
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -202,6 +203,18 @@ private static void deleteFiles(FileIO io, Set<ManifestFile> allManifests) {
});
}
+ /**
+ * Helper to delete files. Bulk deletion is used if possible, otherwise deletions are done
+ * concurrently for non-bulk FileIO.
+ *
+ * @param io FileIO for deletes
+ * @param files files to delete
+ * @param type type of files being deleted
+ */
+ public static void deleteFiles(FileIO io, Iterable<String> files, String type) {
Review Comment:
I didn't bother deprecating it and I'm not sure we should be deprecating it at this point (maybe there are usages in external libs that use the non-concurrent version of it)
--
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
Re: [PR] Core, Spark: Unify bulk deletion [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on PR #15501:
URL: https://github.com/apache/iceberg/pull/15501#issuecomment-4033451565
Noticed this; I will test some fault injection through #15436
SparkCleanupUtil.deleteFiles() is still handing off to its own implementation of concurrent delete if the FileIO implementation doesn't do bulk delete. Why not have it always pass down and rely on the concurrency in CatalogUtil?
Is it that just because of the backoff/retry logic in SparkCleanupUtil?
FWIW I don't see why SparkCleanupUtil delete() needs recovery except to ensure transient network problems don't stop delete() from doing a best effort attempt at cleanup. But that stuff can and is generally hidden in the client code underneath.
--
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
Re: [PR] Core, Spark: Unify bulk deletion [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #15501:
URL: https://github.com/apache/iceberg/pull/15501
--
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
Re: [PR] Core, Spark: Unify bulk deletion [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #15501:
URL: https://github.com/apache/iceberg/pull/15501#discussion_r2883151499
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -202,6 +203,18 @@ private static void deleteFiles(FileIO io, Set<ManifestFile> allManifests) {
});
}
+ /**
+ * Helper to delete files. Bulk deletion is used if possible, otherwise deletions are done
+ * concurrently for non-bulk FileIO.
+ *
+ * @param io FileIO for deletes
+ * @param files files to delete
+ * @param type type of files being deleted
+ */
+ public static void deleteFiles(FileIO io, Iterable<String> files, String type) {
Review Comment:
it's a bit inconvenient to always have to pass `true` in all call sites, given that in pretty much all cases you'd want to do concurrent deletes anyway. Also I think `deleteFiles(io, files, type, concurrent)` should have been private, since there's really only a single place that passes `false` there
--
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
Re: [PR] Core, Spark: Unify bulk deletion [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #15501:
URL: https://github.com/apache/iceberg/pull/15501#discussion_r2878642557
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/source/SparkCleanupUtil.java:
##########
@@ -84,34 +84,13 @@ private static String taskInfo() {
*/
public static void deleteFiles(String context, FileIO io, List<? extends ContentFile<?>> files) {
List<String> paths = Lists.transform(files, ContentFile::location);
- deletePaths(context, io, paths);
- }
-
- private static void deletePaths(String context, FileIO io, List<String> paths) {
if (io instanceof SupportsBulkOperations) {
- SupportsBulkOperations bulkIO = (SupportsBulkOperations) io;
- bulkDelete(context, bulkIO, paths);
+ CatalogUtil.deleteFiles(io, paths, "");
} else {
delete(context, io, paths);
Review Comment:
ideally we would fully delegate to `CatalogUtil.deleteFiles` here as well, but there is custom retry logic here that we don't use in `CatalogUtil`
--
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