EQL: Add field resolution and verification #51872
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Pinging @elastic/es-search (:Search/EQL) |
Add basic field resolution inside the Analyzer and a basic Verifier to check for any unresolved fields
costin
force-pushed
the
eql/add-basic-analysis
branch
from
9434d38 to
14893bf
Compare
costin
commented
Feb 4, 2020
| } | ||
| } | ||
|
|
||
| abstract static class AnalyzeRule<SubPlan extends LogicalPlan> extends Rule<SubPlan, LogicalPlan> { |
There was a problem hiding this comment.
There will be a separate PR to externalize this to QL so it can be shared across SQL and EQL.
| public abstract class LogicalPlanBuilder extends ExpressionBuilder { | ||
|
|
||
| // TODO: these need to be made configurable | ||
| private static final String EVENT_TYPE = "event.category"; | ||
| private static final EsIndex esIndex = new EsIndex("<not-specified>", emptyMap()); | ||
| private static final String EVENT_TYPE = "event_type"; |
There was a problem hiding this comment.
I changed this since the data sample from existing EQL documentation uses event_type. If the default field should be changed, that is best addressed separately:
- figure out what's the impact on backwards compatibility (ECS schema) and existing data
- update datasets
- update code (last).
There was a problem hiding this comment.
was assuming that the data will be in ECS format
"user": {
"group": {}
},
"host": {
"os": {
"platform": "windows",
"name": "Windows"
},
"ip": "127.0.0.1",
"hostname": "localhost",
"name": "localhost"
},
"event": {
"module": "endgame",
"dataset": "esensor",
"action": "already_running",
"category": "process",
"kind": "event"
},
thus the event.category for EVENT_TYPE
|
Follow-up issue to beef the verifier tests further here: #51873 |
rw-access
reviewed
Feb 4, 2020
| @@ -43,6 +39,6 @@ public LogicalPlan visitEventQuery(EqlBaseParser.EventQueryContext ctx) { | |||
|
|
|||
| } | |||
|
|
|||
| return new Filter(source(ctx), new EsRelation(Source.EMPTY, esIndex, false), condition); | |||
| return new Filter(source(ctx), new UnresolvedRelation(Source.EMPTY, null, "", false, ""), condition); | |||
rw-access
reviewed
Feb 4, 2020
| @@ -89,11 +89,11 @@ public boolean equals(Object obj) { | |||
| } | |||
|
|
|||
| UnresolvedRelation other = (UnresolvedRelation) obj; | |||
| return source().equals(other.source()) | |||
| && table.equals(other.table) | |||
| return Objects.equals(source(), other.source()) | |||
There was a problem hiding this comment.
why does source() need to be checked here, but isn't in some of the other hashCode and equals functions?
There was a problem hiding this comment.
It's an inconsistency - the source isn't relevant. Likely for UnresolvedRelation was added since a table or index declaration is not common and thus, its source was considered relevant.
The change above was added to avoid a NPE caused by a null (vs an empty) location.
rw-access
reviewed
Feb 4, 2020
| } | ||
|
|
||
| return u.withUnresolvedMessage( | ||
| "Reference [" + u.qualifiedName() + "] is ambiguous (to disambiguate use quotes or qualifiers); matches any of " |
There was a problem hiding this comment.
We may have to come back and clarify "quotes" in this message for #51443
rw-access
approved these changes
Feb 4, 2020
aleksmaus
approved these changes
Feb 4, 2020
costin
added a commit
that referenced
this pull request
Feb 5, 2020
* 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)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add basic field resolution inside the Analyzer and a basic Verifier to
check for any unresolved fields