[GitHub] [arrow] coryan commented on a change in pull request #12763: ARROW-14892: [Python][C++] GCS Bindings

[GitBox <gi...@apache.org>]

coryan commented on a change in pull request #12763:
URL: https://github.com/apache/arrow/pull/12763#discussion_r841075393



##########
File path: cpp/src/arrow/filesystem/gcsfs.cc
##########
@@ -536,8 +546,7 @@ class GcsFileSystem::Impl {
                                                            gcs::ReadFromOffset offset) {
     auto stream = client_.ReadObject(path.bucket, path.object, generation, offset);
     ARROW_GCS_RETURN_NOT_OK(stream.status());
-    return std::make_shared<GcsInputStream>(std::move(stream), path, gcs::Generation(),
-                                            offset, client_);
+    return std::make_shared<GcsInputStream>(std::move(stream), path, generation, client_);

Review comment:
       Assume `offset` into this function is 1000.   Without the `offset` parameter passed to `GcsInputStream` its `Tell()` function will return 0 when you are in fact reading byte 1000.  That seems like the wrong semantics to me, but maybe it is the expected behavior?
   

##########
File path: cpp/src/arrow/filesystem/gcsfs.cc
##########
@@ -33,13 +33,23 @@
 
 namespace arrow {
 namespace fs {
-struct GcsCredentials {
-  explicit GcsCredentials(std::shared_ptr<google::cloud::Credentials> c)
+struct GcsCredentialsHolder {
+  explicit GcsCredentialsHolder(std::shared_ptr<google::cloud::Credentials> c)
       : credentials(std::move(c)) {}
 
   std::shared_ptr<google::cloud::Credentials> credentials;
 };
 
+bool GcsCredentials::Equals(const GcsCredentials& other) const {
+  if (holder_->credentials == other.holder_->credentials) {
+    return true;
+  }
+  return anonymous_ == other.anonymous_ && access_token_ == other.access_token_ &&

Review comment:
       I am not sure I understand the motivation for these changes around credentials.  Is that to provide more "accurate" results from `operator==`?  If so, consider just adding a `debug_string_` or something similar, new credential types are possible in the future, and it seems sad to change a lot of code just to add a new credential type.
   
   Ultimately this is your call, I do not understand the expectations in this project around values vs. opaque types.
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org