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 REST based remote function #23568

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[native] Add support for REST based remote function #23568

wants to merge 1 commit into from

Conversation

Joe-Abraham
Copy link
Contributor

@Joe-Abraham Joe-Abraham commented Sep 2, 2024 •
edited
Loading

@Joe-Abraham Joe-Abraham force-pushed the remote branch 6 times, most recently from 808f416 to 1c7ea23 Compare September 10, 2024 09:49
@Joe-Abraham Joe-Abraham force-pushed the remote branch 7 times, most recently from 86c1f8b to 3345b51 Compare September 17, 2024 14:30
@Joe-Abraham Joe-Abraham force-pushed the remote branch 3 times, most recently from 0d73f09 to f530b51 Compare September 26, 2024 08:08
@Joe-Abraham Joe-Abraham force-pushed the remote branch 10 times, most recently from 32a8715 to 5726f00 Compare October 16, 2024 06:45
@Joe-Abraham Joe-Abraham force-pushed the remote branch 4 times, most recently from a8afff9 to 26f9fe6 Compare October 29, 2024 06:06
@Joe-Abraham Joe-Abraham force-pushed the remote branch 2 times, most recently from 29ef0bb to c2ca02c Compare November 2, 2024 13:00
@Joe-Abraham Joe-Abraham changed the title [WIP] [native] Add support for REST based remote function [native] Add support for REST based remote function Nov 4, 2024
@Joe-Abraham Joe-Abraham force-pushed the remote branch 2 times, most recently from d1d0703 to 83ecc48 Compare November 19, 2024 06:59
@Joe-Abraham Joe-Abraham mentioned this pull request Nov 27, 2024
Open
@Joe-Abraham Joe-Abraham marked this pull request as ready for review November 28, 2024 03:51
@Joe-Abraham Joe-Abraham requested review from a team as code owners November 28, 2024 03:51
@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Nov 28, 2024
@prestodb-ci prestodb-ci requested review from a team, ScrapCodes and aaneja and removed request for a team November 28, 2024 03:51
@Joe-Abraham
Copy link
Contributor Author

@aditi-pandit Can you please review the changes

@Joe-Abraham Joe-Abraham force-pushed the remote branch 2 times, most recently from ef746fc to 098b074 Compare January 13, 2025 06:15
pramodsatya
pramodsatya reviewed Jan 31, 2025
View reviewed changes
Copy link
Contributor

@pramodsatya pramodsatya left a comment

Choose a reason for hiding this comment

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

Thanks @Joe-Abraham, could you add e2e tests as well?

presto-native-execution/presto_cpp/main/types/CMakeLists.txt Outdated
@@ -18,12 +18,20 @@ add_library(
presto_types OBJECT
PrestoToVeloxQueryPlan.cpp PrestoToVeloxExpr.cpp VeloxPlanValidator.cpp
PrestoToVeloxSplit.cpp PrestoToVeloxConnector.cpp)

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: revert newline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted the line

presto-native-execution/presto_cpp/main/types/PrestoToVeloxExpr.cpp Outdated
@@ -412,6 +421,18 @@ std::optional<TypedExprPtr> VeloxExprConverter::tryConvertLike(
returnType, args, getFunctionName(signature));
}

#ifdef PRESTO_ENABLE_REMOTE_FUNCTIONS
PageFormat fromSerdeString(const std::string_view& serdeName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this as a separate function? There is only one caller and this check can be moved to the caller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved it to the function

presto-native-execution/presto_cpp/main/types/PrestoToVeloxExpr.cpp Outdated
@@ -412,6 +421,18 @@ std::optional<TypedExprPtr> VeloxExprConverter::tryConvertLike(
returnType, args, getFunctionName(signature));
}

#ifdef PRESTO_ENABLE_REMOTE_FUNCTIONS
PageFormat fromSerdeString(const std::string_view& serdeName) {
if (serdeName == "presto_page") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid magic literal and use constant variable for "presto_page".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The implementation is done similar to the below code.

presto/presto-native-execution/presto_cpp/main/RemoteFunctionRegisterer.cpp

Line 39 in a7a7f80

if (serdeName == "presto_page") {

presto-native-execution/presto_cpp/main/types/PrestoToVeloxExpr.cpp
return std::make_shared<CallTypedExpr>(
returnType, args, getFunctionName(sqlFunctionHandle->functionId));
}
#ifdef PRESTO_ENABLE_REMOTE_FUNCTIONS
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this logic to a separate function, say registerVeloxRemoteFunction?

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 for the feedback, I have made the improvements

presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.h Outdated
};
class ParseError : public Exception {
public:
explicit ParseError(const std::string& message) : Exception(message){};
explicit ParseError(const std::string& message) : Exception(message) {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Revert these changes. I believe these format-fixes were automatically added on Macos? They would likely cause a failure in format-check CI which runs on linux.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are auto-generated, I have reverted these changes.

presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.yml
@@ -175,7 +175,7 @@ AbstractClasses:
subclasses:
- { name: BuiltInFunctionHandle, key: $static }
- { name: SqlFunctionHandle, key: json_file }

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: revert this newline removal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted back the changes, Is there a need of two empty line?

presto-native-execution/presto_cpp/main/types/tests/CMakeLists.txt Outdated
@@ -74,6 +74,16 @@ target_link_libraries(
${GFLAGS_LIBRARIES}
pthread)

if(PRESTO_ENABLE_REMOTE_FUNCTIONS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change needed? No tests have been added for remote functions so could this be reverted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added these as part of build failure in MacOS, removed it.

@Joe-Abraham Joe-Abraham force-pushed the remote branch 2 times, most recently from 2024364 to 6af6ddb Compare February 3, 2025 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@ScrapCodes ScrapCodes Awaiting requested review from ScrapCodes ScrapCodes was automatically assigned from prestodb/ibm-reviewers

@aaneja aaneja Awaiting requested review from aaneja aaneja was automatically assigned from prestodb/ibm-reviewers

@pramodsatya pramodsatya Awaiting requested review from pramodsatya

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
from:IBM PR from IBM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Joe-Abraham @pramodsatya @prestodb-ci