-
Notifications
You must be signed in to change notification settings - Fork 786
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
fix(compute): LIKE/ILIKE/NLIKE escape parenthesis #1042
Conversation
Signed-off-by: Dmitry Patsura <talk@dmtry.me>
d69347a
to
e6e3244
Compare
Codecov Report
@@ Coverage Diff @@
## master #1042 +/- ##
=======================================
Coverage 82.31% 82.31%
=======================================
Files 168 168
Lines 49031 49037 +6
=======================================
+ Hits 40359 40365 +6
Misses 8672 8672
Continue to review full report at Codecov.
|
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.
vec!["varchar(255)", "int(255)", "varchar", "int"], | ||
"%(%)%", | ||
like_utf8_scalar, | ||
vec![true, true, false, false] |
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.
👍
let re_pattern = right | ||
.replace("%", ".*") | ||
.replace("_", ".") | ||
.replace("(", r#"\("#) |
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.
Thinking about his more, I wonder if other special regexp characters need to be replaced too (e.g. .
and +
)
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.
the list of special characters is here: https://docs.rs/regex/1.5.4/regex/#syntax
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.
good call. maybe we should pass right
through this, then add the wildcards
https://docs.rs/regex/0.2.2/regex/fn.escape.html
(I'm happy to help test this @ovr )
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.
that is a great call @jwdeitch -- so something like
let re_pattern = escape(right)
.replace('%', '.*')
.replace('_', '.')?
Perhaps
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 found the rust regex crate syntax difficult at the time so I would suggest adding test cases for all their escaping. Regex is so powerful it would be easy to miss something.
Signed-off-by: Dmitry Patsura <talk@dmtry.me>
Hello!
Which issue does this PR close?
I found that DF works strange with
LIKE %(%)%
But it should return false for the first case, because It doesn't contains
()
parenthesis.I found that prepared regexp is wrong and contains unescaped
(
&)
, but it should be escaped with\
.Thanks