Prepare function guidelines #23291
Merged
Prepare function guidelines #23291
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
steveburnett
requested changes
Aug 13, 2024
There was a problem hiding this comment.
Thanks for this! A few minor nits.
Suggest considering where to link to this new document from - maybe add a link to FUNCTIONS.md to CONTRIBUTING.md, or to the Presto Development Guidelines.
Perhaps the contents of this document could be a new section in the Presto Development Guidelines.
tdcmeehan
reviewed
Aug 15, 2024
tdcmeehan
reviewed
Aug 16, 2024
FUNCTIONS.md
Outdated
|
|
||
| Presto also includes extension functions that can optionally be enabled in particular deployments. | ||
|
|
||
| Finally, Presto allows users to write user defined functions (UDFs) in either Java or SQL that are not stored in this GitHub repository. |
There was a problem hiding this comment.
Suggested change
| Finally, Presto allows users to write user defined functions (UDFs) in either Java or SQL that are not stored in this GitHub repository. | |
| Finally, Presto allows users to write user defined functions (UDFs) in either Java or SQL that are not stored in this GitHub repository. [This is done through the Plugin SPI.](https://prestodb.io/docs/current/develop/functions.html) |
elharo
force-pushed
the
fx
branch
3 times, most recently
from
0f5cd8a to
3a18c33
Compare
FUNCTIONS.md
Outdated
|
|
||
| Presto includes a large library of built-in, core SQL functions such as AVG and CONCAT that are available in every deployment. | ||
|
|
||
| Presto also includes extension functions that are not enabled by default but can be optionally enabled in particular deployments. |
There was a problem hiding this comment.
Would be great to have an example of these.
There was a problem hiding this comment.
agreed. Anyone happen to know of any? I'll look in the code, but if anyone can point to one it would save me a bit of time
There was a problem hiding this comment.
I believe this is implicit in the fact that functions are a part of the Plugin interface. The way to "enable" functions is to add the plugins. I don't think there's really any examples of functions that are enabled via a toggle.
elharo
force-pushed
the
fx
branch
5 times, most recently
from
3fd0b1a to
a9f69af
Compare
|
One thing just occurred to me, we should probably link to this document from CONTRIBUTING. |
sure, let me figure out links work in markdown |
elharo
force-pushed
the
fx
branch
2 times, most recently
from
239c0c3 to
07b4890
Compare
tdcmeehan
previously approved these changes
Aug 22, 2024
tdcmeehan
approved these changes
Aug 23, 2024
tdcmeehan
requested changes
Aug 23, 2024
FUNCTIONS.md
Outdated
|
|
||
| Built-in functions that extend Presto into new domains should have a detailed plan and ideally an implementation for a library of functions that cover the domain. For instance, a function that only added US dollars would not be accepted. However, a general library for working with currency data along the lines of [Joda-Money](https://www.joda.org/joda-money/) could be considered. It can be helpful to prototype complex functionality like this in UDFs before writing an RFC and submitting it to the Presto project. | ||
|
|
||
| Built-in and extension functions bundled with Presto must be freely available without fee and compatible with the Presto license. Functions using algorithms that are in any way restricted by copyright, patent, trademark, field of use, or other reasons will not be included in this repository. For instance, a patented video CODEC cannot be used. Functions that include trademarked names should be UDFs outside this repository. |
There was a problem hiding this comment.
There is no Presto license.
Suggested change
| Built-in and extension functions bundled with Presto must be freely available without fee and compatible with the Presto license. Functions using algorithms that are in any way restricted by copyright, patent, trademark, field of use, or other reasons will not be included in this repository. For instance, a patented video CODEC cannot be used. Functions that include trademarked names should be UDFs outside this repository. | |
| Built-in and extension functions bundled with Presto must be freely available without fee and compatible with an Apache license. Functions using algorithms that are in any way restricted by copyright, patent, trademark, field of use, or other reasons will not be included in this repository. For instance, a patented video CODEC cannot be used. Functions that include trademarked names should be UDFs outside this repository. |
elharo
commented
Sep 3, 2024
tdcmeehan
approved these changes
Sep 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Elucidate what functions do and don't belong in core
Motivation and Context
Mutliple PRs have been proposing individual functions for very specific purposes without taking a holistic view of what Presto's function library should look like.
Impact
none
Test Plan
CI
Contributor checklist
Release Notes