Skip to content

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)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 11, 2020
@plamut plamut requested a review from shollyman July 11, 2020 12:51
@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 13, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 13, 2020
Copy link
Contributor

@shollyman shollyman left a comment

Choose a reason for hiding this comment

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

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.

@plamut
Copy link
Contributor Author

plamut commented Jul 17, 2020

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

@plamut
Copy link
Contributor Author

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).

@plamut
Copy link
Contributor Author

plamut commented Jul 20, 2020

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

@tseaver
Copy link
Contributor

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.

@tseaver tseaver merged commit 8fe7254 into googleapis:master Jul 30, 2020
@plamut
Copy link
Contributor Author

plamut commented Jul 31, 2020

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

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.

client.insert_rows should fail when inserting non-existing fields
6 participants