feat: changes for supporting DML with Returning by rajatbhatta · Pull Request #1914 · googleapis/java-spanner

rajatbhatta · 2022-06-10T07:09:52Z

call executeQuery for both Query and Update
statements.
allow executeQuery to execute all Update
statements.

- call executeQuery for both Query and Update statements. - allow executeQuery to execute all Update statements.

SingleUseTransaction

ansh0l · 2022-07-04T17:33:12Z

@rajatbhatta : Is this PR still up to date, if not,should we close it?

olavloite · 2022-07-19T06:25:22Z

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

+            options);
+      case QUERY:
+      case UPDATE:
+        return internalExecuteQueryAsync(parsedStatement, analyzeMode, options);


I think this also needs checks to verify that the result of the statement actually contains results and not just an update count.

olavloite · 2022-07-19T06:27:59Z

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

+              "Statement is not an normal DML statement: "
+                  + parsedStatement.getSqlWithoutComments());
+        }
+        while (rs.next()) ;


We should add a comment to explain why we are doing this. Also, according to the Google style guide, a while statement must have curly braces.

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

olavloite · 2022-07-19T06:35:52Z

...le-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SingleUseTransaction.java

+    return executeStatementAsync(
+        update,
+        callable,
+        ImmutableList.of(SpannerGrpc.getExecuteSqlMethod(), SpannerGrpc.getCommitMethod()));


Hmm.... This is going to be a little bit of a difficult decision:

executeUpdate used to use the executeSql method, while executeQuery used the executeStreamingSql. Now all statements will use executeStreamingSql.

Users can set custom timeout and retry settings per RPC. This meant that they previously could in theory set a different timeout value for queries and DML statements. That is no longer possible, as they use the same RPC.

So users who currently set a timeout value for ExecuteSql will now have to set a timeout value for ExecuteStreamingSql.

It also means that we need to replace all references to SpannerGrpc.getExecuteSqlMethod() with SpannerGrpc.getExecuteStreamingSqlMethod().

Replaced all references of SpannerGrpc.getExecuteSqlMethod() with SpannerGrpc.getExecuteStreamingSqlMethod().

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

olavloite · 2022-07-19T06:36:58Z

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

-    String instanceProperty = System.getProperty(TEST_INSTANCE_PROPERTY, "");
+    // String instanceProperty = System.getProperty(TEST_INSTANCE_PROPERTY, "");
+    String instanceProperty = "projects/span-cloud-testing/instances/rajatrb-test-instance";


Here also: remove commented code

olavloite · 2022-07-19T06:39:44Z

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

-                  .asRuntimeException();
+              // Check if result containing UPDATE_COUNT exists. If not, throw an exception.
+              res = getResult(spannerStatement, StatementResult.StatementResultType.UPDATE_COUNT);
+              if (res == null) {


Does this mean that we don't support RETURNING for batch DML?

olavloite · 2022-07-19T06:45:27Z

...-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionAsyncApiTest.java

  }

-  @Test
-  public void testExecuteInvalidQueryAsync() {


Why is this test case removed? If I understand it correctly, this would execute a DML statement without a returning clause, which means that it should still fail.

Reinstated the test with small change in when the exception will be raised.

olavloite · 2022-07-19T06:50:20Z

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

-        default:
-      }
+        }
+        ResultSet rs = internalExecuteQuery(parsedStatement, AnalyzeMode.NONE);


What happens if this is called while the connection is in read-only mode?

What happens if this is called while a read-only transaction is active?

Have added tests for these scenarios.

olavloite · 2022-07-19T06:51:42Z

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

-      }
+    switch (parsedStatement.getType()) {
+      case UPDATE:
+        return internalExecuteUpdateAsync(parsedStatement);


What happens if this is called with a DML statement with a RETURNING clause?

…nnection/ConnectionImpl.java Co-authored-by: Knut Olav Løite <[email protected]>

rajatbhatta · 2022-08-22T18:44:37Z

Closing this PR in favour of #1978.

feat: changes for supporting DML with Returning

0d1a6f3

- call executeQuery for both Query and Update statements. - allow executeQuery to execute all Update statements.

product-auto-label bot added size: s Pull request size is small. api: spanner Issues related to the googleapis/java-spanner API. labels Jun 10, 2022

feat: call ReadWriteTransaction for DML Returning in

67e49c3

SingleUseTransaction

product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Jun 10, 2022

feat: code changes for returning feature

781c4d2

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Jul 18, 2022

rajatbhatta and others added 5 commits July 18, 2022 09:18

Merge branch 'googleapis:main' into dml-returning-changes

d3c3afc

feat: changes to incorporate retries during aborts

0e58080

fix: linting

c5dcebc

feat: revert changes in env

23b3433

fix: testExecute test to include QUERY

bb391cb

olavloite reviewed Jul 19, 2022

View reviewed changes

rajatbhatta and others added 6 commits July 25, 2022 06:22

feat: add async connection api logic, tests

e54e4ca

Update google-cloud-spanner/src/main/java/com/google/cloud/spanner/co…

0db1f13

…nnection/ConnectionImpl.java Co-authored-by: Knut Olav Løite <[email protected]>

feat: changes for tests

6755510

feat: changes

49a13af

feat: changes

a1c884e

feat: changes

4440b69

rajatbhatta closed this Aug 22, 2022

rajatbhatta deleted the dml-returning-changes branch August 22, 2022 18:44

Conversation

rajatbhatta commented Jun 10, 2022

Uh oh!

ansh0l commented Jul 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajatbhatta Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajatbhatta commented Aug 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rajatbhatta Aug 2, 2022 •

edited

Loading