[native] Adds a native session property provider by pdabre12 · Pull Request #23045 · prestodb/presto

pdabre12 · 2024-06-20T20:16:41Z

Description

Adds a native session property manager.

Motivation and Context

To resolve #23001

Impact

With this change, we can now retrieve session properties which are only supported by both the coordinator and the execution engine.

Test Plan

Tests are included.

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

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

Release Notes

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

== RELEASE NOTES ==

Coordinator Plugin Changes
* Add native system session property provider. :pr:`23045`

cc: @tdcmeehan @abevk2023 @Joe-Abraham

tdcmeehan · 2024-06-25T17:02:57Z

We'll need to spend some time consolidating these commits. It would be great to think about the following commits:

A commit which introduces the necessary SPI. This includes the definition of the SessionPropertyMetadata.
A commit which integrates that SPI with the Presto runtime
An implmenentation of this plugin for the sidecar
An implementation of this plugin with the C++-specific worker properties, and with a config toggle to enable/disable

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

tdcmeehan

Add sidecars to NodeManager and add PluginNodeManager ✅

tdcmeehan

Separate out java worker properties and introduce JavaFeaturesConfig ❓

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

...va/com/facebook/presto/sessionpropertyproviders/JavaWorkerSystemSessionPropertyProvider.java

presto-main/src/test/java/com/facebook/presto/sql/planner/TestLogicalPlanner.java

tdcmeehan

Introduce session properties SPI ✅

tdcmeehan

[native] Integrate session property SPI with Presto engine ❓

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

presto-native-sidecar-plugin/src/main/java/com/facebook/presto/sidecar/NoOpPlugin.java

...om/facebook/presto/sidecar/sessionpropertyproviders/NativeSystemSessionPropertyProvider.java

tdcmeehan

[native] Integrate session property plugin with sidecar ❓

...-native-execution/src/test/java/com/facebook/presto/nativeworker/NativeQueryRunnerUtils.java

...e-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java

tdcmeehan

Open API docs ✅

presto-openapi/src/main/resources/schemas.yaml

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

presto-main/src/main/java/com/facebook/presto/testing/TestingSession.java

pdabre12 · 2024-10-22T23:12:47Z

@tdcmeehan @rschlussel Addressed the comments, please take a look. Also added a CI job for running native-sidecar tests.

This adds an endpoint to the native Presto server that will convert a Presto plan fragment to Velox. If the conversion is successful, the server will send an ok response. If it fails, the server will send an error response with a 422 status code as unprocessable. The error message will contain an ExecutionFailureInfo struct with error type, code and message. See also prestodb#23045 RFC: https://github.com/prestodb/rfcs/blob/main/RFC-0008-plan-checker.md

Co-authored-by: Tim Meehan <tdm@fb.com>

Co-authored-by: Joe Abraham <joe.abraham@ibm.com>

Co-authored-by: Abe Varghese Kodiyan <abe.varghese@ibm.com> Co-authored-by: Joe Abraham <joe.abraham@ibm.com> Co-authored-by: Deepthy Davis <deepthy.davis@ibm.com>

Co-authored-by: Tim Meehan <tdm@fb.com>

This adds an endpoint to the native Presto server that will convert a Presto plan fragment to Velox. If the conversion is successful, the server will send an ok response. If it fails, the server will send an error response with a 422 status code as unprocessable. The error message will contain an ExecutionFailureInfo struct with error type, code and message. See also prestodb#23045 RFC: https://github.com/prestodb/rfcs/blob/main/RFC-0008-plan-checker.md

jaystarshot · 2024-11-04T18:11:35Z

@pdabre12 @tdcmeehan Does this change warrant a release note?

tdcmeehan · 2024-11-04T19:38:33Z

@jaystarshot it's in the above PR, but maybe the tool had trouble parsing it.

jaystarshot · 2024-11-04T19:55:43Z

got it, I don't think Coordinator Plugin Changes is a topic yet. Is this change for some sidecar or for general presto c++ coordinator?

tdcmeehan · 2024-11-04T19:56:13Z

Let's put it under general C++ changes

pdabre12 changed the title ~~System session properties~~ [WIP] System session properties Jun 20, 2024

pdabre12 force-pushed the system-session-properties branch from 35ed111 to fa2ac66 Compare August 27, 2024 23:58

pdabre12 force-pushed the system-session-properties branch from 708018a to 90ec7bd Compare September 4, 2024 21:58

tdcmeehan self-assigned this Sep 5, 2024

pdabre12 force-pushed the system-session-properties branch from 6d2f109 to 41da0f3 Compare September 5, 2024 18:24

pdabre12 changed the title ~~[WIP] System session properties~~ [native] Adds a native system property provider Sep 5, 2024

pdabre12 marked this pull request as ready for review September 5, 2024 19:28

pdabre12 requested review from a team, elharo, feilong-liu, jaystarshot, shrinidhijoshi and vivek-bharathan as code owners September 5, 2024 19:28

pdabre12 requested a review from presto-oss September 5, 2024 19:28

rschlussel self-requested a review September 5, 2024 20:38

elharo reviewed Sep 6, 2024

View reviewed changes

presto-main/src/main/java/com/facebook/presto/metadata/SessionPropertyManager.java Outdated Show resolved Hide resolved

presto-main/src/main/java/com/facebook/presto/metadata/SessionPropertyManager.java Outdated Show resolved Hide resolved

tdcmeehan reviewed Sep 9, 2024

View reviewed changes

tdcmeehan requested changes Sep 9, 2024

View reviewed changes

tdcmeehan reviewed Sep 9, 2024

View reviewed changes

...-native-execution/src/test/java/com/facebook/presto/nativeworker/NativeQueryRunnerUtils.java Show resolved Hide resolved

...e-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java Outdated Show resolved Hide resolved

tdcmeehan reviewed Sep 9, 2024

View reviewed changes

presto-openapi/src/main/resources/schemas.yaml Outdated Show resolved Hide resolved

pdabre12 force-pushed the system-session-properties branch 3 times, most recently from 4317618 to ca4bb3b Compare September 17, 2024 22:51

tdcmeehan requested changes Sep 19, 2024

View reviewed changes

presto-main/src/main/java/com/facebook/presto/metadata/SessionPropertyManager.java Outdated Show resolved Hide resolved

tdcmeehan requested changes Sep 20, 2024

View reviewed changes

presto-main/src/main/java/com/facebook/presto/testing/TestingSession.java Outdated Show resolved Hide resolved

pdabre12 force-pushed the system-session-properties branch from ca4bb3b to 7695324 Compare September 30, 2024 22:20

pdabre12 force-pushed the system-session-properties branch from 74432de to 4eb5cf4 Compare October 22, 2024 23:07

pdabre12 requested a review from tdcmeehan October 22, 2024 23:23

tdcmeehan previously approved these changes Oct 25, 2024

View reviewed changes

rschlussel previously approved these changes Oct 28, 2024

View reviewed changes

pdabre12 and others added 7 commits October 28, 2024 08:13

Allow workers to act as coordinator sidecars

29172cc

Co-authored-by: Tim Meehan <tdm@fb.com>

Separate out worker session properties and introduce JavaFeaturesConfig

feeacb1

Co-authored-by: Joe Abraham <joe.abraham@ibm.com>

Introduce session properties SPI

0ff7d21

Co-authored-by: Joe Abraham <joe.abraham@ibm.com>

Integrate session property SPI with Presto engine

bfd6cbc

Co-authored-by: Abe Varghese Kodiyan <abe.varghese@ibm.com> Co-authored-by: Joe Abraham <joe.abraham@ibm.com> Co-authored-by: Deepthy Davis <deepthy.davis@ibm.com>

Integrate session property plugin with sidecar

c1d2eda

[native] Add e2e native session property provider tests with sidecar

0d7ac0f

Co-authored-by: Tim Meehan <tdm@fb.com>

Add OpenAPI documentation for /v1/properties/session

c1e34ed

pdabre12 dismissed stale reviews from rschlussel and tdcmeehan via c1e34ed October 28, 2024 15:17

pdabre12 force-pushed the system-session-properties branch from 4eb5cf4 to c1e34ed Compare October 28, 2024 15:17

tdcmeehan approved these changes Oct 28, 2024

View reviewed changes

tdcmeehan merged commit 5ed9da1 into prestodb:master Oct 28, 2024

jaystarshot mentioned this pull request Nov 1, 2024

Add release notes for 0.290 #23936

Merged

25 tasks

pdabre12 mentioned this pull request Dec 12, 2024

[native] Add session properties for scale writer query configs #24244

Closed

6 tasks

pramodsatya mentioned this pull request Jan 24, 2025

[native] Introduce module presto-native-tests #24234

Merged

6 tasks

pdabre12 deleted the system-session-properties branch January 29, 2026 18:41

Conversation

pdabre12 commented Jun 20, 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

Release Notes

Release Notes

Uh oh!

tdcmeehan commented Jun 25, 2024

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pdabre12 commented Oct 22, 2024

Uh oh!

jaystarshot commented Nov 4, 2024

Uh oh!

tdcmeehan commented Nov 4, 2024

Uh oh!

jaystarshot commented Nov 4, 2024

Uh oh!

tdcmeehan commented Nov 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pdabre12 commented Jun 20, 2024 •

edited

Loading