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

Dynamically Linked Functions Library in CPP #23634

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

Dynamically Linked Functions Library in CPP #23634

wants to merge 2 commits into from

Conversation

soumiiow
Copy link

Description

RFC for the changes prestodb/rfcs#24
These changes will allow users to dynamically load functions in prestissimo using cpp

Motivation and Context

Having these changes will enable users to register custom functions dynamically without requiring a fork of Prestissimo.

Impact

Test Plan

currently the unit test and container tests are still being debugged. and are being actively worked on

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

@soumiiow soumiiow self-assigned this Sep 12, 2024
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Sep 12, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@tdcmeehan tdcmeehan self-assigned this Sep 12, 2024
steveburnett
steveburnett requested changes Sep 12, 2024
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.

Thanks for the documentation! I suggested some changes to the README to simplify some of the wording and improve readability.

If my suggestions change a sentence's meaning in a way that makes it incorrect, let me know and we can find a better phrasing together.

presto-native-execution/presto_cpp/main/functions/README.md Outdated Show resolved Hide resolved
presto-native-execution/presto_cpp/main/functions/README.md Outdated

## Getting started

The Process can be roughly broken down into the steps below:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The Process can be roughly broken down into the steps below:

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The Process can be roughly broken down into the steps below:
The process can be roughly broken down into the steps below:

I think you can delete this sentence entirely, but if you feel it valuable to keep, please lowercase "process".

presto-native-execution/presto_cpp/main/functions/README.md Outdated Show resolved Hide resolved
presto-native-execution/presto_cpp/main/functions/README.md Outdated
target_link_libraries(name_of_dynamic_fn PRIVATE xsimd fmt::fmt velox_expression)
```

### 3. In the prestissimo worker's config.properties file, set the plugin.dir property
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### 3. In the prestissimo worker's config.properties file, set the plugin.dir property
### 3. In the Prestissimo worker's config.properties file, set the plugin.dir property

presto-native-execution/presto_cpp/main/functions/README.md Outdated Show resolved Hide resolved
@soumiiow
Copy link
Author

@steveburnett please take another look when you get a chance, i took your suggestions and made the updates to the readme

steveburnett
steveburnett requested changes Sep 24, 2024
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.

Nice work, thanks! A few new minor suggestions for you to consider.

presto-native-execution/presto_cpp/main/functions/README.md Outdated

## Getting started

The Process can be roughly broken down into the steps below:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The Process can be roughly broken down into the steps below:
The process can be roughly broken down into the steps below:

I think you can delete this sentence entirely, but if you feel it valuable to keep, please lowercase "process".

presto-native-execution/presto_cpp/main/functions/README.md Outdated Show resolved Hide resolved
presto-native-execution/presto_cpp/main/functions/README.md Outdated Show resolved Hide resolved
presto-native-execution/presto_cpp/main/functions/README.md Outdated Show resolved Hide resolved
presto-native-execution/presto_cpp/main/functions/README.md Outdated Show resolved Hide resolved
@soumiiow
Copy link
Author

@steveburnett. ended up accepting all your suggestions. thank you!

steveburnett
steveburnett previously approved these changes Sep 24, 2024
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.

LGTM! (docs)

Viewed README.md in GitHub to see how the text looks once it's displayed. Thanks!

@steveburnett
Copy link
Contributor

@soumiiow you're welcome! When you have time, please sign the CLA as mentioned in this comment.

@soumiiow soumiiow force-pushed the dyllib-files-testing-macos branch 2 times, most recently from 75287f9 to 5ff8dff Compare September 25, 2024 17:10
@soumiiow
builds properly but leads to two translation units and the test fails…
2b46057 
… as the function is registered into the wrong map
@soumiiow soumiiow mentioned this pull request Nov 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steveburnett steveburnett steveburnett left review comments

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

Assignees

@tdcmeehan tdcmeehan

@soumiiow soumiiow

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@soumiiow @steveburnett @tdcmeehan