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: add support for describe statement #125

Merged
merged 23 commits into from
May 30, 2022

Conversation

olavloite
Copy link
Collaborator

@olavloite olavloite commented Apr 25, 2022

Adds support for DescribeStatement by transforming sql statements into a select statement that queries the (types of) the parameters in the original query. This can be used to temporarily support prepared statements until a DescribeStatement endpoint is available in the backend.

With this change the entire extended query protocol is supported by PGAdapter.

Adds support for DescribeStatement by transforming sql statements into a
select statement that queries the (types of) the parameters in the
original query. This can be used to temporarily support prepared
statements until a DescribeStatement endpoint is available in the
backend.
@olavloite olavloite marked this pull request as ready for review May 23, 2022 17:27
@olavloite olavloite requested a review from gauravsnj May 23, 2022 17:27
@olavloite olavloite changed the title feat: [WIP] add support for describe statement feat: add support for describe statement May 23, 2022
@codecov
Copy link

codecov bot commented May 23, 2022

Codecov Report

Merging #125 (a8bdee7) into postgresql-dialect (c15de51) will increase coverage by 0.87%.
The diff coverage is 83.06%.

@@                   Coverage Diff                    @@
##             postgresql-dialect     #125      +/-   ##
========================================================
+ Coverage                 76.64%   77.51%   +0.87%     
- Complexity                  866      953      +87     
========================================================
  Files                        84       85       +1     
  Lines                      3027     3260     +233     
  Branches                    320      380      +60     
========================================================
+ Hits                       2320     2527     +207     
- Misses                      570      579       +9     
- Partials                    137      154      +17     
Flag Coverage Δ
all_tests 77.51% <83.06%> (+0.87%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...google/cloud/spanner/pgadapter/parsers/Parser.java 73.62% <0.00%> (-5.20%) ⬇️
...d/spanner/pgadapter/parsers/UnspecifiedParser.java 41.66% <20.00%> (-8.34%) ⬇️
...panner/pgadapter/wireprotocol/DescribeMessage.java 80.43% <60.00%> (+6.01%) ⬆️
...pter/statements/IntermediatePreparedStatement.java 84.39% <83.33%> (+1.57%) ⬆️
...oud/spanner/pgadapter/statements/SimpleParser.java 93.67% <93.67%> (ø)
...dapter/statements/IntermediatePortalStatement.java 96.55% <100.00%> (ø)
...ud/spanner/pgadapter/wireprotocol/BindMessage.java 73.91% <100.00%> (ø)
...d/spanner/pgadapter/wireprotocol/ParseMessage.java 80.76% <100.00%> (ø)
...ud/spanner/pgadapter/statements/CopyStatement.java 70.89% <0.00%> (+0.74%) ⬆️
... and 3 more

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 c15de51...a8bdee7. Read the comment docs.

if (parser.eat("select")) {
// This is an `insert into <table> [(...)] select ...` statement. Then we can just use the
// select statement as the result.
return Statement.of(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make the Select statement method reusable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

}
}

private Statement transformSelectToSelectParams(Set<String> parameters) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add examples as comment? Same for other parsers

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

}

@VisibleForTesting
Statement transformToSelectParams(Set<String> parameters) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be made more simple by just fetching columns name and the respective table name? Because from what I understand the resultant query can be made from just those?
If yes, we can use regex to fetch the respective values. Which will be more readable and be less error prone.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, that is not possible, as the type of a parameter might be different from the type of the column that it is used to for example filter. Consider for example the following statement:

select *
from my_table
where created_at > to_timestamp($1, 'YYYY-MM-DD HH:MI')

The type of the column created_at is TIMESTAMP, but the type of the parameter is text.

@olavloite olavloite merged commit 52452d7 into postgresql-dialect May 30, 2022
@olavloite olavloite deleted the test-describe-statement branch May 30, 2022 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants