-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-16248: [C++][FS] Actually enable GCS support in FileSystemFromUri() #12932
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Cc: @coryan |
|
|
pitrou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| #cmakedefine ARROW_IPC | ||
| #cmakedefine ARROW_JSON | ||
|
|
||
| #cmakedefine ARROW_GCS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't have this, ARROW_GCS macro isn't defined even when we build Apache Arrow with -DARROW_GCS=ON.
If ARROW_GCS macro isn't defined, we always get a "Got GCS URI but Arrow compiled without GCS support" error even when we build Apache Arrow with -DARROW_GCS=ON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see, does this mean there are no tests for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My PR also covers this
coryan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I guess I missed that. LGTM.
|
I would prefer not to merge this separately because I think without the python binding PR users are going to get unexpected results when reading existing datasets |
|
For the record, @emkornfield 's PR is #12763. |
|
OK. I withdraw this in favor of #12763. |
Define the
ARROW_GCSmacro, otherwise the relevant code wasn't compiled.