perf: remove all calls to getSqlWithoutComments (googleapis#3822)

Calls to getSqlWithoutComments() have been replaced with calls to getSql().
This reduces the number of times that the SQL string needs to be parsed, and the number
of strings that are created by the Connection API.

The only part of the Connection API that still depends on the SQL string without comments,
is the check whether a DML statement contains a THEN RETURN / RETURNING clause. This will
be removed in a follow-up change.

Author: olavloite

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

@@ -317,7 +317,7 @@ <ResponseT, MetadataT> ResponseT getWithStatementTimeout(
     } catch (TimeoutException e) {
       throw SpannerExceptionFactory.newSpannerException(
           ErrorCode.DEADLINE_EXCEEDED,
-          "Statement execution timeout occurred for " + statement.getSqlWithoutComments(),
+          "Statement execution timeout occurred for " + statement.getSql(),
           e);
     } catch (ExecutionException e) {
       Throwable cause = e.getCause();
@@ -331,7 +331,7 @@ <ResponseT, MetadataT> ResponseT getWithStatementTimeout(
       }
       throw SpannerExceptionFactory.newSpannerException(
           ErrorCode.fromGrpcStatus(Status.fromThrowable(e)),
-          "Statement execution failed for " + statement.getSqlWithoutComments(),
+          "Statement execution failed for " + statement.getSql(),
           e);
     } catch (InterruptedException e) {
       throw SpannerExceptionFactory.newSpannerException(

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

@@ -425,7 +425,17 @@ Statement mergeQueryOptions(Statement statement, QueryOptions defaultQueryOption
           .build();
     }
 
-    /** @return the SQL statement with all comments removed from the SQL string. */
+    /** @return the original SQL statement */
+    @InternalApi
+    public String getSql() {
+      return statement.getSql();
+    }
+
+    /**
+     * @return the SQL statement with all comments removed from the SQL string.
+     * @deprecated use {@link #getSql()} instead
+     */
+    @Deprecated
     @InternalApi
     public String getSqlWithoutComments() {
       return sqlWithoutComments.get();

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

@@ -46,8 +46,7 @@ class ClientSideStatementBeginExecutor implements ClientSideStatementExecutor {
   @Override
   public StatementResult execute(ConnectionStatementExecutor connection, ParsedStatement statement)
       throws Exception {
-    return (StatementResult)
-        method.invoke(connection, getParameterValue(statement.getSqlWithoutComments()));
+    return (StatementResult) method.invoke(connection, getParameterValue(statement.getSql()));
   }
 
   IsolationLevel getParameterValue(String sql) {

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

@@ -50,8 +50,7 @@ class ClientSideStatementExplainExecutor implements ClientSideStatementExecutor
   @Override
   public StatementResult execute(ConnectionStatementExecutor connection, ParsedStatement statement)
       throws Exception {
-    return (StatementResult)
-        method.invoke(connection, getParameterValue(statement.getSqlWithoutComments()));
+    return (StatementResult) method.invoke(connection, getParameterValue(statement.getSql()));
   }
 
   String getParameterValue(String sql) {

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

@@ -49,7 +49,7 @@ public StatementResult execute(
   }
 
   String getParameterValue(ParsedStatement parsedStatement) {
-    Matcher matcher = statement.getPattern().matcher(parsedStatement.getSqlWithoutComments());
+    Matcher matcher = statement.getPattern().matcher(parsedStatement.getSql());
     if (matcher.find() && matcher.groupCount() >= 2) {
       String space = matcher.group(1);
       String value = matcher.group(2);

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

@@ -45,8 +45,7 @@ class ClientSideStatementPgBeginExecutor implements ClientSideStatementExecutor
   @Override
   public StatementResult execute(ConnectionStatementExecutor connection, ParsedStatement statement)
       throws Exception {
-    return (StatementResult)
-        method.invoke(connection, getParameterValue(statement.getSqlWithoutComments()));
+    return (StatementResult) method.invoke(connection, getParameterValue(statement.getSql()));
   }
 
   PgTransactionMode getParameterValue(String sql) {

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

@@ -65,7 +65,7 @@ String getParameterValue(ParsedStatement parsedStatement) {
     // 2. If the matcher matches and returns zero groups, we know that the statement is valid, but
     //    that it does not contain a partition-id in the SQL statement. The partition-id must then
     //    be included in the statement as a query parameter.
-    Matcher matcher = statement.getPattern().matcher(parsedStatement.getSqlWithoutComments());
+    Matcher matcher = statement.getPattern().matcher(parsedStatement.getSql());
     if (matcher.find() && matcher.groupCount() >= 1) {
       String value = matcher.group(1);
       if (!Strings.isNullOrEmpty(value)) {

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

@@ -50,7 +50,7 @@ public StatementResult execute(
   }
 
   String getParameterValue(ParsedStatement parsedStatement) {
-    Matcher matcher = statement.getPattern().matcher(parsedStatement.getSqlWithoutComments());
+    Matcher matcher = statement.getPattern().matcher(parsedStatement.getSql());
     if (matcher.find() && matcher.groupCount() >= 2) {
       // Include the spacing group in case the query is enclosed in parentheses like this:
       // `run partitioned query(select * from foo)`

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

@@ -17,6 +17,7 @@
 package com.google.cloud.spanner.connection;
 
 import com.google.cloud.Tuple;
+import com.google.cloud.spanner.Dialect;
 import com.google.cloud.spanner.ErrorCode;
 import com.google.cloud.spanner.SpannerExceptionFactory;
 import com.google.cloud.spanner.connection.AbstractStatementParser.ParsedStatement;
@@ -27,6 +28,7 @@
 import com.google.common.util.concurrent.UncheckedExecutionException;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Method;
+import java.nio.CharBuffer;
 import java.util.concurrent.ExecutionException;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -104,8 +106,8 @@ public StatementResult execute(ConnectionStatementExecutor connection, ParsedSta
     try {
       value =
           this.cache.get(
-              statement.getSqlWithoutComments(),
-              () -> getParameterValue(statement.getSqlWithoutComments()));
+              statement.getSql(),
+              () -> getParameterValue(connection.getDialect(), statement.getSql()));
     } catch (ExecutionException | UncheckedExecutionException executionException) {
       throw SpannerExceptionFactory.asSpannerException(executionException.getCause());
     }
@@ -115,8 +117,13 @@ public StatementResult execute(ConnectionStatementExecutor connection, ParsedSta
     return (StatementResult) method.invoke(connection, value.x());
   }
 
-  Tuple<T, Boolean> getParameterValue(String sql) {
-    Matcher matcher = allowedValuesPattern.matcher(sql);
+  Tuple<T, Boolean> getParameterValue(Dialect dialect, String sql) {
+    // Get rid of any spaces/comments at the start of the string.
+    SimpleParser simpleParser = new SimpleParser(dialect, sql);
+    simpleParser.skipWhitespaces();
+    // Create a wrapper around the SQL string from the point after the first whitespace.
+    CharBuffer sqlAfterWhitespaces = CharBuffer.wrap(sql, simpleParser.getPos(), sql.length());
+    Matcher matcher = allowedValuesPattern.matcher(sqlAfterWhitespaces);
     if (matcher.find() && matcher.groupCount() >= 2) {
       boolean local = matcher.group(1) != null && "local".equalsIgnoreCase(matcher.group(1).trim());
       String value = matcher.group(2);
@@ -130,7 +137,7 @@ Tuple<T, Boolean> getParameterValue(String sql) {
               "Unknown value for %s: %s",
               this.statement.getSetStatement().getPropertyName(), value));
     } else {
-      Matcher invalidMatcher = this.statement.getPattern().matcher(sql);
+      Matcher invalidMatcher = this.statement.getPattern().matcher(sqlAfterWhitespaces);
       int valueGroup = this.supportsLocal ? 2 : 1;
       if (invalidMatcher.find() && invalidMatcher.groupCount() == valueGroup) {
         String invalidValue = invalidMatcher.group(valueGroup);

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

@@ -382,6 +382,7 @@ public Spanner getSpanner() {
   private DdlClient createDdlClient() {
     return DdlClient.newBuilder()
         .setDatabaseAdminClient(spanner.getDatabaseAdminClient())
+        .setDialectSupplier(this::getDialect)
         .setProjectId(options.getProjectId())
         .setInstanceId(options.getInstanceId())
         .setDatabaseName(options.getDatabaseName())
@@ -1424,8 +1425,7 @@ private StatementResult internalExecute(
       default:
     }
     throw SpannerExceptionFactory.newSpannerException(
-        ErrorCode.INVALID_ARGUMENT,
-        "Unknown statement: " + parsedStatement.getSqlWithoutComments());
+        ErrorCode.INVALID_ARGUMENT, "Unknown statement: " + parsedStatement.getSql());
   }
 
   @VisibleForTesting
@@ -1470,8 +1470,7 @@ private static ResultType getResultType(ParsedStatement parsedStatement) {
       case UNKNOWN:
       default:
         throw SpannerExceptionFactory.newSpannerException(
-            ErrorCode.INVALID_ARGUMENT,
-            "Unknown statement: " + parsedStatement.getSqlWithoutComments());
+            ErrorCode.INVALID_ARGUMENT, "Unknown statement: " + parsedStatement.getSql());
     }
   }
 
@@ -1503,8 +1502,7 @@ public AsyncStatementResult executeAsync(Statement statement) {
       default:
     }
     throw SpannerExceptionFactory.newSpannerException(
-        ErrorCode.INVALID_ARGUMENT,
-        "Unknown statement: " + parsedStatement.getSqlWithoutComments());
+        ErrorCode.INVALID_ARGUMENT, "Unknown statement: " + parsedStatement.getSql());
   }
 
   @Override
@@ -1699,7 +1697,7 @@ private ResultSet parseAndExecuteQuery(
               throw SpannerExceptionFactory.newSpannerException(
                   ErrorCode.FAILED_PRECONDITION,
                   "DML statement with returning clause cannot be executed in read-only mode: "
-                      + parsedStatement.getSqlWithoutComments());
+                      + parsedStatement.getSql());
             }
             return internalExecuteQuery(callType, parsedStatement, analyzeMode, options);
           }
@@ -1710,8 +1708,7 @@ private ResultSet parseAndExecuteQuery(
     }
     throw SpannerExceptionFactory.newSpannerException(
         ErrorCode.INVALID_ARGUMENT,
-        "Statement is not a query or DML with returning clause: "
-            + parsedStatement.getSqlWithoutComments());
+        "Statement is not a query or DML with returning clause: " + parsedStatement.getSql());
   }
 
   private AsyncResultSet parseAndExecuteQueryAsync(Statement query, QueryOption... options) {
@@ -1741,7 +1738,7 @@ private AsyncResultSet parseAndExecuteQueryAsync(Statement query, QueryOption...
               throw SpannerExceptionFactory.newSpannerException(
                   ErrorCode.FAILED_PRECONDITION,
                   "DML statement with returning clause cannot be executed in read-only mode: "
-                      + parsedStatement.getSqlWithoutComments());
+                      + parsedStatement.getSql());
             }
             return internalExecuteQueryAsync(
                 CallType.ASYNC, parsedStatement, AnalyzeMode.NONE, options);
@@ -1753,8 +1750,7 @@ private AsyncResultSet parseAndExecuteQueryAsync(Statement query, QueryOption...
     }
     throw SpannerExceptionFactory.newSpannerException(
         ErrorCode.INVALID_ARGUMENT,
-        "Statement is not a query or DML with returning clause: "
-            + parsedStatement.getSqlWithoutComments());
+        "Statement is not a query or DML with returning clause: " + parsedStatement.getSql());
   }
 
   private boolean isInternalMetadataQuery(QueryOption... options) {
@@ -1781,7 +1777,7 @@ public long executeUpdate(Statement update) {
             throw SpannerExceptionFactory.newSpannerException(
                 ErrorCode.FAILED_PRECONDITION,
                 "DML statement with returning clause cannot be executed using executeUpdate: "
-                    + parsedStatement.getSqlWithoutComments()
+                    + parsedStatement.getSql()
                     + ". Please use executeQuery instead.");
           }
           return get(internalExecuteUpdateAsync(CallType.SYNC, parsedStatement));
@@ -1794,7 +1790,7 @@ public long executeUpdate(Statement update) {
     }
     throw SpannerExceptionFactory.newSpannerException(
         ErrorCode.INVALID_ARGUMENT,
-        "Statement is not an update statement: " + parsedStatement.getSqlWithoutComments());
+        "Statement is not an update statement: " + parsedStatement.getSql());
   }
 
   @Override
@@ -1809,7 +1805,7 @@ public ApiFuture<Long> executeUpdateAsync(Statement update) {
             throw SpannerExceptionFactory.newSpannerException(
                 ErrorCode.FAILED_PRECONDITION,
                 "DML statement with returning clause cannot be executed using executeUpdateAsync: "
-                    + parsedStatement.getSqlWithoutComments()
+                    + parsedStatement.getSql()
                     + ". Please use executeQueryAsync instead.");
           }
           return internalExecuteUpdateAsync(CallType.ASYNC, parsedStatement);
@@ -1822,7 +1818,7 @@ public ApiFuture<Long> executeUpdateAsync(Statement update) {
     }
     throw SpannerExceptionFactory.newSpannerException(
         ErrorCode.INVALID_ARGUMENT,
-        "Statement is not an update statement: " + parsedStatement.getSqlWithoutComments());
+        "Statement is not an update statement: " + parsedStatement.getSql());
   }
 
   @Override
@@ -1845,7 +1841,7 @@ public ResultSetStats analyzeUpdate(Statement update, QueryAnalyzeMode analyzeMo
     }
     throw SpannerExceptionFactory.newSpannerException(
         ErrorCode.INVALID_ARGUMENT,
-        "Statement is not an update statement: " + parsedStatement.getSqlWithoutComments());
+        "Statement is not an update statement: " + parsedStatement.getSql());
   }
 
   @Override
@@ -1867,7 +1863,7 @@ public ResultSet analyzeUpdateStatement(
     }
     throw SpannerExceptionFactory.newSpannerException(
         ErrorCode.INVALID_ARGUMENT,
-        "Statement is not an update statement: " + parsedStatement.getSqlWithoutComments());
+        "Statement is not an update statement: " + parsedStatement.getSql());
   }
 
   @Override
@@ -1899,7 +1895,7 @@ private List<ParsedStatement> parseUpdateStatements(Iterable<Statement> updates)
           throw SpannerExceptionFactory.newSpannerException(
               ErrorCode.INVALID_ARGUMENT,
               "The batch update list contains a statement that is not an update statement: "
-                  + parsedStatement.getSqlWithoutComments());
+                  + parsedStatement.getSql());
       }
     }
     return parsedStatements;

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

@@ -16,6 +16,7 @@
 
 package com.google.cloud.spanner.connection;
 
+import com.google.cloud.spanner.Dialect;
 import com.google.cloud.spanner.Options.RpcPriority;
 import com.google.cloud.spanner.Statement;
 import com.google.cloud.spanner.TimestampBound;
@@ -36,6 +37,7 @@
  * <p>The client side statements are defined in the ClientSideStatements.json file.
  */
 interface ConnectionStatementExecutor {
+  Dialect getDialect();
 
   StatementResult statementSetAutocommit(Boolean autocommit);
 

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

@@ -154,6 +154,11 @@ ConnectionImpl getConnection() {
     return connection;
   }
 
+  @Override
+  public Dialect getDialect() {
+    return getConnection().getDialect();
+  }
+
   @Override
   public StatementResult statementSetAutocommit(Boolean autocommit) {
     Preconditions.checkNotNull(autocommit);

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

@@ -191,13 +191,11 @@ public ApiFuture<Void> executeDdlAsync(CallType callType, ParsedStatement ddl) {
         "The batch is no longer active and cannot be used for further statements");
     Preconditions.checkArgument(
         ddl.getType() == StatementType.DDL,
-        "Only DDL statements are allowed. \""
-            + ddl.getSqlWithoutComments()
-            + "\" is not a DDL-statement.");
+        "Only DDL statements are allowed. \"" + ddl.getSql() + "\" is not a DDL-statement.");
     Preconditions.checkArgument(
-        !DdlClient.isCreateDatabaseStatement(ddl.getSqlWithoutComments()),
+        !DdlClient.isCreateDatabaseStatement(dbClient.getDialect(), ddl.getSql()),
         "CREATE DATABASE is not supported in DDL batches.");
-    statements.add(ddl.getSqlWithoutComments());
+    statements.add(ddl.getSql());
     return ApiFutures.immediateFuture(null);
   }