-
Notifications
You must be signed in to change notification settings - Fork 48
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 PostgreSQL dialect #739
Conversation
final String positiveBigNumeric = | ||
String.join("", Collections.nCopies(131072, "1")) | ||
+ "." | ||
+ String.join("", Collections.nCopies(16383, "2")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -1,3 +1,4 @@ | |||
/*GSQL*/ | |||
/* | |||
* Copyright 2019 Google LLC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is helpful.
if (EmulatorSpannerHelper.isUsingEmulator()) { | ||
scriptFile = "CreateMusicTables_Emulator.sql"; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've removed the else block for scriptFile
. Wouldn't this be needed for the regular getDialect() == Dialect. GOOGLE_STANDARD_SQL
case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because we are now initializing it on the line above to the default value (i.e. non-emulator value).
@@ -66,8 +72,23 @@ | |||
.setMetadata(RESULTSET_METADATA) | |||
.build(); | |||
|
|||
private static final String GSQL_STATEMENT = "/*GSQL*/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seem to be relying on the first line starting with /*GSQL*/
. Are there any other possible approaches for differentiating? Example: different directory structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -306,4 +331,97 @@ public void getSchemasInDdlBatch() throws SQLException { | |||
connection.createStatement().execute("ABORT BATCH"); | |||
} | |||
} | |||
|
|||
@Test | |||
public void verifyGoogleSql() throws SQLException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
|
||
@Test | ||
public void testInvalidSql() { | ||
String sql = "SELECT * FROM Singers WHERE SingerId='"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think there is a missing '
character around the end of String sql = "SELECT * FROM Singers WHERE SingerId='";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is intentional. This is testing the behavior when an invalid SQL string is used.
.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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
131_072
or 16_383
do not look like int
, can you explain the above 2 lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java actually allows underscores in int literals for better readability: https://docs.oracle.com/javase/7/docs/technotes/guides/language/underscores-literals.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that is helpful.
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious, why do we use BigDecimal
instead of int
/long
for calculating overflow and underflow of Short?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
final Map<String, String> googleStandardSqlScripts = | ||
ImmutableMap.of("TEST_READ_ONLY", "ITReadOnlySpannerTest.sql"); | ||
final Map<String, String> postgresScripts = | ||
ImmutableMap.of("TEST_READ_ONLY", "ITPgReadOnlySpannerTest.sql"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regarding ITPgReadOnlySpannerTest.sql
and ITPg*.sql
files: Should we consider moving them to a separate folder with more verbose name of the folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a good idea. I've moved the PG script files to a separate PostgreSQL folder.
@@ -1,282 +0,0 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding LGTM. This depends on other changes being merged and released, so the Do Not Release tag is already applied on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
Adds support for PostgreSQL dialect databases.