Skip to content
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: Support query hints for DML statements #1030

Merged
merged 3 commits into from Apr 6, 2021

Conversation

ktchana
Copy link
Contributor

@ktchana ktchana commented Mar 31, 2021

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 ☕️

@ktchana ktchana requested a review from as a code owner Mar 31, 2021
@product-auto-label product-auto-label bot added the api: spanner label Mar 31, 2021
@google-cla google-cla bot added the cla: yes label Mar 31, 2021
@codecov
Copy link

@codecov codecov bot commented Mar 31, 2021

Codecov Report

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

Impacted file tree graph

@@             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.

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

@anantdamle anantdamle Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Contributor

@olavloite olavloite Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ktchana ktchana Apr 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated both the for-loop and comment.

Copy link
Contributor

@olavloite olavloite left a comment

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.

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

@olavloite olavloite Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@olavloite olavloite Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ktchana ktchana Apr 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ktchana ktchana changed the title spanner-jdbc: Support query hints for DML statements feat: Support query hints for DML statements Apr 1, 2021
int startQueryIndex = -1;
String upperCaseSql = sql.toUpperCase();
for (String keyword : selectStatements) {
Set<String> selectAndDmlStatements =
Sets.union(selectStatements, dmlStatements).immutableCopy();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ktchana ktchana Apr 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@olavloite olavloite left a comment

LGTM

@olavloite olavloite added the automerge label Apr 1, 2021
@gcf-merge-on-green
Copy link
Contributor

@gcf-merge-on-green gcf-merge-on-green bot commented Apr 1, 2021

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.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge label Apr 1, 2021
@olavloite olavloite added the kokoro:force-run label Apr 1, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Apr 1, 2021
@olavloite olavloite added the kokoro:force-run label Apr 6, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Apr 6, 2021
@olavloite olavloite merged commit 6a58433 into googleapis:master Apr 6, 2021
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants