-
Notifications
You must be signed in to change notification settings - Fork 173
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
base: main
Are you sure you want to change the base?
Conversation
If ignore_case is true, an error is thrown for dbms without ILIKE. If ignore_case is false, LIKE is used consistently across dbms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, it's much appreciated!
As well as the changes below, could you please add a bullet to the top of NEWS.md
? It should briefly describe the change and end with (@yourname, #issuenumber)
.
tests/testthat/test-backend-.R
Outdated
@@ -103,9 +103,9 @@ test_that("lead and lag translate n to integers", { | |||
|
|||
test_that("can translate case insensitive like", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
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
R/backend-.R
Outdated
@@ -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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work here, @edward-burn! Just merged upstream and made a few small changes.
Relates to issue #1488
This pr would change behaviour to the following
Created on 2024-04-04 with reprex v2.0.2