Statement parameter binding #87

dmitry-s · 2019-05-30T21:00:18Z

fixes #13

…-r2dbc into codecs-refactoring # Conflicts: # src/test/java/com/google/cloud/spanner/r2dbc/codecs/DefaultCodecsNegativeTest.java

…-r2dbc into codecs-refactoring

…-r2dbc into codecs-refactoring # Conflicts: # src/test/java/com/google/cloud/spanner/r2dbc/codecs/DefaultCodecsNegativeTest.java

…-r2dbc into codecs-refactoring # Conflicts: # src/main/java/com/google/cloud/spanner/r2dbc/SpannerStatement.java # src/main/java/com/google/cloud/spanner/r2dbc/client/Client.java # src/main/java/com/google/cloud/spanner/r2dbc/client/GrpcClient.java

…-r2dbc into codecs-refactoring # Conflicts: # src/main/java/com/google/cloud/spanner/r2dbc/client/GrpcClient.java # src/test/java/com/google/cloud/spanner/r2dbc/it/SpannerIT.java

src/test/java/com/google/cloud/spanner/r2dbc/it/SpannerIT.java

elefeint

Looks good; I would just rearrange a bit what information we are accumulating and how.

elefeint · 2019-05-31T16:25:38Z

src/main/java/com/google/cloud/spanner/r2dbc/SpannerStatement.java

+  public Statement bind(Object identifier, Object value) {
+    requireNonNull(identifier);
+    if (identifier instanceof String) {
+      this.bindings.getLast().put((String)identifier, value);


LinkedList.getLast() is efficient, but if we keep a map of in-progress/incomplete bindings separately, then there is no need for getting it, and also no need to add an empty list in constructor.
Then add() would add the incomplete map (that has just become complete) and get a new map going.

I guess the only thing here would be that after completing the last one, you'd have to call add() just to put it into the completed list, or deal with merging completed and incomplete somewhere else.

Yea I think that might complicate things. Especially since the user doesn't need to call add() to close/finalize the final one. That would mean some sort of code in the execute() that would add on this final map, which feels out of place to be in in the execute() method.

elefeint · 2019-05-31T16:30:42Z

src/main/java/com/google/cloud/spanner/r2dbc/SpannerStatement.java

+
+    Flux<Struct> structFlux = Flux.fromIterable(paramsStructs);
+
+    if (this.sql != null && this.sql.trim().toLowerCase().startsWith("select")) {


Could you add assertion for sql being non-null to the constructor? A statement with null sql would not be valid, so we should fail early on that.

elefeint · 2019-05-31T16:31:43Z

src/main/java/com/google/cloud/spanner/r2dbc/SpannerStatement.java

+
+    Flux<Struct> structFlux = Flux.fromIterable(paramsStructs);
+
+    if (this.sql != null && this.sql.trim().toLowerCase().startsWith("select")) {


I know it's trivial logic, but can the second half of the condition go into a isSelectQuery helper method?

src/main/java/com/google/cloud/spanner/r2dbc/SpannerStatement.java

elefeint · 2019-05-31T16:40:42Z

src/main/java/com/google/cloud/spanner/r2dbc/SpannerStatement.java

+      Builder paramsStructBuilder = Struct.newBuilder();
+      Map<String, Type> types = this.types.isEmpty() ? new HashMap<>() : null;
+
+      for (Map.Entry<String, Object> binding : bindingsBatch.entrySet()) {


Would it make sense to put the logic in a helper method and call it from .add()? We are building up a map, might as well be building up a list of Structs.

Yeah I would recommend this approach too. You could keep a List<Struct.Builder> instead of Map which will save a layer of conversions; then just build them all when you are ready to execute the query.

elefeint · 2019-05-31T16:41:44Z

src/main/java/com/google/cloud/spanner/r2dbc/SpannerStatement.java

+
+      for (Map.Entry<String, Object> binding : bindingsBatch.entrySet()) {
+        String paramName = binding.getKey();
+        Codec codec = this.resolvedCodecs.computeIfAbsent(paramName,


The same binding for all rows should be of the same type, right? Can we save time by only doing this on the first add()?

How do we know that all rows will use the same type for a particular column? Is that the contract in R2DBC?
Frankly, I would try to avoid this resolvedCodecs caching altogether. If anything, Codecs itself can cache based on type instead. There doesn't seem to be an advantage to caching here vs in the Codecs class.

These aren't actually columns if i'm not mistaken. These are param tags in the query string, which means they don't need to be the same type. For example , you can get the size in bytes in googleSQL of a STRING or BYTES, so that single tag could take on either value.

Furthermore, I think the codec can change too? For example, if the tag in the SQL is like where 100 < @tag then could two different codecs convert either a long or a string into a number to fill that tag?

dzou · 2019-05-31T19:41:37Z

src/main/java/com/google/cloud/spanner/r2dbc/SpannerStatement.java


  private String sql;

+  private LinkedList<Map<String, Object>> bindings = new LinkedList<>();


I think in practice ArrayList is typically preferred over LinkedList because of caching advantages.

dzou · 2019-05-31T19:43:11Z

src/main/java/com/google/cloud/spanner/r2dbc/SpannerStatement.java

  }

  @Override
  public Publisher<? extends Result> execute() {
+    List<Struct> paramsStructs = new ArrayList<>();
+    for (Map<String, Object> bindingsBatch : this.bindings) {
+      Builder paramsStructBuilder = Struct.newBuilder();


minor style thing - it would be better to qualify this Builder class with Struct.Builder

dzou · 2019-05-31T19:50:04Z

src/main/java/com/google/cloud/spanner/r2dbc/client/GrpcClient.java


-      if (transaction != null) {
+      if (transactionContext != null && transactionContext.getTransaction() != null) {


I think you should just add a Assert.notNull(transaction) in the constructor of TransactionContext, and then simplify this to just check transactionContext != null. This way users in the future will not have to worry about doing 2 null-checks for transactions.

meltsufin · 2019-05-31T19:16:02Z

src/main/java/com/google/cloud/spanner/r2dbc/SpannerConnection.java

@@ -100,7 +100,8 @@ public Batch createBatch() {

  @Override
  public Statement createStatement(String sql) {
-    return new SpannerStatement(this.client, this.session, this.currentTransaction, sql);
+    return new SpannerStatement(this.client, this.session,
+        SpannerTransactionContext.from(this.currentTransaction), sql);


Why do SpannerTransactionContext.from here rather than in the SpannerStatement?
Or, why not replace the currentTransaction in SpannerConnection with SpannerTransactionContext?

meltsufin · 2019-05-31T19:28:07Z

src/main/java/com/google/cloud/spanner/r2dbc/SpannerStatement.java

+  public Statement bind(Object identifier, Object value) {
+    requireNonNull(identifier);
+    if (identifier instanceof String) {
+      this.bindings.getLast().put((String)identifier, value);


I guess the only thing here would be that after completing the last one, you'd have to call add() just to put it into the completed list, or deal with merging completed and incomplete somewhere else.

meltsufin · 2019-05-31T19:29:39Z

src/main/java/com/google/cloud/spanner/r2dbc/SpannerStatement.java

  }

  @Override
  public Statement bind(int i, Object o) {
-    return null;
+    throw new IllegalArgumentException("Only named parameters are supported");


Do we have an issue to track support for index based binding?
I think UnsupportedOperationException is more appropriate here.

I thought we decided that we don't want to support that, because it would require parsing of the SQL string.

Either way, let's track this as an issue.

meltsufin · 2019-05-31T19:30:32Z

src/main/java/com/google/cloud/spanner/r2dbc/SpannerStatement.java

  }

  @Override
  public Statement bindNull(int i, Class<?> type) {
-    return null;
+    throw new IllegalArgumentException("Only named parameters are supported");


Ditto UnsupportedOperationException.

meltsufin · 2019-05-31T19:50:40Z

src/main/java/com/google/cloud/spanner/r2dbc/SpannerStatement.java

+
+      for (Map.Entry<String, Object> binding : bindingsBatch.entrySet()) {
+        String paramName = binding.getKey();
+        Codec codec = this.resolvedCodecs.computeIfAbsent(paramName,


How do we know that all rows will use the same type for a particular column? Is that the contract in R2DBC?
Frankly, I would try to avoid this resolvedCodecs caching altogether. If anything, Codecs itself can cache based on type instead. There doesn't seem to be an advantage to caching here vs in the Codecs class.

meltsufin · 2019-05-31T19:53:01Z

src/main/java/com/google/cloud/spanner/r2dbc/SpannerStatement.java

  }

  @Override
  public Publisher<? extends Result> execute() {
+    List<Struct> paramsStructs = new ArrayList<>();


Is there anything wrong with building the Structs in the bind immediately, rather than first keeping them as maps and then converting to structs here?

meltsufin · 2019-05-31T20:03:09Z

src/test/java/com/google/cloud/spanner/r2dbc/SpannerStatementTest.java

+  }
+
+  @Test
+  public void executeDummyImplementationBind() {


Can you add some comments in this method to separate blocks that are setting up the dummy values and mocks vs. where the actual method under test is executed? These reactive tests are a pain to read. Hopefully, some comments would help.

meltsufin · 2019-05-31T20:04:17Z

src/test/java/com/google/cloud/spanner/r2dbc/it/SpannerIT.java

+
+    DatabaseAdminClient dbAdminClient = spanner.getDatabaseAdminClient();
+
+    dbAdminClient.updateDatabaseDdl(


This is not exactly setting up the stubs. :)
Rename the method and/or put this in a separate method?

elefeint

Excellent, this unblocks the remainder of integration testing.

elefeint · 2019-06-03T20:16:37Z

src/main/java/com/google/cloud/spanner/r2dbc/SpannerStatement.java

    return structFlux.concatMapDelayError(this::runSingleStatement);
  }

+  private boolean isSelectQuery() {
+    return this.sql.trim().toLowerCase().startsWith("select");


optimization for the future: only lowercasing the prefix. Does not need to be in this PR.

elefeint · 2019-06-03T20:18:25Z

Integration test is unhappy tho:

SEVERE: *~*~*~ Channel ManagedChannelImpl{logId=41, target=spanner.googleapis.com:443} was not shutdown properly!!! ~*~*~*
    Make sure to call shutdown()/shutdownNow() and wait until awaitTermination() returns true.
java.lang.RuntimeException: ManagedChannel allocation site

dzou · 2019-06-03T20:22:54Z

src/main/java/com/google/cloud/spanner/r2dbc/SpannerStatement.java

+      }
+      return this;
+    }
+    throw new IllegalArgumentException("Only String identifiers are supported");


You could make this into a helper in the Assert helper; i.e. Assert.requireInstanceOf(..)

meltsufin

Please update to use StepVerifier in tests.

src/main/java/com/google/cloud/spanner/r2dbc/SpannerStatement.java

…-r2dbc into codecs-refactoring # Conflicts: # src/test/java/com/google/cloud/spanner/r2dbc/SpannerStatementTest.java

elefeint

Let's get it in, and deal with the integration test separately.

dmitry-s added 9 commits May 15, 2019 17:17

refactoring codec classes

c5a4315

Merge branch 'master' of github.com:GoogleCloudPlatform/cloud-spanner…

cb64255

…-r2dbc into codecs-refactoring # Conflicts: # src/test/java/com/google/cloud/spanner/r2dbc/codecs/DefaultCodecsNegativeTest.java

Merge branch 'master' of github.com:GoogleCloudPlatform/cloud-spanner…

799b719

…-r2dbc into codecs-refactoring # Conflicts: # src/test/java/com/google/cloud/spanner/r2dbc/codecs/DefaultCodecsNegativeTest.java

Merge branch 'master' of github.com:GoogleCloudPlatform/cloud-spanner…

30eb940

…-r2dbc into codecs-refactoring

Merge branch 'master' of github.com:GoogleCloudPlatform/cloud-spanner…

b09ff89

…-r2dbc into codecs-refactoring # Conflicts: # src/test/java/com/google/cloud/spanner/r2dbc/codecs/DefaultCodecsNegativeTest.java

bind support

90881d8

remove unused field

145b282

Merge branch 'master' of github.com:GoogleCloudPlatform/cloud-spanner…

3baaaa0

…-r2dbc into codecs-refactoring # Conflicts: # src/main/java/com/google/cloud/spanner/r2dbc/client/GrpcClient.java # src/test/java/com/google/cloud/spanner/r2dbc/it/SpannerIT.java

dmitry-s requested review from elefeint, dzou and meltsufin and removed request for dzou May 30, 2019 21:00

ChengyuanZhao changed the title ~~Codecs refactoring~~ Statement parameter binding May 31, 2019

ChengyuanZhao reviewed May 31, 2019

View reviewed changes

src/test/java/com/google/cloud/spanner/r2dbc/it/SpannerIT.java Show resolved Hide resolved

elefeint suggested changes May 31, 2019

View reviewed changes

automatically create a table for CI

c8db22d

dzou reviewed May 31, 2019

View reviewed changes

meltsufin requested changes May 31, 2019

View reviewed changes

elefeint mentioned this pull request Jun 3, 2019

Natively support multiple DML statements #92

Closed

addressing PR comments

d890864

elefeint previously approved these changes Jun 3, 2019

View reviewed changes

dzou reviewed Jun 3, 2019

View reviewed changes

meltsufin reviewed Jun 3, 2019

View reviewed changes

meltsufin reviewed Jun 4, 2019

View reviewed changes

dmitry-s added 2 commits June 4, 2019 12:28

addressing PR comments

5e57ba0

Merge branch 'master' of github.com:GoogleCloudPlatform/cloud-spanner…

74ade6a

…-r2dbc into codecs-refactoring # Conflicts: # src/test/java/com/google/cloud/spanner/r2dbc/SpannerStatementTest.java

dmitry-s dismissed elefeint’s stale review via 74ade6a June 4, 2019 18:55

ChengyuanZhao previously approved these changes Jun 4, 2019

View reviewed changes

elefeint previously approved these changes Jun 4, 2019

View reviewed changes

dzou previously approved these changes Jun 4, 2019

View reviewed changes

fix tests

d551ba9

dmitry-s dismissed stale reviews from dzou, elefeint, and ChengyuanZhao via d551ba9 June 4, 2019 20:15

dzou previously approved these changes Jun 4, 2019

View reviewed changes

Apply suggestions from code review

32a429d

meltsufin dismissed stale reviews from dzou via 32a429d June 4, 2019 20:26

meltsufin approved these changes Jun 4, 2019

View reviewed changes

dmitry-s merged commit 43b1284 into master Jun 4, 2019

dmitry-s deleted the codecs-refactoring branch June 4, 2019 21:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Statement parameter binding #87

Statement parameter binding #87

dmitry-s commented May 30, 2019

elefeint left a comment

elefeint May 31, 2019

meltsufin May 31, 2019

ChengyuanZhao Jun 3, 2019

elefeint May 31, 2019

elefeint May 31, 2019

elefeint May 31, 2019

dzou May 31, 2019

elefeint May 31, 2019

meltsufin May 31, 2019

ChengyuanZhao Jun 3, 2019

dzou May 31, 2019

dzou May 31, 2019

dzou May 31, 2019

meltsufin May 31, 2019

meltsufin May 31, 2019

meltsufin May 31, 2019

dmitry-s May 31, 2019

meltsufin May 31, 2019

ChengyuanZhao Jun 3, 2019

meltsufin May 31, 2019

meltsufin May 31, 2019

meltsufin May 31, 2019

meltsufin May 31, 2019

meltsufin May 31, 2019

elefeint left a comment

elefeint Jun 3, 2019

elefeint commented Jun 3, 2019

dzou Jun 3, 2019

meltsufin left a comment

elefeint left a comment


		Flux<Struct> structFlux = Flux.fromIterable(paramsStructs);

		if (this.sql != null && this.sql.trim().toLowerCase().startsWith("select")) {


		private String sql;

		private LinkedList<Map<String, Object>> bindings = new LinkedList<>();


		if (transaction != null) {
		if (transactionContext != null && transactionContext.getTransaction() != null) {


		DatabaseAdminClient dbAdminClient = spanner.getDatabaseAdminClient();

		dbAdminClient.updateDatabaseDdl(

Statement parameter binding #87

Statement parameter binding #87

Conversation

dmitry-s commented May 30, 2019

elefeint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elefeint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elefeint commented Jun 3, 2019

Choose a reason for hiding this comment

meltsufin left a comment

Choose a reason for hiding this comment

elefeint left a comment

Choose a reason for hiding this comment