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

Simplify adding table functions #18660

Closed
wants to merge 1 commit into from
Closed

Simplify adding table functions #18660

wants to merge 1 commit into from

Conversation

vlad-lyutenko
Copy link
Contributor

Description

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Aug 13, 2023
@vlad-lyutenko vlad-lyutenko marked this pull request as draft August 13, 2023 15:20
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/simplify-add-table-function branch 2 times, most recently from 5397b92 to 2a2a6aa Compare August 13, 2023 19:31
@findepi findepi changed the title Simlify adding table functions Simplify adding table functions Aug 14, 2023
@findepi
Copy link
Member

findepi commented Aug 14, 2023

nit: typo in cmt msg Simlify -> Simplify

@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/simplify-add-table-function branch 3 times, most recently from 24bc132 to 3871173 Compare August 14, 2023 16:39
findepi
findepi reviewed Aug 16, 2023
View reviewed changes
core/trino-main/src/main/java/io/trino/metadata/TableFunctionProcessorProviderFactory.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/metadata/TableFunctionProcessorProviderFactory.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/metadata/TableFunctionProcessorProviderFactory.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/metadata/TableFunctionProcessorProviderFactory.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/metadata/TableFunctionProcessorProviderFactory.java Outdated
import io.trino.spi.function.table.ConnectorTableFunctionHandle;
import io.trino.spi.function.table.TableFunctionProcessorProvider;

public interface TableFunctionProcessorProviderFactory
Copy link
Member

Choose a reason for hiding this comment

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

Should this and ConnectorSplitSourceFactory be one interface?

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 to single TableFunctionFeaturesFactory interface,
but I am not sure about the name

core/trino-main/src/main/java/io/trino/operator/table/ExcludeColumns.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/operator/table/Sequence.java Outdated Show resolved Hide resolved
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/simplify-add-table-function branch from 3871173 to 47ca30c Compare August 16, 2023 10:25
@vlad-lyutenko vlad-lyutenko requested a review from findepi August 16, 2023 10:27
@findepi findepi requested a review from kokosing August 16, 2023 12:26
kokosing
kokosing reviewed Aug 16, 2023
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

I like this refactor.

core/trino-main/src/main/java/io/trino/metadata/TableFunctionFeaturesFactory.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/connector/system/GlobalSystemConnector.java Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/connector/system/GlobalSystemConnector.java Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/connector/system/GlobalSystemConnector.java Show resolved Hide resolved
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/simplify-add-table-function branch 5 times, most recently from e374de0 to b2b2f64 Compare August 17, 2023 15:00
@kokosing
Copy link
Member

Is this still a draft?

@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/simplify-add-table-function branch from b2b2f64 to 743f4d7 Compare August 21, 2023 08:46
@vlad-lyutenko vlad-lyutenko marked this pull request as ready for review August 21, 2023 08:47
Praveen2112
Praveen2112 reviewed Aug 21, 2023
View reviewed changes
core/trino-main/src/main/java/io/trino/connector/system/GlobalSystemConnector.java Outdated

@Inject
public GlobalSystemConnector(Set<SystemTable> systemTables, Set<Procedure> procedures, Set<ConnectorTableFunction> tableFunctions)
public GlobalSystemConnector(Set<SystemTable> systemTables, Set<Procedure> procedures, Set<ConnectorTableFunction> tableFunctions, Set<TableFunctionProvider> tableFunctionProviders)
Copy link
Member

Choose a reason for hiding this comment

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

nit: Can we swap the arguments ?

Copy link
Member

Choose a reason for hiding this comment

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

Are we going to replace the tableFunctions to tableFunctionProvider

Copy link
Contributor Author

@vlad-lyutenko vlad-lyutenko Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think no.
The problem is with naming.
TableFunctionProvider - it's kind of interface, which is needed in some rare cases for functions like Sequence and Exclude Columns.
For this functions we need some additional functionality like TableFunctionProcessorProvider and ConnectorSplitSource. But for all other cases we don need them.

core/trino-main/src/main/java/io/trino/connector/system/GlobalSystemConnector.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/connector/system/SystemConnectorModule.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/metadata/GlobalFunctionCatalog.java Outdated Show resolved Hide resolved
@vlad-lyutenko
Introduce TableFunctionProvider
ef99a66 
to simplify adding table functions, we are not tightly coupled
with concrete table connector function implementations in
GlobalSystemConnector and GlobalFunctionCatalog now.
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/simplify-add-table-function branch from 743f4d7 to ef99a66 Compare August 21, 2023 10:38
martint
martint reviewed Aug 21, 2023
View reviewed changes
Copy link
Member

@martint martint left a comment •
edited
Loading

Choose a reason for hiding this comment

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

What's the motivation for this change?

Except for a controlled set of functions, table functions should be provided via connectors and mounted under a catalog/schema namespace. This seems to generalize the mechanism for adding functions to the global catalog, which is the opposite of the direction we're trying to go in (see #8)

core/trino-main/src/main/java/io/trino/connector/system/GlobalSystemConnector.java

throw new UnsupportedOperationException();
checkArgument(tableFunctionAdditionalFeatures.size() <= 1, "we should have only one implementation for concrete function handle or nothing");
Copy link
Member

Choose a reason for hiding this comment

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

This should be a TrinoException with FUNCTION_IMPLEMENTATION_ERROR error code. Also, rephrase as "multiple implementations found for a function handle".

@Praveen2112
Copy link
Member

What's the motivation for this change?

The goal is to simplify the registration of a function which is closer to the engine like if we are having a different table function like SequenceFunctionHandle we could need to introduce another if statement validating for the functionHandle implementation.

Is there is any other approaches we could take to simplify the table function registration.

@martint
Copy link
Member

martint commented Aug 29, 2023

As I described above, we should avoid adding functions in the global namespace and, instead, implement them via connectors that get mounted in a catalog/schema. It's very likely that in the future we'll move those existing functions to a connector.

@github-actions GitHub Actions
Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Jan 11, 2024
@mosabua
Copy link
Member

mosabua commented Jan 11, 2024

@vlad-lyutenko I assume you are still continuing on this work. If not, please close the PR.

@vlad-lyutenko vlad-lyutenko deleted the vlad-lyutenko/simplify-add-table-function branch January 11, 2024 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Praveen2112 Praveen2112 Praveen2112 left review comments

@martint martint martint left review comments

@kokosing kokosing kokosing approved these changes

@findepi findepi Awaiting requested review from findepi

Assignees
No one assigned
Labels
cla-signed stale
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vlad-lyutenko @findepi @kokosing @Praveen2112 @martint @mosabua