Skip to content
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

fix: Do not set job timeout extra property if None #1987

Merged
merged 2 commits into from
Aug 19, 2024

Conversation

bmwant
Copy link
Contributor

@bmwant bmwant commented Aug 5, 2024

Description

job_timeout_ms declared as optional (see docs), but setting it as None results in error:

google.api_core.exceptions.BadRequest: 400 POST https://bigquery.googleapis.com/bigquery/v2/projects/my-bq-project/jobs?prettyPrint=false: Invalid value at 'job.configuration.job_timeout_ms.value' (TYPE_INT64), "None"

Location: None
Job ID: 5f5bf5bd-dc77-7eda-a2d3-69420a6bf6d6
 [{'@type': 'type.googleapis.com/google.rpc.BadRequest', 'fieldViolations': [{'field': 'job.configuration.job_timeout_ms.value', 'description': 'Invalid value at \'job.configuration.job_timeout_ms.value\' (TYPE_INT64), "None"'}]}]

because of allowing None, but not handling it properly. This fix doesn't cast None to a string in setter. Getter still works as expected, but we do not send invalid value in remote call.

Simple script to reproduce

from typing import Optional

from google.cloud import bigquery

query = "SELECT * FROM `project.table` LIMIT 1"
client = bigquery.Client()
config = bigquery.QueryJobConfig(use_query_cache=True, allow_large_results=True)
my_timeout: Optional[int] = None
config.job_timeout_ms = my_timeout
client.query(query, job_config=config)

@bmwant bmwant requested review from a team as code owners August 5, 2024 14:35
@bmwant bmwant requested a review from PhongChuong August 5, 2024 14:35
Copy link

google-cla bot commented Aug 5, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link

conventional-commit-lint-gcf bot commented Aug 5, 2024

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. api: bigquery Issues related to the googleapis/python-bigquery API. labels Aug 5, 2024
@bmwant bmwant changed the title Allow setting job_timeout_ms to None Do not set job timeout extra property if None Aug 5, 2024
@bmwant bmwant force-pushed the bmwant/fix-job-timeout-none branch from d4a5eaf to f76a4fe Compare August 5, 2024 15:05
@Linchin Linchin assigned Linchin and unassigned GaoleMeng Aug 6, 2024
@bmwant bmwant force-pushed the bmwant/fix-job-timeout-none branch from f76a4fe to 431802f Compare August 8, 2024 11:57
@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: xs Pull request size is extra small. labels Aug 8, 2024
@bmwant
Copy link
Contributor Author

bmwant commented Aug 8, 2024

@Linchin updated, could you review?

@bmwant bmwant force-pushed the bmwant/fix-job-timeout-none branch from f2e77dc to b30ca9f Compare August 12, 2024 18:51
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Aug 12, 2024
@bmwant bmwant force-pushed the bmwant/fix-job-timeout-none branch 2 times, most recently from 07ca2d2 to 602c0b5 Compare August 12, 2024 18:55
@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: m Pull request size is medium. labels Aug 12, 2024
@Linchin Linchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 13, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 13, 2024
@Linchin Linchin changed the title Do not set job timeout extra property if None fix: Do not set job timeout extra property if None Aug 13, 2024
@Linchin Linchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 13, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 13, 2024
@bmwant bmwant force-pushed the bmwant/fix-job-timeout-none branch from 786a5ef to 6417497 Compare August 14, 2024 15:11
@bmwant
Copy link
Contributor Author

bmwant commented Aug 14, 2024

@Linchin rebased on the latest main branch

@Linchin Linchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 14, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 14, 2024
@Linchin Linchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 15, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 15, 2024
@Linchin
Copy link
Contributor

Linchin commented Aug 15, 2024

Thank you for making the contribution and fix this bug for us! I wonder if you would like to lint the code, as this seems to be the only test that's failing. I haven't had the time to do it. To lint you need to install nox (pip install nox) and run nox -s lint in the repo.

@bmwant bmwant force-pushed the bmwant/fix-job-timeout-none branch from e73a9ae to 166ca14 Compare August 18, 2024 11:37
@bmwant
Copy link
Contributor Author

bmwant commented Aug 18, 2024

Fixed linting, you can re-run checks now

@Linchin Linchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 19, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 19, 2024
Copy link
Contributor

@Linchin Linchin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Linchin Linchin merged commit edcb79c into googleapis:main Aug 19, 2024
17 checks passed
@bmwant bmwant deleted the bmwant/fix-job-timeout-none branch August 19, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants