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

Issue #6300 - Fix nested schema parsing in insert_rows #7022

Merged
merged 3 commits into from
Jan 2, 2019

Conversation

adamf
Copy link
Contributor

@adamf adamf commented Dec 24, 2018

Hi! Here's a potential fix for handling JSON serializing of nested schema in BigQuery (issue 6300).

I think before accepting this it would need tests, but I wanted to hear the maintainer's thoughts on the location of the fix (client.py or _helpers.py). Also, I'm sure there's a better way to do this, but I needed a fix for this problem in my project :)

Closes #6300

@adamf adamf requested a review from crwilcox as a code owner December 24, 2018 17:55
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Dec 24, 2018
@adamf
Copy link
Contributor Author

adamf commented Dec 24, 2018

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Dec 24, 2018
Copy link
Contributor

@tswast tswast left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I've requested a few changes in the review comments, but I'll actually handle these today. I appreciate your efforts and want to respect your time. We require 100% unit test coverage, so it'll probably be easier for me to tackle the refactor and unit testing.

bigquery/google/cloud/bigquery/client.py Outdated Show resolved Hide resolved
bigquery/google/cloud/bigquery/client.py Outdated Show resolved Hide resolved
bigquery/google/cloud/bigquery/client.py Outdated Show resolved Hide resolved
bigquery/google/cloud/bigquery/client.py Outdated Show resolved Hide resolved
@tswast tswast added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 28, 2018
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 28, 2018
Copy link
Contributor

@tswast tswast left a comment

Choose a reason for hiding this comment

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

I've updated the PR to move the added methods to _helpers.py with names matching the existing *_to_json functions in that module. I've updated existing unit tests to cover the new logic.

@tswast tswast added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 28, 2018
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 28, 2018
@WillianFuks
Copy link
Contributor

I was also working on a fix for the issue but ended up being too late as it seems lol.

Still, did some unit tests and system tests, maybe those can also be useful. For system tests I used the same issue as the one observed in the bitcoin table and now it's working :). Won't do PR as it doesn't seem necessary, the issue seems already fixed by now.

@adamf
Copy link
Contributor Author

adamf commented Dec 28, 2018

Thanks @tswast ! Appreciate the fixes and tests!

@tswast tswast merged commit e633ffd into googleapis:master Jan 2, 2019
tseaver added a commit that referenced this pull request Feb 7, 2019
Adds explicit unit tests for helpers added in #7022.

Closes #7294.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BigQuery: TypeError: b'v\xa9\' is not JSON serializable
5 participants