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

- 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
staticboolIsDirectory(gcs::ObjectMetadataconst& o) {
staticboolIsDirectory(constgcs::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

- "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

- 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

+ 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

 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

- 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

 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

+ // 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

- 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

+ [](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

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

- 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
staticboolIsDirectory(gcs::ObjectMetadataconst& o) {
staticboolIsDirectory(constgcs::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

- "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

- 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

+ 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

 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

- 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

 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

+ // 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

- 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

+ [](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

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

- 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
staticboolIsDirectory(gcs::ObjectMetadataconst& o) {
staticboolIsDirectory(constgcs::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

- "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

- 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

+ 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

 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

- 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

 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

+ // 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

- 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

+ [](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

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

- 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
staticboolIsDirectory(gcs::ObjectMetadataconst& o) {
staticboolIsDirectory(constgcs::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

- "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

- 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

+ 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

 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

- 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

 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

+ // 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

- 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

+ [](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

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

- 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
staticboolIsDirectory(gcs::ObjectMetadataconst& o) {
staticboolIsDirectory(constgcs::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

- "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

- 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

+ 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

 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

- 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

 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

+ // 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

- 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

+ [](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

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

- 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
staticboolIsDirectory(gcs::ObjectMetadataconst& o) {
staticboolIsDirectory(constgcs::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

- "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

- 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

+ 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

 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

- 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

 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

+ // 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

- 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

+ [](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

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

- 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
staticboolIsDirectory(gcs::ObjectMetadataconst& o) {
staticboolIsDirectory(constgcs::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

- "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

- 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

+ 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

 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

- 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

 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

+ // 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

- 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

+ [](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

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

- 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
staticboolIsDirectory(gcs::ObjectMetadataconst& o) {
staticboolIsDirectory(constgcs::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

- "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

- 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

+ 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

 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

- 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

 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

+ // 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

- 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

+ [](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-actionsBot 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@pitroufor catching that
1ec19e0

Fix lints
aba03e0

pitrou approved these changes Jan 11, 2022
View reviewed changes

pitrou closed this in 2164b0bJan 11, 2022

asfimport mentioned this pull request Jan 13, 2022
[C++] GcsFileSystem should use metadata to distinguish directory markers #30625
Closed

github-actionsBot 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@pitroufor catching that
1ec19e0

Fix lints
aba03e0

pitrou approved these changes Jan 11, 2022
View reviewed changes

pitrou closed this in 2164b0bJan 11, 2022

asfimport mentioned this pull request Jan 13, 2022
[C++] GcsFileSystem should use metadata to distinguish directory markers #30625
Closed

github-actionsBot 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@pitroufor catching that
1ec19e0

Fix lints
aba03e0

pitrou approved these changes Jan 11, 2022
View reviewed changes

pitrou closed this in 2164b0bJan 11, 2022

asfimport mentioned this pull request Jan 13, 2022
[C++] GcsFileSystem should use metadata to distinguish directory markers #30625
Closed

github-actionsBot 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@pitroufor catching that
1ec19e0

Fix lints
aba03e0

pitrou approved these changes Jan 11, 2022
View reviewed changes

pitrou closed this in 2164b0bJan 11, 2022

asfimport mentioned this pull request Jan 13, 2022
[C++] GcsFileSystem should use metadata to distinguish directory markers #30625
Closed

github-actionsBot 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@pitroufor catching that
1ec19e0

Fix lints
aba03e0

pitrou approved these changes Jan 11, 2022
View reviewed changes

pitrou closed this in 2164b0bJan 11, 2022

asfimport mentioned this pull request Jan 13, 2022
[C++] GcsFileSystem should use metadata to distinguish directory markers #30625
Closed

github-actionsBot 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@pitroufor catching that
1ec19e0

Fix lints
aba03e0

pitrou approved these changes Jan 11, 2022
View reviewed changes

pitrou closed this in 2164b0bJan 11, 2022

asfimport mentioned this pull request Jan 13, 2022
[C++] GcsFileSystem should use metadata to distinguish directory markers #30625
Closed

github-actionsBot 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@pitroufor catching that
1ec19e0

Fix lints
aba03e0

pitrou approved these changes Jan 11, 2022
View reviewed changes

pitrou closed this in 2164b0bJan 11, 2022

asfimport mentioned this pull request Jan 13, 2022
[C++] GcsFileSystem should use metadata to distinguish directory markers #30625
Closed

github-actionsBot 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@pitroufor catching that
1ec19e0

Fix lints
aba03e0

pitrou approved these changes Jan 11, 2022
View reviewed changes

pitrou closed this in 2164b0bJan 11, 2022

asfimport mentioned this pull request Jan 13, 2022
[C++] GcsFileSystem should use metadata to distinguish directory markers #30625
Closed

	staticboolIsDirectory(gcs::ObjectMetadataconst& o) {
	staticboolIsDirectory(constgcs::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));
	}

Uh oh!

Conversation

coryan commented Dec 20, 2021

Uh oh!

github-actionsBot 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

ursabot commented Jan 11, 2022•
edited
Loading