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

[native] Add support for TPC-H connector #18367

Merged
merged 1 commit into from
Sep 28, 2022
Merged

[native] Add support for TPC-H connector #18367

merged 1 commit into from
Sep 28, 2022

Conversation

majetideepak
Copy link
Collaborator

@majetideepak majetideepak commented Sep 19, 2022 •
edited
Loading

presto-native-execution has been extended to support the TPC-H connector.

Test plan
TPC-H queries are added to TestTpchQueries.java

== RELEASE NOTES ==
TPC-H Connector Changes
* Add support for the TPC-H connector in Presto Native Execution.
  Velox only supports standard column naming. 
  The tpch connector property ``tpch.column-naming=standard`` must be set on the Java side.

@majetideepak majetideepak changed the title Add support for TPC-H connector [native] Add support for TPC-H connector Sep 19, 2022
@majetideepak majetideepak force-pushed the add-tpch-connector-support branch 2 times, most recently from 8152f21 to 54da2ef Compare September 22, 2022 14:09
@majetideepak majetideepak marked this pull request as ready for review September 22, 2022 14:09
@majetideepak majetideepak requested review from a team as code owners September 22, 2022 14:09
@majetideepak majetideepak force-pushed the add-tpch-connector-support branch from 54da2ef to df85b17 Compare September 22, 2022 14:12
@majetideepak
Copy link
Collaborator Author

@mbasmanova this is now ready for review. Thanks.

mbasmanova
mbasmanova approved these changes Sep 22, 2022
View reviewed changes
Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

@majetideepak This is super cool. Thank you, Deepak!!!

@mbasmanova
Copy link
Contributor

== NO RELEASE NOTE ==

Let's mention this enhancement in the release notes.

@majetideepak
Copy link
Collaborator Author

@mbasmanova I followed the guidelines here https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines and wrote the release note. Can you take another look? Thanks.

@mbasmanova
Copy link
Contributor

@mbasmanova I followed the guidelines here https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines and wrote the release note. Can you take another look? Thanks.

Perfect. Thank you.

@majetideepak majetideepak force-pushed the add-tpch-connector-support branch 3 times, most recently from 2c77580 to 83b1361 Compare September 22, 2022 20:51
@majetideepak majetideepak force-pushed the add-tpch-connector-support branch from 83b1361 to 8a51bfd Compare September 28, 2022 17:12
@majetideepak majetideepak merged commit 818df29 into prestodb:master Sep 28, 2022
@majetideepak majetideepak mentioned this pull request Nov 3, 2022
Closed
@mbasmanova
Copy link
Contributor

@majetideepak Deepak, it seems like this PR may include manual changes to auto-generated presto_protocol.cpp/h files. Would you clarify whether this is the case? If so, we need to figure out how to re-do this change to avoid manually editing auto-generated files.

@mbasmanova
Copy link
Contributor

CC: @tanjialiang @spershin

@majetideepak
Copy link
Collaborator Author

@mbasmanova I did manually change these files. I incorrectly thought we have to.
Looks like I have to follow the instructions here https://github.com/prestodb/presto/tree/master/presto-native-execution/presto_cpp/presto_protocol
I will fix this.

@mbasmanova
Copy link
Contributor

@majetideepak

I will fix this.

Thanks. This would be much appreciated. CC: @tanjialiang @spershin @xiaoxmeng

@tanjialiang
Copy link
Contributor

@mbasmanova I did manually change these files. I incorrectly thought we have to. Looks like I have to follow the instructions here https://github.com/prestodb/presto/tree/master/presto-native-execution/presto_cpp/presto_protocol I will fix this.

Thanks @majetideepak . I was playing around with this auto generation yesterday for a bit as well but did not have a solution yet. Let me know if you need some additional help/info from me, that case I can help to continue to look at the issue, and asking internal folks who's most familiar in this area.

@majetideepak
Copy link
Collaborator Author

@tanjialiang The README does need improvement. This is what I have so far.
There is make target make presto_protocol that will generate the files needed.
Not all Java classes can be automatically converted. It looks like in this case, we can manually provide a translation.
I am exploring further.
We should definitely aim to avoid checking in generated code and automate this in the build process.

@majetideepak majetideepak mentioned this pull request Nov 10, 2022
Merged
@majetideepak
Copy link
Collaborator Author

I have a PR now #18656

majetideepak added a commit to majetideepak/presto that referenced this pull request Nov 11, 2022
@majetideepak
[native] Add support for TPC-H Connector Protocol Bindings
41de996 
The protocol bindings for TPC-H Connector were manually added in PR prestodb#18367.
These bindings will now be generated automatically.
The following improvements have been made:

1. Improve README of presto_cpp/presto_protocol.
2. Add default make target to generate Presto protocol files.
3. Include TpchConnector classes in the yml file.
4. Extend special Connector files to support tpch.
5. Customize TpchTransactionHandle since the corresponding class in Java is an enum.
6. Customize TpchColumnHandle since we require an implementation of operator<().
majetideepak added a commit that referenced this pull request Nov 11, 2022
@majetideepak
[native] Add support for TPC-H Connector Protocol Bindings
c9ebcd3 
The protocol bindings for TPC-H Connector were manually added in PR #18367.
These bindings will now be generated automatically.
The following improvements have been made:

1. Improve README of presto_cpp/presto_protocol.
2. Add default make target to generate Presto protocol files.
3. Include TpchConnector classes in the yml file.
4. Extend special Connector files to support tpch.
5. Customize TpchTransactionHandle since the corresponding class in Java is an enum.
6. Customize TpchColumnHandle since we require an implementation of operator<().
wypb pushed a commit to wypb/presto that referenced this pull request Dec 22, 2023
@majetideepak
[native] Add support for TPC-H Connector Protocol Bindings
e37caea 
The protocol bindings for TPC-H Connector were manually added in PR prestodb#18367.
These bindings will now be generated automatically.
The following improvements have been made:

1. Improve README of presto_cpp/presto_protocol.
2. Add default make target to generate Presto protocol files.
3. Include TpchConnector classes in the yml file.
4. Extend special Connector files to support tpch.
5. Customize TpchTransactionHandle since the corresponding class in Java is an enum.
6. Customize TpchColumnHandle since we require an implementation of operator<().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbasmanova mbasmanova mbasmanova approved these changes

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@majetideepak @mbasmanova @tanjialiang