Skip to content

chore: add namespace for client side statements in PostgreSQL dialect #1737

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

Merged
merged 12 commits into from
Mar 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,13 @@ implementation 'com.google.cloud:google-cloud-spanner'
If you are using Gradle without BOM, add this to your dependencies

```Groovy
implementation 'com.google.cloud:google-cloud-spanner:6.21.1'
implementation 'com.google.cloud:google-cloud-spanner:6.21.2'
```

If you are using SBT, add this to your dependencies

```Scala
libraryDependencies += "com.google.cloud" % "google-cloud-spanner" % "6.21.1"
libraryDependencies += "com.google.cloud" % "google-cloud-spanner" % "6.21.2"
```

## Authentication
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,11 @@
import com.google.cloud.spanner.SpannerException;
import com.google.cloud.spanner.SpannerExceptionFactory;
import com.google.cloud.spanner.Statement;
import com.google.cloud.spanner.connection.ClientSideStatementImpl.CompileException;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.spanner.v1.ExecuteSqlRequest.QueryOptions;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
Expand Down Expand Up @@ -322,14 +320,9 @@ ClientSideStatement getClientSideStatement() {
private final Dialect dialect;
private final Set<ClientSideStatementImpl> statements;

AbstractStatementParser(Dialect dialect) {
AbstractStatementParser(Dialect dialect, Set<ClientSideStatementImpl> statements) {
this.dialect = dialect;
try {
statements =
Collections.unmodifiableSet(ClientSideStatements.INSTANCE.getCompiledStatements());
} catch (CompileException e) {
throw new RuntimeException(e);
}
this.statements = statements;
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
* Copyright 2022 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.cloud.spanner.connection;

import com.google.cloud.spanner.ErrorCode;
import com.google.cloud.spanner.SpannerExceptionFactory;
import com.google.cloud.spanner.connection.ClientSideStatementImpl.CompileException;
import com.google.cloud.spanner.connection.ClientSideStatementValueConverters.PgTransactionModeConverter;
import java.lang.reflect.Method;
import java.util.regex.Matcher;

/** Specific executor for the BEGIN statement for PostgreSQL. */
class ClientSideStatementPgBeginExecutor implements ClientSideStatementExecutor {
private final ClientSideStatementImpl statement;
private final Method method;
private final PgTransactionModeConverter converter;

ClientSideStatementPgBeginExecutor(ClientSideStatementImpl statement) throws CompileException {
try {
this.statement = statement;
this.converter = new PgTransactionModeConverter();
this.method =
ConnectionStatementExecutor.class.getDeclaredMethod(
statement.getMethodName(), converter.getParameterClass());
} catch (Exception e) {
throw new CompileException(e, statement);
}
}

@Override
public StatementResult execute(ConnectionStatementExecutor connection, String sql)
throws Exception {
return (StatementResult) method.invoke(connection, getParameterValue(sql));
}

PgTransactionMode getParameterValue(String sql) {
Matcher matcher = statement.getPattern().matcher(sql);
if (matcher.find() && matcher.groupCount() >= 1) {
String value = matcher.group(1);
if (value != null) {
PgTransactionMode res = converter.convert(value.trim());
if (res != null) {
return res;
}
throw SpannerExceptionFactory.newSpannerException(
ErrorCode.INVALID_ARGUMENT, String.format("Unknown transaction mode: %s", value));
}
}
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.EnumSet;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.TimeUnit;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

Expand Down Expand Up @@ -121,6 +122,48 @@ public Duration convert(String value) {
}
}

/** Converter from string to {@link Duration}. */
static class PgDurationConverter implements ClientSideStatementValueConverter<Duration> {
private final Pattern allowedValues;

public PgDurationConverter(String allowedValues) {
// Remove the parentheses from the beginning and end.
this.allowedValues =
Pattern.compile(
"(?is)\\A" + allowedValues.substring(1, allowedValues.length() - 1) + "\\z");
}

@Override
public Class<Duration> getParameterClass() {
return Duration.class;
}

@Override
public Duration convert(String value) {
Matcher matcher = allowedValues.matcher(value);
if (matcher.find()) {
Duration duration;
if (matcher.group(0).equalsIgnoreCase("default")) {
return Durations.fromNanos(0L);
} else if (matcher.group(2) == null) {
duration =
ReadOnlyStalenessUtil.createDuration(
Long.parseLong(matcher.group(0)), TimeUnit.MILLISECONDS);
} else {
duration =
ReadOnlyStalenessUtil.createDuration(
Long.parseLong(matcher.group(1)),
ReadOnlyStalenessUtil.parseTimeUnit(matcher.group(2)));
}
if (duration.getSeconds() == 0L && duration.getNanos() == 0) {
return null;
}
return duration;
}
return null;
}
}

/** Converter from string to possible values for read only staleness ({@link TimestampBound}). */
static class ReadOnlyStalenessConverter
implements ClientSideStatementValueConverter<TimestampBound> {
Expand Down Expand Up @@ -243,6 +286,33 @@ public TransactionMode convert(String value) {
}
}

/**
* Converter for converting string values to {@link PgTransactionMode} values. Includes no-op
* handling of setting the isolation level of the transaction to default or serializable.
*/
static class PgTransactionModeConverter
implements ClientSideStatementValueConverter<PgTransactionMode> {
private final CaseInsensitiveEnumMap<PgTransactionMode> values =
new CaseInsensitiveEnumMap<>(
PgTransactionMode.class, PgTransactionMode::getStatementString);

PgTransactionModeConverter() {}

public PgTransactionModeConverter(String allowedValues) {}

@Override
public Class<PgTransactionMode> getParameterClass() {
return PgTransactionMode.class;
}

@Override
public PgTransactionMode convert(String value) {
// Transaction mode may contain multiple spaces.
String valueWithSingleSpaces = value.replaceAll("\\s+", " ");
return values.get(valueWithSingleSpaces);
}
}

/** Converter for converting strings to {@link RpcPriority} values. */
static class RpcPriorityConverter implements ClientSideStatementValueConverter<Priority> {
private final CaseInsensitiveEnumMap<Priority> values =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@

package com.google.cloud.spanner.connection;

import com.google.cloud.spanner.Dialect;
import com.google.cloud.spanner.ErrorCode;
import com.google.cloud.spanner.SpannerExceptionFactory;
import com.google.cloud.spanner.connection.ClientSideStatementImpl.CompileException;
import com.google.gson.Gson;
import java.io.InputStreamReader;
Expand All @@ -24,7 +27,21 @@
/** This class reads and parses the {@link ClientSideStatement}s from the json file. */
class ClientSideStatements {
private static final String STATEMENTS_DEFINITION_FILE = "ClientSideStatements.json";
static final ClientSideStatements INSTANCE = importStatements();
private static final String PG_STATEMENTS_DEFINITION_FILE = "PG_ClientSideStatements.json";
private static final ClientSideStatements INSTANCE = importStatements();
private static final ClientSideStatements PG_INSTANCE = pgImportStatements();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call these GSQL_INSTANCE_STATEMENTS and PG_INSTANCE_STATEMENTS?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that is better to separate them, but I think we can drop the INSTANCE part. So GSQL_STATEMENTS and PG_STATEMENTS.


static ClientSideStatements getInstance(Dialect dialect) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call this getInstanceClientSideStatements?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't feel that is right. getInstance is a very common method name in Java for getting a singleton instance.

switch (dialect) {
case GOOGLE_STANDARD_SQL:
return INSTANCE;
case POSTGRESQL:
return PG_INSTANCE;
default:
throw SpannerExceptionFactory.newSpannerException(
ErrorCode.INVALID_ARGUMENT, "Unknown or unsupported dialect: " + dialect);
}
}

/**
* Reads statement definitions from ClientSideStatements.json and parses these as Java objects.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename importStatements to gsqlImportStatements?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, but I would propose making it more natural for reading, so importPgStatements and importGsqlStatements

Expand All @@ -37,6 +54,18 @@ private static ClientSideStatements importStatements() {
ClientSideStatements.class);
}

/**
* Reads statement definitions from PG_ClientSideStatements.json and parses these as Java objects.
*/
private static ClientSideStatements pgImportStatements() {
Gson gson = new Gson();
return gson.fromJson(
new InputStreamReader(
ClientSideStatements.class.getResourceAsStream(PG_STATEMENTS_DEFINITION_FILE)),
ClientSideStatements.class);
}

// This field is set automatically by the importStatements / pgImportStatements methods.
private Set<ClientSideStatementImpl> statements;

private ClientSideStatements() {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,12 +85,19 @@ interface ConnectionStatementExecutor {

StatementResult statementBeginTransaction();

StatementResult statementBeginPgTransaction(PgTransactionMode transactionMode);

StatementResult statementCommit();

StatementResult statementRollback();

StatementResult statementSetTransactionMode(TransactionMode mode);

StatementResult statementSetPgTransactionMode(PgTransactionMode transactionMode);

StatementResult statementSetPgSessionCharacteristicsTransactionMode(
PgTransactionMode transactionMode);

StatementResult statementStartBatchDdl();

StatementResult statementStartBatchDml();
Expand Down
Loading