feat: [Spanner] Add query options support #4512

skuruppu · 2020-03-05T05:37:58Z

Adds the ability to set QueryOptions when running Cloud Spanner queries.
For now, only setting the query_optimizer_version is added.

QueryOptions can be configured through the following mechanisms.

At the SpannerConnection level.
Through the SPANNER_OPTIMIZER_VERSION environment variable.
At a query level.

If the options are configured through multiple mechanisms then:

Options set at an environment variable level will override options
configured at the SpannerConnection level.
Options set at a query-level will override options set at either the
SpannerConnection or environment variable level.

If no options are set, the optimizer version will default to:

The optimizer version the database is pinned to.
If the database is not pinned to a specific version, then the Cloud Spanner
backend will use the "latest" version.

It's a wrapper around Google.Cloud.Spanner.Data.V1.ExecuteSqlRequest.Types.QueryOptions to avoid exposing protos in the API.

Verifying whether the implementation respects the precedence rules.

jskeet

Just a single comment for now so we can talk about the overall "effective options" approach. If it's not obvious what I'm talking about, I could potentially clone your fork and create a PR to show you what I mean :)

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerCommand.cs

Also no longer setting QueryOptions when creating SpannerCommands.

skuruppu

PTAL

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerCommandTests.cs

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerCommand.cs

jskeet · 2020-03-06T11:59:51Z

Thanks @skuruppu - I'm at a conference today, but will look at this first thing Monday (assuming I don't get time to look today). @amanda-tarafa can also look in the meantime, of course :)

jskeet

Thanks - the GetEffectiveQueryOptions() approach is exactly what I had in mind, thanks :)

jskeet · 2020-03-10T09:25:46Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerCommand.ExecutableCommand.cs

+                    return null;
+                }
+
+                var queryOptionsProto = new V1.ExecuteSqlRequest.Types.QueryOptions();


This can be simplified to use an object initializer:

return new V1.ExecuteSqlRequest.Types.QueryOptions { OptimizerVersion = optimizerVersion };

Done.

I actually simplified the whole function and made it a bit more future-proof by using the proto merge functionality. That way when we add new options, the merging will happen automatically. The only thing to update manually would be reading the new env vars.

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerCommand.ExecutableCommand.cs

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerCommand.cs

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerConnection.cs

jskeet · 2020-03-10T09:30:23Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/QueryOptions.cs

@@ -84,11 +84,6 @@ public QueryOptions() : this(new V1.ExecuteSqlRequest.Types.QueryOptions())
            return new QueryOptions(proto.Clone());
        }

-        /// <summary>


If you don't really need the proto form, it might be worth just having the OptimizerVersion string property. You can still have a method to construct an instance from a proto if you want.
I don't much mind.

I actually reverted this change because @larkee made me realize that merging protos is much easier than explicitly checking for each field. So leaving the protos for now but let me know if you think there's a better way to do this.

This reverts commit 0ebebb3.

Use proto merging to simplify the logic and future-proof the code so that new options will be merged automatically.

skuruppu · 2020-03-11T04:07:21Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerCommandTests.cs

+                    // Returning null since we can't return a SpannerClient.ExecuteStreamingSqlStream()
+                    // since it's an abstract class.


I couldn't find a type to return an empty result here so had to resort to null. It's not too bad because I don't care about the return value but I'm not sure if this is bad practice.

You could find the concrete class involved. Personally I generally avoid using mocks, preferring fakes - but I can see how a mock is possibly simpler here.

The lambda expression can be just (request, _) => null though.

I did find the concrete class but it's private to the implementation. But it's possible that I wasn't accessing it properly.

I'm happy to not return anything here so I simplified the lambda expr as you suggested.

skuruppu · 2020-03-11T04:21:40Z

I'm a bit uncertain about adding an integration test to this given that the backend won't respond in a noticeable way if a valid version was given. But if an invalid version was given it will let us know.

@jskeet let me know your thoughts on whether this type of IT is typically added in this repo.

jskeet

I think it would be "good but not necessary" to have an integration test of "invalid optimizer version causes a failure". If you're happy to write the test (which shouldn't take long) I think it would be worth including.

Is there anything in the returned result that indicates the query optimizer version used, so we can test valid values?

jskeet · 2020-03-11T08:04:35Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerCommandTests.cs

+                    // Returning null since we can't return a SpannerClient.ExecuteStreamingSqlStream()
+                    // since it's an abstract class.


You could find the concrete class involved. Personally I generally avoid using mocks, preferring fakes - but I can see how a mock is possibly simpler here.

The lambda expression can be just (request, _) => null though.

jskeet · 2020-03-11T08:05:01Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/QueryOptions.cs

+        /// <summary>
+        /// The query optimizer version configured in the options.
+        /// </summary>
+        public string OptimizerVersion


Nit: simplify to

public string OptimizerVersion => Proto.OptimizerVersion;

jskeet · 2020-03-11T08:06:57Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/QueryOptions.cs

+        public V1.ExecuteSqlRequest.Types.QueryOptions ToProto() => Proto.Clone();
+
+        /// <inheritdoc />
+        public bool Equals(QueryOptions other)


I'd probably write this as:

public bool Equals(QueryOptions other) => other is object && OptimizerVersion == other.OptimizerVersion;

(As other.OptimizerVersion can never return null, and OptimizerVersion can never be null, you could just return OptimizerVersion == other?.OptimizerVersion, but the reasoning required makes that harder to understand.)

That's neat but I agree that's harder to reason about. But I made the first change you suggested.

This reverts commit becf5e4.

skuruppu

PTAL

Now includes a couple of simple integration tests and they also pass :)

skuruppu · 2020-03-12T03:39:24Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerCommandTests.cs

+                    // Returning null since we can't return a SpannerClient.ExecuteStreamingSqlStream()
+                    // since it's an abstract class.


I did find the concrete class but it's private to the implementation. But it's possible that I wasn't accessing it properly.

I'm happy to not return anything here so I simplified the lambda expr as you suggested.

skuruppu · 2020-03-12T03:43:15Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/QueryOptions.cs

+        public V1.ExecuteSqlRequest.Types.QueryOptions ToProto() => Proto.Clone();
+
+        /// <inheritdoc />
+        public bool Equals(QueryOptions other)


That's neat but I agree that's harder to reason about. But I made the first change you suggested.

jskeet

Generally happy, with a few possible changes.

I might need to have a closer look at SpannerCommand and SpannerConnection to see what other properties do in terms of cloning - I have a feeling we sometimes create copies of those (sometimes through SpannerConnectionStringBuilder) which might need to be updated to propagate this, but I may be wrong. I'll have a look at the code separately - I think what's here would be good enough for a beta release though.

jskeet · 2020-03-12T12:11:29Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.IntegrationTests/QueryOptionsTests.cs

+        public QueryOptionsTests(ReadTableFixture fixture) =>
+            _fixture = fixture;
+
+        private async Task<T> ExecuteAsync<T>(string sql)


I don't think this is actually used, is it?

Yikes sorry, copy/pasta error.

jskeet · 2020-03-12T12:12:09Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.IntegrationTests/QueryOptionsTests.cs

+            }
+        }
+
+        // [START spanner_test_single_key_read_with_query_options]


It probably wouldn't hurt to add equivalent command-level option tests; I'll leave that as a call for you to make though.

I added a success case but I think it would be redundant to have a failure case here since that's already tested.

jskeet · 2020-03-12T12:13:28Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerCommandTests.cs

+            }
+
+            // Set the environment back.
+            Environment.SetEnvironmentVariable(optimizerVersionVariable, savedOptimizerVersion);


It would be good to do this in a finally block. You might even want to write a method accepting an Action that will set the environment variable, execute the action, then reset the environment variable.

Cool, didn't know about Action. Updated to how I think actions should be used. Also using a finally block now.

jskeet · 2020-03-12T12:14:36Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerCommandTests.cs

+            // variable.
+            Mock<SpannerClient> spannerClientMock = SetupExecuteStreamingSql(cmdOptimizerVersion);
+
+            const string optimizerVersion = "1";


Nit: rename to connOptimizerVersion to clarify? The other two variables are pleasantly clear :)

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/QueryOptions.cs

jskeet · 2020-03-12T12:17:26Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/QueryOptions.cs

+
+        private QueryOptions(V1.ExecuteSqlRequest.Types.QueryOptions proto) => Proto = proto;
+
+        /// <summary>


An optional alternative to this would be to have a static Empty property, e.g.

public static QueryOptions Empty { get; } = new QueryOptions(new V1.ExecuteSqlRequest.Types.QueryOptions());

Then code which is currently new QueryOptions().WithOptimizerVersion(...) would become QueryOptions.Empty.WithOptimizerVersion(...) which might be clearer/simpler.

+amanda-tarafa do you have a preference on this one?

I like this idea because I find it to be more readable so I changed it.

jskeet · 2020-03-12T12:18:48Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerConnection.cs

@@ -110,6 +110,12 @@ public override ConnectionState State

        internal bool IsOpen => (State & ConnectionState.Open) == ConnectionState.Open;

+        /// <summary>


Currently this is only available as a property in the connection. Do you think we want people to be able to provide it in the connection string itself? I don't know what the expected use is likely to be.

We could potentially do that later.

At the moment, I'm not sure. In contexts like the JDBC driver, it is specified in the connection string.

I would be happy to do this in a separate PR.

jskeet

Now that I've had another look:

I believe we should be copying the property in SpannerCommand.Clone (which should be a matter of adding QueryOptions = QueryOptions within the call).
We can probably get away without it being in SpannerConnectionStringBuilder for now, but we should consider it for the future.

skuruppu

Thanks for the review @js. All done. I didn't make the change for SpannerConnectionStringBuilder in this PR but will be happy to try out what it looks like in a separate PR.

skuruppu · 2020-03-13T04:46:45Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.IntegrationTests/QueryOptionsTests.cs

+        public QueryOptionsTests(ReadTableFixture fixture) =>
+            _fixture = fixture;
+
+        private async Task<T> ExecuteAsync<T>(string sql)


Yikes sorry, copy/pasta error.

skuruppu · 2020-03-13T04:51:15Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.IntegrationTests/QueryOptionsTests.cs

+            }
+        }
+
+        // [START spanner_test_single_key_read_with_query_options]


I added a success case but I think it would be redundant to have a failure case here since that's already tested.

skuruppu · 2020-03-13T05:18:17Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerCommandTests.cs

+            }
+
+            // Set the environment back.
+            Environment.SetEnvironmentVariable(optimizerVersionVariable, savedOptimizerVersion);


Cool, didn't know about Action. Updated to how I think actions should be used. Also using a finally block now.

skuruppu · 2020-03-13T05:19:46Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerCommandTests.cs

+            // variable.
+            Mock<SpannerClient> spannerClientMock = SetupExecuteStreamingSql(cmdOptimizerVersion);
+
+            const string optimizerVersion = "1";


skuruppu · 2020-03-13T09:51:10Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/QueryOptions.cs

+
+        private QueryOptions(V1.ExecuteSqlRequest.Types.QueryOptions proto) => Proto = proto;
+
+        /// <summary>


I like this idea because I find it to be more readable so I changed it.

skuruppu · 2020-03-13T09:53:39Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerConnection.cs

@@ -110,6 +110,12 @@ public override ConnectionState State

        internal bool IsOpen => (State & ConnectionState.Open) == ConnectionState.Open;

+        /// <summary>


At the moment, I'm not sure. In contexts like the JDBC driver, it is specified in the connection string.

I would be happy to do this in a separate PR.

jskeet · 2020-03-13T10:44:39Z

Great, thanks for all of that. Are you now happy for me to squash into a single commit and merge?

skuruppu · 2020-03-14T03:00:31Z

Great, thanks for all of that. Are you now happy for me to squash into a single commit and merge?

All good, sorry for the late reply. I will squash and merge now.

skuruppu · 2020-03-14T03:03:00Z

@jskeet I have to ask you about cutting a release with these changes. What process should I follow?

jskeet · 2020-03-14T10:08:23Z

@skuruppu: Will email you internally.

skuruppu added 10 commits March 5, 2020 16:31

Added QueryOptions class and tests.

33c056a

It's a wrapper around Google.Cloud.Spanner.Data.V1.ExecuteSqlRequest.Types.QueryOptions to avoid exposing protos in the API.

Added QueryOptions property to a SpannerConnection.

00fd37f

Added method to get proto of QueryOptions.

71c7447

Added QueryOptions property to SpannerCommand.

38d1754

Set QueryOptions when ExecutableCommand is created.

33259c0

Pass QueryOptions from the connection to SpannerCommand.

7ee5116

Added equality checking for QueryOptions.

b6262ea

Added unit tests for QueryOptions.

f332fc7

Verifying whether the implementation respects the precedence rules.

Fixes to QueryOptions based on review feedback.

3463c91

Fixes QueryOptions calls based on immutability constraint.

3d35057

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 5, 2020

skuruppu added the api: spanner Issues related to the Spanner API. label Mar 5, 2020

skuruppu requested review from amanda-tarafa and jskeet March 5, 2020 05:38

jskeet reviewed Mar 5, 2020

View reviewed changes

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerCommand.cs Outdated Show resolved Hide resolved

skuruppu added 5 commits March 6, 2020 20:43

Implemented GetEffectiveQueryOptions().

68293b2

Simplified QueryOptions setting in SpannerCommand.

d5cea1b

Simplified QueryOptions setting in SpannerConnection.

6184037

Also no longer setting QueryOptions when creating SpannerCommands.

Removed ToProto() since it's no longer used.

0ebebb3

Started fixing tests to check QueryOptions in the client.

5cd9d38

skuruppu commented Mar 6, 2020

View reviewed changes

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerCommandTests.cs Outdated Show resolved Hide resolved

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerCommand.cs Outdated Show resolved Hide resolved

skuruppu changed the title ~~Spanner qo versioning support~~ feat: Spanner qo versioning support Mar 8, 2020

skuruppu changed the title ~~feat: Spanner qo versioning support~~ feat: [Spanner] Query optimizer versioning support Mar 9, 2020

skuruppu changed the title ~~feat: [Spanner] Query optimizer versioning support~~ feat: [Spanner] Add query options support Mar 9, 2020

jskeet reviewed Mar 10, 2020

View reviewed changes

This was referenced Mar 10, 2020

feat: add backend query options googleapis/java-spanner#90

Merged

feat: add support for QueryOptions googleapis/java-spanner-jdbc#76

Merged

skuruppu added 2 commits March 11, 2020 14:32

Fixed the unit tests to use mocks.

299ffa6

Revert "Removed ToProto() since it's no longer used."

4da2633

This reverts commit 0ebebb3.

skuruppu added 2 commits March 11, 2020 14:52

Simplified GetEffectiveQueryOptions().

cf6b811

Use proto merging to simplify the logic and future-proof the code so that new options will be merged automatically.

Fixes according to review feedback.

c1f2cd5

skuruppu commented Mar 11, 2020

View reviewed changes

skuruppu added 2 commits March 11, 2020 15:10

Removed unnecessary line-wrapping.

032f668

Updated the env var name to be more descriptive.

becf5e4

jskeet reviewed Mar 11, 2020

View reviewed changes

skuruppu added 3 commits March 12, 2020 10:50

Revert "Updated the env var name to be more descriptive."

9a3f2df

This reverts commit becf5e4.

Fixes according to review feedback.

ff642e9

Added integration tests for QueryOptions.

06bb864

skuruppu commented Mar 12, 2020

View reviewed changes

jskeet approved these changes Mar 12, 2020

View reviewed changes

jskeet reviewed Mar 12, 2020

View reviewed changes

skuruppu added 5 commits March 13, 2020 15:53

Added an IT for query-level options setting.

94cb5ad

Refactored SpannerCommand tests based on feedback.

c8f2de2

Added an Empty initializer to QueryOptions.

15221a5

Clone QueryOptions when cloning SpannerCommand.

1b59e1b

Fixed formatting issue.

ec4b8c5

skuruppu commented Mar 13, 2020

View reviewed changes

skuruppu merged commit c5090d1 into master Mar 14, 2020

jskeet deleted the spanner-qo-versioning-support branch March 30, 2020 11:42

		// Returning null since we can't return a SpannerClient.ExecuteStreamingSqlStream()
		// since it's an abstract class.


		private QueryOptions(V1.ExecuteSqlRequest.Types.QueryOptions proto) => Proto = proto;

		/// <summary>

		@@ -110,6 +110,12 @@ public override ConnectionState State

		internal bool IsOpen => (State & ConnectionState.Open) == ConnectionState.Open;

		/// <summary>

feat: [Spanner] Add query options support #4512

feat: [Spanner] Add query options support #4512

Conversation

skuruppu commented Mar 5, 2020 • edited Loading

jskeet left a comment

Choose a reason for hiding this comment

skuruppu left a comment

Choose a reason for hiding this comment

jskeet commented Mar 6, 2020

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skuruppu Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skuruppu commented Mar 11, 2020

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skuruppu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

skuruppu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet commented Mar 13, 2020

skuruppu commented Mar 14, 2020

skuruppu commented Mar 14, 2020

jskeet commented Mar 14, 2020

skuruppu commented Mar 5, 2020 •

edited

Loading

skuruppu Mar 11, 2020 •

edited

Loading