perf(spanner): avoid allocating empty RequestOptions on read and query paths

[olavloite]

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.

[gemini-code-assist]

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.

[olavloite]

/gemini review

[gemini-code-assist]

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.

[olavloite]

/gemini review

[gemini-code-assist]

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.

[olavloite]

/gemini review

[gemini-code-assist]

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.