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.

Sign up for GitHub

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

Jump to bottom

Add HANA connector #21034

Merged
merged 1 commit into from Oct 25, 2023
Merged

Add HANA connector #21034

merged 1 commit into from Oct 25, 2023

Conversation

JiamingMai
Copy link
Contributor

@JiamingMai JiamingMai commented Oct 4, 2023 •
edited

Description

This PR introduces new JDBC connector for HANA .

Motivation and Context

Some enterprises requires new connector to integrate with HANA database (there is also a related issue before: related discussion).

Impact

No impact.

Test Plan

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Connectors
* Added HANA connector

@JiamingMai JiamingMai requested review from shrinidhijoshi and a team as code owners October 4, 2023 09:53
@JiamingMai JiamingMai force-pushed the add-hana-connector branch 2 times, most recently from 583e31f to eaff83b Compare October 4, 2023 10:42
@github-actions
Copy link

github-actions bot commented Oct 4, 2023 •
edited

Codenotify: Notifying subscribers in CODENOTIFY files for diff 580f2e3...13ebc4f.

Notify File(s)
@steveburnett presto-docs/src/main/sphinx/connector.rst
presto-docs/src/main/sphinx/connector/hana.rst

steveburnett
steveburnett requested changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

This looks great! I have a few suggestions and revisions in comments that I hope you will consider.

presto-docs/src/main/sphinx/connector.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/hana.rst Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/hana.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/hana.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/hana.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/hana.rst Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/hana.rst Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/hana.rst Outdated Show resolved Hide resolved
@JiamingMai
Copy link
Contributor Author

This looks great! I have a few suggestions and revisions in comments that I hope you will consider.

Thanks @steveburnett. I just updated the docs according to your comments.

steveburnett
steveburnett approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment •
edited

Choose a reason for hiding this comment

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

LGTM! (docs)
Pulled and built a new local build of the docs, everything looks great here. Thanks for your work!

kiersten-stokes
kiersten-stokes reviewed Oct 5, 2023
View reviewed changes
Copy link
Contributor

@kiersten-stokes kiersten-stokes left a comment

Choose a reason for hiding this comment

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

Thank you so much for this contribution, @JiamingMai! It's always great to have new connectors, especially when there is community interest. I just had a few initial observations below, mostly NITs 🙂

I think overall we could also benefit from adding some additional tests, which is always useful in clarifying the supported features of a given connector

presto-hana/src/main/java/com/facebook/presto/plugin/hana/HanaClient.java Outdated Show resolved Hide resolved
presto-hana/pom.xml Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/hana.rst
Comment on lines +5 to +8
The HANA connector allows querying and creating tables in an
external HANA database. This can be used to join data between
different systems like HANA and Hive, or between two different
HANA instances.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused about the support for creating tables. It's mentioned here that creating tables is supported, but then it says in the limitations section below that it isn't. We should probably clarify that and then add some tests as well if applicable

Copy link
Contributor Author

@JiamingMai JiamingMai Oct 6, 2023 •
edited

Choose a reason for hiding this comment

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

Thanks @kiersten-stokes. I just removed the redundant codes according to your comments. I have tested and clarified that CREATE TABLE and DROP TABLE statements are supported. But it is difficult to add the tests for similar reasons as SQL Server. It is very heavy to create an embedded HANA server. The only way to create an embedded HANA server is to build with docker image SAP HANA, express edition. Unfortunately, it is no longer maintained (it takes a large amount of RAM to create even if it still works).

@JiamingMai JiamingMai force-pushed the add-hana-connector branch 2 times, most recently from 6a2427b to 37fd655 Compare October 6, 2023 16:03
@tdcmeehan
Copy link
Contributor

@JiamingMai thanks for adding this connector. I am a bit concerned about the lack of automated tests. Do you commit to supporting this connector, including triaging Github issues and answering questions about the connector?

@JiamingMai
Copy link
Contributor Author

JiamingMai commented Oct 11, 2023 •
edited

@JiamingMai thanks for adding this connector. I am a bit concerned about the lack of automated tests. Do you commit to supporting this connector, including triaging Github issues and answering questions about the connector?

Thanks @tdcmeehan. I am committed to supporting this connector and would like to take responsibility for triaging GitHub issues and answering questions about this connector. It is difficult to add the tests for HANA connector as the official does not provide a lightweight embedded HANA server.

beinan
beinan reviewed Oct 18, 2023
View reviewed changes
Copy link
Member

@beinan beinan left a comment

Choose a reason for hiding this comment

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

lgtm, let me ask Shengxuan to take a look also.

presto-hana/src/main/java/com/facebook/presto/plugin/hana/HanaClientModule.java Outdated
@Override
public void configure(Binder binder)
{
binder.bind(JdbcClient.class).to(HanaClient.class).in(Scopes.SINGLETON);
Copy link
Member

Choose a reason for hiding this comment

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

It might be better to static import Scopes.SINGLETON

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @beinan. Updated. I used static import instead.

liushengxuan
liushengxuan reviewed Oct 18, 2023
View reviewed changes
presto-hana/src/main/java/com/facebook/presto/plugin/hana/HanaClient.java Outdated
{
// Abort connection before closing. Without this, the Hana driver
// attempts to drain the connection by reading all the results.
connection.abort(directExecutor());
Copy link

Choose a reason for hiding this comment

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

It would be better to use try catch here

Copy link
Member

Choose a reason for hiding this comment

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

Thank you Shengxuan for the review! @JiamingMai do you mind to take a look?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Thanks @liushengxuan. I used try catch here.

@tdcmeehan tdcmeehan self-assigned this Oct 18, 2023
@JiamingMai JiamingMai force-pushed the add-hana-connector branch 2 times, most recently from e726cd8 to b597e0c Compare October 20, 2023 02:45
@beinan
Copy link
Member

beinan commented Oct 20, 2023

Hi @JiamingMai, looks like the maven checks are still failing, do you mind taking a look? Thanks!

@JiamingMai JiamingMai force-pushed the add-hana-connector branch 2 times, most recently from 670f15b to 476cb39 Compare October 20, 2023 09:10
@JiamingMai
Copy link
Contributor Author

Hi @JiamingMai, looks like the maven checks are still failing, do you mind taking a look? Thanks!

I just updated the pom.xml file. Now all checks have passed.

beinan
beinan approved these changes Oct 25, 2023
View reviewed changes
Copy link
Member

@beinan beinan left a comment

Choose a reason for hiding this comment

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

lgtm

@beinan
Copy link
Member

beinan commented Oct 25, 2023 •
edited

@JiamingMai could you help to refine a little bit on your commit message and release notes. here is the doc: https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines
Thanks!

@JiamingMai
Copy link
Contributor Author

@JiamingMai could you help to refine a little bit on your commit message and release notes. here is the doc: https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines Thanks!

Thanks @beinan. I just updated the release notes according to the guidelines.

@beinan beinan merged commit 4c8704c into prestodb:master Oct 25, 2023
57 checks passed
@wanglinsong wanglinsong mentioned this pull request Dec 8, 2023
Closed
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kiersten-stokes kiersten-stokes kiersten-stokes left review comments

@steveburnett steveburnett steveburnett approved these changes

@beinan beinan beinan approved these changes

@liushengxuan liushengxuan liushengxuan approved these changes

@shrinidhijoshi shrinidhijoshi Awaiting requested review from shrinidhijoshi shrinidhijoshi is a code owner

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

Assignees

@tdcmeehan tdcmeehan

Labels
pr bootcamp
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@JiamingMai @tdcmeehan @beinan @steveburnett @kiersten-stokes @liushengxuan