-
Notifications
You must be signed in to change notification settings - Fork 306
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: search statistics #1616
feat: search statistics #1616
Changes from 8 commits
b491836
0cbb7f4
1dbf528
9695712
fcf7012
ae992bf
3815371
48086bd
5b14d5d
9689a71
6af7d6f
53437f2
4fd77ba
bb2b52c
e728883
966ddb1
e9fca23
ba2eb65
a0a0e5b
852514f
2a14bc4
8493c3a
7991e4d
62f9bcc
705084b
4871339
93af6d9
b0196c1
711d752
ccf87a9
5bc0082
0089524
fe23c18
9bb8f14
ddd86bd
c55ba91
dc150bd
7d68a8a
2e438d3
93c345b
0423fdc
496ab6d
392db37
a30f23c
c1dfc35
5a2f268
744d932
81f8aab
861eb98
20ec9e3
5659dd3
dd1b749
e056787
5ebcbb0
faafac2
9ab5c25
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -198,6 +198,39 @@ def from_api_repr(cls, stats: Dict[str, str]) -> "DmlStats": | |
return cls(*args) | ||
|
||
|
||
class SearchReason(typing.NamedTuple): | ||
"""docstring""" | ||
|
||
code: str | ||
message: str | ||
baseTable: TableReference | ||
indexName: str | ||
|
||
@classmethod | ||
def from_api_repr(cls, reason): | ||
code = reason.get("code") | ||
message = reason.get("message") | ||
baseTable = reason.get("baseTable") | ||
indexName = reason.get("indexName") | ||
|
||
return cls(code, message, baseTable, indexName) | ||
|
||
|
||
class SearchStats(typing.NamedTuple): | ||
"""docstring""" | ||
|
||
mode: str | ||
reason: list | ||
|
||
@classmethod | ||
def from_api_repr(cls, stats: Dict[str, Any]): | ||
mode = stats.get("indexUsageMode", "INDEX_USAGE_MODE_UNSPECIFIED") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally, we want to avoid the library injecting values not present in the API response, so returning the default value here may not be more desirable than empty/none. |
||
reason = [ | ||
SearchReason.from_api_repr(r) for r in stats.get("indexUnusedReasons") | ||
] | ||
return cls(mode, reason) | ||
|
||
|
||
class ScriptOptions: | ||
"""Options controlling the execution of scripts. | ||
|
||
|
@@ -724,7 +757,6 @@ def to_api_repr(self) -> dict: | |
Dict: A dictionary in the format used by the BigQuery API. | ||
""" | ||
resource = copy.deepcopy(self._properties) | ||
|
||
# Query parameters have an addition property associated with them | ||
# to indicate if the query is using named or positional parameters. | ||
query_parameters = resource["query"].get("queryParameters") | ||
|
@@ -858,6 +890,13 @@ def priority(self): | |
""" | ||
return self.configuration.priority | ||
|
||
@property | ||
def search_stats(self): | ||
chalmerlowe marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"""docstring""" | ||
|
||
stats = self._job_statistics().get("searchStatistics") | ||
return SearchStats.from_api_repr(stats) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need to handle the not-present case, e.g. |
||
|
||
@property | ||
def query(self): | ||
"""str: The query text used in this query job. | ||
|
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.
the underlying type is IndexUnusedReason, why the name change?
Also, I'd expect property definitions for this type, and I'm not seeing them here.
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.
Regarding naming:
I simply mirrored naming conventions that were already present in the code:
BiEngineStats
BiEngineReason
My take was that
SearchReason
andSearchStats
specifically highlights the broader purpose/use of the Reason (i.e. this reason is related to the Search function):If we think we get greater clarity by calling the class IndexUnusedReason, I am game to do that.
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.
Can you elaborate on what you mean by property definitions?
Are to talking about expanding on the docstring and parameter definitions? If yes, that is yet to be done. At this second, I am focused on does the code work/seem reasonable/have adequate tests.
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 mostly mean getters with the "@Property" annotation for referencing fields in the message types. As these are primarily readonly fields setters shouldn't be necessary.
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.
And re: type names, we largely use the names from the API representation / discovery doc:
https://cloud.google.com/bigquery/docs/reference/rest/v2/Job#SearchStatistics
https://bigquery.googleapis.com/discovery/v1/apis/bigquery/v2/rest
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.
@shollyman I got the pandas references out.
I will see about renaming the underlying type.
Did you come across any ideas on whether we need/want to add @properties when using a
typing.NamedTuple