-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Fix KQL case-sensitivity for keyword fields in ES|QL #135776
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
Conversation
The KQL implementation used by the ES|QL kql() function incorrectly defaulted to case-insensitive matching for keyword fields. This behavior was inconsistent with the KQL specification and the implementation in Kibana, which both perform exact, case-sensitive matches on keyword fields. This commit corrects the default behavior to enforce case-sensitivity for keyword fields within ES|QL, aligning it with the documented and expected behavior. Fixes elastic#135772
Hi @afoucret, I've created a changelog YAML for you. |
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
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.
LGTM!
|
||
for (boolean caseInsensitive : List.of(true, false)) { | ||
KqlQueryBuilder kqlQuery = new KqlQueryBuilder(KEYWORD_FIELD_NAME + ": foo*"); | ||
// Check case case_insensitive is true by default |
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.
// Check case case_insensitive is true by default | |
// Check case case_insensitive is false by default |
|
||
for (boolean caseInsensitive : List.of(true, false)) { | ||
KqlQueryBuilder kqlQuery = new KqlQueryBuilder(KEYWORD_FIELD_NAME + ": foo"); | ||
// Check case case_insensitive is true by default |
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.
// Check case case_insensitive is true by default | |
// Check case case_insensitive is false by default |
|
||
for (boolean caseInsensitive : List.of(true, false)) { | ||
KqlQueryBuilder kqlQuery = new KqlQueryBuilder(KEYWORD_FIELD_NAME + ": foo*"); | ||
// Check case case_insensitive is true by default |
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.
// Check case case_insensitive is true by default | |
// Check case case_insensitive is false by default |
|
||
for (boolean caseInsensitive : List.of(true, false)) { | ||
KqlQueryBuilder kqlQuery = new KqlQueryBuilder(KEYWORD_FIELD_NAME + ": foo"); | ||
// Check case case_insensitive is true by default |
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.
// Check case case_insensitive is true by default | |
// Check case case_insensitive is false by default |
Despite this being an inconsistent behavior, we actually have been exploiting this behavior in ES|QL to do case-insensitive searches on keywords where we ordinarily would have to resort to other hacks, like doing a regex DSL with case-insensitive set to true, or doing a to_lower()/upper() in ES|QL. That being said, those functions are much slower than the KQL() function. It may be wise for there to be a toggle on this so users can choose the behavior they want to see. There has also been a long standing ticket asking for a similar behavior, like this: elastic/kibana#55378 |
That sounds great. Perhaps the result can be slightly more user-friendly, especially if there are no other variables that would need to be passed. Perhaps naming the function KQLi/kqli with and passing the Just like using the DSL like this would be cumbersome:
Perhaps adding a toggle in Classic Discover would make it more friendly, although it got closed as not planned under: elastic/kibana#55378. Also, out of scope of this ticket... :) |
@swg0101 There are probably some improvements that can be done in the UX but I am very reluctant to introduce new functions for things that can accessible through an optional parameter. My plan for KQL function is to expose the
Last but mot least, our plan with the KQL query is to allow users to migrate to ES|QL more easily. We do not want to add new features to the KQL language and would prefer that people rewrite their queries using native ES|QL syntax / functions. |
That makes sense. I am guessing that as long as autocomplete works well, then it probably wouldn't be that much of a problem.
|
This pull request addresses an inconsistency in the KQL implementation used by ES|QL, where queries on keyword fields were incorrectly case-insensitive by default. This behavior contradicted the KQL documentation and the behavior of KQL in Kibana, which both specify exact, case-sensitive matching for keyword fields.
The change adjusts the KQL parsing context to ensure that matching on keyword fields is case-sensitive, aligning the ES|QL implementation with the expected standard.
Fixes #135772