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

fix: raise error if inserting rows with unknown fields #163

Merged
merged 7 commits into from Jul 30, 2020

Conversation

@plamut
Copy link
Contributor

@plamut plamut commented Jul 11, 2020

Fixes #151.

This PR makes sure that unknown fields are not silently ignored when inserting rows, but instead such errors are detected and raised loudly.

PR checklist

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)
Copy link
Contributor

@shollyman shollyman left a comment

I think I'm missing something here. The user controls whether they want this behavior in tabledata.insertall via ignoreUnknownValues, which is present in the insert_rows_json() method signature, but I don't see how that's plumbed to this helper.

Loading

google/cloud/bigquery/_helpers.py Show resolved Hide resolved
Loading
google/cloud/bigquery/_helpers.py Outdated Show resolved Hide resolved
Loading
@plamut
Copy link
Contributor Author

@plamut plamut commented Jul 17, 2020

@shollyman Hmm, good point. I'll check how the helper can support the ignore_unknown_values parameter.

Loading

@plamut
Copy link
Contributor Author

@plamut plamut commented Jul 20, 2020

Changed the helper to include missing fields and let the backend handle that.

The ignore_unknown_values argument to user-facing methods in the client is orthogonal to that, thus the helper signature didn't need a change (FWIW, the backend honors the argument, checked it).

Loading

tswast
tswast approved these changes Jul 20, 2020
@plamut
Copy link
Contributor Author

@plamut plamut commented Jul 20, 2020

Just the coverage failure it seems, will cover the missed code path.

Loading

@tseaver
Copy link
Contributor

@tseaver tseaver commented Jul 30, 2020

Assuming that the Python3 test failures in test_download_arrow_tabledata_list_unknown_field_type due to a pyarrow release, I've kicked off a merge.

Loading

@tseaver tseaver merged commit 8fe7254 into googleapis:master Jul 30, 2020
7 of 8 checks passed
Loading
@plamut
Copy link
Contributor Author

@plamut plamut commented Jul 31, 2020

Indeed, and those tests were updated in https://github.com/googleapis/python-bigquery/pull/192/files

Loading

@plamut plamut deleted the iss-151 branch Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants