Re: [PR] GH-38618: [C++] S3FileSystem: fix regression in deleting explicitly created sub-directories [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #38845:
URL: https://github.com/apache/arrow/pull/38845#discussion_r1413710622


##########
python/pyarrow/tests/test_fs.py:
##########
@@ -760,6 +760,34 @@ def test_delete_dir(fs, pathfn):
         fs.delete_dir(d)
 
 
+def test_delete_dir_with_explicit_subdir(fs, pathfn):
+    skip_fsspec_s3fs(fs)

Review Comment:
   Also, mention the GH issue and explain what this test is about? Otherwise, it's not obvious why the test is needed at all.



##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -2408,7 +2408,12 @@ class S3FileSystem::Impl : public std::enable_shared_from_this<S3FileSystem::Imp
                   std::vector<std::string> file_paths;
                   for (const auto& file_info : file_infos) {
                     DCHECK_GT(file_info.path().size(), bucket.size());
-                    file_paths.push_back(file_info.path().substr(bucket.size() + 1));
+                    auto file_path = file_info.path().substr(bucket.size() + 1);
+                    if (file_info.IsDirectory()) {
+                      DCHECK_OK(internal::AssertNoTrailingSlash(file_path));
+                      file_path = file_path + kSep;

Review Comment:
   Can you please add a comment here referring to the GH issue? Otherwise this will easily get lost.



-- 
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: github-unsubscribe@arrow.apache.org

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