Support job priority in BigQuery #1673
Conversation
There was a problem hiding this comment.
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.
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.INTERACTIVEThe 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.