perf: keep comments when searching for params (#2951)

Keep all comments in the SQL string in place when converting positional parameters to named parameters. This reduces the amount of string operations that are needed for each query that is executed, and also enables actually sending comments from the client to Spanner when using positional parameters (e.g. in JDBC).

This is step 3 in the refactoring to share more code between the SpannerStatementParser and PostgreSQLStatementParser.

google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/AbstractStatementParser.java

@@ -634,36 +634,33 @@ public static class ParametersInfo {
 
   /**
    * Converts all positional parameters (?) in the given sql string into named parameters. The
-   * parameters are named @p1, @p2, etc. This method is used when converting a JDBC statement that
-   * uses positional parameters to a Cloud Spanner {@link Statement} instance that requires named
-   * parameters. The input SQL string may not contain any comments, except for PostgreSQL-dialect
-   * SQL strings.
+   * parameters are named @p1, @p2, etc. for GoogleSQL, and $1, $2, etc. for PostgreSQL. This method
+   * is used when converting a JDBC statement that uses positional parameters to a Cloud Spanner
+   * {@link Statement} instance that requires named parameters.
    *
-   * @param sql The sql string that should be converted
-   * @return A {@link ParametersInfo} object containing a string with named parameters instead of
-   *     positional parameters and the number of parameters.
-   * @throws SpannerException If the input sql string contains an unclosed string/byte literal.
-   */
-  @InternalApi
-  abstract ParametersInfo convertPositionalParametersToNamedParametersInternal(
-      char paramChar, String sql);
-
-  /**
-   * Converts all positional parameters (?) in the given sql string into named parameters. The
-   * parameters are named @p1, @p2, etc. This method is used when converting a JDBC statement that
-   * uses positional parameters to a Cloud Spanner {@link Statement} instance that requires named
-   * parameters. The input SQL string may not contain any comments. There is an exception case if
-   * the statement starts with a GSQL comment which forces it to be interpreted as a GoogleSql
-   * statement.
-   *
-   * @param sql The sql string without comments that should be converted
+   * @param sql The sql string that should be converted to use named parameters
    * @return A {@link ParametersInfo} object containing a string with named parameters instead of
    *     positional parameters and the number of parameters.
    * @throws SpannerException If the input sql string contains an unclosed string/byte literal.
    */
   @InternalApi
   public ParametersInfo convertPositionalParametersToNamedParameters(char paramChar, String sql) {
-    return convertPositionalParametersToNamedParametersInternal(paramChar, sql);
+    Preconditions.checkNotNull(sql);
+    final String namedParamPrefix = getQueryParameterPrefix();
+    StringBuilder named = new StringBuilder(sql.length() + countOccurrencesOf(paramChar, sql));
+    int index = 0;
+    int paramIndex = 1;
+    while (index < sql.length()) {
+      char c = sql.charAt(index);
+      if (c == paramChar) {
+        named.append(namedParamPrefix).append(paramIndex);
+        paramIndex++;
+        index++;
+      } else {
+        index = skip(sql, index, named);
+      }
+    }
+    return new ParametersInfo(paramIndex - 1, named.toString());
   }
 
   /** Convenience method that is used to estimate the number of parameters in a SQL statement. */
@@ -700,7 +697,8 @@ public boolean checkReturningClause(String sql) {
   }
 
   /**
-   * <<<<<<< HEAD Returns true if this dialect supports nested comments.
+   * <<<<<<< HEAD Returns true if this dialect supports nested comments. ======= <<<<<<< HEAD
+   * Returns true if this dialect supports nested comments. >>>>>>> main
    *
    * <ul>
    *   <li>This method should return false for dialects that consider this to be a valid comment:
@@ -755,6 +753,9 @@ public boolean checkReturningClause(String sql) {
    */
   abstract boolean supportsLineFeedInQuotedString();
 
+  /** Returns the query parameter prefix that should be used for this dialect. */
+  abstract String getQueryParameterPrefix();
+
   /**
    * Returns true for characters that can be used as the first character in unquoted identifiers.
    */

google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PostgreSQLStatementParser.java

@@ -88,6 +88,11 @@ boolean supportsLineFeedInQuotedString() {
     return true;
   }
 
+  @Override
+  String getQueryParameterPrefix() {
+    return "$";
+  }
+
   /**
    * Removes comments from and trims the given sql statement. PostgreSQL supports two types of
    * comments:
@@ -181,27 +186,6 @@ String removeStatementHint(String sql) {
     return sql;
   }
 
-  @InternalApi
-  @Override
-  ParametersInfo convertPositionalParametersToNamedParametersInternal(char paramChar, String sql) {
-    Preconditions.checkNotNull(sql);
-    final String namedParamPrefix = "$";
-    StringBuilder named = new StringBuilder(sql.length() + countOccurrencesOf(paramChar, sql));
-    int index = 0;
-    int paramIndex = 1;
-    while (index < sql.length()) {
-      char c = sql.charAt(index);
-      if (c == paramChar) {
-        named.append(namedParamPrefix).append(paramIndex);
-        paramIndex++;
-        index++;
-      } else {
-        index = skip(sql, index, named);
-      }
-    }
-    return new ParametersInfo(paramIndex - 1, named.toString());
-  }
-
   /**
    * Note: This is an internal API and breaking changes can be made without prior notice.
    *

google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SpannerStatementParser.java

@@ -90,6 +90,11 @@ boolean supportsLineFeedInQuotedString() {
     return false;
   }
 
+  @Override
+  String getQueryParameterPrefix() {
+    return "@p";
+  }
+
   /**
    * Removes comments from and trims the given sql statement. Spanner supports three types of
    * comments:
@@ -250,68 +255,6 @@ String removeStatementHint(String sql) {
     return sql;
   }
 
-  @InternalApi
-  @Override
-  ParametersInfo convertPositionalParametersToNamedParametersInternal(char paramChar, String sql) {
-    boolean isInQuoted = false;
-    char startQuote = 0;
-    boolean lastCharWasEscapeChar = false;
-    boolean isTripleQuoted = false;
-    int paramIndex = 1;
-    StringBuilder named = new StringBuilder(sql.length() + countOccurrencesOf(paramChar, sql));
-    for (int index = 0; index < sql.length(); index++) {
-      char c = sql.charAt(index);
-      if (isInQuoted) {
-        if ((c == '\n' || c == '\r') && !isTripleQuoted) {
-          throw SpannerExceptionFactory.newSpannerException(
-              ErrorCode.INVALID_ARGUMENT, "SQL statement contains an unclosed literal: " + sql);
-        } else if (c == startQuote) {
-          if (lastCharWasEscapeChar) {
-            lastCharWasEscapeChar = false;
-          } else if (isTripleQuoted) {
-            if (sql.length() > index + 2
-                && sql.charAt(index + 1) == startQuote
-                && sql.charAt(index + 2) == startQuote) {
-              isInQuoted = false;
-              startQuote = 0;
-              isTripleQuoted = false;
-            }
-          } else {
-            isInQuoted = false;
-            startQuote = 0;
-          }
-        } else if (c == '\\') {
-          lastCharWasEscapeChar = true;
-        } else {
-          lastCharWasEscapeChar = false;
-        }
-        named.append(c);
-      } else {
-        if (c == paramChar) {
-          named.append("@p" + paramIndex);
-          paramIndex++;
-        } else {
-          if (c == SINGLE_QUOTE || c == DOUBLE_QUOTE || c == BACKTICK_QUOTE) {
-            isInQuoted = true;
-            startQuote = c;
-            // check whether it is a triple-quote
-            if (sql.length() > index + 2
-                && sql.charAt(index + 1) == startQuote
-                && sql.charAt(index + 2) == startQuote) {
-              isTripleQuoted = true;
-            }
-          }
-          named.append(c);
-        }
-      }
-    }
-    if (isInQuoted) {
-      throw SpannerExceptionFactory.newSpannerException(
-          ErrorCode.INVALID_ARGUMENT, "SQL statement contains an unclosed literal: " + sql);
-    }
-    return new ParametersInfo(paramIndex - 1, named.toString());
-  }
-
   private boolean isReturning(String sql, int index) {
     return (index >= 1)
         && (index + 12 <= sql.length())

google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SpannerStatementParserTest.java

@@ -16,9 +16,11 @@
 
 package com.google.cloud.spanner.connection;
 
+import static com.google.cloud.spanner.connection.StatementParserTest.assertUnclosedLiteral;
 import static org.junit.Assert.assertEquals;
 
 import com.google.cloud.spanner.Dialect;
+import com.google.cloud.spanner.connection.StatementParserTest.CommentInjector;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -80,4 +82,158 @@ public void testSkip() {
     assertEquals("'foo\\''", skip("r'foo\\''  ", 1));
     assertEquals("'''foo\\'\\'\\'bar'''", skip("'''foo\\'\\'\\'bar'''  ", 0));
   }
+
+  @Test
+  public void testConvertPositionalParametersToNamedParameters() {
+    AbstractStatementParser parser =
+        AbstractStatementParser.getInstance(Dialect.GOOGLE_STANDARD_SQL);
+
+    for (String comment :
+        new String[] {
+          "-- test comment\n",
+          "/* another test comment */",
+          "/* comment\nwith\nmultiple\nlines\n */",
+          "/* comment /* with nested */ comment */"
+        }) {
+      for (CommentInjector injector : CommentInjector.values()) {
+        assertEquals(
+            injector.inject("select * %sfrom foo where name=@p1", comment),
+            parser.convertPositionalParametersToNamedParameters(
+                    '?', injector.inject("select * %sfrom foo where name=?", comment))
+                .sqlWithNamedParameters);
+        assertEquals(
+            injector.inject("@p1%s'?test?\"?test?\"?'@p2", comment),
+            parser.convertPositionalParametersToNamedParameters(
+                    '?', injector.inject("?%s'?test?\"?test?\"?'?", comment))
+                .sqlWithNamedParameters);
+        assertEquals(
+            injector.inject("@p1'?it\\'?s'%s@p2", comment),
+            parser.convertPositionalParametersToNamedParameters(
+                    '?', injector.inject("?'?it\\'?s'%s?", comment))
+                .sqlWithNamedParameters);
+        assertEquals(
+            injector.inject("@p1'?it\\\"?s'%s@p2", comment),
+            parser.convertPositionalParametersToNamedParameters(
+                    '?', injector.inject("?'?it\\\"?s'%s?", comment))
+                .sqlWithNamedParameters);
+        assertEquals(
+            injector.inject("@p1\"?it\\\"?s\"%s@p2", comment),
+            parser.convertPositionalParametersToNamedParameters(
+                    '?', injector.inject("?\"?it\\\"?s\"%s?", comment))
+                .sqlWithNamedParameters);
+        assertEquals(
+            injector.inject("@p1%s'''?it\\''?s'''@p2", comment),
+            parser.convertPositionalParametersToNamedParameters(
+                    '?', injector.inject("?%s'''?it\\''?s'''?", comment))
+                .sqlWithNamedParameters);
+        assertEquals(
+            injector.inject("@p1\"\"\"?it\\\"\"?s\"\"\"%s@p2", comment),
+            parser.convertPositionalParametersToNamedParameters(
+                    '?', injector.inject("?\"\"\"?it\\\"\"?s\"\"\"%s?", comment))
+                .sqlWithNamedParameters);
+
+        // GoogleSQL does not support dollar-quoted strings, so these are all ignored.
+        assertEquals(
+            injector.inject("@p1$$@p2it$@p3s$$%s@p4", comment),
+            parser.convertPositionalParametersToNamedParameters(
+                    '?', injector.inject("?$$?it$?s$$%s?", comment))
+                .sqlWithNamedParameters);
+        assertEquals(
+            injector.inject("@p1$tag$@p2it$$@p3s$tag$%s@p4", comment),
+            parser.convertPositionalParametersToNamedParameters(
+                    '?', injector.inject("?$tag$?it$$?s$tag$%s?", comment))
+                .sqlWithNamedParameters);
+        assertEquals(
+            injector.inject("@p1%s$$@p2it\\'?s \t ?it\\'?s'$$@p3", comment),
+            parser.convertPositionalParametersToNamedParameters(
+                    '?', injector.inject("?%s$$?it\\'?s \t ?it\\'?s'$$?", comment))
+                .sqlWithNamedParameters);
+
+        // Note: GoogleSQL does not allowa a single-quoted string literal to contain line feeds.
+        assertUnclosedLiteral(parser, injector.inject("?'?it\\''?s \n ?it\\''?s'%s?", comment));
+        assertEquals(
+            "@p1'?it\\''@p2s \n @p3it\\''@p4s@p5",
+            parser.convertPositionalParametersToNamedParameters('?', "?'?it\\''?s \n ?it\\''?s?")
+                .sqlWithNamedParameters);
+        assertEquals(
+            injector.inject("@p1%s'''?it\\''?s \n ?it\\''?s'''@p2", comment),
+            parser.convertPositionalParametersToNamedParameters(
+                    '?', injector.inject("?%s'''?it\\''?s \n ?it\\''?s'''?", comment))
+                .sqlWithNamedParameters);
+
+        assertEquals(
+            injector.inject(
+                "select 1, @p1, 'test?test', \"test?test\", %sfoo.* from `foo` where col1=@p2 and col2='test' and col3=@p3 and col4='?' and col5=\"?\" and col6='?''?''?'",
+                comment),
+            parser.convertPositionalParametersToNamedParameters(
+                    '?',
+                    injector.inject(
+                        "select 1, ?, 'test?test', \"test?test\", %sfoo.* from `foo` where col1=? and col2='test' and col3=? and col4='?' and col5=\"?\" and col6='?''?''?'",
+                        comment))
+                .sqlWithNamedParameters);
+
+        assertEquals(
+            injector.inject(
+                "select * "
+                    + "%sfrom foo "
+                    + "where name=@p1 "
+                    + "and col2 like @p2 "
+                    + "and col3 > @p3",
+                comment),
+            parser.convertPositionalParametersToNamedParameters(
+                    '?',
+                    injector.inject(
+                        "select * "
+                            + "%sfrom foo "
+                            + "where name=? "
+                            + "and col2 like ? "
+                            + "and col3 > ?",
+                        comment))
+                .sqlWithNamedParameters);
+        assertEquals(
+            injector.inject("select * " + "from foo " + "where id between @p1%s and @p2", comment),
+            parser.convertPositionalParametersToNamedParameters(
+                    '?',
+                    injector.inject(
+                        "select * " + "from foo " + "where id between ?%s and ?", comment))
+                .sqlWithNamedParameters);
+        assertEquals(
+            injector.inject("select * " + "from foo " + "limit @p1 %s offset @p2", comment),
+            parser.convertPositionalParametersToNamedParameters(
+                    '?',
+                    injector.inject("select * " + "from foo " + "limit ? %s offset ?", comment))
+                .sqlWithNamedParameters);
+        assertEquals(
+            injector.inject(
+                "select * "
+                    + "from foo "
+                    + "where col1=@p1 "
+                    + "and col2 like @p2 "
+                    + " %s "
+                    + "and col3 > @p3 "
+                    + "and col4 < @p4 "
+                    + "and col5 != @p5 "
+                    + "and col6 not in (@p6, @p7, @p8) "
+                    + "and col7 in (@p9, @p10, @p11) "
+                    + "and col8 between @p12 and @p13",
+                comment),
+            parser.convertPositionalParametersToNamedParameters(
+                    '?',
+                    injector.inject(
+                        "select * "
+                            + "from foo "
+                            + "where col1=? "
+                            + "and col2 like ? "
+                            + " %s "
+                            + "and col3 > ? "
+                            + "and col4 < ? "
+                            + "and col5 != ? "
+                            + "and col6 not in (?, ?, ?) "
+                            + "and col7 in (?, ?, ?) "
+                            + "and col8 between ? and ?",
+                        comment))
+                .sqlWithNamedParameters);
+      }
+    }
+  }
 }