-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
HHH-20050:- Create Spanner PostgreSQL dialect (Basic version) #11572
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
HHH-20050:- Create Spanner PostgreSQL dialect (Basic version) #11572
Conversation
f653970 to
1d7bdc0
Compare
...y-dialects/src/main/java/org/hibernate/community/dialect/SpannerPostgreSQLTableExporter.java
Show resolved
Hide resolved
hibernate-core/src/main/java/org/hibernate/dialect/SpannerPostgreSQLDialect.java
Outdated
Show resolved
Hide resolved
| import org.hibernate.procedure.spi.CallableStatementSupport; | ||
| import org.hibernate.tool.schema.internal.StandardTableExporter; | ||
|
|
||
| public class SpannerPostgreSQLDialect extends PostgreSQLDialect { |
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.
With all the overrides here and potentially missed cases in the SqlAstTranslator I'm kind of starting to think that extending the PostgreSQLDialect is not a good idea.
The use of the Spanner JDBC driver in Database#SPANNER_PG also makes me think that it might be better to extend the SpannerDialect here and override the functions and DDL types or whatever is different from standard Spanner.
Finally, I would prefer to move this dialect to the hibernate-community-dialects project until we can figure out a way to regularly test 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.
@beikov Spanner PG is more aligned(more closer) with PostgreSQL compared to Spanner dialect. I agree that we override lot of methods. We have plans on providing support for them so that we can get rid of these overriden methods. The current dialect shows the gist of what's different compared to OSS PG.
I am trying to onboard the dialect in phases so that it will be easy for review(though I have fixed all the tests in my local). I am planning to onboard testing in CI as soon as I make all changes related to the Dialect. Do you suggest we should move to community dialect first and later bring it to core once we configure the testing pipeline?
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 am trying to onboard the dialect in phases so that it will be easy for review(though I have fixed all the tests in my local).
What do you mean by "fixed all the tests"? I would hope that we don't require many @SkipDialect uses.
I am planning to onboard testing in CI as soon as I make all changes related to the Dialect. Do you suggest we should move to community dialect first and later bring it to core once we configure the testing pipeline?
Yes please. Until we can properly and regularly test this new dialect, I would prefer to have it in the community dialects module.
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.
What do you mean by "fixed all the tests"? I would hope that we don't require many @SkipDialect uses.
Spanner doesn't support integer sequences due to it's nature as a distributed databases(to avoid hotspots). We might have to disable some tests which uses integer identity column(@id with Integer).
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.
@beikov Thanks for the suggestions. I have moved the dialect to inside community dialect.
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.
Spanner doesn't support integer sequences due to it's nature as a distributed databases(to avoid hotspots). We might have to disable some tests which uses integer identity column(@id with Integer).
Cockroach has a similar problem and solved it in the past by always emitting serial8 or int8 as column type. See CockroachDBIdentityColumnSupport
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.
@beikov I think cockroach serial type has a capability to produce auto-incrementing sequences of int type(to avoid hotspots). In Spanner, serial type also produces Long values which can't fit into Integer type.
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.
@sakthivelmanii this is fine for now, though it'll need to be addressed once we aim to set-up testing and move this into core.
|
Thanks for your pull request! This pull request appears to follow the contribution rules. › This message was automatically generated. |
61c16a4 to
c443500
Compare
...y-dialects/src/main/java/org/hibernate/community/dialect/SpannerPostgreSQLTableExporter.java
Outdated
Show resolved
Hide resolved
fab4e1e to
d5c82a1
Compare
|
@beikov Can we merge this? |
d5c82a1 to
823386d
Compare
hibernate-core/src/main/java/org/hibernate/tool/schema/internal/ColumnValue.java
Outdated
Show resolved
Hide resolved
a508bb5 to
d8ce825
Compare
|
@beikov @gavinking can we please merge this if everything is okay? |
gavinking
left a comment
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.
One very minor thing, but apart from that I have no further objection.
hibernate-core/src/main/java/org/hibernate/tool/schema/internal/ColumnValue.java
Outdated
Show resolved
Hide resolved
da81691 to
bfc8759
Compare
All the issues are fixed from my side. there was a bug when I moved dialect from core to community. |
|
@gavinking Can we merge this? |
|
Don't look at me, not my area. |
|
@sakthivelmanii static code analysis is failing, please run |
bfc8759 to
8e44b30
Compare
|
@mbellade Thanks. I have pushed the code again. |
|
Great, please squash everything to a single commit and we're good to go |
8e44b30 to
1e39292
Compare
|
@mbellade Thanks. I have squashed into one commit. |
1e39292 to
73598b6
Compare
|
@mbellade build is green. Jenkins CI (hibernate-orm-pipeline) is not approved. |
[Please describe here what your change is about]
Created a basic version of Spanner version of Dialect
Upcoming:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.
https://hibernate.atlassian.net/browse/HHH-20050