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

[native] Advance velox and use velox companionFunction metadata in FunctionMetadata.cpp #24482

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pramodsatya
Copy link
Contributor

Description

Velox companion functions are excluded in the function signature map returned along the v1/functions endpoint by Presto native sidecar. Currently, these companion functions are detected by searching for specific suffixes in the registered velox function name. companionFunction metadata was added to velox scalar, aggregate, and window function metadata recently as per this discussion. This PR detects companion functions using velox function metadata instead, and advances velox to include the respective commit.

Motivation and Context

Follows from this discussion and from this PR.

== NO RELEASE NOTE ==

@pramodsatya pramodsatya requested a review from a team as a code owner February 4, 2025 03:22
@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Feb 4, 2025
@prestodb-ci prestodb-ci requested a review from a team February 4, 2025 03:22
@@ -287,7 +262,7 @@ json getFunctionsMetadata() {
// Skip internal functions. They don't have any prefix.
if (kBlockList.count(name) != 0 ||
name.find("$internal$") != std::string::npos ||
isCompanionFunctionName(name, aggregateFunctions)) {
getScalarMetadata(name).companionFunction) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this change in the same PR as the velox update?
It is ok if we update velox and then make the change as a follow up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:IBM PR from IBM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants