perf(spanner): add ToValue for &[u8] - #6070
Open
fornwall wants to merge 4 commits into
Open
Conversation
fornwall
changed the title
fornwall
force-pushed
the
up-2-tovalue-for-byte-slice
branch
from
July 16, 2026 12:56
f651f16 to
47d507f
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements the ToValue trait for byte slices (&[u8]) in the Spanner crate, enabling them to be encoded as base64 strings. It also adds a corresponding unit test to verify the correctness of this implementation. There are no review comments, and I have no feedback to provide.
ToValue was implemented for Vec<u8> but not for byte slices, so a caller holding a &[u8] had to allocate an owned Vec<u8> (via .to_vec()) purely to encode it, adding a copy per binary cell on potentially hot binding paths. Add impl ToValue for &[u8] holding the BYTES/base64 encoding, and have the Vec<u8> impl delegate to it via self.as_slice(). This keeps a single source of truth for the encoding (no risk of the two drifting) while both produce an identical Value. Signed-off-by: Fredrik Fornwall <fredrik@fornwall.net>
fornwall
force-pushed
the
up-2-tovalue-for-byte-slice
branch
from
July 16, 2026 12:59
47d507f to
f26e3bc
Compare
olavloite
reviewed
Jul 17, 2026
Co-authored-by: Knut Olav Løite <koloite@gmail.com>
olavloite
approved these changes
Jul 27, 2026
Contributor
|
/gcbrun |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6070 +/- ##
=======================================
Coverage 96.57% 96.58%
=======================================
Files 264 264
Lines 66577 66597 +20
=======================================
+ Hits 64297 64320 +23
+ Misses 2280 2277 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sakthivelmanii
approved these changes
Jul 27, 2026
This was referenced Jul 27, 2026
olavloite
added a commit
to olavloite/google-cloud-rust
that referenced
this pull request
Jul 27, 2026
Changes `StatementBuilder::add_param`, `StatementBuilder::add_typed_param`, and `ValueBinder::to` to accept `T: Into<Value>` instead of `&T: ToValue`. This allows callers to pass owned values directly (like `String`, `Value`, or `Option<T>`) zero-copy without unnecessary cloning, while maintaining complete backward compatibility for references (`&str`, `&i64`, `&Option<T>`). ### Key Changes - **Direct `From<T> for Value` Implementations**: Added zero-copy `From` conversions for owned types (`String`, `i64`, `i32`, `bool`, `f64`, `f32`, `Decimal`, `Date`, `OffsetDateTime`, `SystemTime`, `wkt::Timestamp`, `Vec<u8>`, `Option<T>`, `Vec<T>`, `ProtoValue`). - **Decoupled `ToValue`**: All `ToValue` trait implementations now delegate directly to `From` / `.into()`, decoupling conversion logic from `ToValue` and paving the way for eventual deprecation of the `ToValue` trait. - **Unsized Slice Support (`[u8]` and `str`)**: Implemented `ToValue` for `[u8]` and `str` (and `&[u8]`, `&str`), enabling zero-copy slice conversions and generic trait bounds `T: ToValue + ?Sized`. This supersedes both PR googleapis#6070 and PR googleapis#6087. - **Untyped NULL Support**: Added `Value::null()`, `From<()>` and `ToValue for ()` to easily support untyped NULL parameters (`Value::null()`, `None::<()>`, `None::<Value>`). - **Cleaned up Examples & Tests**: Updated example scripts and tests to pass owned values directly, eliminating needless reference borrows (`clippy::needless_borrows_for_generic_args`). BREAKING CHANGE: This is a minor breaking change only for code that passes inline `&None` with a method-level turbofish: ```rust // Old (fails to compile): builder.set("Col").to::<Option<bool>>(&None); // Works both before and after this change: builder.set("Col").to(&None::<bool>); // New (remove the &): builder.set("Col").to::<Option<bool>>(None); // Or: builder.set("Col").to(None::<bool>); builder.set("Col").to(Value::null()); ``` Code passing existing `Option` variables (like `.to(&my_var)`) is unaffected and continues to compile unchanged.
olavloite
added a commit
to olavloite/google-cloud-rust
that referenced
this pull request
Jul 27, 2026
Changes `StatementBuilder::add_param`, `StatementBuilder::add_typed_param`, and `ValueBinder::to` to accept `T: Into<Value>` instead of `&T: ToValue`. This allows callers to pass owned values directly (like `String`, `Value`, or `Option<T>`) zero-copy without unnecessary cloning, while maintaining complete backward compatibility for references (`&str`, `&i64`, `&Option<T>`). - **Direct `From<T> for Value` Implementations**: Added zero-copy `From` conversions for owned types (`String`, `i64`, `i32`, `bool`, `f64`, `f32`, `Decimal`, `Date`, `OffsetDateTime`, `SystemTime`, `wkt::Timestamp`, `Vec<u8>`, `Option<T>`, `Vec<T>`, `ProtoValue`). - **Decoupled `ToValue`**: All `ToValue` trait implementations now delegate directly to `From` / `.into()`, decoupling conversion logic from `ToValue` and paving the way for eventual deprecation of the `ToValue` trait. - **Unsized Slice Support (`[u8]` and `str`)**: Implemented `ToValue` for `[u8]` and `str` (and `&[u8]`, `&str`), enabling zero-copy slice conversions and generic trait bounds `T: ToValue + ?Sized`. This supersedes both PR googleapis#6070 and PR googleapis#6087. - **Untyped NULL Support**: Added `Value::null()`, `From<()>` and `ToValue for ()` to easily support untyped NULL parameters (`Value::null()`, `None::<()>`, `None::<Value>`). - **Cleaned up Examples & Tests**: Updated example scripts and tests to pass owned values directly, eliminating needless reference borrows (`clippy::needless_borrows_for_generic_args`). BREAKING CHANGE: This is a minor breaking change only for code that passes inline `&None` with a method-level turbofish: ```rust // Old (fails to compile): builder.set("Col").to::<Option<bool>>(&None); // Works both before and after this change: builder.set("Col").to(&None::<bool>); // New (remove the &): builder.set("Col").to::<Option<bool>>(None); // Or: builder.set("Col").to(None::<bool>); builder.set("Col").to(Value::null()); ``` Code passing existing `Option` variables (like `.to(&my_var)`) is unaffected and continues to compile unchanged.
olavloite
added a commit
to olavloite/google-cloud-rust
that referenced
this pull request
Jul 27, 2026
Changes `StatementBuilder::add_param`, `StatementBuilder::add_typed_param`, and `ValueBinder::to` to accept `T: Into<Value>` instead of `&T: ToValue`. This allows callers to pass owned values directly (like `String`, `Value`, or `Option<T>`) zero-copy without unnecessary cloning, while maintaining complete backward compatibility for references (`&str`, `&i64`, `&Option<T>`). - **Direct `From<T> for Value` Implementations**: Added zero-copy `From` conversions for owned types (`String`, `i64`, `i32`, `bool`, `f64`, `f32`, `Decimal`, `Date`, `OffsetDateTime`, `SystemTime`, `wkt::Timestamp`, `Vec<u8>`, `Option<T>`, `Vec<T>`, `ProtoValue`). - **Decoupled `ToValue`**: All `ToValue` trait implementations now delegate directly to `From` / `.into()`, decoupling conversion logic from `ToValue` and paving the way for eventual deprecation of the `ToValue` trait. - **Unsized Slice Support (`[u8]` and `str`)**: Implemented `ToValue` for `[u8]` and `str` (and `&[u8]`, `&str`), enabling zero-copy slice conversions and generic trait bounds `T: ToValue + ?Sized`. This supersedes both PR googleapis#6070 and PR googleapis#6087. - **Untyped NULL Support**: Added `Value::null()`, `From<()>` and `ToValue for ()` to easily support untyped NULL parameters (`Value::null()`, `None::<()>`, `None::<Value>`). - **Cleaned up Examples & Tests**: Updated example scripts and tests to pass owned values directly, eliminating needless reference borrows (`clippy::needless_borrows_for_generic_args`). BREAKING CHANGE: This is a minor breaking change only for code that passes inline `&None` with a method-level turbofish: ```rust // Old (fails to compile): builder.set("Col").to::<Option<bool>>(&None); // Works both before and after this change: builder.set("Col").to(&None::<bool>); // New (remove the &): builder.set("Col").to::<Option<bool>>(None); // Or: builder.set("Col").to(None::<bool>); builder.set("Col").to(Value::null()); ``` Code passing existing `Option` variables (like `.to(&my_var)`) is unaffected and continues to compile unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ToValuewas implemented forVec<u8>but not for byte slices, so a caller holding a&[u8]had to allocate an ownedVec<u8>purely to encode it, adding a copy per binary cell on potentially hot binding paths.Add
impl ToValue for &[u8]holding the BYTES/base64 encoding, and have theVec<u8>impl delegate to it viaself.as_slice().