perf(spanner): optimize query option merging and prevent in-place mutation - #18358
perf(spanner): optimize query option merging and prevent in-place mutation#18358olavloite wants to merge 1 commit into
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.
1 participant
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.