chore: add namespace for client side statements in PostgreSQL dialect by olavloite · Pull Request #1737 · googleapis/java-spanner

olavloite · 2022-03-08T20:40:31Z

Adds PostgreSQL specific client side statements. These are added as a separate JSON file with client side statements to ensure that there are no interference between the statements supported by the two different parsers.

b/223320448

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

…er into spangres-parser-ga

...anner/src/main/java/com/google/cloud/spanner/connection/ConnectionStatementExecutorImpl.java

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

...-spanner/src/main/resources/com/google/cloud/spanner/connection/PG_ClientSideStatements.json

...ud-spanner/src/test/java/com/google/cloud/spanner/connection/AbstractConnectionImplTest.java

google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java

pom.xml

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

…er into spangres-parser-ga

...-spanner/src/main/resources/com/google/cloud/spanner/connection/PG_ClientSideStatements.json

thiagotnunes · 2022-03-15T03:26:06Z

...ner/src/test/java/com/google/cloud/spanner/connection/SetReadOnlyStalenessSqlScriptTest.java

+        return "postgresql/SetReadOnlyStalenessTest.sql";
+      case GOOGLE_STANDARD_SQL:
+      default:
+        return "SetReadOnlyStalenessTest.sql";


nit: should we have a googlesql directory? (nevermind if it is too much work)

I would like to defer that to a separate PR.

google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/StatementParserTest.java

...rc/test/resources/com/google/cloud/spanner/connection/postgresql/SetStatementTimeoutTest.sql

thiagotnunes

LGTM

ansh0l

LGTM. @olavloite : Sorry, got delayed in reviewing. I've added a few minor nits.

ansh0l · 2022-03-15T06:15:21Z

...le-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ClientSideStatements.java

-  static final ClientSideStatements INSTANCE = importStatements();
+  private static final String PG_STATEMENTS_DEFINITION_FILE = "PG_ClientSideStatements.json";
+  private static final ClientSideStatements INSTANCE = importStatements();
+  private static final ClientSideStatements PG_INSTANCE = pgImportStatements();


Should we call these GSQL_INSTANCE_STATEMENTS and PG_INSTANCE_STATEMENTS?

Yes, I think that is better to separate them, but I think we can drop the INSTANCE part. So GSQL_STATEMENTS and PG_STATEMENTS.

ansh0l · 2022-03-15T06:26:49Z

...le-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ClientSideStatements.java

+  private static final ClientSideStatements INSTANCE = importStatements();
+  private static final ClientSideStatements PG_INSTANCE = pgImportStatements();
+
+  static ClientSideStatements getInstance(Dialect dialect) {


Should we call this getInstanceClientSideStatements?

No, I don't feel that is right. getInstance is a very common method name in Java for getting a singleton instance.

ansh0l · 2022-03-15T06:29:01Z

...le-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ClientSideStatements.java

+  }

  /**
   * Reads statement definitions from ClientSideStatements.json and parses these as Java objects.


Should we rename importStatements to gsqlImportStatements?

Agree, but I would propose making it more natural for reading, so importPgStatements and importGsqlStatements

ansh0l · 2022-03-15T06:37:36Z

...anner/src/main/java/com/google/cloud/spanner/connection/ConnectionStatementExecutorImpl.java

 import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;

 /**


Just for my understanding, We seem to have made changes to all statementShow* methods here, except statementShowAutocommit. Is that because show variable autocommit would have an same result for both case (gsql/pg)?

ansh0l · 2022-03-15T06:47:14Z

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

+  READ_ONLY_TRANSACTION("READ ONLY"),
+  READ_WRITE_TRANSACTION("READ WRITE"),
+  ISOLATION_LEVEL_DEFAULT("ISOLATION LEVEL DEFAULT"),
+  ISOLATION_LEVEL_SERIALIZABLE("ISOLATION LEVEL SERIALIZABLE");


It seems that PG has 4 isolation levels, which one does DEFAULT map to? https://www.postgresql.org/docs/14/transaction-iso.html

Yes, this is an interesting one:

The default in PostgreSQL is read committed. That is not supported by Spangres.

Spangres only supports serializable. So in Spangres we default to that.

ansh0l · 2022-03-15T06:52:20Z

...loud-spanner/src/test/java/com/google/cloud/spanner/connection/ClientSideStatementsTest.java

              new OutputStreamWriter(
-                  new FileOutputStream(SCRIPT_FILE, false), StandardCharsets.UTF_8),
+                  new FileOutputStream(
+                      "src/test/resources/com/google/cloud/spanner/connection/"


Should we have this as SCRIPT_FILE_PATH?

I personally find it easier to read and see where it's going by having the string literal here, instead of having to look up the constant value somewhere else.

ansh0l · 2022-03-15T06:55:03Z

...c/test/java/com/google/cloud/spanner/connection/ConnectionImplTransactionalReadOnlyTest.java

                  .setUri(ConnectionImplTest.URI)
                  .build());
-      log("SET READONLY=TRUE;");
+      logWithNamespace("SET %sREADONLY=TRUE;");


I'm not able to find more details on logWithNamespace - it seems to have been introduced in this PR. Is it a junit log method? Can you point me to its documentation?

It's a simple helper method that we added ourselves in the PR here:

java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbstractConnectionImplTest.java

Line 144 in 4a341a2

void logWithNamespace(String statement) {

...rc/test/resources/com/google/cloud/spanner/connection/postgresql/SetStatementTimeoutTest.sql

olavloite added 3 commits March 8, 2022 07:05

chore: namespace Spanner statements for PostgreSQL dialect

0b6a66f

feat: add namespace for client side statements in PostgreSQL

9652a25

Merge branch 'main' into spangres-parser-ga

f3ad43e

product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Mar 8, 2022

olavloite changed the title ~~feat: add namespace for client side statements in PostgreSQL dialect~~ chore: add namespace for client side statements in PostgreSQL dialect Mar 8, 2022

gcf-owl-bot bot and others added 3 commits March 8, 2022 20:42

🦉 Updates from OwlBot post-processor

5d4668e

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

feat: add transaction statements

1362734

Merge branch 'spangres-parser-ga' of github.com:googleapis/java-spann…

592c125

…er into spangres-parser-ga

olavloite marked this pull request as ready for review March 9, 2022 11:42

olavloite requested review from a team, ansh0l and thiagotnunes March 9, 2022 11:42

thiagotnunes reviewed Mar 14, 2022

View reviewed changes

olavloite and others added 5 commits March 14, 2022 16:19

chore: address review comments

acef711

Merge branch 'main' into spangres-parser-ga

7e15741

🦉 Updates from OwlBot post-processor

3d25121

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

test: separate statement timeout tests for PG

04127e2

Merge branch 'spangres-parser-ga' of github.com:googleapis/java-spann…

7dbdf1c

…er into spangres-parser-ga

olavloite requested a review from thiagotnunes March 14, 2022 18:45

thiagotnunes reviewed Mar 15, 2022

View reviewed changes

chore: address review comments

ce0dda2

olavloite requested a review from thiagotnunes March 15, 2022 06:10

thiagotnunes approved these changes Mar 15, 2022

View reviewed changes

olavloite merged commit 12a0acb into main Mar 15, 2022

olavloite deleted the spangres-parser-ga branch March 15, 2022 06:36

ansh0l approved these changes Mar 15, 2022

View reviewed changes

olavloite mentioned this pull request Mar 16, 2022

chore(cleanup): address review comments for spangres parser #1752

Merged

Conversation

olavloite commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thiagotnunes left a comment

Choose a reason for hiding this comment

Uh oh!

ansh0l left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

olavloite commented Mar 8, 2022 •

edited

Loading