-
Notifications
You must be signed in to change notification settings - Fork 304
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: support BI Engine statistics in query job #1144
Conversation
The REST API returns BiEngineStatistics for a query which denotes if the query was accelerated by BI Engine or not. This commit adds the necessary function to access this information for executed queries.
Thanks for the review! Appreciate it. I'll get to the changes. Since I mostly write OCaml, I am used to writing enums for everything! Also, just needed some help on the system tests? I'd like to add system tests for the BI Engine statistics, but does the CI have BI Engine reservation? I can reliably trigger BI Engine messages on my local machine, but was wondering what the recommended practice was? |
We do not currently, and I'm not certain that we'd want to due to cost reasons (plus we try to avoid long-lived resources in our CI infrastructure) I'm okay with just unit tests for this. I'll make sure to give a careful look comparing the implementation with the REST docs. |
Yup, that makes sense. Thanks! Do let me know if anything else is required. |
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.
Looking good. Thanks so much for the contribution. Just a little bit more to get this ready for merge.
google/cloud/bigquery/job/query.py
Outdated
if stats is not None: | ||
return None | ||
else: | ||
return BiEngineStats.from_api_repr(stats) |
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 think we've got the logic flipped here. (Possibly an indication we need some unit test coverage for this?)
if stats is not None: | |
return None | |
else: | |
return BiEngineStats.from_api_repr(stats) | |
if stats is None: | |
return None | |
else: | |
return BiEngineStats.from_api_repr(stats) |
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.
Let's add a test much like this one:
python-bigquery/tests/unit/job/test_query.py
Line 644 in 39ade39
def test_ddl_target_routine(self): |
That checks that it can be None
if the stats aren't present, otherwise return the expected object.
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.
My bad! Added a test case as well.
google/cloud/bigquery/job/query.py
Outdated
reasons = stats.get("biEngineReasons") | ||
|
||
if reasons is None: | ||
reasons = [] | ||
else: | ||
reasons = [BiEngineReason.from_api_repr(r) for r in reasons] |
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.
We can simplify this by using the "default" argument for get
reasons = stats.get("biEngineReasons") | |
if reasons is None: | |
reasons = [] | |
else: | |
reasons = [BiEngineReason.from_api_repr(r) for r in reasons] | |
reasons = [BiEngineReason.from_api_repr(r) for r in stats.get("biEngineReasons", []] |
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.
Done! Thank you for the tip. Learnt something new today.
Thanks for your time. :) |
* chore: Add support for accessing BI Engine statistics The REST API returns BiEngineStatistics for a query which denotes if the query was accelerated by BI Engine or not. This commit adds the necessary function to access this information for executed queries. * fix: Removed enums and replaced with string constants * fix: Fixed logic for creating BIEngineStats and added test case * Attempt at mypy fix Co-authored-by: Tim Swast <swast@google.com>
The REST API return BiEngineStatistics which denotes if the query
was accelerated by BI Engine or not. This commit adds the necessary
function to access this information for executed queries.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #1054