Repository navigation
perf(spanner): optimize query option merging and prevent in-place mutation - #18358
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 introduces a new helper function _to_query_options to normalize query options (either dictionaries or QueryOptions objects) and refactors _merge_query_options to utilize this helper, simplifying the merging logic. It also adds comprehensive unit tests to cover various edge cases. The review feedback suggests avoiding direct access to the private _pb attribute in favor of the public proto-plus API type(options).pb(options), and optimizing the merging logic in _merge_query_options to prevent unnecessary double allocations.
c63b25c to
a5947df
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a helper function _to_query_options to normalize query options and refactors _merge_query_options to use it, along with adding comprehensive unit tests. The review feedback suggests optimizing _to_query_options by avoiding allocations when dictionary values are empty and simplifying control flow, as well as optimizing the merging path in _merge_query_options by directly using protobuf's CopyFrom and MergeFrom to bypass proto-plus constructor overhead.
…ation Short-circuit query option merging when options are unset to eliminate allocations on the hot path, make field handling generic, and merge into a fresh protobuf to avoid in-place mutation of base options.
a5947df to
3e476a1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new helper function _to_query_options to normalize query options (either a dictionary or a QueryOptions object) and refactors _merge_query_options to utilize this helper, simplifying the merging logic. Additionally, comprehensive unit tests have been added to verify the behavior of _to_query_options and various edge cases of _merge_query_options. I have no feedback to provide as the changes are well-implemented and thoroughly tested.
|
Nit: a misspelt key is only rejected when some value in the dict is non-empty, because the any(options.values()) check runs before the keys are validated. Previously, unknown keys were always silently dropped. Could we handle them the same way in both cases (either always raise, or always drop them) and add a test for it? Thanks! |
|
"", [] and False used to be treated as "no options" but now raise TypeError (the existing tests only cover "invalid" and 123, which failed before too). If this is intended, can we please add a test? |
- Ensure unknown keys in query options dictionaries consistently raise
ValueError regardless of whether values are empty or non-empty.
- Add test coverage verifying that falsy non-None types ("", [], False)
raise TypeError according to the QueryOptions contract.
Good point, done. |
Done |
🤖 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>
Short-circuit query option merging when options are unset to eliminate allocations on the hot path, make field handling generic, and merge into a fresh protobuf to avoid in-place mutation of base options.