Skip to content

Commit

Permalink
apacheGH-39191: [R] throw error when string_replace is passed vecto…
Browse files Browse the repository at this point in the history
…r of values in `pattern` (apache#39219)

### Rationale for this change
See apache#39191 This PR will hopefully throw an informative error message to let the user know that while the stringr::str_replace_all function can handle a named vector of values as the pattern argument, the arrow R package implementation cannot.  

### What changes are included in this PR?
- [ ] add tests for passing vector to the pattern argument
- [ ] add check for length > 1 to the string replace bindings

### Are these changes tested?
yes (though I need help!)

### Are there any user-facing changes?
 yes. Hopefully the user will be alerted by an informative error message that they cannot pass a vector to the pattern argument. No breaking changes are expected.

* Closes: apache#39191

Authored-by: Abram B. Fleishman <abram@conservationmetrics.com>
Signed-off-by: Nic Crane <thisisnic@gmail.com>
  • Loading branch information
abfleishman authored Dec 19, 2023
1 parent 9cb78ad commit 64fed4e
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 1 deletion.
7 changes: 6 additions & 1 deletion r/R/dplyr-funcs-string.R
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ get_stringr_pattern_options <- function(pattern) {
}

ensure_opts <- function(opts) {

# default options for the simple cases
if (is.character(opts)) {
opts <- list(pattern = opts, fixed = FALSE, ignore_case = FALSE)
Expand Down Expand Up @@ -352,6 +351,12 @@ register_bindings_string_regex <- function() {
# Encapsulate some common logic for sub/gsub/str_replace/str_replace_all
arrow_r_string_replace_function <- function(max_replacements) {
function(pattern, replacement, x, ignore.case = FALSE, fixed = FALSE) {
if (length(pattern) != 1) {
stop("`pattern` must be a length 1 character vector")
}
if (length(replacement) != 1) {
stop("`replacement` must be a length 1 character vector")
}
Expression$create(
ifelse(fixed && !ignore.case, "replace_substring", "replace_substring_regex"),
x,
Expand Down
17 changes: 17 additions & 0 deletions r/tests/testthat/test-dplyr-funcs-string.R
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,23 @@ test_that("sub and gsub with namespacing", {
})

test_that("str_replace and str_replace_all", {
x <- Expression$field_ref("x")

expect_error(
call_binding("str_replace_all", x, c("F" = "_", "b" = "")),
regexp = "`pattern` must be a length 1 character vector"
)

expect_error(
call_binding("str_replace_all", x, c("F", "b"), c("_", "")),
regexp = "`pattern` must be a length 1 character vector"
)

expect_error(
call_binding("str_replace_all", x, c("F"), c("_", "")),
regexp = "`replacement` must be a length 1 character vector"
)

df <- tibble(x = c("Foo", "bar"))

compare_dplyr_binding(
Expand Down

0 comments on commit 64fed4e

Please sign in to comment.