You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by ks...@apache.org on 2019/10/05 09:47:47 UTC
[arrow] 08/45: ARROW-6685: [C++] Ignore trailing slashes in S3FS
This is an automated email from the ASF dual-hosted git repository.
kszucs pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/arrow.git
commit 95fe0e12fe1502b3a1160dcfc9ca14a8d361aa1a
Author: Antoine Pitrou <an...@python.org>
AuthorDate: Mon Sep 30 17:45:13 2019 -0500
ARROW-6685: [C++] Ignore trailing slashes in S3FS
Closes #5546 from pitrou/ARROW-6685-s3fs-trailing-slash and squashes the following commits:
ae315aff8 <Antoine Pitrou> ARROW-6685: Ignore trailing slashes in S3FS
Authored-by: Antoine Pitrou <an...@python.org>
Signed-off-by: Wes McKinney <we...@apache.org>
---
cpp/src/arrow/filesystem/filesystem_test.cc | 12 ++++++++++++
cpp/src/arrow/filesystem/path_util.cc | 2 +-
cpp/src/arrow/filesystem/s3fs.cc | 11 ++++++-----
cpp/src/arrow/filesystem/s3fs_test.cc | 21 +++++++++++++++++++++
4 files changed, 40 insertions(+), 6 deletions(-)
diff --git a/cpp/src/arrow/filesystem/filesystem_test.cc b/cpp/src/arrow/filesystem/filesystem_test.cc
index c8eba30..c199a2d 100644
--- a/cpp/src/arrow/filesystem/filesystem_test.cc
+++ b/cpp/src/arrow/filesystem/filesystem_test.cc
@@ -155,6 +155,18 @@ TEST(PathUtil, EnsureTrailingSlash) {
ASSERT_EQ("/abc/", EnsureTrailingSlash("/abc/"));
}
+TEST(PathUtil, RemoveTrailingSlash) {
+ ASSERT_EQ("", std::string(RemoveTrailingSlash("")));
+ ASSERT_EQ("", std::string(RemoveTrailingSlash("/")));
+ ASSERT_EQ("", std::string(RemoveTrailingSlash("//")));
+ ASSERT_EQ("abc/def", std::string(RemoveTrailingSlash("abc/def")));
+ ASSERT_EQ("abc/def", std::string(RemoveTrailingSlash("abc/def/")));
+ ASSERT_EQ("abc/def", std::string(RemoveTrailingSlash("abc/def//")));
+ ASSERT_EQ("/abc/def", std::string(RemoveTrailingSlash("/abc/def")));
+ ASSERT_EQ("/abc/def", std::string(RemoveTrailingSlash("/abc/def/")));
+ ASSERT_EQ("/abc/def", std::string(RemoveTrailingSlash("/abc/def//")));
+}
+
////////////////////////////////////////////////////////////////////////////
// Generic MockFileSystem tests
diff --git a/cpp/src/arrow/filesystem/path_util.cc b/cpp/src/arrow/filesystem/path_util.cc
index cfc59a6..2ba143e 100644
--- a/cpp/src/arrow/filesystem/path_util.cc
+++ b/cpp/src/arrow/filesystem/path_util.cc
@@ -115,7 +115,7 @@ std::string EnsureTrailingSlash(const std::string& s) {
}
util::string_view RemoveTrailingSlash(util::string_view key) {
- if (!key.empty() && key.back() == kSep) {
+ while (!key.empty() && key.back() == kSep) {
key.remove_suffix(1);
}
return key;
diff --git a/cpp/src/arrow/filesystem/s3fs.cc b/cpp/src/arrow/filesystem/s3fs.cc
index 136d12e..8053c22 100644
--- a/cpp/src/arrow/filesystem/s3fs.cc
+++ b/cpp/src/arrow/filesystem/s3fs.cc
@@ -177,17 +177,18 @@ struct S3Path {
std::vector<std::string> key_parts;
static Status FromString(const std::string& s, S3Path* out) {
- auto first_sep = s.find_first_of(kSep);
+ const auto src = internal::RemoveTrailingSlash(s);
+ auto first_sep = src.find_first_of(kSep);
if (first_sep == 0) {
return Status::Invalid("Path cannot start with a separator ('", s, "')");
}
if (first_sep == std::string::npos) {
- *out = {s, s, "", {}};
+ *out = {std::string(src), std::string(src), "", {}};
return Status::OK();
}
- out->full_path = s;
- out->bucket = s.substr(0, first_sep);
- out->key = s.substr(first_sep + 1);
+ out->full_path = std::string(src);
+ out->bucket = std::string(src.substr(0, first_sep));
+ out->key = std::string(src.substr(first_sep + 1));
out->key_parts = internal::SplitAbstractPath(out->key);
return internal::ValidateAbstractPathParts(out->key_parts);
}
diff --git a/cpp/src/arrow/filesystem/s3fs_test.cc b/cpp/src/arrow/filesystem/s3fs_test.cc
index b19b18e..8564901 100644
--- a/cpp/src/arrow/filesystem/s3fs_test.cc
+++ b/cpp/src/arrow/filesystem/s3fs_test.cc
@@ -375,6 +375,10 @@ TEST_F(TestS3FS, GetTargetStatsBucket) {
AssertFileStats(fs_.get(), "bucket", FileType::Directory);
AssertFileStats(fs_.get(), "empty-bucket", FileType::Directory);
AssertFileStats(fs_.get(), "non-existent-bucket", FileType::NonExistent);
+ // Trailing slashes
+ AssertFileStats(fs_.get(), "bucket/", FileType::Directory);
+ AssertFileStats(fs_.get(), "empty-bucket/", FileType::Directory);
+ AssertFileStats(fs_.get(), "non-existent-bucket/", FileType::NonExistent);
}
TEST_F(TestS3FS, GetTargetStatsObject) {
@@ -393,6 +397,12 @@ TEST_F(TestS3FS, GetTargetStatsObject) {
AssertFileStats(fs_.get(), "bucket/emptyd", FileType::NonExistent);
AssertFileStats(fs_.get(), "bucket/somed", FileType::NonExistent);
AssertFileStats(fs_.get(), "non-existent-bucket/somed", FileType::NonExistent);
+
+ // Trailing slashes
+ AssertFileStats(fs_.get(), "bucket/emptydir/", FileType::Directory, kNoSize);
+ AssertFileStats(fs_.get(), "bucket/somefile/", FileType::File, 9);
+ AssertFileStats(fs_.get(), "bucket/emptyd/", FileType::NonExistent);
+ AssertFileStats(fs_.get(), "non-existent-bucket/somed/", FileType::NonExistent);
}
TEST_F(TestS3FS, GetTargetStatsSelector) {
@@ -447,6 +457,17 @@ TEST_F(TestS3FS, GetTargetStatsSelector) {
ASSERT_OK(fs_->GetTargetStats(select, &stats));
ASSERT_EQ(stats.size(), 0);
select.allow_non_existent = false;
+
+ // Trailing slashes
+ select.base_dir = "empty-bucket/";
+ ASSERT_OK(fs_->GetTargetStats(select, &stats));
+ ASSERT_EQ(stats.size(), 0);
+ select.base_dir = "non-existent-bucket/";
+ ASSERT_RAISES(IOError, fs_->GetTargetStats(select, &stats));
+ select.base_dir = "bucket/";
+ ASSERT_OK(fs_->GetTargetStats(select, &stats));
+ SortStats(&stats);
+ ASSERT_EQ(stats.size(), 3);
}
TEST_F(TestS3FS, GetTargetStatsSelectorRecursive) {