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
[SPARK-31573][R] Apply fixed=TRUE as appropriate to regex usage in R #28367
Conversation
ok to test |
@@ -606,7 +606,7 @@ getClientModeSparkSubmitOpts <- function(submitOps, sparkEnvirMap) { | |||
# process only if --option is not already specified | |||
if (!is.null(opsValue) && | |||
nchar(opsValue) > 1 && | |||
!grepl(sparkConfToSubmitOps[[conf]], submitOps)) { | |||
!grepl(sparkConfToSubmitOps[[conf]], submitOps, fixed = TRUE)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could actually be a bug fix too for very, very unlikely corner cases such as spark.driver.memory
<> sparkAdriverXmemory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes exactly. there were a few other like this in utils.R
e.g.
grep("org.apache.spark.sql.streaming.StreamingQueryException: "
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me
Test build #121894 has finished for PR 28367 at commit
|
Test build #121912 has finished for PR 28367 at commit
|
Let me rerun the AppVeyor build to make sure. |
same transient failure for linter here... |
Let's see if AppVeyor tests pass. |
Test build #121955 has finished for PR 28367 at commit
|
### What changes were proposed in this pull request? For regex functions in base R (`gsub`, `grep`, `grepl`, `strsplit`, `gregexpr`), supplying the `fixed=TRUE` option will be more performant. ### Why are the changes needed? This is a minor fix for performance ### Does this PR introduce any user-facing change? No (although some internal code was applying fixed-as-regex in some cases that could technically have been over-broad and caught unintended patterns) ### How was this patch tested? Not Closes #28367 from MichaelChirico/r-regex-fixed. Authored-by: Michael Chirico <michael.chirico@grabtaxi.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org> (cherry picked from commit c011502) Signed-off-by: HyukjinKwon <gurwls223@apache.org>
Merged to master and branch-3.0. |
What changes were proposed in this pull request?
For regex functions in base R (
gsub
,grep
,grepl
,strsplit
,gregexpr
), supplying thefixed=TRUE
option will be more performant.Why are the changes needed?
This is a minor fix for performance
Does this PR introduce any user-facing change?
No (although some internal code was applying fixed-as-regex in some cases that could technically have been over-broad and caught unintended patterns)
How was this patch tested?
Not