feat: add support for PostgreSQL dialect by olavloite · Pull Request #739 · googleapis/java-spanner-jdbc

olavloite · 2022-02-08T19:15:04Z

Adds support for PostgreSQL dialect databases.

ansh0l · 2022-02-12T15:30:58Z

+    final String positiveBigNumeric =
+        String.join("", Collections.nCopies(131072, "1"))
+            + "."
+            + String.join("", Collections.nCopies(16383, "2"));


Just for understanding: Do the numbers 131072 and 16383 hold any special significance ?
String.join("",` Collections.nCopies(131072, "1")) + "." + String.join("", Collections.nCopies(16383, "2"));

Yes, they represent the maximum number of digits before/after the decimal point in a PostgreSQL numeric: https://www.postgresql.org/docs/current/datatype-numeric.html

ansh0l · 2022-02-12T15:52:54Z

@@ -1,3 +1,4 @@
+/*GSQL*/
 /*
 * Copyright 2019 Google LLC


Question: Usually I've observed Copyright comments to be the first line / comment in code bases. Is it ok to have the /*GSQL*/ as the first line instead, given its trivial nature?

Yes, there is no requirement that the copyright header is the outright first part of a source file (although it is common practice). See also https://opensource.google/documentation/reference/copyright

Thanks, this is helpful.

ansh0l · 2022-02-12T15:59:55Z

+          }
          if (EmulatorSpannerHelper.isUsingEmulator()) {
            scriptFile = "CreateMusicTables_Emulator.sql";
-          } else {


We've removed the else block for scriptFile. Wouldn't this be needed for the regular getDialect() == Dialect. GOOGLE_STANDARD_SQL case?

No, because we are now initializing it on the line above to the default value (i.e. non-emulator value).

ansh0l · 2022-02-12T16:05:21Z

          .setMetadata(RESULTSET_METADATA)
          .build();

+  private static final String GSQL_STATEMENT = "/*GSQL*/";


We seem to be relying on the first line starting with /*GSQL*/. Are there any other possible approaches for differentiating? Example: different directory structure.

No, this is actually used by the backend to determine which query engine to use for the query, so we mimic it here. The metadata queries are defined only once, and they are all defined in standard dialect. The backend will also look for this header, and if found, use the Google standard dialect for the query even if the database is in PG dialect.

ansh0l · 2022-02-12T16:09:30Z

  }
+
+  @Test
+  public void verifyGoogleSql() throws SQLException {


method name is verifyGoogleSql(), but we seem to be validating the true assertions in the case dialect == Dialect.POSTGRESQL. Should the method name be different?

Yeah, the method name is a little confusing. What we are verifying here is that the /*GSQL*/ header is kept in the SQL statement without comments if the dialect is PostgreSQL, and that it is removed if the dialect is Google_Standard_Sql.
I've added a comment to the test and updated the test name to verifyGoogleSqlHeaderIsCorrectlyParsed.

ansh0l · 2022-02-12T16:13:27Z

+
+  @Test
+  public void testInvalidSql() {
+    String sql = "SELECT * FROM Singers WHERE SingerId='";


nit: I think there is a missing ' character around the end of String sql = "SELECT * FROM Singers WHERE SingerId='";

That is intentional. This is testing the behavior when an invalid SQL string is used.

ansh0l · 2022-02-12T16:22:56Z

+          .build();
+  private static final String QUERY = "SELECT " + COLUMN_NAME + " FROM Table WHERE Id = 0";
+  private static final int MAX_PG_NUMERIC_SCALE = 131_072;
+  private static final int MAX_PG_NUMERIC_PRECISION = 16_383;


131_072 or 16_383 do not look like int, can you explain the above 2 lines.

Java actually allows underscores in int literals for better readability: https://docs.oracle.com/javase/7/docs/technotes/guides/language/underscores-literals.html

Thanks, that is helpful.

ansh0l · 2022-02-12T16:27:27Z

+    final String underflow =
+        BigDecimal.valueOf(Short.MIN_VALUE).subtract(BigDecimal.ONE).toString();
+    final String maxValue = Short.MAX_VALUE + "";
+    final String overflow = BigDecimal.valueOf(Short.MAX_VALUE).add(BigDecimal.ONE).toString();


I'm curious, why do we use BigDecimal instead of int/long for calculating overflow and underflow of Short?

Seems like a typical case of copy-paste from the test method for longs. I've updated them to use long and/or int where possible.

ansh0l · 2022-02-12T16:40:32Z

+    final Map<String, String> googleStandardSqlScripts =
+        ImmutableMap.of("TEST_READ_ONLY", "ITReadOnlySpannerTest.sql");
+    final Map<String, String> postgresScripts =
+        ImmutableMap.of("TEST_READ_ONLY", "ITPgReadOnlySpannerTest.sql");


regarding ITPgReadOnlySpannerTest.sql and ITPg*.sql files: Should we consider moving them to a separate folder with more verbose name of the folder?

I think that's a good idea. I've moved the PG script files to a separate PostgreSQL folder.

ansh0l · 2022-02-12T16:52:03Z

@@ -1,282 +0,0 @@
-/*


Note to other reviwers: this file ITJdbcSqlScriptTest.java seems to have been generalized as ITJdbcScriptTest.java and github is not able to detect the diff.

ansh0l

Adding LGTM. This depends on other changes being merged and released, so the Do Not Release tag is already applied on it.

thiagotnunes

LGTM

🤖 I have created a release *beep* *boop* --- ## [2.6.0](v2.5.11...v2.6.0) (2022-02-24) ### Features * add support for PostgreSQL dialect ([#739](#739)) ([f9daa19](f9daa19)) ### Bug Fixes * create specific metadata queries for PG ([#759](#759)) ([caffda0](caffda0)) ### Dependencies * update dependency com.google.cloud:google-cloud-spanner-bom to v6.20.0 ([#758](#758)) ([311d1ca](311d1ca)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- ## [2.6.0](googleapis/java-spanner-jdbc@v2.5.11...v2.6.0) (2022-02-24) ### Features * add support for PostgreSQL dialect ([googleapis#739](googleapis/java-spanner-jdbc#739)) ([c6b2e91](googleapis/java-spanner-jdbc@c6b2e91)) ### Bug Fixes * create specific metadata queries for PG ([googleapis#759](googleapis/java-spanner-jdbc#759)) ([92e05d0](googleapis/java-spanner-jdbc@92e05d0)) ### Dependencies * update dependency com.google.cloud:google-cloud-spanner-bom to v6.20.0 ([googleapis#758](googleapis/java-spanner-jdbc#758)) ([6bcf53b](googleapis/java-spanner-jdbc@6bcf53b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

feat: add support for PostgreSQL dialect

298877f

olavloite added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 8, 2022

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

olavloite marked this pull request as ready for review February 8, 2022 19:32

olavloite requested review from a team, ansh0l and thiagotnunes February 8, 2022 19:32

ansh0l reviewed Feb 12, 2022

View reviewed changes

fix: address review comments

7eb2bd5

ansh0l approved these changes Feb 15, 2022

View reviewed changes

thiagotnunes approved these changes Feb 16, 2022

View reviewed changes

olavloite added 2 commits February 19, 2022 10:19

Merge branch 'main' into postgresql

18ba450

deps: bump java-spanner to 6.19.1

0c31be6

olavloite removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 19, 2022

olavloite added 4 commits February 19, 2022 11:12

feat: support automatic dialect detection

7d1943f

deps: remove threeten + add gax

a4292be

build: use owned instance for integration tests

704855c

build: use same instance as the client library

2df341d

olavloite merged commit f9daa19 into main Feb 19, 2022

olavloite deleted the postgresql branch February 19, 2022 11:20

release-please bot mentioned this pull request Feb 19, 2022

chore(main): release 2.6.0 #753

Merged

Conversation

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

Uh oh!

Uh oh!

ansh0l Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ansh0l Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ansh0l left a comment

Choose a reason for hiding this comment

Uh oh!

thiagotnunes left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

olavloite commented Feb 8, 2022 •

edited

Loading

ansh0l Feb 12, 2022 •

edited

Loading

ansh0l Feb 12, 2022 •

edited

Loading