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

Add "native_expression.max_compiled_regexes" session property. #24282

Merged

Conversation

spershin
Copy link
Contributor

Description

Add "native_expression.max_compiled_regexes" session property.
It can be used to control the limitation of Prestissimo's implementation of regular expression functions.

== NO RELEASE NOTE ==

@spershin spershin requested review from steveburnett, elharo and a team as code owners December 18, 2024 23:20
@spershin spershin requested a review from presto-oss December 18, 2024 23:20
@prestodb-ci prestodb-ci added the from:Meta PR from Meta label Dec 18, 2024
duxiao1212
duxiao1212 previously approved these changes Dec 18, 2024
Copy link
Contributor

@duxiao1212 duxiao1212 left a comment

Choose a reason for hiding this comment

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

LGTM

@spershin spershin force-pushed the Add_max_compiled_regexes_SessionProp branch from ae04ea5 to 2a37c35 Compare December 18, 2024 23:44
NATIVE_EXPRESSION_MAX_COMPILED_REGEXES,
"Native Execution only. Controls maximum number of compiled regular expression patterns " +
"per regular expression function instance per thread of execution.",
100,
Copy link
Contributor

Choose a reason for hiding this comment

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

looks to be standard for all nativeWorkerSessionProperties, so not requesting a change. but why do we define a default value in the session property vs. have the default value be determined by a config property like we do with everything else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe because there is no corresponding config property.
Not sure if one must have such.
There is just a corresponding velox query config property, which is per query.

@spershin spershin merged commit 8136f7c into prestodb:master Dec 20, 2024
67 of 68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:Meta PR from Meta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants