Skip to content

chore: include client statement type in parsed statement #1849

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

Merged
merged 5 commits into from
Apr 28, 2022

Conversation

olavloite
Copy link
Collaborator

Include the ClientSideStatementType in ParsedStatement. This enables
clients to directly check what type of statement it is, which removes
the need for textual checks.

This will for example remove these types of checks: https://github.com/GoogleCloudPlatform/pgadapter/blob/2a3f2eb7cd16cc3c71ffc673544ca9e2baaba21c/src/main/java/com/google/cloud/spanner/pgadapter/statements/IntermediateStatement.java#L354

Include the ClientSideStatementType in ParsedStatement. This enables
clients to directly check what type of statement it is, which removes
the need for textual checks.
@olavloite olavloite requested review from a team as code owners April 22, 2022 14:34
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: spanner Issues related to the googleapis/java-spanner API. labels Apr 22, 2022
@@ -5,6 +5,7 @@
"name": "SHOW VARIABLE AUTOCOMMIT",
"executorName": "ClientSideStatementNoParamExecutor",
"resultType": "RESULT_SET",
"statementType": "SHOW_AUTOCOMMIT",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we seem to have added extra indentation to all modified lines in this file, please check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm... That file needs a small cleanup. It uses both tabs AND spaces for indentation (first indentation is one tab, then second indentation is two spaces).
I've made this change consistent with the current format of the file. Let's do the cleanup in a separate PR.

Copy link
Contributor

@ansh0l ansh0l left a comment

Choose a reason for hiding this comment

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

LGTM with minor nit

@olavloite olavloite added the automerge Merge the pull request once unit tests and other checks pass. label Apr 28, 2022
@gcf-merge-on-green gcf-merge-on-green bot merged commit 6e1aa8e into main Apr 28, 2022
@gcf-merge-on-green gcf-merge-on-green bot deleted the include-client-statement-type branch April 28, 2022 14:20
@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 28, 2022
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. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants