Skip to content

Conversation

@johallar
Copy link
Contributor

Description

With three variants of workers, there has been some desire to be able to determine a workers execution type easily. This adds an executionType field to the /v1/info endpoint in order to be able to determine that.

The eventual goal would be to display this information in a worker list in the Presto UI for quick reference.

Motivation and Context

Impact

Test Plan

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.
  • If adding new dependencies, verified they have an OpenSSF Scorecard score of 5.0 or higher (or obtained explicit TSC approval for lower scores).

Release Notes

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

== RELEASE NOTES ==

General Changes
* ... 
* ... 

Hive Connector Changes
* ... 
* ... 

If release note is NOT required, use:

== NO RELEASE NOTE ==

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Oct 28, 2025

Reviewer's Guide

This PR adds an executionType field to ServerInfo by defining an ExecutionType enum with JSON (de)serialization, extending both the C++ and Java models and updating the server endpoints to populate it, and adjusting existing tests to include executionType.

Class diagram for updated ServerInfo and new ExecutionType (Java)

classDiagram
    class ServerInfo {
        - NodeVersion nodeVersion
        - String environment
        - boolean coordinator
        - boolean starting
        - Optional<Duration> uptime
        - Optional<ExecutionType> executionType
        + ServerInfo(...)
        + getNodeVersion()
        + getEnvironment()
        + isCoordinator()
        + isStarting()
        + getUptime()
        + getExecutionType()
    }
    class ExecutionType {
        <<enum>>
        JAVA
        NATIVE
        NATIVE_GPU
        + getValue()
        + fromValue(String value)
        + toString()
    }
    ServerInfo --> "Optional" ExecutionType
Loading

Class diagram for updated ServerInfo and new ExecutionType (C++)

classDiagram
    class ServerInfo {
        + NodeVersion nodeVersion
        + String environment
        + bool coordinator
        + bool starting
        + shared_ptr<Duration> uptime
        + shared_ptr<ExecutionType> executionType
    }
    class ExecutionType {
        <<enum>>
        JAVA
        NATIVE
        NATIVE_GPU
    }
    ServerInfo --> "shared_ptr" ExecutionType
Loading

File-Level Changes

Change Details Files
Introduce ExecutionType enum and JSON (de)serialization in C++ protocol
  • Define ExecutionType enum with JAVA/NATIVE/NATIVE_GPU variants
  • Add static mapping table for enum-to-JSON values
  • Implement to_json and from_json for ExecutionType in core and special includes
presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.cpp
presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.h
presto-native-execution/presto_cpp/presto_protocol/core/special/ExecutionType.cpp.inc
presto-native-execution/presto_cpp/presto_protocol/core/special/ExecutionType.hpp.inc
Extend C++ ServerInfo to include executionType
  • Add shared_ptr field to ServerInfo struct
  • Update to_json/from_json for ServerInfo to serialize executionType
  • Modify PrestoServer::reportServerInfo to set executionType based on GPU support
presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.cpp
presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.h
presto-native-execution/presto_cpp/main/PrestoServer.cpp
Integrate ExecutionType in Java code
  • Create new ExecutionType enum class with JsonCreator/JsonValue annotations
  • Update ServerInfo.java to include executionType field, constructors, getter, and toString
  • Update ServerInfoResource and ClusterStatusResource to return executionType in endpoints
presto-client/src/main/java/com/facebook/presto/client/ExecutionType.java
presto-client/src/main/java/com/facebook/presto/client/ServerInfo.java
presto-main/src/main/java/com/facebook/presto/server/ServerInfoResource.java
presto-router/src/main/java/com/facebook/presto/router/cluster/ClusterStatusResource.java
Update tests to include executionType in JSON round-trip and client usage
  • Add executionType parameter to TestServerInfo JSON round-trip and compatibility tests
  • Adapt TestPrestoSparkHttpClient to expect executionType in ServerInfo
  • Adjust TestQueryExecutor to include executionType in expected ServerInfo
presto-client/src/test/java/com/facebook/presto/client/TestServerInfo.java
presto-spark-base/src/test/java/com/facebook/presto/spark/execution/http/TestPrestoSparkHttpClient.java
presto-jdbc/src/test/java/com/facebook/presto/jdbc/TestQueryExecutor.java

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@tdcmeehan
Copy link
Contributor

High level question: because Presto doesn't support heterogenous deployments, couldn't we represent the underlying eval used as a more simple flag? And instead of displaying this per server, couldn't it be displayed as a more general property of the entire cluster, given that we won't ever be mixing the different types of eval within a single cluster?

@johallar
Copy link
Contributor Author

High level question: because Presto doesn't support heterogenous deployments, couldn't we represent the underlying eval used as a more simple flag? And instead of displaying this per server, couldn't it be displayed as a more general property of the entire cluster, given that we won't ever be mixing the different types of eval within a single cluster?

@tdcmeehan Ah I was operating under the assumption that it could be heterogeneous, could definitely simplify in that case. Maybe a field on the /v1/cluster response would make more sense?

@tdcmeehan
Copy link
Contributor

tdcmeehan commented Oct 28, 2025

@johallar so I've been thinking that classifying Presto by the eval it uses doesn't fully encapsulate the fullness of what is different in that deployment. For example, when you run with a Presto cluster that uses C++ and Velox, it's not that you've swapped out the eval on the worker, there's a lot of other stuff that changes, like functions and their semantics, session properties, and optimizer rules. Some of this we now encapsulate in an SPI, and we now have a "Velox SPI" of sorts that properly wires Presto to the differences found in Velox's eval (for example, by delegating function evaluation in the coordinator into Velox). There's also lots of "isNativeExecution" property usages in the code, but I'm not a fan of that because it ties a single boolean flag to a singular thing--am I using Presto with Velox as the underlying evaluation engine using vectorized execution? I imagine there are some subtleties we'll eventually need to surface up to the frontend from the flavor we're building that uses GPUs.

So that end, I'm wondering if we can do this in two parts. The first part is, let's just add a generic identifier to the UI, and it can be literally anything. We could indeed just pipe this out from /v1/cluster. And for now we just require people to manually set this, and if the fact that you're using a GPU powered cluster is important to you, you'd probably want to set the value to indicate that so it's clear and obvious.

The second part could be, how do I set all of these flag values depending on which flavor of eval I'm using? And then what we'd do is split out these config values depending on the flavor, and set the defaults depending on the flavor. And for this particular opaque identifier we'll introduce to /v1/cluster, we'll just set a default value, like velox-cudf or something similar when you're using GPU . What do you think?

@johallar
Copy link
Contributor Author

@tdcmeehan I think that makes sense and step 1 would definitely satisfy the initial needs. Do you think a config flag would be appropriate for setting the generic identifier value?

Then just to clarify, step two in this plan would default the flag in a smart way based on the flavor of eval, but would be more nuanced than just java/native/gpu?

@tdcmeehan
Copy link
Contributor

@johallar that's right, a simple config value would suffice. We are moving what defines the "flavor" into a plugin (see RFC-0003), and eventually there we'd simply set the default value of that flag into something that's appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants