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

implement rest endpoint #23214

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

implement rest endpoint #23214

wants to merge 2 commits into from

Conversation

jkhaliqi
Copy link
Contributor

@jkhaliqi jkhaliqi commented Jul 15, 2024

Description

Part of: https://github.com/prestodb/rfcs/pull/25/files
Conveniently right now we have java and c++ functions being called and most of the java functions work with cpp functions. Also the java function has to be identical to the cpp function for it to work. Now people have the choice to create their own functions and if someone creates a function that is not known to java we can either crash of have this method call so that it creates a function in cpp to be called and it does not crash. The point of those functions outside of java is to speed up planning of execution and nodes by these functions that are not there in java.

Motivation and Context

Open issue: https://github.ibm.com/lakehouse/tracker/issues/8413

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 ==

General Changes
* ... :pr:`12345`
* ... :pr:`12345`

Hive Connector Changes
* ... :pr:`12345`
* ... :pr:`12345`

If release note is NOT required, use:

== NO RELEASE NOTE ==

Copy link

linux-foundation-easycla bot commented Jul 15, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: abevk2023 / name: Abe Varghese (5434766)
  • ✅ login: jkhaliqi (7507b50)

@jkhaliqi jkhaliqi force-pushed the rest branch 3 times, most recently from 8ddc7f8 to aee25a0 Compare July 23, 2024 21:14
@jkhaliqi jkhaliqi force-pushed the rest branch 2 times, most recently from aa6e1e4 to 6025e69 Compare July 30, 2024 17:54
Copy link
Contributor

@tdcmeehan tdcmeehan left a comment

Choose a reason for hiding this comment

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

Please rebase your PR and add the OpenAPI documentation to the OpenAPI module

@jkhaliqi jkhaliqi force-pushed the rest branch 2 times, most recently from c4e4f41 to 345c4a9 Compare August 14, 2024 18:32
@jkhaliqi jkhaliqi force-pushed the rest branch 2 times, most recently from cf6e164 to 443e3b8 Compare August 15, 2024 16:36
@jkhaliqi jkhaliqi marked this pull request as ready for review August 15, 2024 16:37
@jkhaliqi jkhaliqi requested a review from presto-oss August 15, 2024 16:37
Copy link
Contributor

@tdcmeehan tdcmeehan left a comment

Choose a reason for hiding this comment

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

We'll need to create a test that proves this works. See an example here for expression evaluation that sets up a mock endpoint for evaluation: https://github.com/prestodb/presto/pull/23331/files#diff-48ca45cdbb379b0774a87a3b94925c737039a2975be40c8a282843c9b26717f2R118

Give it a try and let me know if you need help implementing it.

@aditi-pandit
Copy link
Contributor

@jkhaliqi : Please can you give more information about this PR and the functionality. Is this part of any RFC ?

@tdcmeehan
Copy link
Contributor

It's part of https://github.com/prestodb/rfcs/pull/25/files

@jkhaliqi let's update the PR description to give more context.

@jkhaliqi jkhaliqi force-pushed the rest branch 2 times, most recently from b561100 to 7ef269c Compare August 27, 2024 23:41
@@ -149,6 +149,16 @@
<scope>provided</scope>
</dependency>

<dependency>
<groupId>com.facebook.drift</groupId>
<artifactId>drift-transport-netty</artifactId>
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 dependency? If so, why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this dependency is needed for the MySqlFunctionNamespaceManagerFactory where it is creating a new DriftNettyClientModule(). I just changed the code position around because it was under the testing section so I put it above that section in the pom.xml file.

install(sqlFunctionExecutorModule);
// for SqlFunctionExecutors
binder.bind(SqlFunctionExecutors.class).in(SINGLETON);
binder.bind(new TypeLiteral<Map<Language, FunctionImplementationType>>() {}).toInstance(languageImplementationTypeMap);
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this injected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it is injected in SqlFunctionExecutors

@Inject
public SqlFunctionExecutors(Map<Language, FunctionImplementationType> supportedLanguages, SqlFunctionExecutor sqlFunctionExecutor)
{
    this.supportedLanguages = requireNonNull(supportedLanguages, "supportedLanguages is null");
    this.sqlFunctionExecutor = requireNonNull(sqlFunctionExecutor, "sqlFunctionExecutor is null");
}

@jkhaliqi jkhaliqi force-pushed the rest branch 3 times, most recently from 066697a to afa8ed5 Compare September 5, 2024 20:32
@jkhaliqi jkhaliqi force-pushed the rest branch 7 times, most recently from d8e2ed8 to 42700ba Compare September 11, 2024 16:32
@ZacBlanco ZacBlanco removed their request for review September 11, 2024 20:05
@steveburnett steveburnett removed their request for review September 12, 2024 16:45
@jkhaliqi jkhaliqi force-pushed the rest branch 2 times, most recently from b444c21 to 191c633 Compare September 20, 2024 15:44
@jkhaliqi jkhaliqi force-pushed the rest branch 2 times, most recently from 10fccd8 to 77f71ac Compare October 25, 2024 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants