Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a timestamp_precision parameter to query_and_wait to support picosecond precision for ARROW query results, along with corresponding validation, serialization, and tests. The review feedback recommends enhancing robustness by accepting both enum values and case-insensitive string representations for timestamp_precision, updating type annotations, and adding unit tests for these string inputs.
| "picosTimestampPrecision": ( | ||
| "TIMESTAMP_PRECISION_PICOS" | ||
| if timestamp_precision == enums.TimestampPrecision.PICOSECOND | ||
| else "TIMESTAMP_PRECISION_MICROS" | ||
| ), |
There was a problem hiding this comment.
To make the timestamp_precision parameter more robust and consistent with other parameters like compression_codec, we should support both the Enum members and their raw string representations (including case-insensitive inputs). Currently, passing a string like 'PICOSECOND' or 'picosecond' will silently fall back to 'TIMESTAMP_PRECISION_MICROS' without any warning or error. We can normalize the input using getattr and .upper() before performing the comparison.
"picosTimestampPrecision": (\n "TIMESTAMP_PRECISION_PICOS"\n if timestamp_precision is not None\n and str(getattr(timestamp_precision, "value", timestamp_precision)).upper() == "PICOSECOND"\n else "TIMESTAMP_PRECISION_MICROS"\n ),There was a problem hiding this comment.
I'm keeping this consistent with Client.query() and Client.list_rows(), which take the same timestamp_precision parameter and compare against TimestampPrecision.PICOSECOND the same way. TimestampPrecision is a plain Enum (PICOSECOND = 12), not a str enum, so accepting strings here but not there would be surprising. If we want string support it should be one change across all three methods.
| max_results: Optional[int] = None, | ||
| query_results_format: Optional[str] = None, | ||
| compression_codec: Optional[str] = None, | ||
| timestamp_precision: Optional[enums.TimestampPrecision] = None, |
There was a problem hiding this comment.
To support both the Enum members and their raw string representations (which is standard across Google Cloud Python libraries), please update the type annotation to include str.
| timestamp_precision: Optional[enums.TimestampPrecision] = None, | |
| timestamp_precision: Optional[Union[str, enums.TimestampPrecision]] = None, |
There was a problem hiding this comment.
Same reasoning as in the first thread on this PR: keeping the type the same as Client.query() and Client.list_rows().
| max_results: Optional[int] = None, | ||
| query_results_format: Optional[str] = None, | ||
| compression_codec: Optional[str] = None, | ||
| timestamp_precision: Optional[enums.TimestampPrecision] = None, |
There was a problem hiding this comment.
To support both the Enum members and their raw string representations (which is standard across Google Cloud Python libraries), please update the type annotation to include str.
| timestamp_precision: Optional[enums.TimestampPrecision] = None, | |
| timestamp_precision: Optional[Union[str, enums.TimestampPrecision]] = None, |
There was a problem hiding this comment.
Same reasoning as in the first thread on this PR: keeping the type the same as Client.query() and Client.list_rows().
| max_results: Optional[int] = None, | ||
| query_results_format: Optional[str] = None, | ||
| compression_codec: Optional[str] = None, | ||
| timestamp_precision: Optional[enums.TimestampPrecision] = None, |
There was a problem hiding this comment.
To support both the Enum members and their raw string representations (which is standard across Google Cloud Python libraries), please update the type annotation to include str.
| timestamp_precision: Optional[enums.TimestampPrecision] = None, | |
| timestamp_precision: Optional[Union[str, enums.TimestampPrecision]] = None, |
There was a problem hiding this comment.
Same reasoning as in the first thread on this PR: keeping the type the same as Client.query() and Client.list_rows().
| @pytest.mark.parametrize( | ||
| ("timestamp_precision", "expected"), | ||
| [ | ||
| (None, "TIMESTAMP_PRECISION_MICROS"), | ||
| (TimestampPrecision.MICROSECOND, "TIMESTAMP_PRECISION_MICROS"), | ||
| (TimestampPrecision.PICOSECOND, "TIMESTAMP_PRECISION_PICOS"), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
Let's add test cases for string values (both uppercase and lowercase) to ensure that the robust string normalization works as expected and doesn't regress in the future.
@pytest.mark.parametrize(\n ("timestamp_precision", "expected"),\n [\n (None, "TIMESTAMP_PRECISION_MICROS"),\n (TimestampPrecision.MICROSECOND, "TIMESTAMP_PRECISION_MICROS"),\n (TimestampPrecision.PICOSECOND, "TIMESTAMP_PRECISION_PICOS"),\n ("PICOSECOND", "TIMESTAMP_PRECISION_PICOS"),\n ("picosecond", "TIMESTAMP_PRECISION_PICOS"),\n ("MICROSECOND", "TIMESTAMP_PRECISION_MICROS"),\n ("microsecond", "TIMESTAMP_PRECISION_MICROS"),\n ],\n)There was a problem hiding this comment.
Same reasoning as in the first thread on this PR: keeping the type the same as Client.query() and Client.list_rows().
ARROW results always truncated picosecond TIMESTAMP columns to microseconds, because query_and_wait() had no way to ask for another precision. Client.query() and Client.list_rows() already take timestamp_precision for STRUCT_ENCODING results. Add the same timestamp_precision parameter, with the existing TimestampPrecision enum, to query_and_wait(). PICOSECOND maps to TIMESTAMP_PRECISION_PICOS, which returns picosecond columns at full precision as ISO 8601 strings, and anything else maps to TIMESTAMP_PRECISION_MICROS. Nanoseconds aren't offered, since the picosecond strings already keep full precision. The value goes into the arrowSerializationOptions sent on jobs.query and ReadRows, and setting it without ARROW raises ValueError, like compression_codec. It's labeled [Private Preview], as on Client.query().
9cc4ea4 to
660ff1d
Compare
This branch has not been deployed
1 participant
Adds
timestamp_precisiontoquery_and_wait()for Arrow results, likeClient.query()andClient.list_rows()already have, so picosecond timestamps aren't truncated to microseconds.TimestampPrecision.PICOSECONDreturns full-precision ISO 8601 strings. Anything else returns microseconds.ValueErrorif it's set withoutARROW.TIMESTAMP(12), and picosecond table columns.