-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
EQL: Adds "fields" request field to the eql request (#68962) #69447
Conversation
(cherry picked from commit 22c880e)
Pinging @elastic/es-ql (Team:QL) |
@@ -78,7 +101,14 @@ public BytesReference toBytes(XContentType type) { | |||
hits = new ArrayList<>(); | |||
for (int i = 0; i < size; i++) { | |||
BytesReference bytes = new RandomSource(() -> randomAlphaOfLength(10)).toBytes(xType); | |||
hits.add(new Event(String.valueOf(i), randomAlphaOfLength(10), bytes)); | |||
Map<String, DocumentField> fetchFields = new HashMap<>(); | |||
for (int j = 0; j < randomIntBetween(0, 5); j++) { |
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.
Shouldn't this be a var before the 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.
LGTM. Left 2 comments.
@@ -49,6 +59,55 @@ setup: | |||
- match: {hits.events.1._id: "2"} | |||
- match: {hits.events.2._id: "3"} | |||
|
|||
--- | |||
"Execute EQL events query with fields filtering": | |||
- do: |
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.
should those have maybe the:
- skip:
version: " - 7.12.99"
block?
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.
I guess the 7.12 port will have the versions kept as in original PR?
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.
There won't be a 7.12 port, only 7.x.
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.
Ah, right, >feature
!
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.
I don't think the EQL yaml tests run in a mixed cluster (or any other bwc context) scenario that warrants for this skip
block.
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.
if (version.onOrAfter(Version.V_7_13_0)) { | ||
mutatedEvents.add(new Event(e.index(), e.id(), e.source(), e.fetchFields())); | ||
} else { | ||
mutatedEvents.add(new Event(e.index(), e.id(), e.source(), null)); | ||
} |
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.
nit: optionally, you could add one line (or two) only, if you condition just last parameter's value by the version (like done in the Request test).
@elasticmachine update branch |
Backport of #68962.
(cherry picked from commit 22c880e)