Add a native function namespace manager #23358

pdabre12 · 2024-08-01T19:41:14Z

Description

Adds a native function namespace manager

Motivation and Context

To help resolve : #23000

Impact

Test Plan

Unit and end-to-end tests. More comprehensive end-to-end tests will be written in the future.

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

== RELEASE NOTES ==

General C++ changes
* Add a native function namespace manager :pr:`23358`

Depends on : #23671

linux-foundation-easycla · 2024-08-01T19:41:19Z

The committers listed above are authorized under a signed CLA.

✅ login: pdabre12 / name: Pratik Joseph Dabre (4e031a7, 63b7d00, 0f0836c, dcc7dc9, 908b0a2, 97d2585, c906754, 2e87ea2)

steveburnett

LGTM! (docs)

Pull branch, new local doc build, the doc looks good. Thanks!

steveburnett

LGTM! (docs)

Pull updated branch, new local doc build, doc looks good. Thanks!

tdcmeehan

Just some initial comments

presto-common/src/main/java/com/facebook/presto/common/type/TypeSignature.java

...n/java/com/facebook/presto/functionNamespace/AbstractSqlInvokedFunctionNamespaceManager.java

presto-function-namespace-managers-common/pom.xml

presto-grpc-api/pom.xml

presto-jdbc/pom.xml

presto-main/pom.xml

presto-main/src/main/java/com/facebook/presto/metadata/FunctionAndTypeManager.java

tdcmeehan

Refactored qualifyObjectName from static to instance method ✅

tdcmeehan

Add utility functions to resolve intermediate type in aggregate funct… ❓

Thank you for the really extensive tests for this utility. I think the tests should be broken down to the various different types, because the tests are rather large.

tdcmeehan

[native] Fix bug when handling multiple params and no params aggregat… ❓

Can you help me understand when this bug manifests? Is there any way to add a unit test for it?

presto-spi/src/main/java/com/facebook/presto/spi/ConnectorSystemConfig.java

aditi-pandit · 2025-01-31T21:40:40Z

presto-native-execution/presto_cpp/main/common/tests/ConfigTest.cpp

+  folly::split(
+      '.',
+      prestoDefaultNamespacePrefix,
+      prestoDefaultNamespacePrefixParts,


Add CHECK that prestoDefaultNamespacePrefixParts has only 2 parts.

aditi-pandit

@pdabre12 : Have 2 minor comments.

aditi-pandit · 2025-01-31T21:41:33Z

presto-native-execution/presto_cpp/main/common/tests/ConfigTest.cpp

+
+    std::vector<std::string> parts;
+    folly::split('.', functionName, parts, true);
+    if ((parts.size() != 3) ||


Should this be a check instead ? Are there functions that don't follow since (since you added all the built-ins with prefixes) ?

All the functions should have a prefix. Added the check, thanks.

aditi-pandit · 2025-01-31T21:43:44Z

presto-native-execution/presto_cpp/main/types/PrestoToVeloxExpr.h

 #include "presto_cpp/presto_protocol/core/presto_protocol_core.h"
 #include "velox/core/Expressions.h"

+namespace {


Consider moving this to presto_cpp/main/common/Utils.h

…te functions

…oint

…ation tests

aditi-pandit

Thanks @pdabre12

pdabre12 force-pushed the native-function-namespace-manager branch from 49d3b9d to 5ed5a18 Compare August 13, 2024 23:37

pramodsatya mentioned this pull request Aug 15, 2024

[native] Retrieve Json function metadata for prestissimo functions #22332

Merged

pdabre12 force-pushed the native-function-namespace-manager branch 2 times, most recently from bcb09bc to ce7ca4b Compare August 17, 2024 00:06

steveburnett previously approved these changes Sep 10, 2024

View reviewed changes

pdabre12 dismissed steveburnett’s stale review via 4d00991 September 12, 2024 18:28

pdabre12 force-pushed the native-function-namespace-manager branch from 4d00991 to c3ed29f Compare September 13, 2024 23:03

pdabre12 requested a review from steveburnett September 13, 2024 23:05

pdabre12 changed the title ~~[WIP] Native function namespace manager~~ [WIP] Add a native function namespace manager Sep 13, 2024

steveburnett previously approved these changes Sep 25, 2024

View reviewed changes

pdabre12 dismissed steveburnett’s stale review via 3fde055 September 27, 2024 00:48

pdabre12 force-pushed the native-function-namespace-manager branch 2 times, most recently from 3fde055 to ebfa361 Compare September 27, 2024 05:55

pdabre12 changed the title ~~[WIP] Add a native function namespace manager~~ Add a native function namespace manager Sep 27, 2024

pdabre12 force-pushed the native-function-namespace-manager branch from f203d25 to 88e1152 Compare November 1, 2024 01:09

pdabre12 marked this pull request as ready for review November 1, 2024 16:40

pdabre12 requested review from a team, ClarenceThreepwood, elharo, feilong-liu, jaystarshot and shrinidhijoshi as code owners November 1, 2024 16:40

pdabre12 requested a review from presto-oss November 1, 2024 16:40

tdcmeehan self-assigned this Nov 11, 2024

tdcmeehan requested changes Nov 11, 2024

View reviewed changes

tdcmeehan reviewed Nov 12, 2024

View reviewed changes

tdcmeehan requested changes Nov 13, 2024

View reviewed changes

tdcmeehan reviewed Nov 13, 2024

View reviewed changes

tdcmeehan requested changes Jan 31, 2025

View reviewed changes

presto-spi/src/main/java/com/facebook/presto/spi/ConnectorSystemConfig.java Outdated Show resolved Hide resolved

pdabre12 force-pushed the native-function-namespace-manager branch from d7dec93 to c4fe9ec Compare January 31, 2025 17:18

pdabre12 requested a review from tdcmeehan January 31, 2025 17:21

pdabre12 force-pushed the native-function-namespace-manager branch from c4fe9ec to 38edeb3 Compare January 31, 2025 18:25

tdcmeehan previously approved these changes Jan 31, 2025

View reviewed changes

aditi-pandit reviewed Jan 31, 2025

View reviewed changes

pdabre12 added 8 commits January 31, 2025 15:17

[native] Add presto.default-namespace Prestissimo config property

0f0836c

Add utility functions to resolve intermediate type in generic aggrega…

908b0a2

…te functions

[native] Add additional function metadata to function signatures endp…

4e031a7

…oint

Add a new module presto-function-namespace-managers-common

2e87ea2

Change built-in namespace to JAVA_BUILTIN_NAMESPACE

97d2585

[native] Introduce native function namespace SPI

c906754

[native] Integrate SPI with sidecar and add e2e native function valid…

dcc7dc9

…ation tests

Add OpenAPI documentation for /v1/functions

63b7d00

pdabre12 dismissed tdcmeehan’s stale review via 63b7d00 January 31, 2025 23:25

pdabre12 force-pushed the native-function-namespace-manager branch from 38edeb3 to 63b7d00 Compare January 31, 2025 23:25

aditi-pandit approved these changes Jan 31, 2025

View reviewed changes

tdcmeehan approved these changes Feb 1, 2025

View reviewed changes

pdabre12 merged commit e4be876 into prestodb:master Feb 1, 2025
62 checks passed

This was referenced Feb 1, 2025

[native] Add presto.default-namespace Prestissimo config property #23384

Closed

[native] Enhancing namespace support for Prestissimo functions in native C++ clusters #23385

Closed

pdabre12 deleted the native-function-namespace-manager branch February 1, 2025 03:32

This was referenced Mar 10, 2025

Add release notes for 0.292 unix280/presto#5

Closed

Add release notes for 0.292 unix280/presto#6

Closed

prestodb-ci mentioned this pull request Mar 28, 2025

Add release notes for 0.292 #24825

Merged

30 tasks

unidevel mentioned this pull request Apr 25, 2025

Add release notes for 0.292 unix280/presto#23

Closed

30 tasks

This was referenced May 6, 2025

Add release notes for 0.292 unix280/presto#28

Closed

Add release notes for 0.292 unix280/presto#29

Closed

pdabre12 mentioned this pull request Sep 5, 2025

[native] Introduce presto-native-sql-invoked-functions-plugin for sidecar enabled clusters #25870

Merged

6 tasks

Add a native function namespace manager #23358

Add a native function namespace manager #23358

Uh oh!

Conversation

pdabre12 commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Uh oh!

linux-foundation-easycla bot commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aditi-pandit Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

aditi-pandit Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

pdabre12 Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aditi-pandit Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pdabre12 commented Aug 1, 2024 •

edited

Loading

linux-foundation-easycla bot commented Aug 1, 2024 •

edited

Loading

pdabre12 Jan 31, 2025 •

edited

Loading