Skip to content

Conversation

Joe-Abraham
Copy link

No description provided.

@prestodb-ci prestodb-ci added the from:IBM PRs from IBM label Oct 13, 2025
@Joe-Abraham Joe-Abraham requested a review from pdabre12 October 13, 2025 09:14
@Joe-Abraham Joe-Abraham marked this pull request as ready for review October 13, 2025 09:14
@prestodb-ci prestodb-ci requested review from a team and anandamideShakyan and removed request for a team October 13, 2025 09:14
Copy link
Contributor

@tdcmeehan tdcmeehan left a comment

Choose a reason for hiding this comment

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

LGTM, just one question

```

## Implementation Details
- Functions are registered in Velox as `catalog.schema.function_name`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose built-ins do not have a namespace?

Copy link
Contributor

Choose a reason for hiding this comment

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

They do, they will be under the namespace specified by the config property presto.default-namespace.

Copy link
Contributor

@pdabre12 pdabre12 left a comment

Choose a reason for hiding this comment

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

Thanks @Joe-Abraham .
Please change the RFC title to this Add RFC to support custom schemas in native sidecar function registry or something similar.

```

## Implementation Details
- Functions are registered in Velox as `catalog.schema.function_name`.
Copy link
Contributor

Choose a reason for hiding this comment

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

They do, they will be under the namespace specified by the config property presto.default-namespace.

@Joe-Abraham Joe-Abraham requested a review from pdabre12 October 14, 2025 06:00
@Joe-Abraham Joe-Abraham changed the title Add rfc for custom namespace Add RFC to support custom schemas in native sidecar function registry Oct 14, 2025
@kevintang2022
Copy link

How does the function resolution handle the case when the native worker registers functions under a namespace and the coordinator registers functions under the same function namespace? I wonder if only one function catalog can be mapped to exactly one namespace manager.

# etc/function-namespace/mycatalog.properties
function-namespace-manager.name=native
function-implementation-type=CPP
supported-function-languages=CPP

Choose a reason for hiding this comment

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

What do these 2 fields mean and what are the possible options for them ?

Copy link
Author

Choose a reason for hiding this comment

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

function-implementation-type - Indicates the language in which functions in this namespace are implemented. And it can take the following values

supported-function-languages - Languages supported by the namespace manager.
I don't see any pre-list of values this config could take, In current codebase cpp, sql and java are used

Copy link
Contributor

@pdabre12 pdabre12 Oct 15, 2025

Choose a reason for hiding this comment

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

However, I think the NativeFunctionNamespace manager only supports CPP.

@Joe-Abraham
Copy link
Author

@kevintang2022

How does the function resolution handle the case when the native worker registers functions under a namespace and the coordinator registers functions under the same function namespace? I wonder if only one function catalog can be mapped to exactly one namespace manager.

Only one function catalog can map to one namespace manager, native worker functions use a parallel system through BuiltInFunctionHandle with different kinds to coexist with coordinator functions without catalog collision.

Copy link
Contributor

@pdabre12 pdabre12 left a comment

Choose a reason for hiding this comment

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

Thanks @Joe-Abraham

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

Labels

from:IBM PRs from IBM

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants