Skip to content

[native] Add RestFunctionHandle to presto_protocol serialization#25668

Merged
aditi-pandit merged 1 commit into
prestodb:masterfrom
Joe-Abraham:presto_protocol
Aug 1, 2025
Merged

[native] Add RestFunctionHandle to presto_protocol serialization#25668
aditi-pandit merged 1 commit into
prestodb:masterfrom
Joe-Abraham:presto_protocol

Conversation

@Joe-Abraham

@Joe-Abraham Joe-Abraham commented Aug 1, 2025

Copy link
Copy Markdown
Contributor

Description

Add serialization support for RestFunctionHandle to the Presto native execution protocol.

Motivation and Context

Enables REST-based function calls to be properly serialized and deserialized in the C++ native execution engine.

Impact

No Impact.

Test Plan

NA

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • 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

Please follow release notes guidelines and fill in the release notes below.

== NO RELEASE NOTE ==

@Joe-Abraham Joe-Abraham requested a review from a team as a code owner August 1, 2025 06:04
@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Aug 1, 2025
@prestodb-ci prestodb-ci requested review from a team and Mariamalmesfer and removed request for a team August 1, 2025 06:04
czentgr
czentgr approved these changes Aug 1, 2025
View reviewed changes

@czentgr czentgr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks!

aditi-pandit
aditi-pandit approved these changes Aug 1, 2025
View reviewed changes

@aditi-pandit aditi-pandit left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @Joe-Abraham

@aditi-pandit aditi-pandit merged commit 736a5da into prestodb:master Aug 1, 2025
172 of 173 checks passed
@Joe-Abraham Joe-Abraham deleted the presto_protocol branch August 2, 2025 10:58
@Joe-Abraham Joe-Abraham mentioned this pull request Aug 4, 2025
Merged
6 tasks
@amitkdutta

amitkdutta commented Aug 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

presto_to_velox_query_plan_test fails with this PR

Note: Google Test filter = PrestoToVeloxQueryPlanTest.parseIndexJoinNode
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from PrestoToVeloxQueryPlanTest
[ RUN      ] PrestoToVeloxQueryPlanTest.parseIndexJoinNode

terminate called after throwing an instance of 'facebook::presto::protocol::OutOfRange'
  what():  [json.exception.out_of_range.403] key 'lookupVariables' not found IndexJoinNode List<VariableReferenceExpression> lookupVariables
*** Aborted at 1754344675 (Unix time, try 'date -d @1754344675') ***
*** Signal 6 (SIGABRT) (0x755900001c33) received by PID 7219 (pthread TID 0x7f387556bf00) (linux TID 7219) (maybe from PID 7219, UID 30041) (code: -6), stack trace: ***
...

From the unit tests log https://github.com/prestodb/presto/actions/runs/16667662629/job/47177046993?pr=25668
I see we don't run presto_to_velox_query_plan_test. Following tests are ran:

The following tests passed:
	presto_expressions_test
	presto_function_metadata_test
	presto_http_filter_test
	presto_common_test
	presto_operators_test
	presto_server_remote_function_test
	prometheus_reporter_test
	presto_protocol_test
	presto_http_test
	presto_http_jwt_test
	presto_server_test

100% tests passed, 0 tests failed out of 11

CC: @czentgr @ericyuliu

@Joe-Abraham

Joe-Abraham commented Aug 5, 2025

Copy link
Copy Markdown
Contributor Author

@amitkdutta, The actual changes were made in #25519, and the Presto protocol wasn't updated in that PR. This issue isn't related to my changes. Just wanted to clarify!

CC: @czentgr

@ericyuliu ericyuliu mentioned this pull request Aug 5, 2025
Merged
@czentgr

czentgr commented Aug 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

@amitkdutta Thanks. Yes, the test is built but the executable apparently not run based on the test output.
Because this PR updated the protocol and passed it caused issues down the line which are addressed by #25691.

Turns out there are 2 more tests that aren't run from the looks of it although they all come from the same path and are built. But ctest didn't run them for some reason. Need to investigate. Thanks for finding this out!

ericyuliu added a commit that referenced this pull request Aug 5, 2025
@ericyuliu
[native] Fix broken unit test due to IndexJoinNode change (#25691)
3545b1d 
## Description
Fix broken unit test due to
#25668

## Motivation and Context
Note: Google Test filter = PrestoToVeloxQueryPlanTest.parseIndexJoinNode
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from PrestoToVeloxQueryPlanTest
[ RUN      ] PrestoToVeloxQueryPlanTest.parseIndexJoinNode

terminate called after throwing an instance of
'facebook::presto::protocol::OutOfRange'
what(): [json.exception.out_of_range.403] key 'lookupVariables' not
found IndexJoinNode List<VariableReferenceExpression> lookupVariables

## Impact
No Impact.

## Release Notes
Please follow [release notes
guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines)
and fill in the release notes below.

```
== NO RELEASE NOTE ==
```
shrinidhijoshi pushed a commit to shrinidhijoshi/presto that referenced this pull request Aug 15, 2025
@ericyuliu @shrinidhijoshi
[native] Fix broken unit test due to IndexJoinNode change (prestodb#2…
9078568 
…5691)

## Description
Fix broken unit test due to
prestodb#25668

## Motivation and Context
Note: Google Test filter = PrestoToVeloxQueryPlanTest.parseIndexJoinNode
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from PrestoToVeloxQueryPlanTest
[ RUN      ] PrestoToVeloxQueryPlanTest.parseIndexJoinNode

terminate called after throwing an instance of
'facebook::presto::protocol::OutOfRange'
what(): [json.exception.out_of_range.403] key 'lookupVariables' not
found IndexJoinNode List<VariableReferenceExpression> lookupVariables

## Impact
No Impact.

## Release Notes
Please follow [release notes
guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines)
and fill in the release notes below.

```
== NO RELEASE NOTE ==
```
lga-zurich pushed a commit to lga-zurich/presto-exchange that referenced this pull request Sep 2, 2025
@ericyuliu @lga-zurich
[native] Fix broken unit test due to IndexJoinNode change (prestodb#2…
769c67f 
…5691)

## Description
Fix broken unit test due to
prestodb#25668

## Motivation and Context
Note: Google Test filter = PrestoToVeloxQueryPlanTest.parseIndexJoinNode
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from PrestoToVeloxQueryPlanTest
[ RUN      ] PrestoToVeloxQueryPlanTest.parseIndexJoinNode

terminate called after throwing an instance of
'facebook::presto::protocol::OutOfRange'
what(): [json.exception.out_of_range.403] key 'lookupVariables' not
found IndexJoinNode List<VariableReferenceExpression> lookupVariables

## Impact
No Impact.

## Release Notes
Please follow [release notes
guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines)
and fill in the release notes below.

```
== NO RELEASE NOTE ==
```
lga-zurich pushed a commit to lga-zurich/presto-exchange that referenced this pull request Sep 8, 2025
@ericyuliu @lga-zurich
[native] Fix broken unit test due to IndexJoinNode change (prestodb#2…
5aec5e4 
…5691)

## Description
Fix broken unit test due to
prestodb#25668

## Motivation and Context
Note: Google Test filter = PrestoToVeloxQueryPlanTest.parseIndexJoinNode
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from PrestoToVeloxQueryPlanTest
[ RUN      ] PrestoToVeloxQueryPlanTest.parseIndexJoinNode

terminate called after throwing an instance of
'facebook::presto::protocol::OutOfRange'
what(): [json.exception.out_of_range.403] key 'lookupVariables' not
found IndexJoinNode List<VariableReferenceExpression> lookupVariables

## Impact
No Impact.

## Release Notes
Please follow [release notes
guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines)
and fill in the release notes below.

```
== NO RELEASE NOTE ==
```
lga-zurich pushed a commit to lga-zurich/presto-exchange that referenced this pull request Sep 8, 2025
@ericyuliu
[native] Fix broken unit test due to IndexJoinNode change (prestodb#2…
0f65dea 
…5691)

## Description
Fix broken unit test due to
prestodb#25668

## Motivation and Context
Note: Google Test filter = PrestoToVeloxQueryPlanTest.parseIndexJoinNode
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from PrestoToVeloxQueryPlanTest
[ RUN      ] PrestoToVeloxQueryPlanTest.parseIndexJoinNode

terminate called after throwing an instance of
'facebook::presto::protocol::OutOfRange'
what(): [json.exception.out_of_range.403] key 'lookupVariables' not
found IndexJoinNode List<VariableReferenceExpression> lookupVariables

## Impact
No Impact.

## Release Notes
Please follow [release notes
guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines)
and fill in the release notes below.

```
== NO RELEASE NOTE ==
```
lga-zurich pushed a commit to lga-zurich/presto-exchange that referenced this pull request Sep 8, 2025
@ericyuliu
[native] Fix broken unit test due to IndexJoinNode change (prestodb#2…
4196e1c 
…5691)

## Description
Fix broken unit test due to
prestodb#25668

## Motivation and Context
Note: Google Test filter = PrestoToVeloxQueryPlanTest.parseIndexJoinNode
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from PrestoToVeloxQueryPlanTest
[ RUN      ] PrestoToVeloxQueryPlanTest.parseIndexJoinNode

terminate called after throwing an instance of
'facebook::presto::protocol::OutOfRange'
what(): [json.exception.out_of_range.403] key 'lookupVariables' not
found IndexJoinNode List<VariableReferenceExpression> lookupVariables

## Impact
No Impact.

## Release Notes
Please follow [release notes
guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines)
and fill in the release notes below.

```
== NO RELEASE NOTE ==
```
lga-zurich pushed a commit to lga-zurich/presto-exchange that referenced this pull request Sep 8, 2025
@ericyuliu
[native] Fix broken unit test due to IndexJoinNode change (prestodb#2…
d2a1867 
…5691)

## Description
Fix broken unit test due to
prestodb#25668

## Motivation and Context
Note: Google Test filter = PrestoToVeloxQueryPlanTest.parseIndexJoinNode
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from PrestoToVeloxQueryPlanTest
[ RUN      ] PrestoToVeloxQueryPlanTest.parseIndexJoinNode

terminate called after throwing an instance of
'facebook::presto::protocol::OutOfRange'
what(): [json.exception.out_of_range.403] key 'lookupVariables' not
found IndexJoinNode List<VariableReferenceExpression> lookupVariables

## Impact
No Impact.

## Release Notes
Please follow [release notes
guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines)
and fill in the release notes below.

```
== NO RELEASE NOTE ==
```
lga-zurich pushed a commit to lga-zurich/presto-exchange that referenced this pull request Sep 8, 2025
@ericyuliu
[native] Fix broken unit test due to IndexJoinNode change (prestodb#2…
fd0f47f 
…5691)

## Description
Fix broken unit test due to
prestodb#25668

## Motivation and Context
Note: Google Test filter = PrestoToVeloxQueryPlanTest.parseIndexJoinNode
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from PrestoToVeloxQueryPlanTest
[ RUN      ] PrestoToVeloxQueryPlanTest.parseIndexJoinNode

terminate called after throwing an instance of
'facebook::presto::protocol::OutOfRange'
what(): [json.exception.out_of_range.403] key 'lookupVariables' not
found IndexJoinNode List<VariableReferenceExpression> lookupVariables

## Impact
No Impact.

## Release Notes
Please follow [release notes
guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines)
and fill in the release notes below.

```
== NO RELEASE NOTE ==
```
lga-zurich pushed a commit to lga-zurich/presto-exchange that referenced this pull request Sep 8, 2025
@ericyuliu
[native] Fix broken unit test due to IndexJoinNode change (prestodb#2…
c6a2423 
…5691)

## Description
Fix broken unit test due to
prestodb#25668

## Motivation and Context
Note: Google Test filter = PrestoToVeloxQueryPlanTest.parseIndexJoinNode
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from PrestoToVeloxQueryPlanTest
[ RUN      ] PrestoToVeloxQueryPlanTest.parseIndexJoinNode

terminate called after throwing an instance of
'facebook::presto::protocol::OutOfRange'
what(): [json.exception.out_of_range.403] key 'lookupVariables' not
found IndexJoinNode List<VariableReferenceExpression> lookupVariables

## Impact
No Impact.

## Release Notes
Please follow [release notes
guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines)
and fill in the release notes below.

```
== NO RELEASE NOTE ==
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@czentgr czentgr czentgr approved these changes

@aditi-pandit aditi-pandit aditi-pandit approved these changes

@Mariamalmesfer Mariamalmesfer Awaiting requested review from Mariamalmesfer Mariamalmesfer was automatically assigned from prestodb/ibm-reviewers

Assignees

No one assigned

Labels

from:IBM PR from IBM

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Joe-Abraham @amitkdutta @czentgr @aditi-pandit @prestodb-ci