Codecov Report

Update StatementParser to include statement hints support for DML statements.

The Spanner JDBC driver would consider UPDATE statements that started with a statement hint as invalid statements. This change adds a check for statement hints at the beginning of a query, and accepts these as valid queries.

Fixes #1029 ☕️

Codecov Report

Merging #1030 (03e327a) into master (a2e5803) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1030      +/-   ##
============================================
+ Coverage     85.10%   85.21%   +0.10%     
- Complexity     2623     2644      +21     
============================================
  Files           155      155              
  Lines         14374    14418      +44     
  Branches       1340     1348       +8     
============================================
+ Hits          12233    12286      +53     
+ Misses         1573     1564       -9     
  Partials        568      568

Impacted Files	Coverage Δ	Complexity Δ
...ogle/cloud/spanner/connection/StatementParser.java	`87.50% <100.00%> (+0.33%)`	`52.00 <2.00> (+1.00)`
...a/com/google/cloud/spanner/SessionPoolOptions.java	`69.53% <0.00%> (ø)`	`18.00% <0.00%> (+1.00%)`
...om/google/cloud/spanner/TransactionRunnerImpl.java	`86.24% <0.00%> (+0.17%)`	`10.00% <0.00%> (ø%)`
...ain/java/com/google/cloud/spanner/SessionImpl.java	`86.90% <0.00%> (+0.23%)`	`31.00% <0.00%> (+1.00%)`
.../com/google/cloud/spanner/AbstractReadContext.java	`86.82% <0.00%> (+0.28%)`	`44.00% <0.00%> (+2.00%)`
...ain/java/com/google/cloud/spanner/SessionPool.java	`89.32% <0.00%> (+0.38%)`	`73.00% <0.00%> (+2.00%)`
...oogle/cloud/spanner/PartitionedDmlTransaction.java	`82.60% <0.00%> (+0.58%)`	`15.00% <0.00%> (+1.00%)`
...rc/main/java/com/google/cloud/spanner/Options.java	`92.25% <0.00%> (+4.10%)`	`85.00% <0.00%> (+11.00%)`
.../google/cloud/spanner/AbstractLazyInitializer.java	`100.00% <0.00%> (+7.14%)`	`5.00% <0.00%> (+1.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2e5803...03e327a. Read the comment docs.

anantdamle · 2021-03-31T10:19:00Z

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

@@ -460,6 +464,12 @@ static String removeStatementHint(String sql) {
 startQueryIndex = upperCaseSql.indexOf(keyword);
 if (startQueryIndex > -1) break;
 }
+if (startQueryIndex <= -1) {


instead of this if statement
you can probably make changes in the previous for-loop as follows:
import com.google.common.collect.Sets; ... Set<String> selectAndDmlStatements = Sets.union(selectStatements, dmlStatements).immutableCopy(); for (String keyword : selectAndDmlStatements) {

Please apply this suggestion, as there is no need to do this in two separate loops. Also, please update the comment on line 460 to reflect the fact that statement hints are also supported for DML statements.

Updated both the for-loop and comment.

@anantdamle

Thanks for the contribution and noticing this missing feature. This looks mostly good. PTAL at the comment given by @anantdamle. Also update the PR title to be feat: Support query hints for DML statements to comply with the conventional commits standard.

olavloite · 2021-03-31T11:30:15Z

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

@@ -460,6 +464,12 @@ static String removeStatementHint(String sql) {
 startQueryIndex = upperCaseSql.indexOf(keyword);
 if (startQueryIndex > -1) break;
 }
+if (startQueryIndex <= -1) {


Please apply this suggestion, as there is no need to do this in two separate loops. Also, please update the comment on line 460 to reflect the fact that statement hints are also supported for DML statements.

olavloite · 2021-03-31T11:31:50Z

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

@@ -460,6 +464,12 @@ static String removeStatementHint(String sql) {
 startQueryIndex = upperCaseSql.indexOf(keyword);
 if (startQueryIndex > -1) break;


nit: I know this was not changed in this PR, but please apply the following as well, as the Google style guide dictates that all if statements should always use curly braces.
Suggested change
if (startQueryIndex > -1) break;
if (startQueryIndex > -1) {
break;
}

anantdamle · 2021-04-01T05:15:25Z

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

 int startQueryIndex = -1;
 String upperCaseSql = sql.toUpperCase();
-for (String keyword : selectStatements) {
+Set<String> selectAndDmlStatements =
+Sets.union(selectStatements, dmlStatements).immutableCopy();


if style allows, consider moving the declaration and assignment on a single line.

The line was split by the formatter so I guess this is what is needed to be style-conformant.

LGTM

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help..com/en//administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

…lugin to v3.4.1 (googleapis#1030) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [org.apache.maven.plugins:maven-shade-plugin](https://maven.apache.org/plugins/) | `3.4.0` -> `3.4.1` | [![age](https://badges.renovateapi.com/packages/maven/org.apache.maven.plugins:maven-shade-plugin/3.4.1/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/org.apache.maven.plugins:maven-shade-plugin/3.4.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/org.apache.maven.plugins:maven-shade-plugin/3.4.1/compatibility-slim/3.4.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/org.apache.maven.plugins:maven-shade-plugin/3.4.1/confidence-slim/3.4.0)](https://docs.renovatebot.com/merge-confidence/) | --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dasard#/googleapis/java-spanner-jdbc).

ktchana added 2 commits March 31, 2021 17:30

spanner-jdbc: Support query hints for DML statements

8875eb1

format code for query hints for DML statements change

bc7e092

ktchana requested a review from a team as a code owner March 31, 2021 09:48

product-auto-label bot added the api: spannerIssues related to the googleapis/java-spanner API.label Mar 31, 2021

google-cla bot added the cla: yesThis human has signed the Contributor License Agreement.label Mar 31, 2021

anantdamle reviewed Mar 31, 2021
View reviewed changes

olavloite reviewed Mar 31, 2021
View reviewed changes

feat: Support query hints for DML statements

03e327a

ktchana changed the title ~~spanner-jdbc: Support query hints for DML statements~~ feat: Support query hints for DML statements Apr 1, 2021

anantdamle reviewed Apr 1, 2021
View reviewed changes

olavloite approved these changes Apr 1, 2021
View reviewed changes

olavloite added the automergeMerge the pull request once unit tests and other checks pass.label Apr 1, 2021

gcf-merge-on-green bot removed the automergeMerge the pull request once unit tests and other checks pass.label Apr 1, 2021

olavloite added the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 1, 2021

yoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 1, 2021

olavloite added the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 6, 2021

yoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 6, 2021

olavloite merged commit 6a58433 into googleapis:master Apr 6, 2021

Uh oh!

Uh oh!

Uh oh!

anantdamle Mar 31, 2021•
edited
Loading

Uh oh!

olavloite Mar 31, 2021

Uh oh!

ktchana Apr 1, 2021

Uh oh!

Uh oh!

olavloite Mar 31, 2021

Uh oh!

olavloite Mar 31, 2021

Uh oh!

ktchana Apr 1, 2021

Uh oh!

anantdamle Apr 1, 2021

Uh oh!

ktchana Apr 1, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		@@ -460,6 +464,12 @@ static String removeStatementHint(String sql) {
		startQueryIndex = upperCaseSql.indexOf(keyword);
		if (startQueryIndex > -1) break;

Uh oh!

Conversation

Uh oh!

Codecov Report

Uh oh!

anantdamle Mar 31, 2021•edited LoadingUh oh!There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olavloite Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

ktchana Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olavloite Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

olavloite Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

ktchana Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

anantdamle Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

ktchana Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

anantdamle Mar 31, 2021•
edited
Loading