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

Improved Hibernate support for SAP HANA #2006

Closed
wants to merge 2 commits into from

Conversation

breglerj
Copy link
Contributor

  • Identity column support
  • Spatial support
  • Various minor improvements in HANA dialects

- Identity column support
- Spatial support
- Various minor improvements in HANA dialects
@sebersole
Copy link
Member

@maesenka - I understand you've already looked over the spatial related changes here?

@maesenka
Copy link
Member

@sebersole I have looked at the spatial side of this.

I have just one remark on the code. The class HANASpatialAggregate seems to me superfluous (it overrides StandardSQLFunction::renderType but with code that looks to me equivalent to the overridden implementation).

I also have a couple of questions about the maintenance of this code. First, who is going to run the (spatial) matrix tests? Are they going to be integrated in the CI? If you want me to run this, I'd like some more information on how to set things up (I know virtually nothing about SAP HANA).

@breglerj, are you going to be my contact if I have questions about HANA's spatial features? For example, when I intend to extend the Hibernate Spatial functionality.

libraries.gradle Outdated
@@ -100,6 +100,8 @@ ext {
oracle: 'com.oracle.jdbc:ojdbc7:12.1.0.2',
mssql: 'com.microsoft.sqlserver:mssql-jdbc:6.1.0.jre8',
db2: 'com.ibm.db2:db2jcc:10.5',
//hana: 'com.sap.db.jdbc:ngdbc:1.120.20',
hana: 'com.sap.db.jdbc:ngdbc:2.2.1',
Copy link
Member

Choose a reason for hiding this comment

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

Is this jar not publicly available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean the version 1.120.20? I added it so people using HANA 1 know what the minimum supported version of the client is. If you prefer I can just add a comment stating this instead of the commented out line or leave it out completely.

import org.hibernate.service.spi.Stoppable;

public class HANAMultiTenantConnectionProvider extends AbstractMultiTenantConnectionProvider
implements ServiceRegistryAwareService, Stoppable, Configurable {
Copy link
Member

Choose a reason for hiding this comment

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

This class should be removed. We do not provide specific MultiTenantConnectionProvider implementations

Copy link
Member

Choose a reason for hiding this comment

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

Although I could see this generalized into a provider for multi-tenancy Connections based on providing the tenant name as URL parameter or property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll remove it.

@breglerj
Copy link
Contributor Author

@maesenka: I'll remove the render() method implementation from HANASpatialAggregate. I'd like to keep the class for symmetry reasons if that's ok with you.

We're currently working on how we can integrate the HANA tests into the Hibernate CI infrastructure. I would assume that we can integrate the spatial tests as well. I'll keep you posted.

Yes, you can contact me if you have questions about the HANA spatial features.

@sebersole
Copy link
Member

Applied upstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants