-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
evalEngine: Implement INSTR
#15127
evalEngine: Implement INSTR
#15127
Conversation
Signed-off-by: Noble Mittal <noblemittal@outlook.com>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #15127 +/- ##
===========================================
- Coverage 47.70% 40.89% -6.81%
===========================================
Files 1155 1454 +299
Lines 240218 368302 +128084
===========================================
+ Hits 114593 150616 +36023
- Misses 117020 201493 +84473
- Partials 8605 16193 +7588 ☔ View full report in Codecov by Sentry. |
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 current implementation here is wrong and we need to add support for finding substrings to the collations package in order to properly support this.
substr.bytes = bytes.ToLower(substr.bytes) | ||
} | ||
|
||
pos := bytes.Index(str.bytes, substr.bytes) + 1 |
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.
There's a reason why INSTR
is not implemented yet, and that is that we need to add functionality to the go/mysql/collations
package to do the searching for a substring. This implementation here is wrong and doesn't consider collations correctly. It assumes that it's all only ASCII by the ToLower
and doesn't handle non ASCII characters this way, nor does it handle any of the non-Unicode encodings here.
Oh I see. Thanks for the explanation. Closing this one. |
Description
This PR adds implementation of
INSTR
func in evalengine.Related Issue(s)
Fixes part of #9647
Checklist
Deployment Notes