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.

Sign up for GitHub

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

Jump to bottom

str_like case sensitivity #1490

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

str_like case sensitivity #1490

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d22e8e9
str_like case sensitivity
edward-burn Apr 4, 2024
e4da507
snapshot test
edward-burn Apr 4, 2024
e6ea1e3
cli message and snaps
edward-burn Apr 4, 2024
029aaee
updated sqlite version
edward-burn Apr 4, 2024
127a932
another snap
edward-burn Apr 4, 2024
43f9fb5
updates
edward-burn Apr 5, 2024
a8d36a6
formatting
edward-burn Apr 10, 2024
1cfd161
change ignore_case default to FALSE
edward-burn Apr 10, 2024
eff93fe
Merge branch 'main' into main
edward-burn May 3, 2024
608631c
update snaps
edward-burn May 3, 2024
8bd216c
delete extra line
edward-burn May 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions R/backend-.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,9 +282,13 @@ base_scalar <- sql_translator(
str_sub = sql_str_sub("SUBSTR"),
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 = "Note, using `tolower()` to cast string and pattern to lower case would also achieve a case insenitive match."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
i = "Note, using `tolower()` to cast string and pattern to lower case would also achieve a case insenitive match."
i = "Or use `tolower()` on both arguments to achieve a case insensitive match."

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have updated as suggested

))
} 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
View file
Open in desktop
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
View file
Open in desktop
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>

4 changes: 2 additions & 2 deletions tests/testthat/test-backend-.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,9 @@ test_that("lead and lag translate n to integers", {

test_that("can translate case insensitive like", {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
test_that("can translate case insensitive like", {
test_that("can't translate case insensitive like", {

?

Could you please also add a test for case sensitive like? Or maybe come up with a more encompassing test name and test both sides of this function in the same test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the tests. I'm not sure how to add more tests for case sensitive like here beyond checking for the expected errors. Where ILIKE is being used, for example for postgres, there are tests of it in the backend specific tests

local_con(simulate_dbi())
test_translate_sql(str_like(x, "abc"))
test_translate_sql(str_like(x, "abc", ignore_case = FALSE))
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