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 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.92%. Comparing base (5754bd1) to head (b991074).
⚠️ Report is 10 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4952      +/-   ##
==========================================
- Coverage   93.95%   93.92%   -0.04%     
==========================================
  Files         221      222       +1     
  Lines        8538     8540       +2     
==========================================
- Hits         8022     8021       -1     
- Misses        516      519       +3

☔ 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.

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,

}

coryan · 2026-03-09T20:57:27Z

.gcb/integration.yaml

+  - id: 'Start Spanner Emulator'
+    name: 'gcr.io/cloud-spanner-emulator/emulator'
+    waitFor: ['-']


This is fine for CI, but now I need to install the spanner emulator to do cargo test --all-features, event if I am working on something unrelated.

Oh I see, you skip the tests if the environment variable is not set. SGTM, thanks.

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.

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.

coryan · 2026-03-09T21:35:31Z

tests/spanner/tests/driver.rs

+        // TODO: Re-write this to use the admin clients once those also support the Emulator.
+        let rest_endpoint = endpoint.replace("9010", "9020");
+        let rest_endpoint =
+            if rest_endpoint.starts_with("http://") || rest_endpoint.starts_with("https://") {
+                rest_endpoint
+            } else {
+                format!("http://{}", rest_endpoint)
+            };


Consider using url::Url to make this parsing and modifications to the Url.

coryan · 2026-03-09T21:39:48Z

tests/spanner/tests/driver.rs

+    // Provisions the Spanner Emulator with a test instance and database.
+    // ALREADY_EXISTS errors that are returned when creating an instance or database are ignored.
+    async fn provision_emulator(endpoint: &str) {
+        // TODO: Re-write this to use the admin clients once those also support the Emulator.


You can certainly override the endpoint and the authentication for them.

coryan · 2026-03-09T21:51:05Z

tests/spanner/tests/driver.rs

+        let mut count = 0;
+        while let Some(row_result) = rs.next().await {


Consider:

Suggested change

let mut count = 0;

while let Some(row_result) = rs.next().await {

let mut rows = Vec::new();

while let Some(row) = rs.next().await.transpose()? {

rows.push_back(row)

}

let (row1, row2, row3) = match &rows[..] {

[r1, r2, r3] => (r1, r2, r3),

_ => panic!("unexpected number of rows, got={}, want=3\n{rows:?}", rows.len());

};

And then you can make assertions (maybe in helper functions?) about row1, row2 and row3. You could also capture the results if that is useful.

coryan · 2026-03-09T21:52:36Z

tests/spanner/tests/driver.rs

+                    );
+                }
+                2 => {
+                    // Assert row 1


Ugh. I wished there was a better way to write these...

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

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

		let mut count = 0;
		while let Some(row_result) = rs.next().await {

-        let mut count = 0;
-        while let Some(row_result) = rs.next().await {
+        let mut rows = Vec::new();
+        while let Some(row) = rs.next().await.transpose()? {
+            rows.push_back(row)
+        }
+        let (row1, row2, row3) = match &rows[..] {
+            [r1, r2, r3] => (r1, r2, r3),
+            _ => panic!("unexpected number of rows, got={}, want=3\n{rows:?}", rows.len());
+        };

Conversation

olavloite commented Mar 9, 2026

Uh oh!

codecov bot commented Mar 9, 2026

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants