From 1a41f432e66a2a5c47f78aa7586deec049416fac Mon Sep 17 00:00:00 2001
From: Antoine Pitrou <antoine@python.org>
Date: Mon, 16 Mar 2020 14:27:04 +0100
Subject: [PATCH] ARROW-8132: [C++] Fix S3FileSystem tests on Windows

The new Minio version is less robust when deleting directories.
---
 cpp/src/arrow/filesystem/s3fs.cc      | 4 +++-
 cpp/src/arrow/filesystem/s3fs_test.cc | 9 +++++++++
 cpp/src/arrow/filesystem/test_util.cc | 8 ++++++--
 cpp/src/arrow/filesystem/test_util.h  | 2 ++
 4 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/cpp/src/arrow/filesystem/s3fs.cc b/cpp/src/arrow/filesystem/s3fs.cc
index 8abbf81bda0..36dd456861a 100644
--- a/cpp/src/arrow/filesystem/s3fs.cc
+++ b/cpp/src/arrow/filesystem/s3fs.cc
@@ -1144,7 +1144,9 @@ class S3FileSystem::Impl {
     }
     // First delete all "files", then delete all child "directories"
     RETURN_NOT_OK(DeleteObjects(bucket, file_keys));
-    // XXX This doesn't seem necessary on Minio
+    // Delete directories in reverse lexicographic order, to ensure children
+    // are deleted before their parents (Minio).
+    std::sort(dir_keys.rbegin(), dir_keys.rend());
     return DeleteObjects(bucket, dir_keys);
   }
 
diff --git a/cpp/src/arrow/filesystem/s3fs_test.cc b/cpp/src/arrow/filesystem/s3fs_test.cc
index d751c9e6a40..1476ffc74eb 100644
--- a/cpp/src/arrow/filesystem/s3fs_test.cc
+++ b/cpp/src/arrow/filesystem/s3fs_test.cc
@@ -840,6 +840,15 @@ class TestS3FSGeneric : public S3TestMixin, public GenericFileSystemTest {
   bool allow_move_dir() const override { return false; }
   bool allow_append_to_file() const override { return false; }
   bool have_directory_mtimes() const override { return false; }
+  bool have_flaky_directory_tree_deletion() const override {
+#ifdef _WIN32
+    // Recent Minio versions on Windows may not register deletion of all
+    // directories in a tree when doing a bulk delete.
+    return true;
+#else
+    return false;
+#endif
+  }
 
   S3Options options_;
   std::shared_ptr<S3FileSystem> s3fs_;
diff --git a/cpp/src/arrow/filesystem/test_util.cc b/cpp/src/arrow/filesystem/test_util.cc
index 590e07c963e..990708a4edd 100644
--- a/cpp/src/arrow/filesystem/test_util.cc
+++ b/cpp/src/arrow/filesystem/test_util.cc
@@ -237,13 +237,17 @@ void GenericFileSystemTest::TestDeleteDirContents(FileSystem* fs) {
 
   // Also with "" (== wipe filesystem)
   ASSERT_OK(fs->DeleteDirContents(""));
-  AssertAllDirs(fs, {});
+  if (!have_flaky_directory_tree_deletion()) {
+    AssertAllDirs(fs, {});
+  }
   AssertAllFiles(fs, {});
 
   // Not a directory
   CreateFile(fs, "abc", "");
   ASSERT_RAISES(IOError, fs->DeleteDirContents("abc"));
-  AssertAllDirs(fs, {});
+  if (!have_flaky_directory_tree_deletion()) {
+    AssertAllDirs(fs, {});
+  }
   AssertAllFiles(fs, {"abc"});
 }
 
diff --git a/cpp/src/arrow/filesystem/test_util.h b/cpp/src/arrow/filesystem/test_util.h
index 6e3160888c6..29b01379ecd 100644
--- a/cpp/src/arrow/filesystem/test_util.h
+++ b/cpp/src/arrow/filesystem/test_util.h
@@ -132,6 +132,8 @@ class ARROW_EXPORT GenericFileSystemTest {
   virtual bool allow_append_to_file() const { return true; }
   // - Whether the filesystem supports directory modification times
   virtual bool have_directory_mtimes() const { return true; }
+  // - Whether some directory tree deletion tests may fail randomly
+  virtual bool have_flaky_directory_tree_deletion() const { return false; }
 
   void TestEmpty(FileSystem* fs);
   void TestCreateDir(FileSystem* fs);