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

feat: add support and tests for struct fields #146

Merged
merged 7 commits into from Aug 3, 2020

Conversation

@HemangChothani
Copy link
Contributor

@HemangChothani HemangChothani commented Jun 24, 2020

Fixes #21

  • Pyarrow version 0.17.0 has released so issue has been resolved but it doesn't support python2 so add condition for valueerror which raised when python2 excecuted.
@plamut
Copy link
Contributor

@plamut plamut commented Jun 24, 2020

Just a thought - if I understand correctly, this fix will only work in Python 3 due to the new pyarrow's incompatibility with Python 2?

If so, we might want to wait with this for a bit, since a green light was given to start transitioning the libraries to the new code generator, which also implies dropping Python 2 support.

Before this transition one final Python 2 compatible release will be made, but since this fix only affects Python 3, we might want to wait for an extra week or two, and then merge a simplified version of it.

Loading

@plamut
Copy link
Contributor

@plamut plamut commented Jul 20, 2020

It turned out there is still some work needed to transition the library to the new microgenerator (e.g. transitioning the dependencies first), which might take a while. Considering that, it's probably not worth waiting for it and getting the fix in sooner has value.

I'll review this tomorrow and see if it's good to merge and include in the next (non-major) release.

@HemangChothani It probably makes sense to conditionally bump the minimum pyarrow version to 0.17.0+ so that users on Python 3 will get the correct version that contains the fix.

Loading

Copy link
Contributor

@plamut plamut left a comment

Looks good, but we now also have to bump the minimum pyarrow version to 0.17.0 if python version >= 3.

Loading

plamut
plamut approved these changes Jul 22, 2020
Copy link
Contributor

@plamut plamut left a comment

LGTM, thanks for this.

Don't mind the samples, #174 contains a fix for them.

Loading

@tseaver tseaver changed the title feat(bigquery): add support and tests for struct fields feat: add support and tests for struct fields Jul 29, 2020
google/cloud/bigquery/_pandas_helpers.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/_pandas_helpers.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/_pandas_helpers.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/_pandas_helpers.py Outdated Show resolved Hide resolved
Loading
@tseaver
Copy link
Contributor

@tseaver tseaver commented Jul 30, 2020

@shollyman, @plamut I can't see any log for the failed Kokoro docs-presubmit job. Is that a tooling thing?

Loading

@tseaver
Copy link
Contributor

@tseaver tseaver commented Jul 30, 2020

On chat, I see that the Kokoro docs presubmit failures are known / expected while the infrastructure / configs get rolled out.

Loading

@tseaver
Copy link
Contributor

@tseaver tseaver commented Jul 30, 2020

@HemangChothani It looks like you have turned off commits from mainainers for this PR's branch, which means the automerge bot cannot "push the button" for you (nor can I).

Loading

@HemangChothani
Copy link
Contributor Author

@HemangChothani HemangChothani commented Jul 31, 2020

@tseaver I thnik that's my bad, can i merge this PR?

Loading

@busunkim96 busunkim96 closed this Jul 31, 2020
@plamut
Copy link
Contributor

@plamut plamut commented Jul 31, 2020

@HemangChothani In principle yes, but in the meantime @busunkim96 closed all PRs. It might be because of the CI maintenance work or something, don't know yet.

Loading

@busunkim96 busunkim96 reopened this Jul 31, 2020
@HemangChothani HemangChothani merged commit fee2ba8 into googleapis:master Aug 3, 2020
7 of 8 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants