Skip to content
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

ARROW-16248: [C++][FS] Actually enable GCS support in FileSystemFromUri() #12932

Closed
wants to merge 1 commit into from

Conversation

kou
Copy link
Member

@kou kou commented Apr 20, 2022

Define the ARROW_GCS macro, otherwise the relevant code wasn't compiled.

@kou kou requested review from pitrou and emkornfield April 20, 2022 07:17
@kou
Copy link
Member Author

kou commented Apr 20, 2022

Cc: @coryan

@github-actions
Copy link

@github-actions
Copy link

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@pitrou pitrou changed the title ARROW-16248: [C++][FS] Add GCS support to FileSystemFromUri() ARROW-16248: [C++][FS] Update FileSystemFromUri() docstring for GCS support Apr 20, 2022
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -45,6 +45,7 @@
#cmakedefine ARROW_IPC
#cmakedefine ARROW_JSON

#cmakedefine ARROW_GCS
Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Contributor

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

@pitrou pitrou changed the title ARROW-16248: [C++][FS] Update FileSystemFromUri() docstring for GCS support ARROW-16248: [C++][FS] Actually enable GCS support in FileSystemFromUri() Apr 20, 2022
Copy link
Contributor

@coryan coryan left a 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.

@emkornfield
Copy link
Contributor

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

@pitrou
Copy link
Member

pitrou commented Apr 20, 2022

For the record, @emkornfield 's PR is #12763.

@kou
Copy link
Member Author

kou commented Apr 20, 2022

OK. I withdraw this in favor of #12763.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants