From e0793a3c3a3c79ef4558a394bb0462cb2f520bf7 Mon Sep 17 00:00:00 2001 From: Kenta Murata Date: Wed, 4 Dec 2019 13:08:40 +0900 Subject: [PATCH] Add Result-returning version of FileSystemFromUri --- cpp/src/arrow/filesystem/filesystem.cc | 16 ++++++---------- cpp/src/arrow/filesystem/filesystem.h | 21 ++++++++++++++++++--- cpp/src/arrow/filesystem/filesystem_test.cc | 12 ++++++------ cpp/src/arrow/filesystem/hdfs_test.cc | 2 +- cpp/src/arrow/filesystem/localfs_test.cc | 2 +- 5 files changed, 32 insertions(+), 21 deletions(-) diff --git a/cpp/src/arrow/filesystem/filesystem.cc b/cpp/src/arrow/filesystem/filesystem.cc index 0a954db132e..279791765ae 100644 --- a/cpp/src/arrow/filesystem/filesystem.cc +++ b/cpp/src/arrow/filesystem/filesystem.cc @@ -333,8 +333,8 @@ Result> SlowFileSystem::OpenAppendStream( return base_fs_->OpenAppendStream(path); } -Status FileSystemFromUri(const std::string& uri_string, - std::shared_ptr* out_fs, std::string* out_path) { +Result> FileSystemFromUri(const std::string& uri_string, + std::string* out_path) { Uri uri; RETURN_NOT_OK(uri.Parse(uri_string)); if (out_path != nullptr) { @@ -348,21 +348,18 @@ Status FileSystemFromUri(const std::string& uri_string, if (out_path != nullptr) { *out_path = uri_string; } - *out_fs = std::make_shared(); - return Status::OK(); + return std::make_shared(); } #endif if (scheme == "" || scheme == "file") { - *out_fs = std::make_shared(); - return Status::OK(); + return std::make_shared(); } if (scheme == "hdfs") { #ifdef ARROW_HDFS ARROW_ASSIGN_OR_RAISE(auto options, HdfsOptions::FromUri(uri)); ARROW_ASSIGN_OR_RAISE(auto hdfs, HadoopFileSystem::Make(options)); - *out_fs = hdfs; - return Status::OK(); + return hdfs; #else return Status::NotImplemented("Arrow compiled without HDFS support"); #endif @@ -375,8 +372,7 @@ Status FileSystemFromUri(const std::string& uri_string, *out_path = std::string(RemoveLeadingSlash(*out_path)); } if (scheme == "mock") { - *out_fs = std::make_shared(internal::CurrentTimePoint()); - return Status::OK(); + return std::make_shared(internal::CurrentTimePoint()); } // TODO add support for S3 URIs diff --git a/cpp/src/arrow/filesystem/filesystem.h b/cpp/src/arrow/filesystem/filesystem.h index 293072e91b0..5ed4d968f4a 100644 --- a/cpp/src/arrow/filesystem/filesystem.h +++ b/cpp/src/arrow/filesystem/filesystem.h @@ -316,6 +316,18 @@ class ARROW_EXPORT SlowFileSystem : public FileSystem { std::shared_ptr latencies_; }; +/// \brief Create a new FileSystem by URI +/// +/// A scheme-less URI is considered a local filesystem path. +/// Recognized schemes are "file", "mock" and "hdfs". +/// +/// \param[in] uri a URI-based path, ex: file:///some/local/path +/// \param[out] out_path (optional) Path inside the filesystem. +/// \return out_fs FileSystem instance. +ARROW_EXPORT +Result> FileSystemFromUri(const std::string& uri, + std::string* out_path = NULLPTR); + /// \brief Create a new FileSystem by URI /// /// A scheme-less URI is considered a local filesystem path. @@ -325,9 +337,12 @@ class ARROW_EXPORT SlowFileSystem : public FileSystem { /// \param[out] out_fs FileSystem instance. /// \param[out] out_path (optional) Path inside the filesystem. /// \return Status -ARROW_EXPORT -Status FileSystemFromUri(const std::string& uri, std::shared_ptr* out_fs, - std::string* out_path = NULLPTR); +ARROW_DEPRECATED("Use Result-returning version") +inline Status FileSystemFromUri(const std::string& uri, + std::shared_ptr* out_fs, + std::string* out_path = NULLPTR) { + return FileSystemFromUri(uri, out_path).Value(out_fs); +} } // namespace fs } // namespace arrow diff --git a/cpp/src/arrow/filesystem/filesystem_test.cc b/cpp/src/arrow/filesystem/filesystem_test.cc index e19a94716af..ebd3184d2e1 100644 --- a/cpp/src/arrow/filesystem/filesystem_test.cc +++ b/cpp/src/arrow/filesystem/filesystem_test.cc @@ -423,22 +423,22 @@ TEST_F(TestMockFS, Make) { TEST_F(TestMockFS, FileSystemFromUri) { std::string path; - ASSERT_OK(FileSystemFromUri("mock:", &fs_, &path)); + ASSERT_OK_AND_ASSIGN(fs_, FileSystemFromUri("mock:", &path)); ASSERT_EQ(path, ""); CheckDirs({}); // Ensures it's a MockFileSystem - ASSERT_OK(FileSystemFromUri("mock:foo/bar", &fs_, &path)); + ASSERT_OK_AND_ASSIGN(fs_, FileSystemFromUri("mock:foo/bar", &path)); ASSERT_EQ(path, "foo/bar"); CheckDirs({}); - ASSERT_OK(FileSystemFromUri("mock:/foo/bar", &fs_, &path)); + ASSERT_OK_AND_ASSIGN(fs_, FileSystemFromUri("mock:/foo/bar", &path)); ASSERT_EQ(path, "foo/bar"); CheckDirs({}); - ASSERT_OK(FileSystemFromUri("mock:/foo/bar/?q=xxx", &fs_, &path)); + ASSERT_OK_AND_ASSIGN(fs_, FileSystemFromUri("mock:/foo/bar/?q=xxx", &path)); ASSERT_EQ(path, "foo/bar/"); CheckDirs({}); - ASSERT_OK(FileSystemFromUri("mock:///foo/bar", &fs_, &path)); + ASSERT_OK_AND_ASSIGN(fs_, FileSystemFromUri("mock:///foo/bar", &path)); ASSERT_EQ(path, "foo/bar"); CheckDirs({}); - ASSERT_OK(FileSystemFromUri("mock:///foo/bar?q=zzz", &fs_, &path)); + ASSERT_OK_AND_ASSIGN(fs_, FileSystemFromUri("mock:///foo/bar?q=zzz", &path)); ASSERT_EQ(path, "foo/bar"); CheckDirs({}); } diff --git a/cpp/src/arrow/filesystem/hdfs_test.cc b/cpp/src/arrow/filesystem/hdfs_test.cc index 6f94d77d394..7c7550e5fe5 100644 --- a/cpp/src/arrow/filesystem/hdfs_test.cc +++ b/cpp/src/arrow/filesystem/hdfs_test.cc @@ -119,7 +119,7 @@ class TestHadoopFileSystem : public ::testing::Test { std::shared_ptr uri_fs; std::string path; ARROW_LOG(INFO) << "!!! uri = " << ss.str(); - ASSERT_OK(FileSystemFromUri(ss.str(), &uri_fs, &path)); + ASSERT_OK_AND_ASSIGN(uri_fs, FileSystemFromUri(ss.str(), &path)); ASSERT_EQ(path, "/"); // Sanity check diff --git a/cpp/src/arrow/filesystem/localfs_test.cc b/cpp/src/arrow/filesystem/localfs_test.cc index e2075336913..42a7cc5021d 100644 --- a/cpp/src/arrow/filesystem/localfs_test.cc +++ b/cpp/src/arrow/filesystem/localfs_test.cc @@ -114,7 +114,7 @@ class TestLocalFS : public LocalFSTestMixin { void TestFileSystemFromUri(const std::string& uri) { std::string path; - ASSERT_OK(FileSystemFromUri(uri, &fs_, &path)); + ASSERT_OK_AND_ASSIGN(fs_, FileSystemFromUri(uri, &path)); // Test that the right location on disk is accessed CreateFile(fs_.get(), local_path_ + "abc", "some data");