nealrichardson commented on a change in pull request #9878: URL: https://github.com/apache/arrow/pull/9878#discussion_r606831418
########## File path: r/tests/testthat/test-dplyr-string-functions.R ########## @@ -0,0 +1,120 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +library(dplyr) +library(stringr) + +test_that("sub and gsub", { + df <- tibble(x = c("Foo", "bar")) + + for(fun in list(quote(sub), quote(gsub))) { + + for(fixed in c(TRUE, FALSE)) { + + expect_dplyr_equal( + input %>% + transmute(x = eval(fun)("Foo", "baz", x, fixed = fixed)) %>% + collect(), + df + ) + expect_dplyr_equal( + input %>% + transmute(x = eval(fun)("Foo", "baz", x, fixed = fixed)) %>% + collect(), + df + ) + expect_dplyr_equal( + input %>% + transmute(x = eval(fun)("^B.+", "baz", x, ignore.case = FALSE, fixed = fixed)) %>% + collect(), + df + ) + expect_dplyr_equal( + input %>% + transmute(x = eval(fun)("Foo", "baz", x, ignore.case = FALSE, fixed = fixed)) %>% + collect(), + df + ) + + # the tests below all use ignore.case = TRUE + # but base::sub and base::gsub ignore ignore.case = TRUE with a warning when fixed = TRUE + # so we can't use expect_dplyr_equal() for the tests below + expect_equal( + df %>% + Table$create() %>% + transmute(x = eval(fun)("Foo", "baz", x, ignore.case = TRUE, fixed = fixed)) %>% + collect(), + tibble(x = c("baz", "bar")) + ) + expect_equal( + df %>% + Table$create() %>% + transmute(x = eval(fun)("o", "u", x, ignore.case = TRUE, fixed = fixed)) %>% + collect(), + if (fun == quote(sub)) tibble(x = c("Fuo", "bar")) else tibble(x = c("Fuu", "bar")) + ) + expect_equal( + df %>% + Table$create() %>% + transmute(x = eval(fun)("^B.+", "baz", x, ignore.case = TRUE, fixed = fixed)) %>% + collect(), + if (fixed) tibble(x = c("Foo", "bar")) else tibble(x = c("Foo", "baz")) + ) + + } + + } +}) Review comment: It would be good to have some more interesting regular expressions, just to make sure we're passing them correctly. E.g. `sub("^F(.*)", "B\\1", x)`. Might also be good to use longer, more interesting strings than foo and bar. ########## File path: r/tests/testthat/test-dplyr-string-functions.R ########## @@ -0,0 +1,120 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +library(dplyr) +library(stringr) + +test_that("sub and gsub", { + df <- tibble(x = c("Foo", "bar")) + + for(fun in list(quote(sub), quote(gsub))) { + + for(fixed in c(TRUE, FALSE)) { + + expect_dplyr_equal( + input %>% + transmute(x = eval(fun)("Foo", "baz", x, fixed = fixed)) %>% + collect(), + df + ) + expect_dplyr_equal( + input %>% + transmute(x = eval(fun)("Foo", "baz", x, fixed = fixed)) %>% + collect(), + df + ) + expect_dplyr_equal( + input %>% + transmute(x = eval(fun)("^B.+", "baz", x, ignore.case = FALSE, fixed = fixed)) %>% + collect(), + df + ) + expect_dplyr_equal( + input %>% + transmute(x = eval(fun)("Foo", "baz", x, ignore.case = FALSE, fixed = fixed)) %>% + collect(), + df + ) + + # the tests below all use ignore.case = TRUE + # but base::sub and base::gsub ignore ignore.case = TRUE with a warning when fixed = TRUE + # so we can't use expect_dplyr_equal() for the tests below + expect_equal( + df %>% + Table$create() %>% + transmute(x = eval(fun)("Foo", "baz", x, ignore.case = TRUE, fixed = fixed)) %>% + collect(), + tibble(x = c("baz", "bar")) + ) + expect_equal( + df %>% + Table$create() %>% + transmute(x = eval(fun)("o", "u", x, ignore.case = TRUE, fixed = fixed)) %>% + collect(), + if (fun == quote(sub)) tibble(x = c("Fuo", "bar")) else tibble(x = c("Fuu", "bar")) Review comment: Likewise, this would be more readable as separate tests for sub and gsub ########## File path: r/R/dplyr.R ########## @@ -434,6 +438,75 @@ build_function_list <- function(FUN) { ) } +arrow_r_string_replace_function <- function(FUN, max_replacements) { + function(pattern, replacement, x, ignore.case = FALSE, fixed = FALSE) { + if (ignore.case) { + if (fixed) { + pattern <- paste0("(?i)\\Q", pattern, "\\E") + } else { + pattern <- paste0("(?i)", pattern) + } + } + FUN( + ifelse(fixed && !ignore.case, "replace_substring", "replace_substring_regex"), + x, + options = list( + pattern = pattern, + replacement = replacement, + max_replacements = max_replacements + ) + ) + } +} + +arrow_stringr_string_replace_function <- function(FUN, max_replacements) { + function(string, pattern, replacement) { + # Assign stringr pattern modifier functions locally Review comment: This is clever but I wonder if it is overly complex. You can just let the `stringr` helpers evaluate naturally and pull the attributes from the resulting object: ``` > library(stringr) > fixed("asdf") [1] "asdf" attr(,"options") attr(,"options")$case_insensitive [1] FALSE attr(,"class") [1] "fixed" "pattern" "character" > fixed("asdf", ignore_case=TRUE) [1] "asdf" attr(,"options") attr(,"options")$case_insensitive [1] TRUE attr(,"class") [1] "fixed" "pattern" "character" > regex("asdf") [1] "asdf" attr(,"options") attr(,"options")$case_insensitive [1] FALSE attr(,"options")$comments [1] FALSE attr(,"options")$dotall [1] FALSE attr(,"options")$multiline [1] FALSE attr(,"class") [1] "regex" "pattern" "character" ``` ########## File path: r/tests/testthat/test-dplyr-string-functions.R ########## @@ -0,0 +1,120 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +library(dplyr) +library(stringr) + +test_that("sub and gsub", { + df <- tibble(x = c("Foo", "bar")) + + for(fun in list(quote(sub), quote(gsub))) { Review comment: I like what you're trying to do here but I think the elimination of code duplication isn't worth the sacrifice in readability. Since we know that `sub` and `gsub` use the same C++ function, just with a different option, we probably don't need to test every permutation of them. Likewise with the other options. You could keep the `fixed` for loop, that one is pretty readable; the downside there is that if one of them fails, I don't think the `testthat` output will tell you which one it is (that's solvable but may require hacking into `expect_dplyr_equal`, which probably isn't worth it). Since the business of evaluating the regular expressions is handled (and tested) in the C++ library, our job in this test suite is to make sure that we're correctly translating the R options to the C++ options. We aren't responsible for making sure that the C++ library correctly handles all permutations of its options--we obviously want that, and if we discover ways that it doesn't handle them correctly, we should make sure that gets fixed, but the R test suite doesn't need to assert that. -- 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. For queries about this service, please contact Infrastructure at: [email protected]
