ARROW-15114: [C++] GcsFileSystem uses metadata for directory markers by coryan · Pull Request #11996 · apache/arrow

coryan · 2021-12-20T15:03:15Z

No description provided.

github-actions · 2021-12-20T15:03:37Z

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

coryan · 2021-12-20T15:32:13Z

The appveyor failures look unrelated.

coryan · 2021-12-21T00:58:04Z

Rebased to resolve conflicts.

coryan · 2021-12-22T13:40:41Z

The failure on Java JNI / AMD64 Debian 9 Java JNI (Gandiva, Plasma, ORC, Dataset) looks unrelated, please take a look.

coryan · 2022-01-03T16:08:56Z

Ping

coryan · 2022-01-06T23:43:01Z

@emkornfield can you take a look?

emkornfield · 2022-01-06T23:49:01Z

cpp/src/arrow/filesystem/gcsfs.cc

-                                               const google::cloud::Status& status) {
-    using ::google::cloud::StatusCode;
-    auto canonical = internal::EnsureTrailingSlash(path.full_path);
+  static bool IsDirectory(gcs::ObjectMetadata const& o) {


Suggested change

static bool IsDirectory(gcs::ObjectMetadata const& o) {

static bool IsDirectory(const gcs::ObjectMetadata& o) {

nit

emkornfield

It seems like there might be a lot more branches add here then tested in the changes, not sure how critical these are if they are covered by the generic file system tests?

emkornfield · 2022-01-07T00:02:19Z

cpp/src/arrow/filesystem/gcsfs.cc

-                          "arrow/gcsfs", "directory")))
-        .status();
+    const auto canonical = internal::RemoveTrailingSlash(name).to_string();
+    auto object = client_.InsertObject(


nit, I think it would be clearer to spell out type.

emkornfield · 2022-01-07T00:03:54Z

cpp/src/arrow/filesystem/gcsfs.cc

-      if (status.code() == GcsCode::kAlreadyExists) {
-        break;
+    if (dir.empty()) {
+      // We could not find any of the parent directories in the bucket, the last step is


is this consistent with what the s3 connector does. bucket creation seems pretty heavy weight to maybe do it accidentally

Seems like this is what you do for s3 too:

arrow/cpp/src/arrow/filesystem/s3fs.cc

Lines 2305 to 2311 in b325ef7

// Create object

if (recursive) {

// Ensure bucket exists

ARROW_ASSIGN_OR_RAISE(bool bucket_exists, impl_->BucketExists(path.bucket));

if (!bucket_exists) {

RETURN_NOT_OK(impl_->CreateBucket(path.bucket));

}

emkornfield · 2022-01-07T00:04:15Z

cpp/src/arrow/filesystem/gcsfs.cc

+    if (dir.empty()) {
+      // We could not find any of the parent directories in the bucket, the last step is
+      // to find out if the bucket exists, and if necessary, create it
+      auto b = client_.GetBucketMetadata(bucket);


spelling out type here would be more readable I think.

Done. Come join the almost-always-auto world, it is nice here 😀

its not that it isn't tempting, just not in the style guide used for this project.

emkornfield · 2022-01-07T05:30:02Z

cpp/src/arrow/filesystem/gcsfs.cc

  Status Move(const GcsPath& src, const GcsPath& dest) {
-    if (src.full_path.empty() || src.object.empty() ||
-        src.object.back() == internal::kSep) {
+    if (src == dest) return {};


nit: return Status::OK() is more idiomatic for the code base

Fixed here and elsewhere.

emkornfield · 2022-01-07T05:30:48Z

cpp/src/arrow/filesystem/gcsfs.cc

-  if (!info.IsFile()) {
-    return Status::IOError("Only files can be opened as input streams");
+  if (info.IsDirectory()) {
+    return Status::IOError("Cannot open a directory as an input stream");


maybe include the path requested here?

Fixed. I also searched for any other places where I was not returning the affected path with the IOError.

emkornfield · 2022-01-07T05:31:01Z

cpp/src/arrow/filesystem/gcsfs.cc

 Result<std::shared_ptr<io::RandomAccessFile>> GcsFileSystem::OpenInputFile(
    const FileInfo& info) {
+  if (info.IsDirectory()) {
+    return Status::IOError("Cannot open a directory as an input stream");


same comment above about providing the path?

emkornfield · 2022-01-07T19:42:13Z

Looks like all failures are unrelated. I'll merge this on Monday if there are no further comments.

pitrou

My main concern here is: does this reduce interoperability with filesystem "hierarchies" created by other GCS clients?

pitrou · 2022-01-10T17:29:03Z

cpp/src/arrow/filesystem/gcsfs.cc

+    // To find out if the directory exists we need to perform an additional query.
+    ARROW_ASSIGN_OR_RAISE(auto directory, GetFileInfo(p));
+    if (directory.IsDirectory()) return result;
+    return Status::IOError("No such file or directory '", select.base_dir, "'");


If the error message accurate? If base_dir is an existing file, do we still get this message?

pitrou · 2022-01-10T17:33:18Z

cpp/src/arrow/filesystem/gcsfs.cc

-      if (!status.ok()) {
-        return status;
+    }
+    for (auto d = missing_parents.rbegin(); d != missing_parents.rend(); ++d) {


For the record, is it useful to iterate these in reverse order? Is it to avoid creating a/b if a exists as a file?

Yes. Added a comment to explain that.

Is the comment a bit off? It seems missing_parents is created in reverse depth order (from child to parent), so iterating it in reverse order visits the ancestors first and the descendents last?

pitrou · 2022-01-10T17:38:56Z

cpp/src/arrow/filesystem/gcsfs_test.cc

+                   [](FileInfo const& info) {
+                     if (!info.IsDirectory()) return info;
+                     return Dir(internal::RemoveTrailingSlash(info.path()).to_string());
+                   });


This is non-trivial enough to be factored out in a separate function, IMHO.

coryan · 2022-01-10T23:48:03Z

My main concern here is: does this reduce interoperability with filesystem "hierarchies" created by other GCS clients?

This is how I understand the problem:

The Apache/Arrow APIs want to be able to query if a directory "exists", even if empty.
That means we need to put some kind of marker in the GCS file system or any query will return kNotFound (even listing)
Most tools and libraries using GCS natively do not bother with these markers at all, so whatever we do will need to work when there are no markers (by basically listing all the files prefixed with directory/ whether a marker is found or not).
We can use markers using a trailing slash name/, that makes things more similar to the GCS UI.
That breaks the generic FS tests, and one may need two RPCs to check if name is a file or a directory.
- And still get in trouble because something could create name and name/ in GCS (and name// for that matter).
Using metadata works for the generic FS tests, requires fewer RPCs, and generally seems to fit the APIs in Apache "better"
- It makes things less similar to the GCS UI 🤷

That is, both solutions have downsides. If you want things to be more compatible with the GCS UI we can certainly do that. I think the really interesting case is working when there are no markers at all, which we can get to work in both cases.

pitrou · 2022-01-11T13:25:25Z

I think the really interesting case is working when there are no markers at all, which we can get to work in both cases.

Great, I have no problem with this approach then.

pitrou

+1, thank you @coryan

ursabot · 2022-01-11T17:11:13Z

Benchmark runs are scheduled for baseline = aa59d17 and contender = 2164b0b. 2164b0b 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.45% ⬆️0.0%] ursa-i9-9960x
[Failed ⬇️0.04% ⬆️0.09%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added the Component: C++ label Dec 20, 2021

coryan marked this pull request as ready for review December 20, 2021 15:32

ARROW-15114: [C++] GcsFileSystem uses metadata for directory markers

3f85930

emkornfield reviewed Jan 6, 2022

View reviewed changes

emkornfield requested changes Jan 7, 2022

View reviewed changes

Address review comments

43c14e5

pitrou reviewed Jan 10, 2022

View reviewed changes

Address review comments

b88b90e

coryan added 2 commits January 11, 2022 13:39

Doh! wrong comment, thanks to @pitrou for catching that

1ec19e0

Fix lints

aba03e0

pitrou approved these changes Jan 11, 2022

View reviewed changes

pitrou closed this in 2164b0b Jan 11, 2022

asfimport mentioned this pull request Jan 13, 2022

[C++] GcsFileSystem should use metadata to distinguish directory markers #30625

Closed

	static bool IsDirectory(gcs::ObjectMetadata const& o) {
	static bool IsDirectory(const gcs::ObjectMetadata& o) {

	// Create object
	if (recursive) {
	// Ensure bucket exists
	ARROW_ASSIGN_OR_RAISE(bool bucket_exists, impl_->BucketExists(path.bucket));
	if (!bucket_exists) {
	RETURN_NOT_OK(impl_->CreateBucket(path.bucket));
	}

Conversation

coryan commented Dec 20, 2021

Uh oh!

github-actions bot commented Dec 20, 2021

Uh oh!

coryan commented Dec 20, 2021

Uh oh!

coryan commented Dec 21, 2021

Uh oh!

coryan commented Dec 22, 2021

Uh oh!

coryan commented Jan 3, 2022

Uh oh!

coryan commented Jan 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emkornfield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emkornfield commented Jan 7, 2022

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coryan commented Jan 10, 2022

Uh oh!

pitrou commented Jan 11, 2022

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

ursabot commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

ursabot commented Jan 11, 2022 •

edited

Loading