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.

Sign up for GitHub

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

Jump to bottom

Using Jobs.insert instead of Jobs.query for queries #2323

Merged
merged 2 commits into from
Aug 10, 2017
Merged

Using Jobs.insert instead of Jobs.query for queries #2323

merged 2 commits into from
Aug 10, 2017

Conversation

garrettjonesgoogle
Copy link
Member

Fixes #1588 .

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 9, 2017
tswast
tswast reviewed Aug 9, 2017
View reviewed changes
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 see two changes in here:

  1. add query params to the query job config. Those changes look fine to me.
  2. Add job ID argument to query function. I think we might want to go a step further and remove the QueryResponse / QueryRequest classes.

google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/BigQuery.java
@@ -987,6 +987,19 @@ public static QueryResultsOption maxWaitTime(long maxWaitTime) {
QueryResponse query(QueryRequest request);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@garrettjonesgoogle
Copy link
Member Author

How about I just make the new function @BetaApi, since we'll go and break it later?

tswast
tswast approved these changes Aug 9, 2017
View reviewed changes
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. Makes sense to limit the scope and mark as @BetaAPI to indicate it's likely to break.

These changes as-is are a big improvement. Thanks.

@garrettjonesgoogle garrettjonesgoogle merged commit 303f43c into master Aug 10, 2017
@garrettjonesgoogle garrettjonesgoogle mentioned this pull request Aug 10, 2017
Closed
@pongad pongad deleted the alt branch March 8, 2018 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tswast tswast tswast approved these changes

@vam-google vam-google Awaiting requested review from vam-google

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@garrettjonesgoogle @tswast @googlebot