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 a team as a code owner March 31, 2021 09:48
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Mar 31, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 31, 2021
@codecov
Copy link

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

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
Collaborator

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

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
Collaborator

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

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
Collaborator

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
Collaborator

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

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

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
Collaborator

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

LGTM

@olavloite olavloite added the automerge Merge the pull request once unit tests and other checks pass. label Apr 1, 2021
@gcf-merge-on-green
Copy link
Contributor

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 Merge the pull request once unit tests and other checks pass. label Apr 1, 2021
@olavloite olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 1, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 1, 2021
@olavloite olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 6, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 6, 2021
@olavloite olavloite merged commit 6a58433 into googleapis:master Apr 6, 2021
rajatbhatta pushed a commit to rajatbhatta/java-spanner that referenced this pull request Nov 17, 2022
…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.

---

 - [ ] <!-- rebase-check -->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/dashboard#github/googleapis/java-spanner-jdbc).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yLjMiLCJ1cGRhdGVkSW5WZXIiOiIzNC42LjEifQ==-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/java-spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spanner-jdbc: Support query hints for DML statements
4 participants