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(bigquery): add support of model for extract job #71

Merged
merged 8 commits into from May 11, 2020

Conversation

@HemangChothani
Copy link
Contributor

@HemangChothani HemangChothani commented Apr 1, 2020

Fixes #60

Copy link
Contributor

@plamut plamut left a comment

Looks good generally, just a few minor remarks.

Loading

google.cloud.bigquery.table.TableReference, \
google.cloud.bigquery.model.ModelReference \
]):
Table or Model into which data is to be loaded.
Copy link
Contributor

@plamut plamut Apr 2, 2020

Choose a reason for hiding this comment

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

Shouldn't this be "from which" instead for "into which", considering that it's a source?

Loading

tests/unit/test_client.py Show resolved Hide resolved
Loading
plamut
plamut approved these changes Apr 6, 2020
@HemangChothani
Copy link
Contributor Author

@HemangChothani HemangChothani commented Apr 29, 2020

@shollyman PTAL!

Loading

Copy link
Contributor

@shollyman shollyman left a comment

Do we have a model lifecycle test in the integration tests? If so, let's add extraction to the lifecycle. This feature is now live so no special setup is required.

Loading

google.cloud.bigquery.table.TableReference, \
google.cloud.bigquery.model.ModelReference \
]):
Table or Model from which data is to be loaded.
Copy link
Contributor

@shollyman shollyman May 7, 2020

Choose a reason for hiding this comment

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

Maybe s/loaded/extracted/ in the comment.

Loading

Copy link
Contributor Author

@HemangChothani HemangChothani May 8, 2020

Choose a reason for hiding this comment

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

@shollyman We don't have any model lifecycle test in the integration tests.

Loading

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.

4 participants