Skip to content
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: PostgreSQL dialect databases #1673

Merged
merged 10 commits into from
Feb 10, 2022
Merged

feat: PostgreSQL dialect databases #1673

merged 10 commits into from
Feb 10, 2022

Conversation

olavloite
Copy link
Collaborator

Adds support for Cloud Spanner databases using the PostgreSQL dialect.

@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Feb 8, 2022
@olavloite olavloite marked this pull request as ready for review February 8, 2022 13:51
@olavloite olavloite requested review from a team as code owners February 8, 2022 13:51
import com.google.common.base.Preconditions;

@InternalApi
public class PostgreSQLStatementParser extends AbstractStatementParser {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woa, hand wrote parser. Nice job, we should also have this kind of parser at the backend.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could re-write the backend to Java and then add this parser :-)

@@ -371,6 +373,8 @@ private Object getAsObject(int columnIndex) {
return getDoubleListInternal(columnIndex);
case NUMERIC:
return getBigDecimalListInternal(columnIndex);
case PG_NUMERIC:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PG does not support Struct, right? So, for now, we will not hit this PG_NUMERIC in Struct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PG currently does not support structs in the backend, so in that sense we won't see this coming from the backend. The client library however allows users to convert a row to a struct, and in that case we could encounter it:

Copy link
Contributor

@thiagotnunes thiagotnunes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we did not parameterize some integration tests with the pg dialect. Could you add the parameterization for the ITBatchReadTest?

google-cloud-spanner/clirr-ignored-differences.xml Outdated Show resolved Hide resolved
* Dialect#values()}.
*/
public @Nullable Dialect getDialect() {
return dialect;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it ever be null? Maybe we can say if null return UNSPECIFIED or GOOGLE_SQL_STANDARD?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DatabaseInfo(DatabaseId id, State state) constructor sets it to null (and also a lot of other fields that also could have had non-null values, like the encryption config), so I think we should keep it this way to keep it consistent with the other properties in the class.

@thiagotnunes
Copy link
Contributor

Thanks for doing this!

@olavloite
Copy link
Collaborator Author

I think we did not parameterize some integration tests with the pg dialect. Could you add the parameterization for the ITBatchReadTest?

I've parameterized the ITBatchReadTest, but I had to skip the two tests that use the Read API for the time being, as it returns an error. I'll reach out internally to see if anyone knows why that is happening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/java-spanner API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants