fix: code review comments

Author: olavloite

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java

@@ -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();
  }
 
  /**
@@ -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. */

google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java

@@ -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);
@@ -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);
@@ -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);

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java

@@ -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);
  }
  }
 

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java

@@ -437,11 +437,6 @@ public void testDefaultQueryOptions() {
  public String getOptimizerVersion() {
  return "";
  }
-
- @Override
- public String getOptimizerStatisticsPackage() {
- return "";
- }
  });
  SpannerOptions options =
  SpannerOptions.newBuilder()
@@ -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.

google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITQueryOptionsTest.java

@@ -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;
@@ -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);
  }
 
@@ -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.