ARROW-14222: [C++] implement GCSFileSystem skeleton #11331

coryan · 2021-10-05T22:21:47Z

No description provided.

github-actions · 2021-10-05T22:22:07Z

https://issues.apache.org/jira/browse/ARROW-14222

emkornfield · 2021-10-05T23:35:55Z

cpp/src/arrow/filesystem/gcsfs.h

+  static GCSOptions Anonymous();
+};
+
+class ARROW_EXPORT GCSFileSystem : public FileSystem {


small nit: GcsFileSystem? Also add docstring?

I was wondering what was the style in this project. Fixed.

Generally, google style, There are some cases where we've used the all caps spelling though (e.g. CSV)

coryan · 2021-10-07T15:07:42Z

Ping?

emkornfield · 2021-10-07T15:51:08Z

cpp/src/arrow/filesystem/gcsfs.cc

+
+namespace gcs = google::cloud::storage;
+
+google::cloud::Options AsGoogleCloudOptions(GcsOptions const& o) {


const should be in front of GcsOptions?

Thanks. I did not mean to restart the "east const" vs. "const west" holy way, but muscle memory is what it is.

emkornfield · 2021-10-07T15:51:49Z

cpp/src/arrow/filesystem/gcsfs.cc

+namespace gcs = google::cloud::storage;
+
+google::cloud::Options AsGoogleCloudOptions(GcsOptions const& o) {
+  auto options = google::cloud::Options{};


Is there a reason to prefer this over google::cloud::Options options; ?

"Almost Always Auto", another holy war. I can change it if you think it is confusing.

Yeah, the almost always use auto (my undertanding this is from effective C++) directly contradicts the style guide in use (which is only use auto if type is obvious). In this case type is obvious, it just seems like there is more text in this case.

emkornfield · 2021-10-07T15:52:16Z

cpp/src/arrow/filesystem/gcsfs.cc

+google::cloud::Options AsGoogleCloudOptions(GcsOptions const& o) {
+  auto options = google::cloud::Options{};
+  if (!o.endpoint_override.empty()) {
+    auto scheme = o.scheme;


please spell out the type.

emkornfield · 2021-10-07T15:55:24Z

cpp/src/arrow/filesystem/gcsfs.h

+namespace internal {
+// TODO(ARROW-1231) - during development only tests should create a GcsFileSystem.
+//     Remove, and provide a public API, before declaring the feature complete.
+std::shared_ptr<GcsFileSystem> MakeGcsFileSystemForTest(const GcsOptions& options);


The TODO covers make a static method Make on GcsFileSystem?

Fixed to make the TODO more explicit

I hope that is what you had in mind.

emkornfield · 2021-10-07T15:55:57Z

cpp/src/arrow/filesystem/gcsfs.cc

+namespace internal {
+
+std::shared_ptr<GcsFileSystem> MakeGcsFileSystemForTest(const GcsOptions& options) {
+  return std::shared_ptr<GcsFileSystem>(


maybe comment on why std::make_shared isn't used.

emkornfield · 2021-10-07T15:58:06Z

cpp/src/arrow/filesystem/gcsfs_test.cc

+  EXPECT_THAT(b.get(), NotNull());
+  EXPECT_EQ(b, b);
+
+  EXPECT_NE(a, b);


I think Equals here is mean logically equal, shouldn't this be equal?

I do not know what "logically equals" means. At a guess something like "modulo transient errors, both produce the same results"? If so, I am not sure there is a reliable way to do that: consider storage.googleapis.com vs. private.googleapis.com as endpoint overrides, different strings, but depending on your environment, exactly the same behavior (or not!).

Anyway, implemented something closer to the current implementation for the S3 FileSystem.

yeah, I meant configuration parameters are equal. Before this looked like it was using solely memory address equality.

emkornfield · 2021-10-07T15:58:55Z

cpp/src/arrow/filesystem/gcsfs_test.cc

+  EXPECT_EQ(a, a);
+
+  auto b = internal::MakeGcsFileSystemForTest(GcsOptions{});
+  EXPECT_THAT(b.get(), NotNull());


I don't think .get() is necessary here and above.

Doh, fixed.

emkornfield · 2021-10-07T15:59:11Z

cpp/src/arrow/filesystem/gcsfs_test.cc

+TEST(GCSFileSystem, Compare) {
+  auto a = internal::MakeGcsFileSystemForTest(GcsOptions{});
+  EXPECT_THAT(a.get(), NotNull());
+  EXPECT_EQ(a, a);


should this be comparing the objects EXPECT_EQ(*a, *a)

Actually we want a->Equals() fixed.

emkornfield · 2021-10-07T18:19:51Z

C++ looks good to me. I'll wait and see if there are any comments on CMake (I'll merge later today if nothing comes up)

emkornfield · 2021-10-07T18:27:28Z

cpp/CMakeLists.txt

@@ -801,7 +801,11 @@ endif()
 set(ARROW_SHARED_PRIVATE_LINK_LIBS ${ARROW_STATIC_LINK_LIBS})

 # boost::filesystem is needed for S3 and Flight tests as a boost::process dependency.
-if(((ARROW_FLIGHT OR ARROW_S3) AND (ARROW_BUILD_TESTS OR ARROW_BUILD_INTEGRATION)))
+if(((ARROW_FLIGHT
+     OR ARROW_S3


Not sure if the mac build is a transient error or somehow due to this?

I do not think is this change, as the build has -DARROW_GCS=OFF:

https://github.com/apache/arrow/pull/11331/checks?check_run_id=3830637295#step:9:62

and the change is (modulo reformatting) changing (ARROW_FLIGHT OR ARROW_S3) to (ARROW_FLIGHT OR ARROW_S3 OR ARROW_GCS). In addition, this change has been there since the first commit in the branch.

At a guess this is either a transient or the point in master where I based the branch has a problem, I can rebase if you think that would help.

yeah, lets try a rebase.

Looking at the builds at master, I do not think a rebase would help, and it seems even less likely that these changes caused the build breaks:

https://github.com/apache/arrow/actions/runs/1317260919

https://github.com/apache/arrow/actions?query=event%3Apush+branch%3Amaster

coryan · 2021-10-07T18:46:15Z

Rebased anyway. At least that makes it easier to understand what build problems are coming from where.

kou · 2021-10-08T00:27:00Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

+  include_directories(SYSTEM ${GOOGLE_CLOUD_CPP_INCLUDE_DIR})
+  get_target_property(absl_base_INCLUDE_DIR absl::base INTERFACE_INCLUDE_DIRECTORIES)
+  include_directories(SYSTEM ${absl_base_INCLUDE_DIR})
+  message(STATUS "Found google-cloud-cpp::storage static library: ${GOOGLE_CLOUD_CPP_STATIC_LIBRARY_STORAGE}"
+  )
+  message(STATUS "Found google-cloud-cpp::storage headers: ${GOOGLE_CLOUD_CPP_INCLUDE_DIR}"
+  )


Could you extract include dir and static library path information from google-cloud-cpp::storage?
If we use system Google Cloud Storage (find_package(google_cloud_cpp_storage) is succeeded case), GOOGLE_CLOUD_CPP_* aren't defined.

I just removed the static library, seems to be unused, and extracted the include dir from the target properties.

This is really not my problem, I will say it anyway: there is something wrong, or at least very unusual, about Arrow's usage of CMake. With modern CMake adding google-cloud-cpp::storage to the target_link_libraries() should define all the necessary command-line flags. If one must set include_directories() then some target is missing a dependency.

Yes. You're right.

We have some problems around it but we didn't dig into them yet...

Ack. In case it helps, it might be this:

arrow/cpp/cmake_modules/BuildUtils.cmake

Lines 285 to 287 in 00c2741

# Generate a single "objlib" from all C++ modules and link

# that "objlib" into each library kind, to avoid compiling twice

add_library(${LIB_NAME}_objlib OBJECT ${ARG_SOURCES})

the arrow_objlib target does not seem to link any dependencies, so when its object files are compiled, none of the required flags are set. You may want to add the dependencies to arrow_objlib and then things should "Just Work":tm:

It may be right. We should try it later. :-)

kou · 2021-10-08T20:34:10Z

CI failures on macOS will be fixed by #11374.

emkornfield · 2021-10-08T20:48:54Z

@kou any more concerns, otherwise I think this can be merged.

kou · 2021-10-08T21:17:50Z

Nothing. We can merge this.

ursabot · 2021-10-09T00:41:13Z

Benchmark runs are scheduled for baseline = 00c2741 and contender = 1b0ea15. 1b0ea15 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.0% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

github-actions bot added the Component: C++ label Oct 5, 2021

emkornfield reviewed Oct 5, 2021

View reviewed changes

coryan marked this pull request as ready for review October 6, 2021 01:12

emkornfield requested changes Oct 7, 2021

View reviewed changes

emkornfield reviewed Oct 7, 2021

View reviewed changes

coryan force-pushed the ARROW-14222-implement-gcsfs-skeleton branch from 6898c5a to ea0db36 Compare October 7, 2021 18:45

kou reviewed Oct 8, 2021

View reviewed changes

coryan added 6 commits October 8, 2021 14:56

ARROW-14222: [C++] implement GCSFileSystem skeleton

927f6c6

Fix lints

ffc316c

Address review comments

a49580b

Fix cut&paste error

2a10238

Address review comments

81ad939

Address review comments

44e77cb

coryan force-pushed the ARROW-14222-implement-gcsfs-skeleton branch from ea0db36 to 44e77cb Compare October 8, 2021 15:04

kou closed this in 1b0ea15 Oct 9, 2021

coryan deleted the ARROW-14222-implement-gcsfs-skeleton branch October 9, 2021 03:21

asfimport mentioned this pull request Oct 11, 2021

[C++] Create GcsFileSystem skeleton #29804

Closed


		namespace gcs = google::cloud::storage;

		google::cloud::Options AsGoogleCloudOptions(GcsOptions const& o) {

	# Generate a single "objlib" from all C++ modules and link
	# that "objlib" into each library kind, to avoid compiling twice
	add_library(${LIB_NAME}_objlib OBJECT ${ARG_SOURCES})

ARROW-14222: [C++] implement GCSFileSystem skeleton #11331

ARROW-14222: [C++] implement GCSFileSystem skeleton #11331

Conversation

coryan commented Oct 5, 2021

github-actions bot commented Oct 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coryan commented Oct 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emkornfield Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emkornfield commented Oct 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coryan commented Oct 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kou commented Oct 8, 2021

emkornfield commented Oct 8, 2021

kou commented Oct 8, 2021

ursabot commented Oct 9, 2021 • edited Loading

emkornfield Oct 7, 2021 •

edited

Loading

ursabot commented Oct 9, 2021 •

edited

Loading