TheR1sing3un commented on code in PR #9821:
URL: https://github.com/apache/paimon/pull/9821#discussion_r4012207403
##########
paimon-python/pypaimon/filesystem/pyarrow_file_io.py:
##########
@@ -465,6 +497,34 @@ def delete(self, path: str, recursive: bool = False) ->
bool:
self.filesystem.delete_file(path_str)
return True
+ def _delete_s3_compatible_directory(self, path_str: str) -> bool:
+ selector = pafs.FileSelector(
+ path_str, recursive=True, allow_not_found=True)
+ file_infos = self.filesystem.get_file_info(selector)
+ files = [
+ info.path for info in file_infos
+ if info.type == pafs.FileType.File
+ ]
+ if files:
+ with ThreadPoolExecutor(max_workers=min(16, len(files))) as
executor:
+ list(executor.map(self.filesystem.delete_file, files))
+ directories = sorted(
+ (info.path for info in file_infos
+ if info.type == pafs.FileType.Directory),
+ key=lambda item: item.count("/"),
+ reverse=True,
+ )
+ for directory in directories:
+ self._delete_s3_directory_marker(directory)
+ self._delete_s3_directory_marker(path_str)
+ return True
+
+ def _delete_s3_directory_marker(self, path_str: str):
+ try:
+ self.filesystem.delete_dir(path_str.rstrip("/"))
Review Comment:
**Avoid re-entering `DeleteObjects` during directory-marker cleanup.**
`S3FileSystem.delete_dir()` does more than remove the marker: it recursively
lists the directory and deletes any remaining children via `DeleteObjects`
before deleting the marker. See [the PyArrow 23
implementation](https://github.com/apache/arrow/blob/apache-arrow-23.0.0/cpp/src/arrow/filesystem/s3fs.cc#L3251).
I reproduced this with a real PyArrow 23 client against a local endpoint
requiring `Content-MD5` for batch deletion: list `table/data/{a,b}`, delete
those files, and insert `table/data/late` before this call. The call sends
`POST ?delete` without `Content-MD5` and fails with `MissingContentMD5`,
leaving the new file and directory marker behind.
The current workaround therefore avoids batch deletion only while the
directory remains unchanged. Could we ensure marker cleanup cannot re-enter
that incompatible API, and add a request-level regression test with an object
appearing between listing and marker cleanup? Mocking `delete_dir()` hides this
behavior.
##########
paimon-python/pypaimon/filesystem/pyarrow_file_io.py:
##########
@@ -465,6 +497,34 @@ def delete(self, path: str, recursive: bool = False) ->
bool:
self.filesystem.delete_file(path_str)
return True
+ def _delete_s3_compatible_directory(self, path_str: str) -> bool:
+ selector = pafs.FileSelector(
+ path_str, recursive=True, allow_not_found=True)
+ file_infos = self.filesystem.get_file_info(selector)
+ files = [
+ info.path for info in file_infos
+ if info.type == pafs.FileType.File
+ ]
+ if files:
+ with ThreadPoolExecutor(max_workers=min(16, len(files))) as
executor:
+ list(executor.map(self.filesystem.delete_file, files))
Review Comment:
**Tolerate objects that disappear after the directory listing.**
PyArrow's `delete_file()` performs a HEAD request first and raises
`FileNotFoundError` if the object is already gone. Here that exception
propagates through `executor.map()` and skips the directory-marker cleanup.
I reproduced this by removing one listed object before its HEAD request,
simulating another cleanup task deleting it. The PyArrow 23 fallback raised
`FileNotFoundError` and left a marker behind; the original batch-deletion path
with PyArrow 21 completed successfully for the same scenario.
Please wrap individual deletions so that `FileNotFoundError` is treated as
successful deletion, while permission, transport, and other failures still
propagate. A regression test should remove an object after listing and verify
that cleanup completes.
##########
paimon-python/pypaimon/filesystem/pyarrow_file_io.py:
##########
@@ -465,6 +497,34 @@ def delete(self, path: str, recursive: bool = False) ->
bool:
self.filesystem.delete_file(path_str)
return True
+ def _delete_s3_compatible_directory(self, path_str: str) -> bool:
+ selector = pafs.FileSelector(
+ path_str, recursive=True, allow_not_found=True)
+ file_infos = self.filesystem.get_file_info(selector)
Review Comment:
**Scalability concern: this fallback is substantially more expensive and
applies to every custom S3 endpoint.**
With PyArrow 23, each `delete_file()` performs HEAD + DELETE + a PUT to
preserve the parent directory. In a local HTTP request-count comparison for
1,000 files under one directory, the PyArrow 21 native path issued 10 requests,
while this path issued 3,010: 1,003 HEADs, 4 GETs, 1,002 DELETEs, and 1,001
PUTs. These are request counts from a local test server, not production
throughput measurements.
Also, `get_file_info(selector)` materializes the entire tree, and
`executor.map()` eagerly submits the files on the tested Python version.
Limiting workers to 16 does not bound the number of queued futures.
Could we provide a way to retain native batch deletion for endpoints that
support it, and bound listing/submission memory for the fallback?
`_uses_s3_compatibility()` currently includes all explicit S3 endpoints, even
services that support the newer requests. This is separate from the two
correctness issues above, but the cost should be considered before enabling it
broadly.
--
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]
