-
Notifications
You must be signed in to change notification settings - Fork 875
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 ilike_utf8_scalar kernals #2545
Conversation
@@ -468,7 +468,7 @@ pub fn ilike_utf8_scalar<OffsetSize: OffsetSizeTrait>( | |||
if !right.contains(is_like_pattern) { | |||
// fast path, can use equals | |||
for i in 0..left.len() { | |||
result.append(left.value(i) == right); | |||
result.append(left.value(i).to_uppercase() == right.to_uppercase()); |
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.
This needs to be done once only for the right
scalar?
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.
Sorry, A bit confused.
Did you mean that the right.to_uppercase() and be extracted to outside the if block rather than doing it every time in the if else blocks ?
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, and also outside the inner loop. The compiler often won't find out that it's possible to do this.
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.
Fixed in the latest commit
@@ -551,7 +551,7 @@ pub fn nilike_utf8_scalar<OffsetSize: OffsetSizeTrait>( | |||
if !right.contains(is_like_pattern) { | |||
// fast path, can use equals | |||
for i in 0..left.len() { | |||
result.append(left.value(i) != right); | |||
result.append(left.value(i).to_uppercase() != right.to_uppercase()); |
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.
Can be done once only
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.
Very nice 👍
Benchmark runs are scheduled for baseline = 89f2e45 and contender = b2f0c65. b2f0c65 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2544 .
Rationale for this change
Incorrect logic in equals path of ilike_utf8_scalar kernals
What changes are included in this PR?
Bug fix
Are there any user-facing changes?