Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: [Spanner] Add query options support #4512
feat: [Spanner] Add query options support #4512
Changes from 15 commits
33c056a
00fd37f
71c7447
38d1754
33259c0
7ee5116
b6262ea
f332fc7
3463c91
3d35057
68293b2
d5cea1b
6184037
0ebebb3
5cd9d38
299ffa6
4da2633
cf6b811
c1f2cd5
032f668
becf5e4
9a3f2df
ff642e9
06bb864
94cb5ad
c8f2de2
15221a5
1b59e1b
ec4b8c5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to do this in a
finally
block. You might even want to write a method accepting anAction
that will set the environment variable, execute the action, then reset the environment variable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, didn't know about
Action
. Updated to how I think actions should be used. Also using afinally
block now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: simplify to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An optional alternative to this would be to have a static
Empty
property, e.g.Then code which is currently
new QueryOptions().WithOptimizerVersion(...)
would becomeQueryOptions.Empty.WithOptimizerVersion(...)
which might be clearer/simpler.+amanda-tarafa do you have a preference on this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea because I find it to be more readable so I changed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably write this as:
(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.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's neat but I agree that's harder to reason about. But I made the first change you suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified to use an object initializer:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.