You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "thisisnic (via GitHub)" <gi...@apache.org> on 2023/07/18 19:05:07 UTC
[GitHub] [arrow] thisisnic opened a new pull request, #36758: GH-36720: [R] stringr modifier functions cannot be called with namespace prefix
thisisnic opened a new pull request, #36758:
URL: https://github.com/apache/arrow/pull/36758
### Rationale for this change
Bug in implementation of string modified functions caused them to be swallowed when prefixed with stringr namespace
### What changes are included in this PR?
Strips out the `stringr::` prefix when expressions contain `stringr::fixed`, `stringr::coll`, `string::boundary` or `stringr::regex`
### Are these changes tested?
Yes
### Are there any user-facing changes?
Yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a diff in pull request #36758: GH-36720: [R] stringr modifier functions cannot be called with namespace prefix
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #36758:
URL: https://github.com/apache/arrow/pull/36758#discussion_r1269467548
##########
r/R/dplyr-funcs-string.R:
##########
@@ -56,15 +56,33 @@ get_stringr_pattern_options <- function(pattern) {
)
}
}
+
ensure_opts <- function(opts) {
if (is.character(opts)) {
opts <- list(pattern = opts, fixed = FALSE, ignore_case = FALSE)
}
opts
}
+
+ pattern <- clean_pattern_namespace(pattern)
+
ensure_opts(eval(pattern))
}
+# Ensure that e.g. stringr::regex and regex both work within patterns
+clean_pattern_namespace <- function(pattern) {
+ modifier_funcs <- c("fixed", "regex", "coll", "boundary")
+ if (is_call(pattern, modifier_funcs, ns = "stringr")) {
+ function_called <- call_name(pattern[1])
+
+ if (function_called %in% modifier_funcs) {
+ pattern[1] <- call2(function_called)
+ }
Review Comment:
This bit can be reduced to `pattern[1] <- call2(function_called)` (since `is_call()` took care of the name/namespace matching).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] thisisnic commented on a diff in pull request #36758: GH-36720: [R] stringr modifier functions cannot be called with namespace prefix
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on code in PR #36758:
URL: https://github.com/apache/arrow/pull/36758#discussion_r1269099476
##########
r/R/dplyr-funcs-string.R:
##########
@@ -56,15 +56,33 @@ get_stringr_pattern_options <- function(pattern) {
)
}
}
+
ensure_opts <- function(opts) {
if (is.character(opts)) {
opts <- list(pattern = opts, fixed = FALSE, ignore_case = FALSE)
}
opts
}
+
+ pattern <- clean_pattern_namespace(pattern)
+
ensure_opts(eval(pattern))
}
+# Ensure that e.g. stringr::regex and regex both work within patterns
+clean_pattern_namespace <- function(pattern) {
+ if (is_call(pattern)) {
Review Comment:
Nice!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] conbench-apache-arrow[bot] commented on pull request #36758: GH-36720: [R] stringr modifier functions cannot be called with namespace prefix
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #36758:
URL: https://github.com/apache/arrow/pull/36758#issuecomment-1656150090
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 819b7d5ce7febb4af9ea6f69947244f46403ce53.
There were 5 benchmark results indicating a performance regression:
- Commit Run on `ursa-i9-9960x` at [2023-07-28 15:43:30Z](http://conbench.ursa.dev/compare/runs/5ddeb0f6442f426da1a28471268d8df9...a93dc49bf7274a28973f4b8449ac56a8/)
- [engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-02, scale_factor=1](http://conbench.ursa.dev/compare/benchmarks/064c3cd87f0c7c2a8000440f305d0ddc...064c3fa728477b3e8000ff0396a0f2bc)
- [engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-10, scale_factor=1](http://conbench.ursa.dev/compare/benchmarks/064c3d018e5979ed80001344f0757468...064c3fd591cd77aa80003afee54ce957)
- and 3 more (see the report linked below)
The [full Conbench report](https://github.com/apache/arrow/runs/15437629119) has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a diff in pull request #36758: GH-36720: [R] stringr modifier functions cannot be called with namespace prefix
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #36758:
URL: https://github.com/apache/arrow/pull/36758#discussion_r1268184251
##########
r/R/dplyr-funcs-string.R:
##########
@@ -56,15 +56,33 @@ get_stringr_pattern_options <- function(pattern) {
)
}
}
+
ensure_opts <- function(opts) {
if (is.character(opts)) {
opts <- list(pattern = opts, fixed = FALSE, ignore_case = FALSE)
}
opts
}
+
+ pattern <- clean_pattern_namespace(pattern)
+
ensure_opts(eval(pattern))
}
+# Ensure that e.g. stringr::regex and regex both work within patterns
+clean_pattern_namespace <- function(pattern) {
+ if (is_call(pattern)) {
Review Comment:
```suggestion
modifier_funcs <- c("fixed", "regex", "coll", "boundary")
if (is_call(pattern, modifier_funcs, ns = c("", "stringr")) {
```
...might work too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #36758: GH-36720: [R] stringr modifier functions cannot be called with namespace prefix
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36758:
URL: https://github.com/apache/arrow/pull/36758#issuecomment-1640819639
:warning: GitHub issue #36720 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] thisisnic merged pull request #36758: GH-36720: [R] stringr modifier functions cannot be called with namespace prefix
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic merged PR #36758:
URL: https://github.com/apache/arrow/pull/36758
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] thisisnic commented on pull request #36758: GH-36720: [R] stringr modifier functions cannot be called with namespace prefix
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on PR #36758:
URL: https://github.com/apache/arrow/pull/36758#issuecomment-1643766112
CI failure is due to #36787 and not changes in this PR, so I'll merge
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] thisisnic commented on a diff in pull request #36758: GH-36720: [R] stringr modifier functions cannot be called with namespace prefix
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on code in PR #36758:
URL: https://github.com/apache/arrow/pull/36758#discussion_r1269640964
##########
r/R/dplyr-funcs-string.R:
##########
@@ -56,15 +56,33 @@ get_stringr_pattern_options <- function(pattern) {
)
}
}
+
ensure_opts <- function(opts) {
if (is.character(opts)) {
opts <- list(pattern = opts, fixed = FALSE, ignore_case = FALSE)
}
opts
}
+
+ pattern <- clean_pattern_namespace(pattern)
+
ensure_opts(eval(pattern))
}
+# Ensure that e.g. stringr::regex and regex both work within patterns
+clean_pattern_namespace <- function(pattern) {
+ modifier_funcs <- c("fixed", "regex", "coll", "boundary")
+ if (is_call(pattern, modifier_funcs, ns = "stringr")) {
+ function_called <- call_name(pattern[1])
+
+ if (function_called %in% modifier_funcs) {
+ pattern[1] <- call2(function_called)
+ }
Review Comment:
Nice catch, will chuck this into a follow-up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a diff in pull request #36758: GH-36720: [R] stringr modifier functions cannot be called with namespace prefix
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #36758:
URL: https://github.com/apache/arrow/pull/36758#discussion_r1268184251
##########
r/R/dplyr-funcs-string.R:
##########
@@ -56,15 +56,33 @@ get_stringr_pattern_options <- function(pattern) {
)
}
}
+
ensure_opts <- function(opts) {
if (is.character(opts)) {
opts <- list(pattern = opts, fixed = FALSE, ignore_case = FALSE)
}
opts
}
+
+ pattern <- clean_pattern_namespace(pattern)
+
ensure_opts(eval(pattern))
}
+# Ensure that e.g. stringr::regex and regex both work within patterns
+clean_pattern_namespace <- function(pattern) {
+ if (is_call(pattern)) {
Review Comment:
```suggestion
modifier_funcs <- c("fixed", "regex", "coll", "boundary")
if (is_call(pattern, modifier_funcs, ns = "stringr") {
```
...might work too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org