Skip to content

Add Presto TVF RFC #39

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
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add Presto TVF RFC #39

wants to merge 1 commit into from

Conversation

aditi-pandit
Copy link

No description provided.

@prestodb-ci prestodb-ci added the from:IBM PRs from IBM label Jun 10, 2025
@prestodb-ci prestodb-ci requested review from a team, NivinCS and jp-sivaprasad and removed request for a team June 10, 2025 06:40
@jaystarshot
Copy link
Member

jaystarshot commented Jul 16, 2025 •
edited
Loading

LGTM, it would be great if we could split the RFC into what is configurable and what needs to be configured and what is built in so that its easier for the users to read the RFC directly

jaystarshot
jaystarshot reviewed Jul 16, 2025
View reviewed changes
RFC-0011-tvf.md
private final String schema;
private final String name;
private final List<ArgumentSpecification> arguments;
private final ReturnTypeSpecification returnTypeSpecification;
Copy link
Member

Choose a reason for hiding this comment

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

What is the ReturnTypeSpecification ?

Copy link
Author

Choose a reason for hiding this comment

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

Had missed the api. Have added it.

@rschlussel
Copy link
Contributor

@feilong-liu @kaikalur

@aditi-pandit
Copy link
Author

LGTM, it would be great if we could split the RFC into what is configurable and what needs to be configured and what is built in so that its easier for the users to read the RFC directly

I had split the declaration, analysis parts of the API and the execution API with the planning pieces (from the framework) in between. I can see why this is confusing as the user does not have continuity with the API. Have changed the writing now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaystarshot jaystarshot jaystarshot left review comments

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

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

@tdcmeehan tdcmeehan Awaiting requested review from tdcmeehan

@rschlussel rschlussel Awaiting requested review from rschlussel

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

Successfully merging this pull request may close these issues.
4 participants
@aditi-pandit @jaystarshot @rschlussel @prestodb-ci