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

pramodsatya · 2024-03-26T05:16:47Z

Motivation and Context

For fail fast function validation in Prestissimo, the coordinator needs to know the list of function signatures that are supported by Prestissimo. This is so we can validate during parsing and analysis that a function is supported. In order to expose this, a new HTTP method is added that returns the function signatures supported by the Prestissimo worker.

The format of the functions should be a map of name to function signature. Name is a simple string. Function signature is a JSON object which matches the format of JsonBasedUdfFunctionMetadata in the Java code. All functions currently come from Velox, hence we need to retrieve the list of supported functions in Velox. This map is translated to a format that conforms to JsonBasedUdfFunctionMetadata and is serialized to JSON for consumption by the Java coordinator.

Description

For full context, see #23000

This PR adds a http endpoint /v1/functions which returns the list of function signatures, conforming to format JsonBasedUdfFunctionMetadata, for all the presto functions supported in velox. Depends on Velox changes in the PR. This endpoint is registered in PrestoServer only when it is configured as a sidecar.

Test Plan

Unit tests are added in FunctionMetadataTest.cpp.

== NO RELEASE NOTE ==

cc: @tdcmeehan @soumiiow

aditi-pandit · 2024-06-13T00:55:52Z

@pramodsatya : Please give more information about how this http method will be used in Presto server.

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

presto-native-execution/presto_cpp/main/types/tests/FunctionMetadataTest.cpp

presto-native-execution/presto_cpp/main/types/FunctionMetadata.cpp

pramodsatya

Thanks for the feedback @aditi-pandit, addressed the review comments. Could you please take another look?

mbasmanova

@pramodsatya Would you document the new endpoint somewhere in https://prestodb.io/docs/current/develop/worker-protocol.html ?

tdcmeehan · 2024-08-02T02:13:41Z

@mbasmanova we have started to document our endpoints using OpenAPI, and eventually all sidecar endpoints (and, hopefully all Presto endpoints) will have corresponding OpenAPI documentation.

mbasmanova · 2024-08-02T03:02:29Z

we have started to document our endpoints using OpenAPI, and eventually all sidecar endpoints (and, hopefully all Presto endpoints) will have corresponding OpenAPI documentation.

Sounds cool. Where can I see docs for the new endpoint?

aditi-pandit

@pramodsatya : The changes look good. Please add more test cases though... We should cover functions with different types, template parameters, array, map functions as well.

presto-native-execution/presto_cpp/presto_protocol/presto_protocol.yml

presto-native-execution/presto_cpp/main/types/FunctionMetadata.cpp

presto-native-execution/presto_cpp/main/types/tests/FunctionMetadataTest.cpp

steveburnett

Thanks for the doc! Pulled branch and made a local docs build. Some nits of capitalization, everything else looks fine.

presto-docs/src/main/sphinx/develop/worker-protocol.rst

czentgr

Thanks, I have a few comments.

presto-docs/src/main/sphinx/develop/sidecar-protocol.rst

presto-native-execution/presto_cpp/main/PrestoServer.cpp

presto-native-execution/presto_cpp/main/types/tests/CMakeLists.txt

presto-native-execution/presto_cpp/presto_protocol/presto_protocol.yml

presto-native-execution/presto_cpp/presto_protocol/special/Language.cpp.inc

pramodsatya · 2024-08-03T02:21:32Z

Thanks, I have a few comments.

Thanks for the feedback @czentgr, I have addressed the comments. Could you please take another look?

steveburnett

Thanks for the new doc! I have a suggestion about where the new page should be located, let me know what you think.

presto-docs/src/main/sphinx/develop.rst

pramodsatya · 2024-08-06T16:52:42Z

Thanks for the new doc! I have a suggestion about where the new page should be located, let me know what you think.

Thanks for the suggestion @steveburnett, yes I agree presto-cpp.rst seems more suitable for this doc and I moved it there. Could you please take another look?

steveburnett

LGTM! (docs)

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

presto-native-execution/presto_cpp/presto_protocol/special/Language.cpp.inc

presto-native-execution/presto_cpp/main/types/FunctionMetadata.cpp

presto-native-execution/presto_cpp/main/types/tests/FunctionMetadataTest.cpp

…#10664) Summary: Prefix is not needed to register custom type `HyperLogLog` in `ApproxDistinctAggregates`. `registerHyperLogLogType` is used instead to make registration of customType `HyperLogLog` uniform across velox. Use of prefix in `registerCustomType` here resulted in the following error while running tests in [presto PR](prestodb/presto#22332): ``` Reason: Type doesn't exist: 'HYPERLOGLOG' Retriable: False Function: validateBaseTypeAndCollectTypeParams File: /Users/pramod/Desktop/velox/velox/expression/FunctionSignature.cpp Line: 125 ``` Pull Request resolved: #10664 Reviewed By: bikramSingh91 Differential Revision: D61163251 Pulled By: xiaoxmeng fbshipit-source-id: 8d316a9bc863a779ab81a50fb06b5d3761d8316b

presto-native-execution/presto_cpp/main/types/FunctionMetadata.cpp

steveburnett · 2024-08-15T14:33:07Z

@pramodsatya, I notice that the documentation I reviewed is not part of this PR now. Can I ask why?

pramodsatya · 2024-08-15T16:26:59Z

@pramodsatya, I notice that the documentation I reviewed is not part of this PR now. Can I ask why?

Hi @steveburnett, @tdcmeehan shared that the OpenAPI documentation for this endpoint will be added in: #23358 , so I reverted the previous documentation. Would that be fine?

steveburnett · 2024-08-15T16:36:42Z

Hi @steveburnett, @tdcmeehan shared that the OpenAPI documentation for this endpoint will be added in: #23358 , so I reverted the previous documentation. Would that be fine?

Thanks @pramodsatya, I missed that! I thought there was probably a good reason for it that I had overlooked and didn't know what it was. Yes, that's fine with me, and I appreciate your explanation.

aditi-pandit · 2024-08-22T16:13:26Z

@pramodsatya : Please can you rebase your code. I'll continue with the review then.

pramodsatya · 2024-08-22T23:11:28Z

@pramodsatya : Please can you rebase your code. I'll continue with the review then.

Thanks @aditi-pandit, rebased and fixed the CI failures.

aditi-pandit · 2024-09-06T05:53:07Z

presto-native-execution/presto_cpp/presto_protocol/presto_protocol.yml

Agree that there isn't particular organization in this file. But we should make it in alphabetical order. Will send out a PR for it.

presto-native-execution/presto_cpp/main/types/FunctionMetadata.cpp

presto-native-execution/presto_cpp/main/types/tests/FunctionMetadataTest.cpp

pramodsatya requested review from tdcmeehan, czentgr and aditi-pandit March 26, 2024 05:16

pramodsatya mentioned this pull request Mar 26, 2024

Add isCompanionFunction to function metadata facebookincubator/velox#9250

Open

pramodsatya force-pushed the fn_metadata branch from 3e2eac3 to 330b340 Compare March 26, 2024 16:11

pramodsatya force-pushed the fn_metadata branch from 330b340 to 6386575 Compare April 24, 2024 19:57

pramodsatya changed the title ~~[WIP] [native] Retrieve Json function metadata for prestissimo functions~~ [native] Retrieve Json function metadata for prestissimo functions Jun 7, 2024

pramodsatya force-pushed the fn_metadata branch from 6386575 to bd8b27f Compare June 7, 2024 16:12

aditi-pandit reviewed Jun 13, 2024

View reviewed changes

pramodsatya force-pushed the fn_metadata branch from bd8b27f to edd9e93 Compare August 2, 2024 00:34

pramodsatya marked this pull request as ready for review August 2, 2024 00:34

pramodsatya requested a review from a team as a code owner August 2, 2024 00:34

pramodsatya force-pushed the fn_metadata branch from edd9e93 to 0cf2dd5 Compare August 2, 2024 01:15

pramodsatya commented Aug 2, 2024

View reviewed changes

mbasmanova reviewed Aug 2, 2024

View reviewed changes

pramodsatya force-pushed the fn_metadata branch from 0cf2dd5 to 27d780d Compare August 2, 2024 01:59

pramodsatya requested review from steveburnett, elharo and a team as code owners August 2, 2024 01:59

pramodsatya requested a review from presto-oss August 2, 2024 01:59

aditi-pandit reviewed Aug 2, 2024

View reviewed changes

steveburnett requested changes Aug 2, 2024

View reviewed changes

presto-docs/src/main/sphinx/develop/worker-protocol.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/develop/worker-protocol.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/develop/worker-protocol.rst Outdated Show resolved Hide resolved

tdcmeehan requested changes Aug 2, 2024

View reviewed changes

presto-docs/src/main/sphinx/develop/worker-protocol.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/develop/worker-protocol.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/develop/worker-protocol.rst Outdated Show resolved Hide resolved

pramodsatya force-pushed the fn_metadata branch from 27d780d to 90dd09f Compare August 2, 2024 19:27

czentgr reviewed Aug 2, 2024

View reviewed changes

pramodsatya force-pushed the fn_metadata branch from 90dd09f to a006c3e Compare August 3, 2024 02:20

steveburnett requested changes Aug 6, 2024

View reviewed changes

presto-docs/src/main/sphinx/develop.rst Outdated Show resolved Hide resolved

pramodsatya force-pushed the fn_metadata branch from a691fcf to 34effe2 Compare August 6, 2024 16:50

steveburnett previously approved these changes Aug 6, 2024

View reviewed changes

czentgr reviewed Aug 7, 2024

View reviewed changes

presto-native-execution/presto_cpp/presto_protocol/special/Language.cpp.inc Show resolved Hide resolved

aditi-pandit reviewed Aug 8, 2024

View reviewed changes

pramodsatya dismissed steveburnett’s stale review via cdb557e August 12, 2024 06:18

pramodsatya force-pushed the fn_metadata branch 2 times, most recently from cdb557e to 396ecd2 Compare August 12, 2024 22:57

aditi-pandit reviewed Aug 13, 2024

View reviewed changes

presto-native-execution/presto_cpp/main/types/FunctionMetadata.cpp Show resolved Hide resolved

pramodsatya force-pushed the fn_metadata branch 2 times, most recently from a4723a0 to c607530 Compare August 15, 2024 04:56

pramodsatya force-pushed the fn_metadata branch from c607530 to 719cf99 Compare August 15, 2024 16:24

pramodsatya force-pushed the fn_metadata branch 2 times, most recently from 4ed8961 to 56813d3 Compare August 22, 2024 21:46

pramodsatya force-pushed the fn_metadata branch from 56813d3 to da222f9 Compare August 27, 2024 21:51

pramodsatya requested review from aditi-pandit, tdcmeehan and czentgr August 27, 2024 21:57

pramodsatya force-pushed the fn_metadata branch from da222f9 to 88c8b7a Compare August 27, 2024 22:08

aditi-pandit reviewed Sep 9, 2024

View reviewed changes

pramodsatya added 2 commits September 9, 2024 17:33

[native] Update presto_protocol

a08e427

[native] Retrieve Json function metadata

568e1cf

pramodsatya force-pushed the fn_metadata branch from 88c8b7a to 568e1cf Compare September 10, 2024 01:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pramodsatya commented Mar 26, 2024 •

edited

Loading

aditi-pandit commented Jun 13, 2024

pramodsatya left a comment

mbasmanova left a comment

tdcmeehan commented Aug 2, 2024

mbasmanova commented Aug 2, 2024

aditi-pandit left a comment

steveburnett left a comment

czentgr left a comment

pramodsatya commented Aug 3, 2024

steveburnett left a comment

pramodsatya commented Aug 6, 2024

steveburnett left a comment

steveburnett commented Aug 15, 2024

pramodsatya commented Aug 15, 2024

steveburnett commented Aug 15, 2024

aditi-pandit commented Aug 22, 2024

pramodsatya commented Aug 22, 2024

aditi-pandit Sep 6, 2024

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

Are you sure you want to change the base?

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

Conversation

pramodsatya commented Mar 26, 2024 • edited Loading

Motivation and Context

Description

Test Plan

aditi-pandit commented Jun 13, 2024

pramodsatya left a comment

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

tdcmeehan commented Aug 2, 2024

mbasmanova commented Aug 2, 2024

aditi-pandit left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

czentgr left a comment

Choose a reason for hiding this comment

pramodsatya commented Aug 3, 2024

steveburnett left a comment

Choose a reason for hiding this comment

pramodsatya commented Aug 6, 2024

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett commented Aug 15, 2024

pramodsatya commented Aug 15, 2024

steveburnett commented Aug 15, 2024

aditi-pandit commented Aug 22, 2024

pramodsatya commented Aug 22, 2024

aditi-pandit Sep 6, 2024

Choose a reason for hiding this comment

pramodsatya commented Mar 26, 2024 •

edited

Loading