-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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(bigquery): add support for custom QueryJobConfig in BigQuery.cursor.execute method #9278
feat(bigquery): add support for custom QueryJobConfig in BigQuery.cursor.execute method #9278
Conversation
942cfff
to
dd170ac
Compare
dd170ac
to
7d23584
Compare
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 this!
Before merging, though, we would still need to add the new parameter description to the method's docstring, and cover the change in the tests. We would be happy to provide any assistance, should that be necessary.
…ute cursor method
Thanks for feedback @plamut . I've applied changes suggested by you however still waiting for Kokoro check. |
Kokoro - Recommender check has failed however logs are empty. @plamut do you know maybe why it failed? I have no clue what is the reason. |
@TobKed Seems like flakiness, saw the same on a few other PRs as well. I will try restarting the jobs. Update: Actually, it seems to be a real issue in the Recommender library, not just flakiness. It currently happens on all PRs, and it's not related to the changes made 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.
LGTM, a custom job config indeed changes the SQL mode for the job on the backend.
@plamut thanks for feedback! Good luck with fixing Kokoro :) |
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.
LGTM. I'm a bit concerned about deviating from the DB-API spec for execute()
, but I see we already deviate due to job_id
.
My change allows to support
use_legacy_sql
.