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

str_like case sensitivity #1490

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# dbplyr (development version)
* The translation of `stringr::str_like()` now defaults to use case-sensitive LIKE when argument `ignore_case` is set as FALSE instead of when this argument is set as TRUE.(@edward-burn, #1488)

# dbplyr 2.5.0

Expand Down
9 changes: 7 additions & 2 deletions R/backend-.R
Original file line number Diff line number Diff line change
Expand Up @@ -280,11 +280,16 @@ base_scalar <- sql_translator(
str_trim = sql_str_trim,
str_c = sql_paste(""),
str_sub = sql_str_sub("SUBSTR"),
# https://docs.getdbt.com/sql-reference/like is typically case sensitive
str_like = function(string, pattern, ignore_case = TRUE) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if ignore_case should default to FALSE? I filed an issue in stringr to make sure to fix it there too: tidyverse/stringr#543.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I do see that but for now I have left this as is in this pr because it seems that would first need to change in stringr before changing here in dbplyr? I also have commented on that issue with another idea (stringr exporting a str_ilike function)

Perhaps for this pr it can be kept as is, and then changed in the future to reflect changes as and when they happen in stringr? But equally I'm happy to change things here if you prefer?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to just make the change now — it's technically out of sync but I don't think this function is important enough to manage the updates across multiple package updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I have changed the default to FALSE in my latest commit

(I also opened a pr on stringr to introduce str_ilike tidyverse/stringr#544, but I imagine it would be best to wait for a future version of stringr to be released with that, if you´re happy with it, before adding it in here in dbplyr)

if (isTRUE(ignore_case)) {
hadley marked this conversation as resolved.
Show resolved Hide resolved
sql_expr(!!string %LIKE% !!pattern)
cli_abort(c(
"Backend does not support case insensitve {.fn str_like}.",
i = "Set ignore_case = FALSE for case sensitive match.",
i = "Or use `tolower()` on both arguments to achieve a case insensitive match."
))
} else {
cli::cli_abort("Backend only supports case insensitve {.fn str_like}.")
sql_expr(!!string %LIKE% !!pattern)
}
},

Expand Down
6 changes: 4 additions & 2 deletions tests/testthat/_snaps/backend-.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,12 @@
# can translate case insensitive like

Code
test_translate_sql(str_like(x, "abc", ignore_case = FALSE))
test_translate_sql(str_like(x, "abc", ignore_case = TRUE))
Condition
Error in `str_like()`:
! Backend only supports case insensitve `str_like()`.
! Backend does not support case insensitve `str_like()`.
i Set ignore_case = FALSE for case sensitive match.
i Note, using `tolower()` to cast string and pattern to lower case would also achieve a case insenitive match.

# default raw escapes translated correctly

Expand Down
2 changes: 1 addition & 1 deletion tests/testthat/_snaps/tbl-sql.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,6 @@
The `check_from` argument of `tbl_sql()` is deprecated as of dbplyr 2.5.0.
Output
# Source: table<`x`> [0 x 1]
# Database: sqlite 3.45.0 [:memory:]
# Database: sqlite 3.45.2 [:memory:]
# i 1 variable: y <lgl>

9 changes: 6 additions & 3 deletions tests/testthat/test-backend-.R
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,14 @@ test_that("lead and lag translate n to integers", {

# strings -----------------------------------------------------------------

test_that("can translate case insensitive like", {
test_that("can only translate case sensitive str_like", {
local_con(simulate_dbi())
test_translate_sql(str_like(x, "abc"))
expect_equal(test_translate_sql(str_like(x, "abc", ignore_case = FALSE)),
sql("`x` LIKE 'abc'"))
expect_equal(test_translate_sql(str_like(x, "ABC", ignore_case = FALSE)),
sql("`x` LIKE 'ABC'"))
hadley marked this conversation as resolved.
Show resolved Hide resolved
expect_snapshot(
test_translate_sql(str_like(x, "abc", ignore_case = FALSE)),
test_translate_sql(str_like(x, "abc", ignore_case = TRUE)),
error = TRUE
)
})
Expand Down
Loading