Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces validation for ARROW query results in query_and_wait to ensure that required dependencies (pyarrow and google-cloud-bigquery-storage) are installed and support the necessary features, and that unsupported configurations like WRITE_APPEND are rejected before sending the query. Feedback on the changes suggests importing the inspect module lazily inside the supports_arrow_serialization_options property to avoid overhead during library startup, and explicitly passing project and credentials when instantiating bigquery.Client in system tests to ensure reliability.
| import inspect | ||
| from typing import Any |
There was a problem hiding this comment.
Avoid importing inspect at the module level because _versions_helpers is imported during the startup of google-cloud-bigquery (via _arrow_query -> _job_helpers -> client). inspect is a relatively heavy module to import and is only needed when checking Arrow serialization options. Moving the import inside the supports_arrow_serialization_options property will improve the import/startup time of the library for all users who do not use Arrow queries.
| import inspect | |
| from typing import Any | |
| from typing import Any |
There was a problem hiding this comment.
Fair point, _versions_helpers is on the import path for everyone. Moved the import into the property in 6e55670.
| def supports_arrow_serialization_options(self) -> bool: | ||
| """True if ``BigQueryReadClient.read_rows()`` accepts | ||
| ``arrow_serialization_options``, which ``ARROW`` query results need. | ||
|
|
||
| Checks the signature instead of a version number, so the result is | ||
| also correct for a build of google-cloud-bigquery-storage that | ||
| hasn't been released yet. | ||
| """ | ||
| from google.cloud import bigquery_storage | ||
|
|
||
| read_rows = bigquery_storage.BigQueryReadClient.read_rows | ||
| return "arrow_serialization_options" in inspect.signature(read_rows).parameters |
There was a problem hiding this comment.
Import inspect lazily inside the property to avoid importing it at the module level during startup.
| def supports_arrow_serialization_options(self) -> bool: | |
| """True if ``BigQueryReadClient.read_rows()`` accepts | |
| ``arrow_serialization_options``, which ``ARROW`` query results need. | |
| Checks the signature instead of a version number, so the result is | |
| also correct for a build of google-cloud-bigquery-storage that | |
| hasn't been released yet. | |
| """ | |
| from google.cloud import bigquery_storage | |
| read_rows = bigquery_storage.BigQueryReadClient.read_rows | |
| return "arrow_serialization_options" in inspect.signature(read_rows).parameters | |
| def supports_arrow_serialization_options(self) -> bool: | |
| """True if BigQueryReadClient.read_rows() accepts | |
| arrow_serialization_options, which ARROW query results need. | |
| Checks the signature instead of a version number, so the result is | |
| also correct for a build of google-cloud-bigquery-storage that | |
| hasn't been released yet. | |
| """ | |
| import inspect | |
| from google.cloud import bigquery_storage | |
| read_rows = bigquery_storage.BigQueryReadClient.read_rows | |
| return "arrow_serialization_options" in inspect.signature(read_rows).parameters |
| job_config=append, | ||
| query_results_format=enums.QueryResultsFormat.ARROW, | ||
| ) | ||
| with bigquery.Client(default_query_job_config=append) as client: |
There was a problem hiding this comment.
In system tests, instantiating bigquery.Client() without explicit project and credentials arguments relies on ambient environment variables (like GOOGLE_CLOUD_PROJECT and GOOGLE_APPLICATION_CREDENTIALS). To ensure the test runs reliably and targets the correct test project/credentials configured for the test suite, it is safer to pass the project and credentials from the existing bigquery_client fixture.
| with bigquery.Client(default_query_job_config=append) as client: | |
| with bigquery.Client( | |
| project=bigquery_client.project, | |
| credentials=bigquery_client.credentials, | |
| default_query_job_config=append, | |
| ) as client: |
There was a problem hiding this comment.
The bigquery_client fixture in conftest.py is itself a plain bigquery.Client(), so this picks up the same project and credentials as the rest of the suite. I'd rather match the fixture than special-case this test.
1114b84 to
2f14961
Compare
| if query_results_format != enums.QueryResultsFormat.ARROW: | ||
| if compression_codec is not None: | ||
| raise ValueError("compression_codec requires query_results_format=ARROW.") | ||
| return |
There was a problem hiding this comment.
No action required: I'm wary of adding client-side checks like this. What if we add another results format that also takes a compression_codec? Unlikely, but something to ponder.
There was a problem hiding this comment.
The reason we check this client-side is that compression_codec isn't a top-level jobs.query field. We only serialize it into arrowSerializationOptions.bufferCompression when query_results_format=ARROW. Without this check, passing compression_codec="LZ4_FRAME" on a default JSON query wouldn't be sent on the wire at all and would be silently ignored. If another format supports compression_codec in the future, relaxing this check to allow that format will be backward-compatible.
2f14961 to
43a46d3
Compare
query_and_wait() with query_results_format=ARROW only found a missing pyarrow or google-cloud-bigquery-storage when results were read, after the query had already run. A query could work while its results fit in the jobs.query response and then fail once they needed a stream. It also accepted WRITE_APPEND, whose results can't be read on their own, and compression_codec without ARROW, where it does nothing. Check all three before the request is built. compression_codec without ARROW and an ARROW query with WRITE_APPEND (including through default_query_job_config) raise ValueError. An ARROW query checks pyarrow and google-cloud-bigquery-storage with the existing version helpers, and also checks that BigQueryReadClient.read_rows() accepts arrow_serialization_options. That check looks for ReadRowsStream.cancel(), which shipped in the same change, rather than a version number, so it's correct for the unreleased in-repo build that the system tests install until the next storage release.
43a46d3 to
74a74d8
Compare
|
Adding a |
Validates
query_and_wait(query_results_format=ARROW)before sending the query, so missing dependencies or unsupported options fail up front instead of after the query runs.ValueErrorforcompression_codecwithoutARROW, and forARROWwithWRITE_APPEND.read_rows()option from feat(bigquery-storage): add Arrow options and cancel to read_rows #18643, by looking forReadRowsStream.cancel().systemnox session until it's released.WRITE_APPEND.