Skip to content

Commit 89889ee

Browse files
olavloiteolavloite
committed
chore: address review comments
1 parent 9e6dc6e commit 89889ee

File tree

15 files changed

+36991
-25898
lines changed

15 files changed

+36991
-25898
lines changed

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/Statement.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ private Builder(Statement statement) {
8787
}
8888

8989
/** Replaces the current SQL of this builder with the given string. */
90-
public Builder withSql(String sql) {
90+
public Builder replace(String sql) {
9191
sqlBuffer.replace(0, sqlBuffer.length(), sql);
9292
return this;
9393
}

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,17 +184,18 @@ ResultSet partitionQuery(
184184
PartitionOptions partitionOptions,
185185
ParsedStatement query,
186186
QueryOption... options) {
187+
final String partitionColumnName = "PARTITION";
187188
BatchTransactionId transactionId = transaction.getBatchTransactionId();
188189
List<Partition> partitions =
189190
transaction.partitionQuery(partitionOptions, query.getStatement(), options);
190191
return ResultSets.forRows(
191192
com.google.cloud.spanner.Type.struct(
192-
StructField.of("PARTITION", com.google.cloud.spanner.Type.string())),
193+
StructField.of(partitionColumnName, com.google.cloud.spanner.Type.string())),
193194
partitions.stream()
194195
.map(
195196
partition ->
196197
Struct.newBuilder()
197-
.set("PARTITION")
198+
.set(partitionColumnName)
198199
.to(PartitionId.encodeToString(transactionId, partition))
199200
.build())
200201
.collect(Collectors.toList()));

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ public StatementResult execute(
4545
ConnectionStatementExecutor connection, ParsedStatement parsedStatement) throws Exception {
4646
String sql = getParameterValue(parsedStatement);
4747
return (StatementResult)
48-
method.invoke(connection, parsedStatement.getStatement().toBuilder().withSql(sql).build());
48+
method.invoke(connection, parsedStatement.getStatement().toBuilder().replace(sql).build());
4949
}
5050

5151
String getParameterValue(ParsedStatement parsedStatement) {

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,16 @@ public StatementResult execute(
5555
}
5656

5757
String getParameterValue(ParsedStatement parsedStatement) {
58+
// The statement has the form `RUN PARTITION ['partition-id']`.
59+
// The regex that is defined for this statement is (simplified) `run\s+partition(?:\s*'(.*)')?`
60+
// This regex has one capturing group, which captures the partition-id inside the single quotes.
61+
// That capturing group is however inside a non-capturing optional group.
62+
// That means that:
63+
// 1. If the matcher matches and returns one or more groups, we know that we have a partition-id
64+
// in the SQL statement itself, as that is the only thing that can be in a capturing group.
65+
// 2. If the matcher matches and returns zero groups, we know that the statement is valid, but
66+
// that it does not contain a partition-id in the SQL statement. The partition-id must then
67+
// be included in the statement as a query parameter.
5868
Matcher matcher = statement.getPattern().matcher(parsedStatement.getSqlWithoutComments());
5969
if (matcher.find() && matcher.groupCount() >= 1) {
6070
String value = matcher.group(1);

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ public StatementResult execute(
4646
ConnectionStatementExecutor connection, ParsedStatement parsedStatement) throws Exception {
4747
String sql = getParameterValue(parsedStatement);
4848
return (StatementResult)
49-
method.invoke(connection, parsedStatement.getStatement().toBuilder().withSql(sql).build());
49+
method.invoke(connection, parsedStatement.getStatement().toBuilder().replace(sql).build());
5050
}
5151

5252
String getParameterValue(ParsedStatement parsedStatement) {

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1374,6 +1374,7 @@ public ApiFuture<long[]> executeBatchUpdateAsync(Iterable<Statement> updates) {
13741374

13751375
private QueryOption[] mergeDataBoost(QueryOption... options) {
13761376
if (this.dataBoostEnabled) {
1377+
13771378
// Shortcut for the most common scenario.
13781379
if (options == null || options.length == 0) {
13791380
options = new QueryOption[] {Options.dataBoostEnabled(true)};

‎google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/MergedResultSet.java

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import java.util.concurrent.LinkedBlockingDeque;
3636
import java.util.concurrent.atomic.AtomicBoolean;
3737
import java.util.concurrent.atomic.AtomicInteger;
38+
import javax.annotation.Nonnull;
3839

3940
/**
4041
* {@link MergedResultSet} is a {@link ResultSet} implementation that combines the results from
@@ -108,21 +109,27 @@ static class PartitionExecutorResult {
108109
private final Type type;
109110
private final ResultSetMetadata metadata;
110111

111-
static PartitionExecutorResult data(Struct data) {
112-
return new PartitionExecutorResult(data, null, null, null);
112+
static PartitionExecutorResult data(@Nonnull Struct data) {
113+
return new PartitionExecutorResult(Preconditions.checkNotNull(data), null, null, null);
113114
}
114115

115-
static PartitionExecutorResult typeAndMetadata(Type type, ResultSetMetadata metadata) {
116-
return new PartitionExecutorResult(null, type, metadata, null);
116+
static PartitionExecutorResult typeAndMetadata(
117+
@Nonnull Type type, @Nonnull ResultSetMetadata metadata) {
118+
return new PartitionExecutorResult(
119+
null, Preconditions.checkNotNull(type), Preconditions.checkNotNull(metadata), null);
117120
}
118121

119122
static PartitionExecutorResult dataAndMetadata(
120-
Struct data, Type type, ResultSetMetadata metadata) {
121-
return new PartitionExecutorResult(data, type, metadata, null);
123+
@Nonnull Struct data, @Nonnull Type type, @Nonnull ResultSetMetadata metadata) {
124+
return new PartitionExecutorResult(
125+
Preconditions.checkNotNull(data),
126+
Preconditions.checkNotNull(type),
127+
Preconditions.checkNotNull(metadata),
128+
null);
122129
}
123130

124-
static PartitionExecutorResult exception(Throwable exception) {
125-
return new PartitionExecutorResult(null, null, null, exception);
131+
static PartitionExecutorResult exception(@Nonnull Throwable exception) {
132+
return new PartitionExecutorResult(null, null, null, Preconditions.checkNotNull(exception));
126133
}
127134

128135
static PartitionExecutorResult finished() {

‎google-cloud-spanner/src/main/resources/com/google/cloud/spanner/connection/ClientSideStatements.json

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@
165165
"statementType": "PARTITION",
166166
"regex": "(?is)\\A\\s*partition(\\s+|\\()(.*)\\z",
167167
"method": "statementPartition",
168-
"exampleStatements": []
168+
"exampleStatements": ["partition select col1, col2 from my_table"]
169169
},
170170
{
171171
"name": "RUN PARTITION ['<partition_id>']",
@@ -174,7 +174,7 @@
174174
"statementType": "RUN_PARTITION",
175175
"regex": "(?is)\\A\\s*run\\s+partition(?:\\s*'(.*)')?\\s*\\z",
176176
"method": "statementRunPartition",
177-
"exampleStatements": []
177+
"exampleStatements": ["run partition 'H4sIAAAAAAAA_7VWTWwbRRSeOHESp_lpQyGoomBao1KCvU3s2IkCInWUgMOSlMT8tQcY787ak8zubHZmE5tDVQ7c4MqJAweQOFSiEhJwBa4ICVCFxAGB4MIBCQQIDgjxZr3ebJM64cIe7NmZ9_u9773Z6z-jpPCQZnA7V-e8zkjOYNw3c8LFjkO8nMHhz5CUO7lL2JNUrSomgmd0uTKZQAkdpdzOgUQZHSxpbUtaYEkLLWmR-ryORqSHHYGNtjWJst3VylgajWpcfL7pQshnuoYcOeK_T_y0MPzaxQRK6WjA4My3HSHRSX0T72CNYaeuVSTxcI0RiClJHZM0JRqPHW9Ijzp1OOzbIi1Qvb97nE-S1gaRIHo8gmPNVb-gNvkfUAmFwcBoZKDKtwiAmo6rux6XvOZbWrklSRTf8LZPvFbk8Ex3h3t-jnkEm-HrNrqKhqCUQmJJbHB6aCk3OlIKNqnwU_r9TRceKM35o0sTug0rNA88WkEjNm5GAmIFjUc4bNCXiUpXNF207wF3F2LuOujk9tDJ6FQVme3thKo9CdRzBSVryrBEiSvlpoeyR9ki2LqdIQS69x6uG_NaQX0NLBrtbHxP-b7-28Rf_YPV75QptZ2xzJKJi5hkjVpxJlvIW9NZjGeKWTJdmiOz0yWrVJuB5NNdsb4V4qUEGrmMxnapbCxy26ZS1VDoaAiCtIi3zncBgzvineFIUieeIrSJJS5zLuSSo2pt3ioIJ4xg1dV9ROJ6wAQd9VuUAezhW5JRcCnR8ZiezgPmDrq4TlSBlehEME3qJCB-qDvqesQiMAQWG76zJUKxQdej3KOyJVHuSLJn1l3jUigPLnv3whzewYxCfmTNYS21d7eOxhVIStOmQlJD58ZWcAJ1cePPoUMoapEfbzSsV1_5-2YC9QLaQGlsEwAG0B5rg-FLyrSnsHuwi4vxxDop7UxpS01i-ECrbbZOQEPIzNMxPZWg2GZhR0KQDyg3OeUmt8gZa09zkXnGsblJLapKCu5_fefdf-QfA2Wgp456bKV-OlA_saf-BJAWRJMD33z86V0vfdGLEstoiHFsLsNs5l4FpWTDI6LBmdl0H1to8313EH4TagVN5k6BxQe7ovYsZj7JAPWKhYrtslM3vj_3vDj2OcS0gpI76hD67OEj1C_WBFwvhgzevvpz_f3K619-BBPmMuqnYtVnDJgqWy6R6L7uzKmCwPy-pr6Nsw_eeIu9cPaRYrttIbfTXeWVxZtra798cuLOzQRKQl9hz8OtJRbQRJ0qzM9BcAY3yaHzVwlnFkFKccaihJmi3FoFYgVVg3YLxo9E93QhUJAcqAJOviGXAwMSnb_FIQwJ7sBfQBitYtt-MOh16Am4gN2rh5E_iq8zpceDATkaMEk1f27J8e34oStRsrJaLRagrzqDUm2eFUT5Tz-Utjxup-3Wi0EU6d0G8Uiamo8uUFORdLJrLAe_H8KJuBh0ZEoQIYLtcCKkoO-hpbDt7sOvXYJq5_TAV4zST6mYnwP0NyFooe7qTY1CK2PHICJYaWqY1jB41cyaFjoXWi1vlWatC1PZaVysZQukWMjW8vm5bJ7Mlgoz08WZqfwcpHnqQJpRPI9fm1j98Nv33gaiV1DSwQ6HK3RAEPiAM-He7Pth9ppC1fzs6zeDGToW3D1qdRIW1v8UsxaDSGhT_wJCVcN1bwoAAA=='"]
178178
},
179179
{
180180
"name": "RUN PARTITIONED QUERY <sql>",
@@ -183,7 +183,7 @@
183183
"statementType": "RUN_PARTITIONED_QUERY",
184184
"regex": "(?is)\\A\\s*run\\s+partitioned\\s+query(\\s+|\\()(.*)\\z",
185185
"method": "statementRunPartitionedQuery",
186-
"exampleStatements": []
186+
"exampleStatements": ["run partitioned query select col1, col2 from my_table"]
187187
},
188188
{
189189
"name": "BEGIN TRANSACTION",

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ClientSideStatementsTest.java

Lines changed: 35 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -167,8 +167,11 @@ private static void generateTestStatements(
167167
AbstractStatementParser parser, ClientSideStatementImpl statement) {
168168
for (String sql : statement.getExampleStatements()) {
169169
log(statement.getExamplePrerequisiteStatements(), sql);
170-
log(statement.getExamplePrerequisiteStatements(), upper(sql));
171-
log(statement.getExamplePrerequisiteStatements(), lower(sql));
170+
if (statement.getStatementType() != ClientSideStatementType.RUN_PARTITION) {
171+
// Partition ids are case-sensitive.
172+
log(statement.getExamplePrerequisiteStatements(), upper(sql));
173+
log(statement.getExamplePrerequisiteStatements(), lower(sql));
174+
}
172175
log(statement.getExamplePrerequisiteStatements(), withLeadingSpaces(sql));
173176
log(statement.getExamplePrerequisiteStatements(), withLeadingTabs(sql));
174177
log(statement.getExamplePrerequisiteStatements(), withLeadingLinefeeds(sql));
@@ -183,12 +186,20 @@ private static void generateTestStatements(
183186
statement.getExamplePrerequisiteStatements(),
184187
withInvalidPrefix(sql),
185188
ErrorCode.INVALID_ARGUMENT);
186-
log(
187-
statement.getExamplePrerequisiteStatements(),
188-
withInvalidSuffix(sql),
189-
parser.isQuery(withInvalidSuffix(sql))
190-
? ErrorCode.UNIMPLEMENTED
191-
: ErrorCode.INVALID_ARGUMENT);
189+
190+
boolean anySuffixAllowed =
191+
statement.getStatementType() == ClientSideStatementType.PARTITION
192+
|| statement.getStatementType() == ClientSideStatementType.RUN_PARTITIONED_QUERY;
193+
if (anySuffixAllowed) {
194+
log(statement.getExamplePrerequisiteStatements(), withInvalidSuffix(sql));
195+
} else {
196+
log(
197+
statement.getExamplePrerequisiteStatements(),
198+
withInvalidSuffix(sql),
199+
parser.isQuery(withInvalidSuffix(sql))
200+
? ErrorCode.UNIMPLEMENTED
201+
: ErrorCode.INVALID_ARGUMENT);
202+
}
192203

193204
final String[] replacements = {
194205
"%", "_", "&", "$", "@", "!", "*", "(", ")", "-", "+", "-#", "/", "\\", "?", "-/", "/#",
@@ -199,18 +210,22 @@ private static void generateTestStatements(
199210
statement.getExamplePrerequisiteStatements(),
200211
withPrefix(replacement, sql),
201212
ErrorCode.INVALID_ARGUMENT);
202-
log(
203-
statement.getExamplePrerequisiteStatements(),
204-
withSuffix(replacement, sql),
205-
parser.isQuery(withSuffix(replacement, sql))
206-
? ErrorCode.UNIMPLEMENTED
207-
: ErrorCode.INVALID_ARGUMENT);
208-
log(
209-
statement.getExamplePrerequisiteStatements(),
210-
replaceLastSpaceWith(replacement, sql),
211-
parser.isQuery(replaceLastSpaceWith(replacement, sql))
212-
? ErrorCode.UNIMPLEMENTED
213-
: ErrorCode.INVALID_ARGUMENT);
213+
if (anySuffixAllowed) {
214+
log(statement.getExamplePrerequisiteStatements(), withSuffix(replacement, sql));
215+
} else {
216+
log(
217+
statement.getExamplePrerequisiteStatements(),
218+
withSuffix(replacement, sql),
219+
parser.isQuery(withSuffix(replacement, sql))
220+
? ErrorCode.UNIMPLEMENTED
221+
: ErrorCode.INVALID_ARGUMENT);
222+
log(
223+
statement.getExamplePrerequisiteStatements(),
224+
replaceLastSpaceWith(replacement, sql),
225+
parser.isQuery(replaceLastSpaceWith(replacement, sql))
226+
? ErrorCode.UNIMPLEMENTED
227+
: ErrorCode.INVALID_ARGUMENT);
228+
}
214229
}
215230
}
216231
}

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@
4343
import com.google.cloud.NoCredentials;
4444
import com.google.cloud.Timestamp;
4545
import com.google.cloud.spanner.BatchClient;
46+
import com.google.cloud.spanner.BatchReadOnlyTransaction;
47+
import com.google.cloud.spanner.BatchTransactionId;
4648
import com.google.cloud.spanner.CommitResponse;
4749
import com.google.cloud.spanner.CommitStats;
4850
import com.google.cloud.spanner.DatabaseClient;
@@ -366,7 +368,13 @@ public TransactionRunner allowNestedTransaction() {
366368
};
367369
}
368370
});
369-
return new ConnectionImpl(options, spannerPool, ddlClient, dbClient, mock(BatchClient.class));
371+
BatchClient batchClient = mock(BatchClient.class);
372+
BatchReadOnlyTransaction batchReadOnlyTransaction = mock(BatchReadOnlyTransaction.class);
373+
when(batchClient.batchReadOnlyTransaction(any(TimestampBound.class)))
374+
.thenReturn(batchReadOnlyTransaction);
375+
when(batchClient.batchReadOnlyTransaction(any(BatchTransactionId.class)))
376+
.thenReturn(batchReadOnlyTransaction);
377+
return new ConnectionImpl(options, spannerPool, ddlClient, dbClient, batchClient);
370378
}
371379

372380
@Test

‎google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import com.google.cloud.spanner.connection.AbstractStatementParser.ParsedStatement;
3434
import com.google.cloud.spanner.connection.AbstractStatementParser.StatementType;
3535
import com.google.cloud.spanner.connection.ClientSideStatementImpl.CompileException;
36+
import com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType;
3637
import com.google.common.collect.ImmutableMap;
3738
import com.google.common.collect.ImmutableSet;
3839
import com.google.common.truth.Truth;
@@ -954,7 +955,17 @@ private <T extends ClientSideStatementImpl> void testParseStatement(
954955
assertParsing(withTrailingLinefeeds(statement), statementClass);
955956

956957
assertThat(parse(withInvalidPrefix(statement))).isNull();
957-
assertThat(parse(withInvalidSuffix(statement))).isNull();
958+
959+
ClientSideStatementImpl parseClientSideStatement = parser.parseClientSideStatement(statement);
960+
boolean anySuffixAllowed =
961+
parseClientSideStatement.getStatementType() == ClientSideStatementType.PARTITION
962+
|| parseClientSideStatement.getStatementType()
963+
== ClientSideStatementType.RUN_PARTITIONED_QUERY;
964+
if (anySuffixAllowed) {
965+
assertThat(parse(withInvalidSuffix(statement))).isNotNull();
966+
} else {
967+
assertThat(parse(withInvalidSuffix(statement))).isNull();
968+
}
958969

959970
assertThat(parse(withPrefix("%", statement))).isNull();
960971
assertThat(parse(withPrefix("_", statement))).isNull();
@@ -966,17 +977,19 @@ private <T extends ClientSideStatementImpl> void testParseStatement(
966977
assertThat(parse(withPrefix("(", statement))).isNull();
967978
assertThat(parse(withPrefix(")", statement))).isNull();
968979

969-
Truth.assertWithMessage(withSuffix("%", statement) + " is not a valid statement")
970-
.that(parse(withSuffix("%", statement)))
971-
.isNull();
972-
assertThat(parse(withSuffix("_", statement))).isNull();
973-
assertThat(parse(withSuffix("&", statement))).isNull();
974-
assertThat(parse(withSuffix("$", statement))).isNull();
975-
assertThat(parse(withSuffix("@", statement))).isNull();
976-
assertThat(parse(withSuffix("!", statement))).isNull();
977-
assertThat(parse(withSuffix("*", statement))).isNull();
978-
assertThat(parse(withSuffix("(", statement))).isNull();
979-
assertThat(parse(withSuffix(")", statement))).isNull();
980+
if (!anySuffixAllowed) {
981+
Truth.assertWithMessage(withSuffix("%", statement) + " is not a valid statement")
982+
.that(parse(withSuffix("%", statement)))
983+
.isNull();
984+
assertThat(parse(withSuffix("_", statement))).isNull();
985+
assertThat(parse(withSuffix("&", statement))).isNull();
986+
assertThat(parse(withSuffix("$", statement))).isNull();
987+
assertThat(parse(withSuffix("@", statement))).isNull();
988+
assertThat(parse(withSuffix("!", statement))).isNull();
989+
assertThat(parse(withSuffix("*", statement))).isNull();
990+
assertThat(parse(withSuffix("(", statement))).isNull();
991+
assertThat(parse(withSuffix(")", statement))).isNull();
992+
}
980993
}
981994

982995
private <T extends ClientSideStatementImpl> void testParseStatementWithOneParameterAtTheEnd(

0 commit comments

Comments
 (0)