Skip to content

feat(spanner): accept T: Into<Value> in parameter and mutation bindings - #6184

Open
olavloite wants to merge 1 commit into
googleapis:mainfrom
olavloite:perf-spanner-by-value-param-binding
Open

feat(spanner): accept T: Into<Value> in parameter and mutation bindings#6184
olavloite wants to merge 1 commit into
googleapis:mainfrom
olavloite:perf-spanner-by-value-param-binding

Conversation

@olavloite

@olavloite olavloite commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

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 perf(spanner): add ToValue for &[u8] #6070 and PR feat(spanner): implement ToValue for str #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

BREAKING CHANGE: This is a minor breaking change only for code that passes inline &None with a method-level turbofish:

// 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 pull request replaces the following pull requests:

@olavloite
olavloite requested review from a team as code owners July 27, 2026 15:13
@product-auto-label product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Jul 27, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the Spanner client's parameter and mutation value binding APIs (ValueBinder::to, StatementBuilder::add_param, and StatementBuilder::add_typed_param) to accept values implementing Into<Value> by value, rather than requiring references to types implementing ToValue. This change significantly simplifies client code by eliminating the need for explicit borrows (e.g., .to(&1) becomes .to(1)). Corresponding updates have been made across examples, integration tests, and unit tests, and new tests have been added to verify various value conversions, including owned, borrowed, optional, and null types. No review comments were provided, so there is no additional feedback to address.

@olavloite
olavloite force-pushed the perf-spanner-by-value-param-binding branch from c9bc31a to 7151539 Compare July 27, 2026 15:25
@olavloite olavloite changed the title perf(spanner): add by-value parameter and mutation binding methods feat(spanner): accept T: Into<Value> in parameter and mutation bindings Jul 27, 2026
@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.99749% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.58%. Comparing base (b3a930b) to head (b205c13).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/spanner/src/to_value.rs 98.39% 3 Missing ⚠️
src/spanner/src/mutation.rs 98.83% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #6184    +/-   ##
========================================
  Coverage   96.57%   96.58%            
========================================
  Files         264      264            
  Lines       66577    66894   +317     
========================================
+ Hits        64297    64609   +312     
- Misses       2280     2285     +5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@olavloite
olavloite force-pushed the perf-spanner-by-value-param-binding branch from 7151539 to f078404 Compare July 27, 2026 15:56
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
olavloite force-pushed the perf-spanner-by-value-param-binding branch from f078404 to b205c13 Compare July 27, 2026 16:14
@olavloite
olavloite requested a review from sakthivelmanii July 27, 2026 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: spanner Issues related to the Spanner API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant