-
Notifications
You must be signed in to change notification settings - Fork 123
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
docs: add samples for PostgresSQL #1781
Conversation
Approved presubmit checks to run. Will wait for their results before proceeding with further review. |
try (ResultSet resultSet = | ||
dbClient | ||
.singleUse() | ||
.executeQuery(Statement.of("SELECT singerid as \"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.
is the singlerid as \"\SingerId"
used to keep the output looks same as before?
Or why not just SELECT SingerId, AlbumId, MarketingBudge FROM Albums
?
try (ResultSet resultSet = | ||
dbClient | ||
.singleUse() | ||
.executeQuery(Statement.of("SELECT singerid as \"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.
is the singlerid as \"\SingerId"
used to keep the output looks same as before?
Or why not just SELECT SingerId, AlbumId, MarketingBudge FROM Albums
?
try (ResultSet resultSet = | ||
dbClient | ||
.singleUse() | ||
.executeQuery(Statement.of("SELECT singerid as \"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.
@ansh0l , I do not know much how the client handle the identifier, if we just use SELECT SingerId ...
would the resultSet.getLong("SingerId") work or not?
try (ResultSet resultSet = | ||
dbClient | ||
.singleUse() | ||
.executeQuery(Statement.of("SELECT singerid as \"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.
@ansh0l , I do not know much how the client handle the identifier, if we just use SELECT SingerId ...
would the resultSet.getLong("SingerId") work or not?
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.
@jin-jj No, it would not work. If you execute SELECT SingerId FROM Singers
, then PostgreSQL will fold the identifier to lower case, so it is effectively the same as executing SELECT singerid FROM Singers
.
Identifiers that are quoted will be treated as case-sensitive and keep the actual lower/upper case letters.
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.
Looks good. I think only question is how we wanna handle the case insensitive for PostgreSQL dialect. I saw we use singerid AS "SingerId" to workaround it.
Is this currently the best way to do it? Or should we just switch to lower case or just quote the Identifier always. @ansh0l Do you have an opinion for it?
@@ -80,7 +80,7 @@ static void executeSqlWithCustomTimeoutAndRetrySettings( | |||
.readWriteTransaction() | |||
.run(transaction -> { | |||
String sql = | |||
"INSERT Singers (SingerId, FirstName, LastName)\n" | |||
"INSERT INTO Singers (SingerId, FirstName, LastName)\n" |
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 fixing this syntax issue in old code.
|
||
package com.example.spanner; | ||
|
||
// [START spanner_pg_async_query_to_list] |
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.
Why only this tag has pg_ in between?
Our all tag naming may require update. I will update it here later what is our decision about the tag name for pg code samples.
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.
For package name, we will use spanner_postgresql_*.
Pls fix this one and also apply the same pattern to all other tags.
try (AsyncResultSet resultSet = | ||
client | ||
.singleUse() | ||
.executeQueryAsync(Statement.of("SELECT singerid as \"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.
The same as the comments, I am thinking renaming all column name in DDL from SingerId to singer_id, which is more pg friendly. I will circle back later to update what should we do.
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 will keep the code as-is. No change required.
@@ -0,0 +1,90 @@ | |||
/* | |||
* Copyright 2020 Google Inc. |
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: 2022
(Please check everywhere else as well)
* limitations under the License. | ||
*/ | ||
|
||
package com.example.spanner; |
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.
Would it make more sense to place all the PG samples in a separate sub-package. So something like com.example.spanner.postgresql
? (I'm open to both solutions, both the one that is chosen now by prefixing the samples with Pg
, as well as moving them to a separate package, but I would like us to take an informed decision 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.
Sure, will decide in today call with @jin-jj and decide the approach.
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.
@olavloite : I think this was templated on https://github.com/googleapis/java-spanner/pull/1700/files.
IIRC, If we move to a sub-package - the class path would end up becoming something like samples/snippets/src/main/java/com/example/spanner/postgres/AsyncQueryToListAsyncExample.java
, where the only gain probably will be readability of the class name, and some segregation from the regular GSQL samples.
The seggregation still happens in the current scheme of things, and looking for Spangres samples with an explicit pg in them sounds all right to me.
So we can keep things as is.
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 @olavloite and @ansh0l. So @Sivakumar-Searce we will keep the class name as-is.
executor); | ||
} | ||
|
||
for (AsyncQueryToListAsyncExample.Album album : albums.get(30L, TimeUnit.SECONDS)) { |
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.
for (AsyncQueryToListAsyncExample.Album album : albums.get(30L, TimeUnit.SECONDS)) { | |
for (Album album : albums.get(30L, TimeUnit.SECONDS)) { |
albums = | ||
resultSet.toListAsync( | ||
reader -> { | ||
return new AsyncQueryToListAsyncExample.Album( |
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.
return new AsyncQueryToListAsyncExample.Album( | |
return new Album( |
static void asyncQueryToList(DatabaseClient client) | ||
throws InterruptedException, ExecutionException, TimeoutException { | ||
ExecutorService executor = Executors.newSingleThreadExecutor(); | ||
ApiFuture<? extends List<AsyncQueryToListAsyncExample.Album>> albums; |
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.
ApiFuture<? extends List<AsyncQueryToListAsyncExample.Album>> albums; | |
ApiFuture<? extends List<Album>> albums; |
"%d %s %s\n", | ||
resultSet.getLong(0), | ||
resultSet.getString(1), | ||
// TODO: MarketingBudget uppercase not supported, keeping lowercase for now |
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.
Remove TODO before merging. Just using lower case here is OK with me (actually it's the most important part of the sample, as it shows an important difference between Spanner and PostgreSQL).
try (ResultSet resultSet = | ||
dbClient | ||
.singleUse() | ||
.executeQuery(Statement.of("SELECT singerid as \"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.
@jin-jj No, it would not work. If you execute SELECT SingerId FROM Singers
, then PostgreSQL will fold the identifier to lower case, so it is effectively the same as executing SELECT singerid FROM Singers
.
Identifiers that are quoted will be treated as case-sensitive and keep the actual lower/upper case letters.
dbId.getInstanceId().getInstance(), | ||
dbId.getDatabase(), | ||
Arrays.asList( | ||
"ALTER TABLE Albums ADD COLUMN LastUpdateTime TIMESTAMPTZ"), |
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.
Are commit timestamps not (yet) supported?
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 tried, it is not supported and couldn't find doc as well for commit timestamp for PG.
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.
use spanner.commit_timestamp as the type when adding column
} | ||
|
||
// [START spanner_write_data_for_struct_queries] | ||
static void writeStructExampleData(DatabaseClient dbClient) { |
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.
Maybe I'm missing something here, but I don't see any structs here. I also don't think we support structs for PostgreSQL databases, so this sample is probably void.
"SELECT venueid as \"VenueId\", venuename as \"VenueName\" FROM Venues " + "WHERE" | ||
+ " VenueInfo = $1") |
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:
"SELECT venueid as \"VenueId\", venuename as \"VenueName\" FROM Venues " + "WHERE" | |
+ " VenueInfo = $1") | |
"SELECT venueid as \"VenueId\", venuename as \"VenueName\" FROM Venues " | |
+ "WHERE VenueInfo = $1") |
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
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
Thanks! @Sivakumar-Searce Please fix the issue as Anshul pointed out. |
@Sivakumar-Searce : Thanks! Approved presubmit checks to run. |
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
* chore: Adding release-please annotations to readme files Source-Link: googleapis/synthtool@327d46f Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:bf5639d265d70f6137d57d42ae781a6f4e26d4085ff4e018e71350480f9b3996
Added samples for PG dialect databases.
cc: @ansh0l