Skip to content

Commit

Permalink
fix: code review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@olavloite
olavloite committed Mar 12, 2020
1 parent 7e6b799 commit d27b3a7
Show file tree
Hide file tree
Showing 5 changed files with 87 additions and 35 deletions.
15 changes: 0 additions & 15 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,13 +177,6 @@ public static interface SpannerEnvironment {
*/
@Nonnull
String getOptimizerVersion();

/**
* The optimizer statistics package to use. Must return an empty string to indicate that no
* value has been set.
*/
@Nonnull
String getOptimizerStatisticsPackage();
}

/**
Expand All @@ -193,21 +186,13 @@ public static interface SpannerEnvironment {
private static class SpannerEnvironmentImpl implements SpannerEnvironment {
private static final SpannerEnvironmentImpl INSTANCE = new SpannerEnvironmentImpl();
private static final String SPANNER_OPTIMIZER_VERSION_ENV_VAR = "SPANNER_OPTIMIZER_VERSION";
private static final String SPANNER_OPTIMIZER_STATISTICS_PACKAGE_ENV_VAR =
"SPANNER_OPTIMIZER_STATISTICS_PACKAGE";

private SpannerEnvironmentImpl() {}

@Override
public String getOptimizerVersion() {
return MoreObjects.firstNonNull(System.getenv(SPANNER_OPTIMIZER_VERSION_ENV_VAR), "");
}

@Override
public String getOptimizerStatisticsPackage() {
return MoreObjects.firstNonNull(
System.getenv(SPANNER_OPTIMIZER_STATISTICS_PACKAGE_ENV_VAR), "");
}
}

/** Builder for {@link SpannerOptions} instances. */
Expand Down
9 changes: 3 additions & 6 deletions google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -736,8 +736,7 @@ public void testBackendQueryOptions() {
// Just iterate over the results to execute the query.
while (rs.next()) {}
}
// Check that the last query was executed using a custom optimizer version and statistics
// package.
// Check that the last query was executed using a custom optimizer version.
List<AbstractMessage> requests = mockSpanner.getRequests();
assertThat(requests).isNotEmpty();
assertThat(requests.get(requests.size() - 1)).isInstanceOf(ExecuteSqlRequest.class);
Expand Down Expand Up @@ -776,8 +775,7 @@ public void testBackendQueryOptionsWithAnalyzeQuery() {
while (rs.next()) {}
}
}
// Check that the last query was executed using a custom optimizer version and statistics
// package.
// Check that the last query was executed using a custom optimizer version.
List<AbstractMessage> requests = mockSpanner.getRequests();
assertThat(requests).isNotEmpty();
assertThat(requests.get(requests.size() - 1)).isInstanceOf(ExecuteSqlRequest.class);
Expand Down Expand Up @@ -818,8 +816,7 @@ public void testBackendPartitionQueryOptions() {
// Just iterate over the results to execute the query.
while (rs.next()) {}
}
// Check that the last query was executed using a custom optimizer version and statistics
// package.
// Check that the last query was executed using a custom optimizer version.
List<AbstractMessage> requests = mockSpanner.getRequests();
assertThat(requests).isNotEmpty();
assertThat(requests.get(requests.size() - 1)).isInstanceOf(ExecuteSqlRequest.class);
Expand Down
6 changes: 3 additions & 3 deletions google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,16 +117,16 @@ QueryOptions getEnvironmentQueryOptions() {
}.build();

try (SpannerImpl implWithQueryOptions = new SpannerImpl(rpc, optionsWithQueryOptions);
SpannerImpl implWithouQueryOptions = new SpannerImpl(rpc, optionsWithoutQueryOptions)) {
SpannerImpl implWithoutQueryOptions = new SpannerImpl(rpc, optionsWithoutQueryOptions)) {

// Default query options are on a per-database basis, so we should only get the custom options
// for 'db' and not for 'otherDb'.
assertThat(implWithQueryOptions.getDefaultQueryOptions(db)).isEqualTo(queryOptions);
assertThat(implWithQueryOptions.getDefaultQueryOptions(otherDb)).isEqualTo(defaultOptions);

// The other Spanner instance should return default options for both databases.
assertThat(implWithouQueryOptions.getDefaultQueryOptions(db)).isEqualTo(defaultOptions);
assertThat(implWithouQueryOptions.getDefaultQueryOptions(otherDb)).isEqualTo(defaultOptions);
assertThat(implWithoutQueryOptions.getDefaultQueryOptions(db)).isEqualTo(defaultOptions);
assertThat(implWithoutQueryOptions.getDefaultQueryOptions(otherDb)).isEqualTo(defaultOptions);
}
}

Expand Down
10 changes: 0 additions & 10 deletions google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -437,11 +437,6 @@ public void testDefaultQueryOptions() {
public String getOptimizerVersion() {
return "";
}

@Override
public String getOptimizerStatisticsPackage() {
return "";
}
});
SpannerOptions options =
SpannerOptions.newBuilder()
Expand All @@ -461,11 +456,6 @@ public String getOptimizerStatisticsPackage() {
public String getOptimizerVersion() {
return "2";
}

@Override
public String getOptimizerStatisticsPackage() {
return "";
}
});
// Create options with '1' as the default query optimizer version. This should be overridden by
// the environment variable.
Expand Down
82 changes: 81 additions & 1 deletion google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITQueryOptionsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
import com.google.cloud.spanner.Spanner;
import com.google.cloud.spanner.SpannerException;
import com.google.cloud.spanner.Statement;
import com.google.cloud.spanner.TransactionContext;
import com.google.cloud.spanner.TransactionRunner.TransactionCallable;
import com.google.spanner.v1.ExecuteSqlRequest.QueryOptions;
import org.junit.BeforeClass;
import org.junit.ClassRule;
Expand All @@ -49,7 +51,9 @@ public class ITQueryOptionsTest {
@BeforeClass
public static void setUpDatabase() {
// Empty database.
db = env.getTestHelper().createTestDatabase();
db =
env.getTestHelper()
.createTestDatabase("CREATE TABLE TEST (ID INT64, NAME STRING(100)) PRIMARY KEY (ID)");
client = env.getTestHelper().getDatabaseClient(db);
}

Expand Down Expand Up @@ -98,6 +102,82 @@ public void executeQuery() {
}
}

@Test
public void executeUpdate() {
// Query optimizer version is ignored for DML statements by the backend, but setting it does not
// cause an error.
assertThat(
client
.readWriteTransaction()
.run(
new TransactionCallable<Long>() {
@Override
public Long run(TransactionContext transaction) throws Exception {
return transaction.executeUpdate(
Statement.newBuilder("INSERT INTO TEST (ID, NAME) VALUES (@id, @name)")
.bind("id")
.to(1L)
.bind("name")
.to("One")
.withQueryOptions(
QueryOptions.newBuilder().setOptimizerVersion("1").build())
.build());
}
}))
.isEqualTo(1L);

// Version 'latest' should also work.
assertThat(
client
.readWriteTransaction()
.run(
new TransactionCallable<Long>() {
@Override
public Long run(TransactionContext transaction) throws Exception {
return transaction.executeUpdate(
Statement.newBuilder("INSERT INTO TEST (ID, NAME) VALUES (@id, @name)")
.bind("id")
.to(2L)
.bind("name")
.to("Two")
.withQueryOptions(
QueryOptions.newBuilder().setOptimizerVersion("latest").build())
.build());
}
}))
.isEqualTo(1L);

// Version '100000' is an invalid value, but is ignored by the backend.
assertThat(
client
.readWriteTransaction()
.run(
new TransactionCallable<Long>() {
@Override
public Long run(TransactionContext transaction) throws Exception {
return transaction.executeUpdate(
Statement.newBuilder("INSERT INTO TEST (ID, NAME) VALUES (@id, @name)")
.bind("id")
.to(3L)
.bind("name")
.to("Three")
.withQueryOptions(
QueryOptions.newBuilder().setOptimizerVersion("10000").build())
.build());
}
}))
.isEqualTo(1L);

// Verify that query options are ignored with Partitioned DML as well, and that all the above
// DML INSERT statements succeeded.
assertThat(
client.executePartitionedUpdate(
Statement.newBuilder("UPDATE TEST SET NAME='updated' WHERE 1=1")
.withQueryOptions(QueryOptions.newBuilder().setOptimizerVersion("1").build())
.build()))
.isEqualTo(3L);
}

@Test
public void spannerOptions() {
// Version '1' should work.
Expand Down

0 comments on commit d27b3a7

Please sign in to comment.