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
feat: changes for supporting DML with Returning #1914
Conversation
- call executeQuery for both Query and Update statements. - allow executeQuery to execute all Update statements.
SingleUseTransaction
@rajatbhatta : Is this PR still up to date, if not,should we close it? |
options); | ||
case QUERY: | ||
case UPDATE: | ||
return internalExecuteQueryAsync(parsedStatement, analyzeMode, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this also needs checks to verify that the result of the statement actually contains results and not just an update count.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
"Statement is not an normal DML statement: " | ||
+ parsedStatement.getSqlWithoutComments()); | ||
} | ||
while (rs.next()) ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java
Outdated
Show resolved
Hide resolved
return executeStatementAsync( | ||
update, | ||
callable, | ||
ImmutableList.of(SpannerGrpc.getExecuteSqlMethod(), SpannerGrpc.getCommitMethod())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.... This is going to be a little bit of a difficult decision:
executeUpdate
used to use theexecuteSql
method, whileexecuteQuery
used theexecuteStreamingSql
. Now all statements will useexecuteStreamingSql
.- 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 forExecuteStreamingSql
.
It also means that we need to replace all references to SpannerGrpc.getExecuteSqlMethod()
with SpannerGrpc.getExecuteStreamingSqlMethod()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced all references of SpannerGrpc.getExecuteSqlMethod()
with SpannerGrpc.getExecuteStreamingSqlMethod()
.
google-cloud-spanner/src/test/java/com/google/cloud/spanner/AsyncResultSetImplTest.java
Outdated
Show resolved
Hide resolved
String instanceProperty = System.getProperty(TEST_INSTANCE_PROPERTY, ""); | ||
// String instanceProperty = System.getProperty(TEST_INSTANCE_PROPERTY, ""); | ||
String instanceProperty = "projects/span-cloud-testing/instances/rajatrb-test-instance"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here also: remove commented code
.asRuntimeException(); | ||
// Check if result containing UPDATE_COUNT exists. If not, throw an exception. | ||
res = getResult(spannerStatement, StatementResult.StatementResultType.UPDATE_COUNT); | ||
if (res == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that we don't support RETURNING
for batch DML?
@@ -390,18 +397,6 @@ public void testExecuteClientSideQueryAsync() { | |||
} | |||
} | |||
|
|||
@Test | |||
public void testExecuteInvalidQueryAsync() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reinstated the test with small change in when the exception will be raised.
default: | ||
} | ||
} | ||
ResultSet rs = internalExecuteQuery(parsedStatement, AnalyzeMode.NONE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added tests for these scenarios.
} | ||
switch (parsedStatement.getType()) { | ||
case UPDATE: | ||
return internalExecuteUpdateAsync(parsedStatement); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if this is called with a DML statement with a RETURNING
clause?
…nnection/ConnectionImpl.java Co-authored-by: Knut Olav Løite <koloite@gmail.com>
Closing this PR in favour of #1978. |
call executeQuery for both Query and Update
statements.
allow executeQuery to execute all Update
statements.