EQL: endsWith function implementation#54442
Conversation
|
Pinging @elastic/es-ql (:Query Languages/EQL) |
aleksmaus
left a comment
There was a problem hiding this comment.
one nit, otherwise LGTM
| } | ||
|
|
||
| EndsWithFunctionPipe other = (EndsWithFunctionPipe) obj; | ||
| return Objects.equals(source, other.source) |
There was a problem hiding this comment.
nit: the getters were used instead in the previous PR and EndsWithFunctionProcessor. maybe make it consistent.
| throw new EqlIllegalArgumentException("A string/char is required; received [{}]", pattern); | ||
| } | ||
|
|
||
| return source.toString().toLowerCase(Locale.ROOT).endsWith(pattern.toString().toLowerCase(Locale.ROOT)); |
There was a problem hiding this comment.
based off some of the conversations that have been happening, I think we're going to outsource this to something else eventually.
I don't know if that means we should do case-sensitivity in all PRs now, or come back to this later
There was a problem hiding this comment.
IMHO, this covers the feature parity with the original EQL implementation, and passes test against the EQL test data/queries.
We can merge this as is and keep iterating and moving forward.
Also this gives us a chance to refactor some of the code that can be better shared/reused across all of the functions implementations better.
costin
left a comment
There was a problem hiding this comment.
LGTM.
The string functions have a lot of boilerplate in common and after this PR gets in, it's worth revisiting and extracting a common class for them.
(cherry picked from commit 554a4c8)
Addresses #53854