perf(spanner): add by-value parameter and mutation binding methods

[fornwall]

StatementBuilder::add_(typed_)param and ValueBinder::to bind values through the ToValue trait, whose impl ToValue for Value is self.clone().

A caller that already holds an owned Value therefore pays a redundant deep clone of the whole payload (every string, bytes and array cell) on each bound parameter and each mutation column. This can be costly on bound-DML rows and bulk-ingest cells

Add owned-value counterparts that accept impl Into<Value> and move the value straight into the builder, avoiding that clone.

[gemini-code-assist]

Code Review

This pull request introduces owned-value variants (to_value, add_param_value, and add_typed_param_value) to ValueBinder and StatementBuilder in the Spanner crate, allowing callers to pass owned values and avoid unnecessary deep clones. The existing reference-based methods have been refactored to delegate to these new methods, and corresponding unit tests have been added. The review feedback correctly points out that the newly added tests use .unwrap(), which violates the repository style guide; these should be replaced with .expect() to provide descriptive error messages upon failure.

[olavloite]

Can we solve this in a slightly different way that does not require adding new methods to the public API. Can we instead change the input argument of the existing methods to accept Into<Value>, and just add this blanket implementation:

impl<T: ToValue + ?Sized> From<&T> for Value {
    fn from(t: &T) -> Self {
        t.to_value()
    }
}

So the entire diff would then be something like this:

--- a/src/spanner/src/mutation.rs
+++ b/src/spanner/src/mutation.rs
@@ -298,9 +298,9 @@ pub struct ValueBinder {
 
 impl ValueBinder {
     /// Sets the value for the column.
-    pub fn to<T: ToValue + ?Sized>(mut self, value: &T) -> WriteBuilder {
+    pub fn to<T: Into<Value>>(mut self, value: T) -> WriteBuilder {
         self.builder.columns.push(self.column);
-        self.builder.values.push(value.to_value());
+        self.builder.values.push(value.into());
         self.builder
     }
 }
 
--- a/src/spanner/src/statement.rs
+++ b/src/spanner/src/statement.rs
@@ -69,8 +69,8 @@ impl StatementBuilder {
     /// It is recommended to use untyped parameter values, unless you explicitly want Spanner to
     /// verify that the type of the parameter value is exactly the same as the type that would
     /// otherwise be inferred from the SQL string.
-    pub fn add_param<T: ToValue + ?Sized>(mut self, name: impl Into<String>, value: &T) -> Self {
-        self.params.insert(name.into(), value.to_value());
+    pub fn add_param<T: Into<Value>>(mut self, name: impl Into<String>, value: T) -> Self {
+        self.params.insert(name.into(), value.into());
         self
     }
 
@@ -78,14 +78,14 @@ impl StatementBuilder {
     ///
     /// The parameter value is sent with an explicit type code to Spanner. The type code must
     /// correspond with the expression in the SQL string that the query parameter is bound to.
-    pub fn add_typed_param<T: ToValue + ?Sized>(
+    pub fn add_typed_param<T: Into<Value>>(
         mut self,
         name: impl Into<String>,
-        value: &T,
+        value: T,
         param_type: Type,
     ) -> Self {
         let name = name.into();
-        self.params.insert(name.clone(), value.to_value());
+        self.params.insert(name.clone(), value.into());
         self.param_types.insert(name, param_type);
         self
     }
 
--- a/src/spanner/src/to_value.rs
+++ b/src/spanner/src/to_value.rs
@@ -194,6 +194,13 @@ where
     }
 }
 
+// Blanket implementation to preserve backwards compatibility for callers passing &T
+// where T implements ToValue.
+impl<T: ToValue + ?Sized> From<&T> for Value {
+    fn from(t: &T) -> Self {
+        t.to_value()
+    }
+}
+
 #[cfg(test)]
 mod tests {

[fornwall]

Thanks @olavloite, applied your suggestion. Slight changes compared to your diff:

1. Regression tests for the new construction added.
2. Make the blanket impl comment a doc comment since it shows up in the public API. And reworded it, since it's not just for backward compat IMHO, but also for caller ergonomy - not requiring .to_value() in stmt.add_param("age", age.to_value().