-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Added support for schema auto-detection feature in the job method `Lo… #3648
Added support for schema auto-detection feature in the job method `Lo… #3648
Conversation
I'll defer to @tswast or @jonparrott on review here. (In particular, review of the surface vs. review of the code itself.) |
@tswast is out-of-office. This surface seems reasonable to me. |
@@ -506,6 +529,11 @@ def output_rows(self): | |||
https://cloud.google.com/bigquery/docs/reference/rest/v2/jobs#configuration.load.allowQuotedNewlines | |||
""" | |||
|
|||
autodetect_schema = AutoDetectSchema('autodetect_schema', bool) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@dhermes could you do a review pass, too, and merge if it looks okay to you? |
Working on it now |
@tseaver I noticed you assigned yourself, don't want to cross paths here but I've been actually in a local checkout of the code for about 15 minutes. |
@dhermes Go ahead! |
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
@WillianFuks I just sent a commit into this PR with a few style tweaks (thanks for adhering to the existing style BTW). I did have a question about your expectation of the |
else: | ||
if not all(isinstance(field, SchemaField) for field in value): | ||
raise ValueError('Schema items must be fields') | ||
if self._configuration is not None and self.autodetect: |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Hi @dhermes , just to confirm, is everything ok in this PR? I saw this message from googlebot saying there's an issue with CLA, I wonder if I need to do something related. Thanks :)! |
@WillianFuks The CLA bot gets confused when there are commits from multiple authors, sorry for that. As for "is everything OK", I seem to recall that I was waiting on your for response on something, but I can't find any comment to that effect. |
I think the issue may just be the failed docs build. I'll rebase and hopefully it'll have been fixed by #3683 |
…adTableFromStorageJob`
…ackend API standard.
Most importantly, updating the `getattr(self, '_configuration', None)` to just access normally (the `schema` `@property` was being called in the constructor **before** the `_configuration` attribute was set).
Hello,
I did some implementation adding support for the feature "schema auto-detection" for BigQuery in the job method
LoadTableFromStorageJob
.Not sure if got it right, please let me know what you think and I can fix whatever issues you may find.