Skip to content

Support setting a schema name with @ScalarFunction annotation#24703

Closed
ebyhr wants to merge 2 commits intotrinodb:masterfrom
ebyhr:ebi/core-function
Closed

Support setting a schema name with @ScalarFunction annotation#24703
ebyhr wants to merge 2 commits intotrinodb:masterfrom
ebyhr:ebi/core-function

Conversation

@ebyhr
Copy link
Copy Markdown
Member

@ebyhr ebyhr commented Jan 14, 2025

Description

Add schema to @ScalarFunction so developers can implement catalog functions with the annotation.

Window functions, aggregate functions, and scalar operators are unsupported in this PR. I will support them once we agree on the approach.

This is preparation for #24200

Release notes

## SPI
* Add `getFunctions` method to `io.trino.spi.connector.Connector`. ({issue}`24703`)
* Add `schema` option to `@ScalarFunction`. ({issue}`24703`)

@cla-bot cla-bot bot added the cla-signed label Jan 14, 2025
@ebyhr ebyhr force-pushed the ebi/core-function branch 3 times, most recently from 8d53593 to cb46961 Compare January 15, 2025 05:01
@ebyhr ebyhr marked this pull request as ready for review January 15, 2025 05:08
@ebyhr ebyhr force-pushed the ebi/core-function branch from cb46961 to d2ac6b5 Compare January 15, 2025 05:09
@ebyhr ebyhr requested review from kasiafi, martint and wendigo January 15, 2025 05:11
@github-actions github-actions bot added the stale label Feb 5, 2025
@ebyhr ebyhr force-pushed the ebi/core-function branch from d2ac6b5 to 552f5b2 Compare February 5, 2025 21:11
@github-actions github-actions bot added the faker Faker connector label Feb 5, 2025
@ebyhr ebyhr force-pushed the ebi/core-function branch from 552f5b2 to 66a965d Compare February 5, 2025 22:45
@trinodb trinodb deleted a comment from github-actions bot Feb 6, 2025
@github-actions github-actions bot removed the stale label Feb 6, 2025
@ebyhr ebyhr requested a review from dain February 14, 2025 10:36
@dain
Copy link
Copy Markdown
Member

dain commented Feb 17, 2025

Can you explain the function resolution rules when both systems (new class based and existing metadata lookup) are used?

@electrum
Copy link
Copy Markdown
Member

This would be useful for #24963, but that is more complicated as it needs a way for the connector to provide an instance of the function class, as the instance needs dependencies injected. Something to consider -- we don't need to solve that here.

.map(function -> new CatalogFunctionMetadata(catalogHandle, name.getSchemaName(), function))
.forEach(functions::add);

scalarFunctionRegistry.listFunctions(catalogHandle, name.getSchemaName()).stream()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it possible to provide all functions from a connector in a uniform way?
Here we have two separate mechanisms for the existing connector-provided functions and the new annotation-based connector-provided functions.
It makes the code complicated, and It is brittle in that the precedence is important.
For example here in MetadataManager we first list the "traditional" functions, and then the "new" functions.
However in FunctionManager.getScalarFunctionImplementationInternal, we first check the "new" functions.
If there is a duplicate, it could lead to a mismatch.

@ebyhr
Copy link
Copy Markdown
Member Author

ebyhr commented Mar 21, 2025

Closing as we already added Iceberg bucket function in #25175.

@ebyhr ebyhr closed this Mar 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed faker Faker connector

Development

Successfully merging this pull request may close these issues.

4 participants