EQL: Add AstBuilder to convert to QL tree #51558
Conversation
|
Pinging @elastic/es-search (:Search/EQL) |
x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/parser/ExpressionBuilder.java
Outdated
Show resolved
Hide resolved
|
|
||
| switch (op.getSymbol().getType()) { | ||
| case EqlBaseParser.EQ: | ||
| // TODO: check for left == null after moving IsNotNull from SQL -> QL |
There was a problem hiding this comment.
IsNotNull and IsNull haven't been moved since in SQL null means missing as oppose to null value. This leads to subtle semantics such as 3-value bool logic (TRUE AND null -> null, FALSE AND null -> FALSE, TRUE AND TRUE -> TRUE).
Are the semantics the same ?
There was a problem hiding this comment.
Great point. We've used == null and != null for existence checks in EQL, but with ES can fields be set directly to null? That seems like it would rarely provide value. So I think we should be okay with using null as missing here too.
There was a problem hiding this comment.
In ES null also means missing however one can set a default value to check on it (see https://www.elastic.co/guide/en/elasticsearch/reference/current/null-value.html).
The not null checks map nicely to dedicated queries (missing / not-missing). Not sure about the bool logic though but we can have dedicated AND / OR for EQL if the behavior differs from that of SQL.
x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/parser/IdentifierBuilder.java
Show resolved
Hide resolved
| } | ||
|
|
||
| // unescaped strings can be interpreted directly | ||
| if (text.startsWith("?")) { |
There was a problem hiding this comment.
Should it be ?" or is ?' allowed as well ? In both cases I would do the check a bit stricter to not trip if somehow a different char follows ?
There was a problem hiding this comment.
Both ?"..." and ?'...' are valid syntax, and enforced by the grammar, so we're okay. I can add a unit test for leaving a space between ? and "/' or something like that.
x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/parser/LiteralBuilder.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/parser/QueryBuilder.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/parser/ExpressionTests.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/parser/ExpressionTests.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/parser/ExpressionTests.java
Outdated
Show resolved
Hide resolved
| @@ -113,14 +115,7 @@ static String text(ParseTree node) { | |||
| return node == null ? null : node.getText(); | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Maybe you can keep the comment.
|
|
||
| /* |
| assertEquals(new Or(null, lhs, rhs), booleanOr); | ||
| } | ||
|
|
||
| /* |
There was a problem hiding this comment.
@rw-access Is this still needed as is?
| @@ -33,4 +34,23 @@ public String name() { | |||
| public String toString() { | |||
| return name; | |||
| } | |||
|
|
|||
| @Override | |||
|
@matriv for EQL we agreed to not add any labels during development, only before the actual release. |
* EQL: Plug query params into the AstBuilder (#51886) As the eventType is customizable, plug that into the parser based on the given request. (cherry picked from commit 5b4a3a3) * EQL: Add field resolution and verification (#51872) Add basic field resolution inside the Analyzer and a basic Verifier to check for any unresolved fields. (cherry picked from commit 7087358) * EQL: Introduce basic execution pipeline (#51809) Add main classes that form the 'execution' pipeline are added - most of them have no functionality; the purpose of this PR is to add flesh out the contract between the various moving parts so that work can start on them independently. (cherry picked from commit 9a1bae5) * EQL: Add AstBuilder to convert to QL tree (#51558) * EQL: Add AstBuilder visitors * EQL: Add tests for wildcards and sets * EQL: Fix licensing * EQL: Fix ExpressionTests.java license * EQL: Cleanup imports * EQL: PR feedback and remove LiteralBuilder * EQL: Split off logical plan from expressions * EQL: Remove stray import * EQL: Add predicate handling for set checks * EQL: Remove commented out dead code * EQL: Remove wildcard test, wait until analyzer (cherry picked from commit a462700) * EQL grammar updates and tests (#49658) * EQL: Additional tests and grammar updates * EQL: Add backtick escaped identifiers * EQL: Adding keywords to language * EQL: Add checks for unsupported syntax * EQL: Testing updates and PR feedback * EQL: Add string escapes * EQL: Cleanup grammar for identifier * EQL: Remove tabs from .eql tests (cherry picked from commit 6f1890b)
This adds visit* methods for the AstBuilder to convert an ANTLR tree into a QL tree. I've only scoped this to stateless expressions (no sequence, join, pipes, ancestry).
There are a few cases of EQL are just shorthand for now, unless we add more optimal/direct support within QL:
x in (a, b, c, ...)->x == a or x == b or x == c or ...x == "some*wildcard*expr*"->wildcard(x, "some*wildcard*expr*")Functions get turned into
UnresolvedFunction. I think at some point, we'll need to add QL support for these or have these functions be EQL only with a custom registry. I haven't dived much into that yet to see how this works. I'm assuming that this will be done in a separate follow up PR (created issue #51556 for new functions)Related Issues