feat(spanner): add execute_query method by olavloite · Pull Request #4952 · googleapis/google-cloud-rust

olavloite · 2026-03-09T17:37:29Z

Adds a method for creating a single-use read-only transaction and for executing a query using this transaction.
Adds an integration test using the Spanner emulator to execute a query to show that the client works end-to-end.

Adds a method for creating a single-use read-only transaction and for executing a query using this transaction. Also adds an integration test using the Spanner emulator to show that the client works end-to-end.

codecov · 2026-03-09T18:34:06Z

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.85%. Comparing base (8b70ea8) to head (f10dcef).
⚠️ Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
src/spanner/src/error.rs	0.00%	2 Missing ⚠️
src/spanner/src/statement.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4952      +/-   ##
==========================================
- Coverage   93.89%   93.85%   -0.05%     
==========================================
  Files         221      223       +2     
  Lines        8601     8605       +4     
==========================================
  Hits         8076     8076              
- Misses        525      529       +4

☔ View full report in Codecov by Sentry.
📢 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.

coryan

I think using Error::binding() and Error::service for local errors is a deal-breaker for me.

In the future, please keep PRs under 500 lines, 1,000 in a pinch. I have been known to reject larger PRs with a simple "split please".

coryan · 2026-03-09T20:42:11Z

src/spanner/src/client.rs

+        let mut builder = google_cloud_gax::client_builder::internal::new_builder(Factory);
+        // The Spanner client should automatically use the Spanner emulator if the
+        // SPANNER_EMULATOR_HOST environment variable is set.
+        if let Ok(endpoint) = std::env::var("SPANNER_EMULATOR_HOST") {


Consider:

Suggested change

if let Ok(endpoint) = std::env::var("SPANNER_EMULATOR_HOST") {

let Ok(endpoint) = std::env::var("SPANNER_EMULATOR_HOST") else {

return builder;

};

Saves you some indentation.

coryan · 2026-03-09T20:45:41Z

src/spanner/src/client.rs

+        if let Ok(endpoint) = std::env::var("SPANNER_EMULATOR_HOST") {
+            if !endpoint.is_empty() {
+                // Determine if we need to prefix the endpoint with a scheme
+                let full_endpoint = if endpoint.starts_with("http") {


Hmmm... What about "http_carlos_is_evil.com" ? If you need to parse the endpoint, consider the url crate, which I think we already link.

I've used the url crate to parse and check the URL, and only add a scheme if there is no scheme.

coryan · 2026-03-09T20:49:48Z

src/spanner/src/client.rs

+            if !endpoint.is_empty() {
+                // Determine if we need to prefix the endpoint with a scheme
+                let full_endpoint = if endpoint.starts_with("http") {
+                    endpoint
+                } else {
+                    format!("http://{}", endpoint)
+                };
+
+                builder = builder.with_endpoint(full_endpoint).with_credentials(
+                    google_cloud_auth::credentials::anonymous::Builder::new().build(),
+                );


An alternative would be:

Suggested change

if !endpoint.is_empty() {

// Determine if we need to prefix the endpoint with a scheme

let full_endpoint = if endpoint.starts_with("http") {

endpoint

} else {

format!("http://{}", endpoint)

};

builder = builder.with_endpoint(full_endpoint).with_credentials(

google_cloud_auth::credentials::anonymous::Builder::new().build(),

);

let anonymous = || google_cloud_auth::credentials::anonymous::Builder::new().build();

builder = match endpoint {

e if e.starts_with("https://) => builder.with_endpoint(e).with_credentials(anonymous()),

e if e.starts_with("http://) => builder.with_endpoint(e).with_credentials(anonymous()),

e if !e.is_empty() => builder.with_endpoint(format!("http://{e}").with_credentials(anonymous()),

_ => builder,

}

This is solved in a slightly different way with the url crate.

.gcb/integration.yaml

coryan · 2026-03-09T20:58:22Z

src/spanner/src/read_only_transaction.rs

+/// A builder for [SingleUseReadOnlyTransaction].
+///
+/// # Example
+/// ```rust,no_run


Suggested change

/// ```rust,no_run

/// ```

That should be enough as the example only defines a function, does not invoke it.

Removed here and elsewhere

coryan · 2026-03-09T21:32:04Z

src/spanner/src/timestamp_bound.rs

+        let seconds = timestamp.unix_timestamp();
+        let nanos = timestamp.nanosecond();
+        let timestamp = wkt::Timestamp::new(seconds, nanos as i32)
+            .map_err(|e| crate::Error::binding(format!("timestamp out of range: {}", e)))?;


Error::binding() is intended for an RPC that is missing the inputs for a HTTP request. In general, google_cloud_gax::error::Error is intended for RPC errors. Please avoid using it elsewhere.

Changed. I'm now re-exporting the error from the try_from implementation.

tests/spanner/tests/driver.rs

coryan

LGTM.

PS: When you respond to each comment one by one I get one email for each. If you press "Start review" you can batch all your responses. Just saying.

coryan · 2026-03-10T13:58:16Z

src/spanner/src/server_streaming/stream.rs

 // See the License for the specific language governing permissions and
 // limitations under the License.

+#![allow(dead_code)]


nit: these are easy to forget, consider opening an issue to track them / remind yourself.

FWIW, we use TODO(#NNNN) where NNNN is the GitHub issue number.

Opened an issue and added a TODO referencing it here and elsewhere.

coryan · 2026-03-10T14:00:17Z

src/spanner/src/statement.rs

+#[derive(Clone, Debug, PartialEq)]
+pub struct Statement {
+    pub sql: String,
+    // TODO: Add support for query parameters.


nit: TODO(#KKKK) would be more in the style of this repo.

coryan · 2026-03-10T14:04:17Z

src/spanner/src/result_set.rs

+fn internal_error(message: &str) -> crate::Error {
+    crate::Error::deser(message)
+}


Food for thought. You may consider something like:

Suggested change

fn internal_error(message: &str) -> crate::Error {

crate::Error::deser(message)

}

fn internal_error(e: MyErrorType) -> crate::Error {

crate::Error::deser(e)

}

And then application developers can use Error::source() to get more details about the problem.

In this case it is unclear how that would be better than an error message, as the application can do very little with such an error other than logging.

Oh, you may want to include something in these messages about where to file bugs. That was either a bug in the library or the service, the customer may need some hints on how to proceed.

I added an SpannerInternalError enum following the example from the Storage and PubSub clients, and added a hint to the documentation regarding filing an issue (example also taken from the Storage client). The internal error is then wrapped in crate::Error::deser.

coryan · 2026-03-10T14:05:33Z

src/spanner/src/result_set.rs

+/// the first value in the `values` array of the subsequent `PartialResultSet`.
+///
+/// This function handles the concatenation of split `StringValue` and `ListValue` types.
+fn merge_values(target: &mut prost_types::Value, source: prost_types::Value) -> crate::Result<()> {


As a reminder: you are the Spanner expert, I am not an effective reviewer as to the correctness of this code. If you want an (informed) second opinion maybe ask one of your colleagues to do a drive-by.

I've asked a colleague from the Spanner team to take a look.

coryan · 2026-03-10T14:08:10Z

src/spanner/src/timestamp_bound.rs

+    /// Returns a strong timestamp bound. Strong reads are guaranteed to see the
+    /// effects of all transactions that have committed before the start of the read.
+    ///
+    /// See <https://docs.cloud.google.com/spanner/docs/timestamp-bounds#strong> for more information.


You can also write:

Suggested change

/// See <https://docs.cloud.google.com/spanner/docs/timestamp-bounds#strong> for more information.

/// See the [Timestamp bound types] for more information.

///

/// [Timestamp bound types]: https://docs.cloud.google.com/spanner/docs/timestamp-bounds#strong

Which may read better in the rendered documentation (try cargo doc --no-deps -p google-cloud-spanner locally).

tests/spanner/tests/driver.rs

feat(spanner): add execute_query method

b991074

Adds a method for creating a single-use read-only transaction and for executing a query using this transaction. Also adds an integration test using the Spanner emulator to show that the client works end-to-end.

olavloite requested a review from a team as a code owner March 9, 2026 17:37

product-auto-label bot added the api: spanner Issues related to the Spanner API. label Mar 9, 2026

coryan requested changes Mar 9, 2026

View reviewed changes

olavloite force-pushed the spanner-execute-query branch from 893a509 to b33d091 Compare March 10, 2026 11:07

build(spanner): fix build issues

3db3756

olavloite force-pushed the spanner-execute-query branch from b33d091 to 3db3756 Compare March 10, 2026 12:12

olavloite added 2 commits March 10, 2026 14:51

chore(spanner): address review comments

6a7621d

Merge branch 'main' into spanner-execute-query

751d3dc

coryan approved these changes Mar 10, 2026

View reviewed changes

chore(spanner): address review comments

f10dcef

sakthivelmanii approved these changes Mar 11, 2026

View reviewed changes

olavloite merged commit d66dc1c into googleapis:main Mar 11, 2026
35 checks passed

-        if let Ok(endpoint) = std::env::var("SPANNER_EMULATOR_HOST") {
+        let Ok(endpoint) = std::env::var("SPANNER_EMULATOR_HOST") else {
+            return builder;
+        };

-    /// See <https://docs.cloud.google.com/spanner/docs/timestamp-bounds#strong> for more information.
+    /// See the [Timestamp bound types] for more information.
+    ///
+    /// [Timestamp bound types]: https://docs.cloud.google.com/spanner/docs/timestamp-bounds#strong

Conversation

olavloite commented Mar 9, 2026

Uh oh!

codecov bot commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coryan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coryan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Mar 9, 2026 •

edited

Loading