-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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-14146 Add Presto dialect #3496
Conversation
Thanks! Great to see support for new modern databases being contributed. We'll need to add Presto support to our continuous integration systems; my preference would be a container, as it helps us to diagnose issues locally - should we need to. |
// DdlTransactionIsolatorNonJtaImpl works with Presto driver only in autocommit mode (hibernate.connection.autocommit=true) | ||
// When this is set, DriverConnectionCreator will pass autocommit connection property which Presto | ||
// driver does not accept. Here we remove the property as a workaround, it's being handled on the | ||
// Hibernate's side. |
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 a bit concerned about this, could you expand on "it's being handled" ?
What are the implications of this, and what should users do when configuring a production connection pool and/or a Datasource configuration managed by an application server?
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.
Currently the hibernate.connection.autocommit
mode is needed (at least for DDL in tests).
AFAIU this setting controls Hibernate behavior and also is passed to JDBC driver in the form of autocommit
connection property.
The connection property can be safely ignored (Hibernate uses JDBC APIs to control autocommit of a connection), but Presto JDBC driver does not ignore connection properties that it does not understand.
@Sanne correct. You can bring up Presto container up with eg
|
From tavis log:
Can I safely assume these are not related? |
Rebased. |
The CI build is green. |
@Sanne can i do anything to help move this forward? |
@Sanne should i rebase? |
hi @findepi ! Apologies, I've been overwhelmed by notifications and missed this completely. Yes could you rebase it once again please? I'll see to find someone to setup the CI task on CI. |
93b8803
to
55c5f7a
Compare
Rebased. There are some build failures, like
@Sanne please help me understand what I am doing wrong. |
Thanks! Don't worry for the CI job, these are new jobs which have just been added and we still need to sort out some details - sorry about that. |
Hi @findepi , with CI having issues I tried to run tests locally - but it failed. I used:
Did you test it differently, or perhaps some more changes are needed? |
I didn't run tests recently, only rebased, let me take a look. |
Add dialect for Presto (https://prestosql.io/)
@Sanne i remember now & sorry for the trouble. Presto is not capable of housing a typical application, as it still lacks some features commonly seen in relational databases.
For these reasons the preparation part of the test ( As such, you should not expect a typical Hibernate-based app to ever dream of moving to Presto, which is an analytical database. See I also removed the |
May I know the status of this PR? So there won't be Hibernate support for Presto? |
As per https://hibernate.atlassian.net/browse/HHH-14693 and #3783 we decided not to include any dialects in 5.5 anymore for now and focus on 6.0. IMO it would be fine to add it to 5.5 though if the Dialect for 6 qualifies for hibernate-core according to the requirements document: https://github.com/hibernate/hibernate-orm/blob/wip/6.0/dialects.adoc |
@bradhill99 if i were to continue here, i would create a Trino dialect, as per https://trino.io/blog/2020/12/27/announcing-trino.html @beikov i can reopen the PR (with new target module) if needed. |
You can reopen this PR against the main branch after a Dialect for 6.0 is merged according to the requirements for the hibernate-core inclusion. |
@findepi Thanks for the reply. It will be great to have presto dialect for Hibernate. |
@beikov is there a PR i should follow? |
What do you mean by follow? Adding new dialects is a community or vendor driven effort. We as Hibernate team explicitly do not want to bother with any new dialects anymore unless someone steps up and provides full support for that. Read the document about the process: https://github.com/hibernate/hibernate-orm/blob/wip/6.0/dialects.adoc |
Add dialect for Presto (https://prestosql.io/)