Skip to content

[native] Add session properties for expression evaluation optimizations#23620

Merged
amitkdutta merged 1 commit intoprestodb:masterfrom
bikramSingh91:native_expr_configs
Sep 12, 2024
Merged

[native] Add session properties for expression evaluation optimizations#23620
amitkdutta merged 1 commit intoprestodb:masterfrom
bikramSingh91:native_expr_configs

Conversation

@bikramSingh91
Copy link
Contributor

This change adds session properties that are eventually translate to
velox query configs to toggle optimization in expression evaluation.
These are debug only configs and should not be used in production.
The configs added are:
native_debug_disable_expression_with_peeling,
native_debug_disable_common_sub_expressions,
native_debug_disable_expression_with_memoization,
native_debug_disable_expression_with_lazy_inputs.

Test Plan:
Updated and added unit tests

@bikramSingh91
Copy link
Contributor Author

This change relies on velox submodule to be updated to include (facebookincubator/velox#10902), I will send out a separate PR for updating the module.

@bikramSingh91
Copy link
Contributor Author

Depends on #23621

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

Thanks for the doc! Looks good in a local doc build, just a few minor nits.

This change adds session properties that are eventually translate to
velox query configs to toggle optimization in expression evaluation.
These are debug only configs and should not be used in production.
The configs added are:
`native_debug_disable_expression_with_peeling`,
`native_debug_disable_common_sub_expressions`,
`native_debug_disable_expression_with_memoization`,
`native_debug_disable_expression_with_lazy_inputs`.

Test Plan:
Updated and added unit tests
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

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

Copy link
Contributor

@amitkdutta amitkdutta left a comment

Choose a reason for hiding this comment

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

Looks good.

@amitkdutta amitkdutta merged commit ac510c2 into prestodb:master Sep 12, 2024
@jaystarshot jaystarshot mentioned this pull request Nov 1, 2024
25 tasks
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.

4 participants

Comments