EQL: Adds "fields" request field to the eql request #68962
Conversation
elasticmachine
added
the
Team:QL (Deprecated)
|
Pinging @elastic/es-ql (Team:QL) |
| // NB:this is different from mget | ||
| .size(idQuery.ids().size()); | ||
| if (fetchFields != null) { | ||
| fetchFields.stream().forEach(f -> builder.fetchField(f)); |
There was a problem hiding this comment.
forEach exists directly on List - no need to create the stream.
Also use the method reference : fetchFields.forEach(builder::fetchField)
|
|
||
| // add the "fields" to be fetched | ||
| if (fetchFields != null) { | ||
| fetchFields.stream().forEach(f -> source.fetchField(f)); |
There was a problem hiding this comment.
Same comment as above re: forEach & method reference.
matriv
left a comment
matriv
left a comment
There was a problem hiding this comment.
LGTM. Left a question regarding serialisation.
| if (out.getVersion().onOrAfter(Version.V_7_10_0)) { // TODO: Remove after backport | ||
| out.writeString(resultPosition); | ||
| } | ||
| if (out.getVersion().onOrAfter(Version.V_7_12_0)) { |
There was a problem hiding this comment.
Shouldn't this be V_8_0_0 for now and once backported switch to V_8_0_0?
There was a problem hiding this comment.
@matriv @bpintea Actually, the new test I added - https://github.com/elastic/elasticsearch/pull/68962/files#diff-7e1220049ee22470c58a915155e9d9bd3ef7742523a6bfd0e71b8c463df3e638 - won't let me use V_8_0_0 because a version 8.0 of ES should be compatible with the latest version 7.x of ES (which is 7.12). And the test in AbstractBWCSerializationTestCase is doing serialization and deserialization on 7.12 and compares the deserialized instance with the original (8.0) instance. And the test fails, because the two instances should be equal.
| if (in.getVersion().onOrAfter(Version.V_7_10_0)) { | ||
| resultPosition = in.readString(); | ||
| } | ||
| if (in.getVersion().onOrAfter(Version.V_7_12_0)) { |
There was a problem hiding this comment.
There are a couple more instances of this test. But I assume it's fine, since there aren't bwc integration tests like we have for SQL, so this should work?
| protected EqlSearchRequest createTestInstance() { | ||
| try { | ||
| List<FieldAndFormat> randomFetchFields = new ArrayList<>(); | ||
| for (int j = 0; j < randomIntBetween(0, 5); j++) { |
There was a problem hiding this comment.
Still getting you a random number of iterations, but the exit condition is (likely) going to change with each iteration.
There was a problem hiding this comment.
Indeed. Nice catch!
| if (in.getVersion().onOrAfter(Version.V_7_10_0)) { | ||
| resultPosition = in.readString(); | ||
| } | ||
| if (in.getVersion().onOrAfter(Version.V_7_12_0)) { |
There was a problem hiding this comment.
There are a couple more instances of this test. But I assume it's fine, since there aren't bwc integration tests like we have for SQL, so this should work?
|
@elasticmachine run elasticsearch-ci/2 |
|
@elasticmachine update branch |
(cherry picked from commit 22c880e)
|
CC @jrodewig. Sorry for the delay. |
|
No worries. Thanks @astefan. I'll work on a related docs PR for these changes. |
8 participants
Adds a "fields" section to the body of an eql request, identical to how a _search endpoint handles a "fields" request element.
Addresses #68115.