Repository navigation
perf(spanner): avoid allocating empty RequestOptions on read and query paths - #18422
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 refactors the handling of request_options and transaction_tag in both synchronous and asynchronous snapshot and transaction implementations, along with updating their unit tests. However, the changes removed the conversion of dictionary-based request_options to RequestOptions objects. As pointed out in the review feedback, this will lead to an AttributeError when attempting to set transaction_tag on a dictionary. To resolve this, request_options must be explicitly converted to a RequestOptions instance when a dictionary is provided.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of request_options and transaction_tag in both synchronous and asynchronous snapshot and transaction classes, alongside updating unit tests. However, the changes introduce a critical regression: removing the conversion of request_options from a dictionary to a RequestOptions object will cause an AttributeError when attempting to access or set transaction_tag on a raw dictionary. This issue is present across multiple files in the codebase. Additionally, several unit test helpers mask this bug by converting dictionaries to RequestOptions objects before invoking the production code, so adding test cases that pass raw dictionaries directly is recommended.
a51381a to
9ae7aca
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of request_options and transaction_tag in both the synchronous and asynchronous implementations of Spanner's Snapshot and Transaction classes, ensuring that RequestOptions are only instantiated when necessary. It also simplifies credential validation in Client and adds corresponding unit tests. However, the reviewer identified a regression in both the sync and async Snapshot.read and Snapshot.execute_sql methods: if _read_only is False and self.transaction_tag is None, any user-provided transaction_tag in request_options is unconditionally overwritten with None. To preserve the original behavior, the code should be updated to only overwrite the tag when self.transaction_tag is explicitly provided.
…y paths Previously, Snapshot.read(), Snapshot.execute_sql(), Transaction.execute_update(), and Transaction.batch_update() unconditionally initialized an empty `RequestOptions` message when no options were supplied, even when no transaction tag or client context was present. This empty submessage was then copied into the outgoing ExecuteSqlRequest or ReadRequest. Avoid allocating `RequestOptions` on the default execution path when both `request_options` and `transaction_tag` are unset. Only construct `RequestOptions` when an explicit option or transaction tag is provided, while preserving the existing behavior of overriding or clearing tags when `request_options` is supplied.
9ae7aca to
f53eaeb
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of request_options and transaction_tag in both synchronous and asynchronous snapshot and transaction classes, and updates the corresponding unit tests. However, the changes remove the logic that converts dictionary-based request_options into RequestOptions objects. As pointed out in the review feedback, this will cause AttributeError exceptions when attempting to access or modify transaction_tag on a dictionary. It is recommended to restore the dictionary check and conversion logic across all affected methods.
Combines all non-draft, non-'do not merge' Spanner micro-optimization PRs: - #18329: perf(spanner): optimize built-in metrics hot path and harden concurrency - #18359: perf(spanner): optimize query parameter encoding with direct type dispatch - #18379: perf(spanner): build ExecuteSqlRequest on the raw protobuf message - #18408: perf(spanner): optimize request ID header generation and retry closures - #18420: perf(spanner): prune lock and begin event allocations on single-use snapshots - #18422: perf(spanner): avoid allocating empty RequestOptions on read and query paths - #18602: perf(spanner): optimize single-chunk results and single-row lookups - #18604: test(spanner): add tests for consuming PartialResultSet streams
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of request_options in both synchronous and asynchronous Spanner snapshots and transactions. Instead of unconditionally initializing an empty RequestOptions object when none is provided, the code now only instantiates RequestOptions when a transaction tag is actually present, ensuring cleaner requests and preserving caller-specified options correctly. Corresponding unit tests have been added and updated to validate these changes, and I have no additional feedback to provide.
There was a problem hiding this comment.
Was this intentionally left because this PR is only for read/query path?
There was a problem hiding this comment.
Good point. It was originally intentionally left out as it was only for the read and query path, but then this change was also applied to update and batch_update, and then it also makes total sense to apply it to commit.
sinhasubham
left a comment
sinhasubham
left a comment
There was a problem hiding this comment.
Approving. Open comment is nit pick: if valid can be taken in separate PR.
🤖 I have created a release *beep* *boop* --- ## [3.72.0](google-cloud-spanner-v3.71.0...google-cloud-spanner-v3.72.0) (2026-11-12) ### Features * **dbapi:** wire timeout parameter through Connection to execute_sql ([#18484](#18484)) ([98fc304](98fc304)) * declare Python3.15 support ([8775794](8775794)) * **spanner:** auth login support for Spanner Omni endpoints ([#18273](#18273)) ([9e0548d](9e0548d)) * update API sources and regenerate ([#18797](#18797)) ([be4d954](be4d954)) ### Bug Fixes * **google-cloud-spanner:** clear buffered DDL statements on rollback and flush on autocommit ([#18636](#18636)) ([15d6a2c](15d6a2c)) * **google-cloud-spanner:** do not replay DDL statements on transaction retry ([#18625](#18625)) ([78b9da2](78b9da2)) * **spanner:** correctly wrap async streaming calls in metrics interceptor ([#18581](#18581)) ([2d19b0a](2d19b0a)) * **spanner:** eliminate duplicate metrics capture and measure full streaming latency ([#18605](#18605)) ([a7678fd](a7678fd)) * **spanner:** record streaming operation completion when stream finishes ([#18704](#18704)) ([ddeae47](ddeae47)) * **spanner:** release transaction lock if inline begin fails ([#18409](#18409)) ([dd24029](dd24029)) * **spanner:** stop deleting multiplexed sessions on rotation and close ([#18332](#18332)) ([c38e960](c38e960)) ### Performance Improvements * **spanner:** add fast-path for multiplexed session acquisition ([#18317](#18317)) ([9569b39](9569b39)) * **spanner:** avoid allocating empty RequestOptions on read and query paths ([#18422](#18422)) ([3743ff0](3743ff0)) * **spanner:** build ExecuteSqlRequest on the raw protobuf message ([#18379](#18379)) ([6d91fec](6d91fec)) * **spanner:** eliminate redundant tracing span in run_in_transaction ([#18357](#18357)) ([181e26a](181e26a)) * **spanner:** optimize built-in metrics hot path and harden concurrency ([#18329](#18329)) ([f2a791f](f2a791f)) * **spanner:** optimize query option merging and prevent in-place mutation ([#18358](#18358)) ([8cb3c6a](8cb3c6a)) * **spanner:** optimize query parameter encoding with direct type dispatch ([#18359](#18359)) ([f06497c](f06497c)) * **spanner:** optimize query result decoding and type resolution ([#18330](#18330)) ([ce72986](ce72986)) * **spanner:** optimize request ID header generation and retry closures ([#18408](#18408)) ([ad03b95](ad03b95)) * **spanner:** optimize row popping in StreamedResultSet ([#18316](#18316)) ([e66103d](e66103d)) * **spanner:** optimize single-chunk results and single-row lookups ([#18602](#18602)) ([05b3775](05b3775)) * **spanner:** prune lock and begin event allocations on single-use snapshots ([#18420](#18420)) ([ab56473](ab56473)) * **spanner:** short-circuit trace_call when tracing is inactive ([#18356](#18356)) ([58031be](58031be)) * **spanner:** support PartialResultSet.last with background stream draining ([#18320](#18320)) ([80efd63](80efd63)) ### Documentation * fix typos ([#18979](#18979)) ([5c8baeb](5c8baeb)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Previously, Snapshot.read(), Snapshot.execute_sql(), Transaction.execute_update(), and Transaction.batch_update() unconditionally initialized an empty
RequestOptionsmessage when no options were supplied, even when no transaction tag or client context was present. This empty submessage was then copied into the outgoing ExecuteSqlRequest or ReadRequest.Avoid allocating
RequestOptionson the default execution path when bothrequest_optionsandtransaction_tagare unset. Only constructRequestOptionswhen an explicit option or transaction tag is provided, while preserving the existing behavior of overriding or clearing tags whenrequest_optionsis supplied.