chore: add namespace for client side statements in PostgreSQL dialect #1737

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

+        throw SpannerExceptionFactory.newSpannerException(
+            ErrorCode.INVALID_ARGUMENT, "Unknown or unsupported dialect: " + dialect);
+    }
+  }

  /**
   * 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.Map;
 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

    try {
      writer =
          new PrintWriter(
              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

@@ -43,7 +43,7 @@ Connection getConnection() {
                  .setCredentials(NoCredentials.getInstance())
                  .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 as code owners March 9, 2022 11:42

olavloite requested review from thiagotnunes and ansh0l March 9, 2022 11:43

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add namespace for client side statements in PostgreSQL dialect #1737

chore: add namespace for client side statements in PostgreSQL dialect #1737

olavloite commented Mar 8, 2022 •

edited

Loading

thiagotnunes Mar 15, 2022

olavloite Mar 15, 2022

thiagotnunes left a comment

ansh0l left a comment

ansh0l Mar 15, 2022

olavloite Mar 15, 2022

ansh0l Mar 15, 2022

olavloite Mar 15, 2022

ansh0l Mar 15, 2022

olavloite Mar 15, 2022

ansh0l Mar 15, 2022

olavloite Mar 15, 2022

ansh0l Mar 15, 2022

olavloite Mar 15, 2022

ansh0l Mar 15, 2022

olavloite Mar 15, 2022

ansh0l Mar 15, 2022

olavloite Mar 15, 2022

ansh0l Mar 16, 2022

chore: add namespace for client side statements in PostgreSQL dialect #1737

chore: add namespace for client side statements in PostgreSQL dialect #1737

Conversation

olavloite commented Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagotnunes left a comment

Choose a reason for hiding this comment

ansh0l left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olavloite commented Mar 8, 2022 •

edited

Loading