-
Notifications
You must be signed in to change notification settings - Fork 308
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
Merged
Merged
feat: search statistics #1616
Changes from 19 commits
Commits
Show all changes
56 commits
Select commit
Hold shift + click to select a range
b491836
experimental tweaks
chalmerlowe 0cbb7f4
feat: adds two search statistics classes and property
chalmerlowe 1dbf528
removes several personal debugging sentinels
chalmerlowe 9695712
Merge branch 'main' into feat-search-statistics
chalmerlowe fcf7012
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] ae992bf
Merge branch 'feat-search-statistics' of https://github.com/googleapi…
gcf-owl-bot[bot] 3815371
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] 48086bd
Merge branch 'feat-search-statistics' of https://github.com/googleapi…
gcf-owl-bot[bot] 5b14d5d
adds tests
chalmerlowe 9689a71
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] 6af7d6f
cleans up conflict
chalmerlowe 53437f2
adds comment
chalmerlowe 4fd77ba
adds some type hints, adds a test for SearchReasons
chalmerlowe bb2b52c
cleans up some comments
chalmerlowe e728883
Merge branch 'main' into feat-search-statistics
chalmerlowe 966ddb1
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] e9fca23
Merge branch 'feat-search-statistics' of https://github.com/googleapi…
gcf-owl-bot[bot] ba2eb65
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] a0a0e5b
Merge branch 'feat-search-statistics' of https://github.com/googleapi…
gcf-owl-bot[bot] 852514f
Update tests/unit/job/test_query_stats.py
chalmerlowe 2a14bc4
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] 8493c3a
Merge branch 'main' into feat-search-statistics
chalmerlowe 7991e4d
updated type checks to be isinstance checks per linter
chalmerlowe 62f9bcc
update linting
chalmerlowe 705084b
Update tests/unit/job/test_query_stats.py
chalmerlowe 4871339
Update tests/unit/job/test_query_stats.py
chalmerlowe 93af6d9
experiments with some tests that are failing
chalmerlowe b0196c1
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] 711d752
Fix linting
chalmerlowe ccf87a9
update package verification approach
chalmerlowe 5bc0082
update pandas installed version constant
chalmerlowe 0089524
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] fe23c18
remove unused package
chalmerlowe 9bb8f14
set pragma no cover
chalmerlowe ddd86bd
adds controls to skip testing if pandas exceeds 2.0
chalmerlowe c55ba91
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] dc150bd
adds pragma no cover to a simple check
chalmerlowe 7d68a8a
add checks against pandas 2.0 on system test
chalmerlowe 2e438d3
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] 93c345b
Merge branch 'main' into feat-search-statistics
chalmerlowe 0423fdc
Merge branch 'main' into feat-search-statistics
chalmerlowe 496ab6d
experiments with some tests that are failing
chalmerlowe 392db37
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] a30f23c
resolves merge conflict
chalmerlowe c1dfc35
resolves merge conflict
chalmerlowe 5a2f268
resolve conflicts
chalmerlowe 744d932
resolve merge conflicts
chalmerlowe 81f8aab
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] 861eb98
updates due to faulty confict resolution
chalmerlowe 20ec9e3
adds docstrings to two classes
chalmerlowe 5659dd3
corrects formatting
chalmerlowe dd1b749
Update tests/unit/job/test_query_stats.py
chalmerlowe e056787
Update tests/unit/job/test_query_stats.py
chalmerlowe 5ebcbb0
updates default values and corrects mypy errors
chalmerlowe faafac2
corrects linting
chalmerlowe 9ab5c25
Update google/cloud/bigquery/job/query.py
chalmerlowe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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