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

Refactor ConnectorTableFunction into interface #12531

Merged
merged 2 commits into from
May 25, 2022

Conversation

kasiafi
Copy link
Member

@kasiafi kasiafi commented May 24, 2022

This change refactors ConnectorTableFunction from abstract class into interface. It also adds an abstract class AbstractConnectorTableFunction, which is meant to be base for all implementations of ConnectorTableFunction.

This is a SPI change.

Related to: #12325 (comment) (an interface is needed to apply a class-loader-safe wrapper)

Docs change included.

# SPI
* Change `ConnectorTableFunction` into an interface and add `AbstractConnectorTableFunction` class as the base implementation of table functions.


import static java.util.Objects.requireNonNull;

public class Preconditions
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 final, per https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#additional-ide-configuration

However, this should also be package-private, we don't want to make this part of SPI's contract.
Since this is used shared between io.trino.spi.ptf and io.trino.spi it cannot be package-private.

Please add this as a package-private utility class in io.trino.spi.ptf package, and use it only there.
Revert io.trino.spi.procedure changes.

@@ -14,7 +14,8 @@ through implementing dedicated interfaces.
Table function declaration
--------------------------

To declare a table function, you need to subclass ``ConnectorTableFunction``.
To declare a table function, you need to implement ``ConnectorTableFunction``
by subclassing ``AbstractConnectorTableFunction``.
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 say that AbstractConnectorTableFunction is helpful.
We shouldn't require extending from the class. The interface is the contract.

When thinking more about this, I am thinking what are these checks that AbstractConnectorTableFunction does. What would happen if a connector implemented the interface directly. Maybe the checks should be moved to TableFunctionRegistry?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that AbstractConnectorTableFunction is helpful because it provides the constructor.

However, it is probably a must to move the checks to the registry in case someone implements the interface directly.

Copy link
Member

Choose a reason for hiding this comment

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

let's do this. to avoid duplicated logic, let's remove them from AbstractConnectorTableFunction.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm going to leave only rnn there, and do the full validation in the registry.

@github-actions github-actions bot added the docs label May 24, 2022
@kasiafi kasiafi force-pushed the 353ConnectorTFAsInterface branch 2 times, most recently from 5d8a279 to 8805581 Compare May 24, 2022 13:42
@kasiafi
Copy link
Member Author

kasiafi commented May 24, 2022

@findepi PTAL

Introduce an abstract class as base for implementations
@kasiafi kasiafi force-pushed the 353ConnectorTFAsInterface branch from 8805581 to 4a7d72a Compare May 24, 2022 14:31
@kasiafi kasiafi merged commit 6030c40 into trinodb:master May 25, 2022
@github-actions github-actions bot added this to the 382 milestone May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants