-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
ES|QL kql function. #116764
ES|QL kql function. #116764
Conversation
Documentation preview: |
e13af7c
to
44bb5a0
Compare
44bb5a0
to
a271aea
Compare
…esql-kql-function
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.
Thanks for putting these together @afoucret ! It looks pretty good to me, I added some comments.
Here are my understanding of the KQL
function, and please correct me if I missed anything:
- In terms of the interface, the
KQL
function is very similar toQSTR
so it makes sense to follow a similar approach asQSTR
to support it in theES|QL
. - However they are different in syntax according to this google doc, although their syntax look very similar to me, I won't be surprised that I might make mistakes on the syntax when using them. From users perspective, it will be helpful to have a page(or a link to existing ES docs) in the
ES|QL
doc to describe when to useKQL
and when to useQSTR
, and have examples to describe the usages. Especially they are now both available throughES|QL
we may want to suggest the best practice to reduce confusions. Perhaps it is only me, but I'm not clear when I should use which function. This is not a needed for a snapshot function, but it will be helpful to get us prepared for a tech preview or GA. - I suggest to add ES|QL-ui label to notify kibana about this new function, it is near the FF of 8.17, it will be nice to give them a heads up if this is targeting to 8.17.
- I suggest to add test-release label to run this through release tests, sometimes it does catch surprise for us.
@@ -788,6 +789,14 @@ private static void checkFullTextQueryFunctions(LogicalPlan plan, Set<Failure> f | |||
qsf -> "[" + qsf.functionName() + "] " + qsf.functionType(), | |||
failures | |||
); | |||
checkCommandsBeforeExpression( |
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.
We could do a refactor here to reduce repetitive code, if Kql
and QueryString
have the same restrictions, they can be added in a collection and call checkCommandsBeforeExpression
in a loop.
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.
Added a loop that remove the duplicated code.
@@ -409,7 +410,8 @@ private static FunctionDefinition[][] snapshotFunctions() { | |||
// This is an experimental function and can be removed without notice. | |||
def(Delay.class, Delay::new, "delay"), | |||
def(Categorize.class, Categorize::new, "categorize"), | |||
def(Rate.class, Rate::withUnresolvedTimestamp, "rate") } }; | |||
def(Rate.class, Rate::withUnresolvedTimestamp, "rate"), | |||
def(Kql.class, Kql::new, "kql") } }; |
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.
Shall we follow the alphabetical order here?
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.
Done. Thank you.
@Override | ||
public String functionName() { | ||
return "KQL"; | ||
} |
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 override to functionName
can be skipped, as the function name is the same as class name.
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.
Removed the override.
import java.util.List; | ||
import java.util.function.Supplier; | ||
|
||
@FunctionName("kql") |
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 annotation can be skipped, as the class name is the same as function name.
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.
Removed the annotation.
Pinging @elastic/kibana-esql (ES|QL-ui) |
@fang-xing-esql Thanks for the detailed review and feedback! You're absolutely right that the That said, for KQL, our intention is not to promote it as a standalone feature but rather to facilitate users migrating to ES|QL. This is why we’ve kept communication about it relatively low-key. Few other points:
|
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.
Thank you @afoucret! LGTM. And thanks to @carlosdelest for reviewing as well.
8bd0239
to
2354e07
Compare
2354e07
to
02ea75e
Compare
…esql-kql-function
…esql-kql-function
💚 Backport successful
|
This PR introduces support for the KQL query language as an ES|QL function.
Our main goal is to provides users a way forward to migrate from KQL to ES|QL by integrating KQL queries directly into ES|QL queries.
We recently added the support of KQL directly form the query DSL:
This PR expose this new capacity of the query DSL through a new
KQL
fulltext filtering functionThe
KQL
function is conceptually very similar to theQSTR
function and come with the same limitations.The new function is enabled only in snapshots builds (same for the KQL query in the query DSL)