-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(bigquery): route JOB_CREATION_REQUIRED through fast query path #13437
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
Open
jinseopkim0
wants to merge
2
commits into
main
Choose a base branch
from
fix/fast-query-latency
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+66
−9
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
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 hidden or 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 hidden or 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 hidden or 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 hidden or 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
Oops, something went wrong.
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.
qq, wouldn't this just end up always doing a fast query (default is set to required)? IIUC, I think should be a fast query only for
JobCreationMode.JOB_CREATION_OPTIONAL?Is there any performance impact or behavioral change if default to a fast query even if a user explicitly sets job_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.
Thanks for the questions.
Yes, this is intended.
Fast query should be for both. The BigQuery backend always creates a job in the background and returns a
jobReference(including thejobId, see https://docs.cloud.google.com/bigquery/docs/reference/rest/v2/jobs/query#QueryResponse) for alljobs.queryrequests, so fast query makes sense.The result is faster queries and lower latencies. There is no behavioral change as a job is still created in the background and tracked as expected.
Uh oh!
There was an error while loading. Please reload this page.
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.
Gotcha, I think faster queries is always better for the user, but something seems a bit weird about
JobCreationModeto me. Why even have anJOB_CREATION_OPTIONALconfiguration on the client side and not just do it under the hood on the server side?Since we have this configuration, it seems odd to have a customer specify
JOB_CREATION_REQUIREDand potentially not result in a job back. Could the original issue be solved if fast query runs only whenJOB_CREATION_OPTIONALis specified? It seems like the issue was the fast query logic was running on the wrong conditions?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.
Thanks for the questions.
JOB_CREATION_OPTIONALmay run stateless queries andjobReferencemay be null, enabling stateless optimizations.If a customer specifies
JOB_CREATION_REQUIRED, ajobReference(with ajobId) is returned in the response. (https://docs.cloud.google.com/bigquery/docs/reference/rest/v2/jobs/query#QueryResponse)JOB_CREATION_REQUIREDis the default mode. If fast query only ran forOPTIONAL, all default queries would remain on the slow fallback path. The original issue (b/522363981) was a latency issue due to fast query logic not running whenJOB_CREATION_REQUIREDwas true.Uh oh!
There was an error while loading. Please reload this page.
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.
Sorry I meant, I think it's weird for BigQuery service team to even expose a
JobCreationModeproto if the intention was to do fast query by default. That's why it makes me think that we shouldn't do this by default and only do it if the mode is set to optional (but if other BQ clients are doing this, then happy to stand corrected).From what I see in b/522363981, they fixed it by explicitly setting the
JOB_CREATION_OPTIONAL. I think the only fix we need to setconfig.getJobCreationMode() == JobCreationMode.JOB_CREATION_OPTIONALto ensure that the benchmark uses fast query.