GH-32206: [C++] GcsFileSystem::Make should return Result #44503
Conversation
|
GH-32206: Added back a line I had erroneously removed ( |
| @@ -962,7 +962,7 @@ Result<std::shared_ptr<io::OutputStream>> GcsFileSystem::OpenAppendStream( | |||
| return Status::NotImplemented("Append is not supported in GCS"); | |||
| } | |||
|
|
|||
| std::shared_ptr<GcsFileSystem> GcsFileSystem::Make(const GcsOptions& options, | |||
| Result<std::shared_ptr<GcsFileSystem>> GcsFileSystem::Make(const GcsOptions& options, | |||
There was a problem hiding this comment.
So this is for compatible reason? 🤔
There was a problem hiding this comment.
Essentially that's correct, as per the original issue: "For consistency with S3FileSystem" which, incidentally, makes it consistent with other similar filesystem classes.
github-actions
bot
added
awaiting committer review
cpp/src/arrow/filesystem/gcsfs.h
Outdated
| @@ -232,7 +232,7 @@ class ARROW_EXPORT GcsFileSystem : public FileSystem { | |||
|
|
|||
| /// Create a GcsFileSystem instance from the given options. | |||
| // TODO(ARROW-16884): make this return Result for consistency | |||
There was a problem hiding this comment.
Complete! Ref 49fd6d0 (EDIT: incorrect commit hash).
github-actions
bot
added
awaiting changes
| @@ -271,7 +271,7 @@ class TestGCSFSGeneric : public GcsIntegrationTest, public GenericFileSystemTest | |||
| void SetUp() override { | |||
| ASSERT_NO_FATAL_FAILURE(GcsIntegrationTest::SetUp()); | |||
| auto bucket_name = RandomBucketName(); | |||
| gcs_fs_ = GcsFileSystem::Make(TestGcsOptions()); | |||
| ASSERT_OK_AND_ASSIGN(std::shared_ptr<GcsFileSystem> gcs_fs_, GcsFileSystem::Make(TestGcsOptions())); | |||
There was a problem hiding this comment.
| ASSERT_OK_AND_ASSIGN(std::shared_ptr<GcsFileSystem> gcs_fs_, GcsFileSystem::Make(TestGcsOptions())); | |
| ASSERT_OK_AND_ASSIGN(gcs_fs_, GcsFileSystem::Make(TestGcsOptions())); |
There was a problem hiding this comment.
Complete! Ref 49fd6d0 (EDIT: incorrect commit hash).
| @@ -489,17 +489,13 @@ TEST(GcsFileSystem, FileSystemCompare) { | |||
| a_options.project_id = "test-only-invalid-project-id"; | |||
| auto a = GcsFileSystem::Make(a_options); | |||
| EXPECT_THAT(a, NotNull()); | |||
| EXPECT_TRUE(a->Equals(*a)); | |||
There was a problem hiding this comment.
Why do we need to remove this?
Should we change auto a = GcsFileSystem::Make(a_options); to ASSERT_OK_AND_ASSIGN(auto a, GcsFileSystem::Make(a_options); instead?
There was a problem hiding this comment.
There is no need to remove this with your suggested change. That change has been made, and the above line has been restored. Ref 49fd6d0 (EDIT: incorrect commit hash).
| @@ -622,7 +618,7 @@ TEST(GcsFileSystem, ObjectMetadataRoundtrip) { | |||
| } | |||
|
|
|||
| TEST_F(GcsIntegrationTest, GetFileInfoBucket) { | |||
| auto fs = GcsFileSystem::Make(TestGcsOptions()); | |||
| ASSERT_OK_AND_ASSIGN(std::shared_ptr<GcsFileSystem> fs, GcsFileSystem::Make(TestGcsOptions())); | |||
There was a problem hiding this comment.
Complete! Ref 49fd6d0 (EDIT: incorrect commit hash).
This has been done. |
trifleneurotic
force-pushed
the
GH-32206
branch
from
6167a6c
to
6b741af
Compare
github-actions
bot
added
awaiting change review
|
Tests were re-run locally as a result of the local compilation of commit 49fd6d0; |
This PR includes breaking changes to public APIs.
What changes are included in this PR?
GcsFileSystem::Make now returns Result, with corresponding header & test changes.
Are these changes tested?
Yes,
arrow-gcsfs-testpassed.Are there any user-facing changes?
None known.