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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
