-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Support job priority in BigQuery #1673
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works great @sjwhitworth!
For flags like this, I think it's a good idea to add some quick tests. It's an easy thing to miss in a refactor, and it wouldn't be obvious in manual testing if we introduced a regression here.
Can you add a unit test here? I think you should just be able to set a priority in one of the raw_profile
targets, then assert that it comes through in the corresponding Connection object on the other side.
Thanks for making this one!
@@ -192,6 +195,9 @@ def raw_execute(self, sql, fetch=False): | |||
|
|||
job_config = google.cloud.bigquery.QueryJobConfig() | |||
job_config.use_legacy_sql = False | |||
priority = conn.credentials.get('priority', 'interactive') | |||
job_config.priority = google.cloud.bigquery.QueryPriority.BATCH if priority == 'batch' \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just split this out into a couple of lines. Maybe:
if priority == 'batch':
job_config.priority = google.cloud.bigquery.QueryPriority.BATCH
else:
job_config.priority = google.cloud.bigquery.QueryPriority.INTERACTIVE
The else
clause probably isn't necessary here, but it's nice to be explicit!
Sure thing: like I mentioned, I hadn't got round to testing it properly. I'll add this. Do you want me to add anything on the documentation side? |
Here you go. I think it's not that great a test, given that it doesn't test the actual logic of setting the priority on the job config: just that we pass through the field to the connection obejct. Let me know if you had something else in mind. I also had to split setting the priority to interactive onto a new line to appease flake8. |
Great, thanks! I just kicked off the tests here. Don't worry about the docs - I usually touch those up when we cut a release - it's easier to do them all at once, and it's not ideal to document functionality that isn't actually live yet. I think this test is a-ok. It's unlikely that we'd outright break this code in the connection implementation - instead, these regressions tend to creep in around contract validation. Will merge this one when the tests pass! Thanks for your contribution! |
Supports the feature discussed in #1456. Haven't managed to test this properly yet, so if you want to merge it, feel free to take it for a spin.
Let me know what you'd like me to do on the documentation side of things as well.