Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate native_execution_enabled #22153

Closed
tdcmeehan opened this issue Mar 11, 2024 · 5 comments · Fixed by #22158
Closed

Deprecate native_execution_enabled #22153

tdcmeehan opened this issue Mar 11, 2024 · 5 comments · Fixed by #22158
Assignees
Labels
feature request prestissimo Presto Native Execution

Comments

@tdcmeehan
Copy link
Contributor

Native execution is an invariant for the cluster that will never change--it should just be a system property.

Expected Behavior or Use Case

We should not have session properties that represent invariants as it increases the footprint of session properties visible to the user and makes Presto harder to operate.

Presto Component, Service, or Connector

Engine

Possible Implementation

Just replace all references of SystemSessionProperties#isNativeExecutionEnabled with a new system property that accomplishes this.

Example Screenshots (if appropriate):

Context

This came out of a discussion in #22047 which discussed how to expose this session property to connectors. It was revealed that it might have been more optimal to originally expose it as a config property.

@mbasmanova
Copy link
Contributor

@mbasmanova
Copy link
Contributor

@shrinidhijoshi Shrinidhi, do you happen to know why 'native_execution_enabled' was added as a session property and not just config property. I'm starting to remember that it is somehow difficult to change config properties when launching PoS queries. Did we make this a session property to allow switching between PoS and PoS-on-Velox easily?

CC: @arhimondr

@mbasmanova
Copy link
Contributor

Outside of PoS, this session property is used in 3 optimizer rules

  • PushPartialAggregationThroughExchange
  • AddLocalExchanges
  • AddExchanges

and SqlToRowExpressionTranslator.

None of these have direct access to FeaturesConfig (which is needed to fetch configuration property value).

All these rules are created in com/facebook/presto/sql/planner/PlanOptimizers.java which can inject FeaturesConfig and get access to the property. It can then pass the value to rule's constructors.

SqlToRowExpressionTranslator is called from many places and these places themselves do not have access to FeaturesConfig (e.g. StatementAnalyzer, QueryPlanner, etc.). It would require a good amount of plumbing to make this flag available.

@tdcmeehan @arhimondr Tim, Andrii, do you have any suggestion about how to best approach this?

@shrinidhijoshi
Copy link
Collaborator

@shrinidhijoshi Shrinidhi, do you happen to know why 'native_execution_enabled' was added as a session property and not just config property. I'm starting to remember that it is somehow difficult to change config properties when launching PoS queries. Did we make this a session property to allow switching between PoS and PoS-on-Velox easily?

@mbasmanova Yes. That would be my best guess too. As at the time of PoC, internally at Meta, there wasn't any way to pass config props at invocation time for PoS queries.
Later however, we added a mode=NATIVE_EXECUTION option which we can use now, and deprecate this session prop.

Also, maybe all of these should change to config prop

    public static final String NATIVE_EXECUTION_ENABLED = "native_execution_enabled";
    public static final String NATIVE_EXECUTION_EXECUTABLE_PATH = "native_execution_executable_path";
    public static final String NATIVE_EXECUTION_PROGRAM_ARGUMENTS = "native_execution_program_arguments";
    public static final String NATIVE_EXECUTION_PROCESS_REUSE_ENABLED = "native_execution_process_reuse_enabled";
    public static final String NATIVE_DEBUG_VALIDATE_OUTPUT_FROM_OPERATORS = "native_debug_validate_output_from_operators";

@mbasmanova
Copy link
Contributor

Possibly related issue: #20008

@github-project-automation github-project-automation bot moved this from Backlog to Done in Prestissimo Gap Mar 12, 2024
@mbasmanova mbasmanova self-assigned this Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request prestissimo Presto Native Execution
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants