feat(spanner): support comments and statement hints in untyped commands #6848

olavloite · 2021-07-31T14:04:42Z

SpannerCommandTextBuilder will try to determine the type of command based on the first word of the command text if no type is specified. This works well as long as the command text does not contain any comments or statement hints at the beginning of the command text.
This change adds a simple parser that will remove any comments and statement hints before checking the type of command. The command that is stripped of comments and hints is only used for determining the type of command. The original command will be passed on to Cloud Spanner.

Fixes #6847

SpannerCommandTextBuilder will try to determine the type of command based on the first word of the command text if no type is specified. This works well as long as the command text does not contain any comments or statement hints at the beginning of the command text. This change adds a simple parser that will remove any comments and statement hints before checking the type of command. The command that is stripped of comments and hints is only used for determining the type of command. The original command will be passed on to Cloud Spanner. Fixes #6847

Adds a sample for how to use query hints with Cloud Spanner in Entity Framework Core. The sample tags a query with a specific text to indicate that the query should be intercepted by a command interceptor that will add a query hint to the query. This interceptor currently also removes the tag (comment) from the query, as the current version of the client library does not support untyped commands that start with either a comment or a statement hint. Once googleapis/google-cloud-dotnet#6848 has been merged and released, the sample can be edited so it does not remove the tag from the query. It will then also be possible to add a sample for a statement hint (as opposed to the current sample which adds a table hint).

Adds a sample for how to use query hints with Cloud Spanner in Entity Framework Core. The sample tags a query with a specific text to indicate that the query should be intercepted by a command interceptor that will add a query hint to the query. This sample will only work once googleapis/google-cloud-dotnet#6848 has been merged and released, as the client library currently does not support untyped commands that start with either a comment or a statement hint.

amanda-tarafa

I've left some code specific comments on the PR.
But I'll leave my general view on this change on the issue #6847 .

amanda-tarafa · 2021-08-02T08:25:20Z

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

@@ -218,9 +220,9 @@ internal string DatabaseToDrop
        public static SpannerCommandTextBuilder FromCommandText(string commandText)
        {
            GaxPreconditions.CheckNotNullOrEmpty(commandText, nameof(commandText));
-            commandText = commandText.Trim();
+            var trimmedCommandText = RemoveCommentsAndStatementHint(commandText).Trim();


commandText is still being used in many places, let's keep it trimmed.

Suggested change

var trimmedCommandText = RemoveCommentsAndStatementHint(commandText).Trim();

var trimmedCommandText = RemoveCommentsAndStatementHint(commandText).Trim();

commandText = commandText.Trim();

super tiny nit: And just maybe move the second trim to inside RemoveCommentsAnd... so that we can skip it here.

Suggested change

var trimmedCommandText = RemoveCommentsAndStatementHint(commandText).Trim();

var trimmedCommandText = RemoveCommentsAndStatementHint(commandText);

amanda-tarafa · 2021-08-02T08:26:17Z

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

            // Split(new char[0]) splits the string using all whitespace characters.
-            var commandSections = commandText.Split((char[]) null, StringSplitOptions.RemoveEmptyEntries);
+            var commandSections = trimmedCommandText.Split((char[]) null, StringSplitOptions.RemoveEmptyEntries);
            if (commandSections.Length < 2)
            {
                throw new ArgumentException($"'{commandText}' is not a recognized Spanner command.", nameof(commandText));


Shoul this be:

Suggested change

throw new ArgumentException($"'{commandText}' is not a recognized Spanner command.", nameof(commandText));

throw new ArgumentException($"'{trimmedCommandText}' is not a recognized Spanner command.", nameof(commandText));

amanda-tarafa · 2021-08-02T08:32:19Z

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

            // Split(new char[0]) splits the string using all whitespace characters.
-            var commandSections = commandText.Split((char[]) null, StringSplitOptions.RemoveEmptyEntries);
+            var commandSections = trimmedCommandText.Split((char[]) null, StringSplitOptions.RemoveEmptyEntries);


In line 262 here (I can't leave a comment there), the code is doing commandText = $"{commandName} {targetTable}"; and then a SpannerCommandTextBuilder is returned based on commandText, which means you loose any comments or hints, etc.

That is true, but that is not a command that supports comments or hints anyways. That is a pseudo command text that indicates that the command should use a mutation and not a DML statement, and mutations do not support comments or hints.

Should we fail if we get a command that does not support comments or hints that has comments or hints? I would be OK if se send it all to the backend for it to fail, but here, we are just silently stripping whatever the user sent. What if they are "expecting" a hint to be used?
Is EF Core generating mutations with comments?

Good question. In this case the backend will not fail it, as the command text is only used to determine the type of command. The generated mutation will not contain the hint or comment, and will therefore be accepted by the backend. For comments, I would say that it's not really a problem. It basically means that we are introducing support for comments in these mutation command texts. For hints it might be better to fail in the client, as they will otherwise just silently be stripped away without any use.
Note: These mutation command texts is a .NET specific feature. Cloud Spanner (and also the client libraries for the other programming languages) do not support a command text like INSERT <table_name> or UPDATE <table_name>.

Is EF Core generating mutations with comments?

No.

amanda-tarafa · 2021-08-02T08:34:33Z

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

+        private static readonly char s_doubleQuote = '"';
+        private static readonly char s_backtickQuote = '`';
+        private static readonly char s_hyphen = '-';
+        private static readonly char s_dash = '#';


I think this is sharp?

Suggested change

private static readonly char s_dash = '#';

private static readonly char s_sharp = '#';

TBH, I'm open to calling it anything =) But formally this is the number sign, hash (so certainly not dash...) or pound sign, while the sharp sign is slightly different: https://en.wikipedia.org/wiki/Number_sign and https://en.wikipedia.org/wiki/Sharp_(music)

amanda-tarafa · 2021-08-02T08:38:04Z

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

+                {
+                    if ((c == '\n' || c == '\r') && !isTripleQuoted)
+                    {
+                        throw new SpannerException(


I think this being a parsing issue, it should throw the same exception as the FromCommandText method, which is InvalidOperationException. And applies to the rest of the exceptions here.

I assume you mean ArgumentException (which is thrown by FromCommandText), so I've changed it to that.

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

amanda-tarafa · 2021-08-02T08:54:05Z

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

+            // keywords SELECT, UPDATE, DELETE, WITH, and that we can keep the check simple by just
+            // searching for the first occurrence of a keyword that should be preceded by a closing curly
+            // brace at the end of the statement hint.
+            int startStatementHintIndex = sql.IndexOf('{');


What if the curly bracket is in a string literal?

Good point. It returns the correct value in that case, as a string literal (in a valid SQL statement) can only come after the statement hint and keyword that determines the type of statement (SELECT, UPDATE, ...). That means that the index of the first curly brace will always be after the first keyword, which means that the substring that is taken on line 455 will never contain a closing curly brace. That again means that the parser sees it as an invalid statement and just returns the original sql string.

It is however better to specifically detect this situation and document that it is a valid statement, but that we can ignore the curly braces as they come after the first keyword, and are therefore not part of a statement hint.

Note also that:

We have at this point already removed all comments, so we don't have to care about that (which is also not your point in this case, but just to be complete)

Only a statement hint, as opposed to a table hint, may be added at the beginning of a statement. Statement hints have a very limited number of possible values, non of which include string literals: https://cloud.google.com/spanner/docs/query-syntax#statement-hints

OK, but I think this can be made simpler, just for the purposes of maintenance.

If there's no { at the start of the command, we just assume it has not hints and return it as is, caller code or backend will handle the failure.

If there was a { at the start then:

We find and strip everything from the start until the first }.

If there are no } we return as is and the caller will fail.

WDYT?

You're right, this can be done a lot simpler, especially when we assume that the statement is valid and well-formed. I've updated the implementation according to the above suggestion.

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

amanda-tarafa

OK, I'm proposing a few simplifications that I think are possible, given that we really just want to know the type of the command and that we can let the backend fail for ill formatted commands.

Hope it makes sense. Else I can try a draft PR next week.

amanda-tarafa · 2021-08-06T15:34:08Z

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

@@ -218,9 +220,9 @@ internal string DatabaseToDrop
        public static SpannerCommandTextBuilder FromCommandText(string commandText)
        {
            GaxPreconditions.CheckNotNullOrEmpty(commandText, nameof(commandText));
-            commandText = commandText.Trim();
+            var trimmedCommandText = RemoveCommentsAndStatementHint(commandText).Trim();


super tiny nit: And just maybe move the second trim to inside RemoveCommentsAnd... so that we can skip it here.

Suggested change

var trimmedCommandText = RemoveCommentsAndStatementHint(commandText).Trim();

var trimmedCommandText = RemoveCommentsAndStatementHint(commandText);

amanda-tarafa · 2021-08-06T15:36:36Z

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

            // Split(new char[0]) splits the string using all whitespace characters.
-            var commandSections = commandText.Split((char[]) null, StringSplitOptions.RemoveEmptyEntries);
+            var commandSections = trimmedCommandText.Split((char[]) null, StringSplitOptions.RemoveEmptyEntries);


Should we fail if we get a command that does not support comments or hints that has comments or hints? I would be OK if se send it all to the backend for it to fail, but here, we are just silently stripping whatever the user sent. What if they are "expecting" a hint to be used?
Is EF Core generating mutations with comments?

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

amanda-tarafa · 2021-08-06T15:56:06Z

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

+            // keywords SELECT, UPDATE, DELETE, WITH, and that we can keep the check simple by just
+            // searching for the first occurrence of a keyword that should be preceded by a closing curly
+            // brace at the end of the statement hint.
+            int startStatementHintIndex = sql.IndexOf('{');


OK, but I think this can be made simpler, just for the purposes of maintenance.

If there's no { at the start of the command, we just assume it has not hints and return it as is, caller code or backend will handle the failure.

If there was a { at the start then:

We find and strip everything from the start until the first }.

If there are no } we return as is and the caller will fail.

WDYT?

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

amanda-tarafa · 2021-08-06T16:36:59Z

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

+        /// Throws ArgumentException if the sql string contains an unclosed
+        /// literal or unclosed quoted identifier
+        /// </exception>
+        internal static string RemoveComments(string sql)


OK, I think we can make this simpler as well:

We have 2 kinds of comments, line comments, starting with -- for instance, and block comments /* */. And we only care about those at the beginning of the command.

In a loop:

If the command string starts with any of the line comment starters --, #, or others, remove the entire line and loop back.

If the commnd string starts with any of the block comment starterts, /* (others?) just find the first comment block closer that's not escaped */.

If found, remove everything up to and including the closer */ and loop back again.

Else, return the original string and let the caller or the backend fail.

Return what you have when it doesn't start with a line or block comment.

WDTY? Or am I missing something?

I think this will break because line comments can also start in the middle of a line, which would not be stripped by such a simplified approach. And I worry that those 'inline line comments' that would be left in the command string could break the rest of the parsing. So the following kind of statements would probably not be parsed correctly:

@{ OPTIMIZER_VERSION=1 -- Use the first optimizer for now. Change this to @{OPTIMIZER_VERSION=2} once XYZ is done. } SELECT * FROM Singers @{OPTIMIZER_VERSION=1} -- This is a comment /* this is not a block comment SELECT * FROM Singers /* This is a block comment */

The current statement hint parsing is simplified to just look for the first and next curly brace, but that can only be safely done because we at that moment know that:

String literals cannot occur in that part of a valid SQL string.

All comments in that part of the SQL string have been removed.

/* (others?)

No, /* is the only valid start of a block comment.

Edit

I guess the biggest problem with such a simplified parsing approach would be that both types of comments can contain both the start and end marker of the other type, which could make the parsing break:

If we remove block comments first, the following could not simply be stripped by looking for a /*:

-- This is a comment /* Still a comment, but not a block comment. SELECT 1

While if we remove line comments first, the following could break the parsing of block comments afterwards:

/* This is a block comment -- This is not a line comment */ SELECT 1

skuruppu · 2021-08-10T07:50:53Z

.../Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerCommandTextBuilderTests.cs

+        [InlineData("SELECT * FROM Foo@{FORCE_INDEX=`INDEX FOR SELECT`}", "SELECT * FROM Foo@{FORCE_INDEX=`INDEX FOR SELECT`}")]
+        [InlineData("@{OPTIMIZER_VERSION=1 SELECT 1", "@{OPTIMIZER_VERSION=1 SELECT 1")]
+        [InlineData("{OPTIMIZER_VERSION=1} SELECT 1", "{OPTIMIZER_VERSION=1} SELECT 1")]
+        public void RemoveStatementHintAndComments(string sql, string sqlWithoutHintsAndComments)


Givent the name, should some of the inputs also have comments, or should we change the test name?

You're right. I've updated the test name, as we are already testing the stripping of comments above. I also changed the name of the method that removes comments and statement hints, as there could be more than one.

Alternative to googleapis#6848 Closes googleapis#6847

Alternative to #6848 Closes #6847

jskeet · 2021-08-16T07:52:05Z

Presumably this PR can be closed now that #6947 has been merged?

olavloite · 2021-08-16T08:11:36Z

Presumably this PR can be closed now that #6947 has been merged?

Yes

Changes in Google.Cloud.Spanner.Data version 3.12.0: - [Commit 7c6a6f1](googleapis@7c6a6f1): feat: add support for JSON data type in Spanner ([issue 6390](googleapis#6390)) - [Commit ac367e2](googleapis@ac367e2): feat: Regenerate all APIs to support self-signed JWTs - [Commit 61938b6](googleapis@61938b6): - feat(Spanner): Support comments and statement hints in untyped commands. - Alternative to [issue 6848](googleapis#6848) - Closes [issue 6847](googleapis#6847) - [Commit d26b04c](googleapis@d26b04c): fix: address review comments - [Commit d2025be](googleapis@d2025be): fix: use logger from SpannerSettings - [Commit b34f6f4](googleapis@b34f6f4): cleanup: fix comment + remove unnecessary import - [Commit fc7a41b](googleapis@fc7a41b): test: remove connectionstring tests and add settings test - [Commit 6016ef0](googleapis@6016ef0): - feat: support custom SpannerSettings in SessionPoolManager - Support setting custom SpannerSettings when creating a SessionPoolManager. - [Commit 0ab6b8b](googleapis@0ab6b8b): - feat: allow adding an additional version header - Allows adding an additional version header to the connection string. This will be - added to the `x-goog-api-client` header that is used by the underlying Spanner client. - Only a fixed set of values may be set for the header (currently only 'efcore' is - allowed), and the property is not intended for public use. - [Commit 250124f](googleapis@250124f): - fix: synchronize access to the underlying transaction for ambient transactions ([issue 6616](googleapis#6616)) - * fix: synchronize access to the underlying transaction for ambient transactions - Synchronizes access to the underlying Spanner transaction to prevent - multiple transactions from being created if parallel commands are - executed on the ambient transaction. - * test: add integration test - * fix: use Lazy for initialization - * chore: address review comments - * fix: remove unnecessary read call Packages in this release: - Release Google.Cloud.Spanner.Admin.Database.V1 version 3.12.0 - Release Google.Cloud.Spanner.Admin.Instance.V1 version 3.12.0 - Release Google.Cloud.Spanner.Common.V1 version 3.12.0 - Release Google.Cloud.Spanner.Data version 3.12.0 - Release Google.Cloud.Spanner.V1 version 3.12.0

Changes in Google.Cloud.Spanner.Data version 3.12.0: - [Commit 7c6a6f1](7c6a6f1): feat: add support for JSON data type in Spanner ([issue 6390](#6390)) - [Commit ac367e2](ac367e2): feat: Regenerate all APIs to support self-signed JWTs - [Commit 61938b6](61938b6): - feat(Spanner): Support comments and statement hints in untyped commands. - Alternative to [issue 6848](#6848) - Closes [issue 6847](#6847) - [Commit d26b04c](d26b04c): fix: address review comments - [Commit d2025be](d2025be): fix: use logger from SpannerSettings - [Commit b34f6f4](b34f6f4): cleanup: fix comment + remove unnecessary import - [Commit fc7a41b](fc7a41b): test: remove connectionstring tests and add settings test - [Commit 6016ef0](6016ef0): - feat: support custom SpannerSettings in SessionPoolManager - Support setting custom SpannerSettings when creating a SessionPoolManager. - [Commit 0ab6b8b](0ab6b8b): - feat: allow adding an additional version header - Allows adding an additional version header to the connection string. This will be - added to the `x-goog-api-client` header that is used by the underlying Spanner client. - Only a fixed set of values may be set for the header (currently only 'efcore' is - allowed), and the property is not intended for public use. - [Commit 250124f](250124f): - fix: synchronize access to the underlying transaction for ambient transactions ([issue 6616](#6616)) - * fix: synchronize access to the underlying transaction for ambient transactions - Synchronizes access to the underlying Spanner transaction to prevent - multiple transactions from being created if parallel commands are - executed on the ambient transaction. - * test: add integration test - * fix: use Lazy for initialization - * chore: address review comments - * fix: remove unnecessary read call Packages in this release: - Release Google.Cloud.Spanner.Admin.Database.V1 version 3.12.0 - Release Google.Cloud.Spanner.Admin.Instance.V1 version 3.12.0 - Release Google.Cloud.Spanner.Common.V1 version 3.12.0 - Release Google.Cloud.Spanner.Data version 3.12.0 - Release Google.Cloud.Spanner.V1 version 3.12.0

olavloite requested a review from a team as a code owner July 31, 2021 14:04

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 31, 2021

product-auto-label bot added the api: spanner Issues related to the Spanner API. label Jul 31, 2021

olavloite requested a review from skuruppu July 31, 2021 14:04

olavloite mentioned this pull request Aug 1, 2021

docs: add sample for using query hints googleapis/dotnet-spanner-entity-framework#84

Merged

amanda-tarafa requested changes Aug 2, 2021

View reviewed changes

fix: address review comments

d0f7bbb

amanda-tarafa reviewed Aug 6, 2021

View reviewed changes

fix: simplify stripping statement hints

37e1083

skuruppu reviewed Aug 10, 2021

View reviewed changes

fix: update test and method name

03b8f37

amanda-tarafa added a commit to amanda-tarafa/google-cloud-dotnet that referenced this pull request Aug 11, 2021

feat(Spanner): Support comments and statement hints in untyped commands.

f0a08af

Alternative to googleapis#6848 Closes googleapis#6847

amanda-tarafa mentioned this pull request Aug 11, 2021

feat(Spanner): Support comments and statement hints in untyped commands. #6947

Merged

amanda-tarafa added a commit to amanda-tarafa/google-cloud-dotnet that referenced this pull request Aug 12, 2021

feat(Spanner): Support comments and statement hints in untyped commands.

1606c92

Alternative to googleapis#6848 Closes googleapis#6847

amanda-tarafa added a commit to amanda-tarafa/google-cloud-dotnet that referenced this pull request Aug 12, 2021

feat(Spanner): Support comments and statement hints in untyped commands.

d3c7b20

Alternative to googleapis#6848 Closes googleapis#6847

amanda-tarafa added a commit that referenced this pull request Aug 12, 2021

feat(Spanner): Support comments and statement hints in untyped commands.

61938b6

Alternative to #6848 Closes #6847

olavloite closed this Aug 16, 2021

amanda-tarafa mentioned this pull request Aug 24, 2021

Release Spanner libraries version 3.12.0 #7032

Merged

jskeet deleted the spanner-parse-comments-and-hints branch October 14, 2021 10:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(spanner): support comments and statement hints in untyped commands #6848

feat(spanner): support comments and statement hints in untyped commands #6848

olavloite commented Jul 31, 2021

amanda-tarafa left a comment

amanda-tarafa Aug 2, 2021

olavloite Aug 4, 2021

amanda-tarafa Aug 6, 2021

olavloite Aug 6, 2021

amanda-tarafa Aug 2, 2021

olavloite Aug 4, 2021

amanda-tarafa Aug 2, 2021

olavloite Aug 4, 2021

amanda-tarafa Aug 6, 2021

olavloite Aug 6, 2021 •

edited

Loading

amanda-tarafa Aug 2, 2021

olavloite Aug 4, 2021

amanda-tarafa Aug 2, 2021

olavloite Aug 4, 2021

amanda-tarafa Aug 2, 2021

olavloite Aug 4, 2021

amanda-tarafa Aug 6, 2021

olavloite Aug 6, 2021 •

edited

Loading

amanda-tarafa left a comment

amanda-tarafa Aug 6, 2021

amanda-tarafa Aug 6, 2021

amanda-tarafa Aug 6, 2021

amanda-tarafa Aug 6, 2021

olavloite Aug 6, 2021 •

edited

Loading

skuruppu Aug 10, 2021

olavloite Aug 11, 2021

jskeet commented Aug 16, 2021

olavloite commented Aug 16, 2021

	var trimmedCommandText = RemoveCommentsAndStatementHint(commandText).Trim();
	var trimmedCommandText = RemoveCommentsAndStatementHint(commandText).Trim();
	commandText = commandText.Trim();

	throw new ArgumentException($"'{commandText}' is not a recognized Spanner command.", nameof(commandText));
	throw new ArgumentException($"'{trimmedCommandText}' is not a recognized Spanner command.", nameof(commandText));

	private static readonly char s_dash = '#';
	private static readonly char s_sharp = '#';

feat(spanner): support comments and statement hints in untyped commands #6848

feat(spanner): support comments and statement hints in untyped commands #6848

Conversation

olavloite commented Jul 31, 2021

amanda-tarafa 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

olavloite Aug 6, 2021 • edited Loading

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

olavloite Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

amanda-tarafa 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

olavloite Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

Edit

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet commented Aug 16, 2021

olavloite commented Aug 16, 2021

olavloite Aug 6, 2021 •

edited

Loading

olavloite Aug 6, 2021 •

edited

Loading

olavloite Aug 6, 2021 •

edited

Loading