EQL: Adds "fields" request field to the eql request #68962
Conversation
elasticmachine
added
the
Team:QL (Deprecated)
|
Pinging @elastic/es-ql (Team:QL) |
| @@ -137,6 +140,9 @@ public void fetchHits(Iterable<List<HitReference>> refs, ActionListener<List<Lis | |||
| // the default size is 10 so be sure to change it | |||
| // 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)
| @@ -61,6 +64,11 @@ public static SearchSourceBuilder sourceBuilder(QueryContainer container, QueryB | |||
| // disable the source, as we rely on "fields" API | |||
| source.fetchSource(false); | |||
|
|
|||
| // 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.
| @@ -324,6 +363,12 @@ public void writeTo(StreamOutput out) throws IOException { | |||
| 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.
| @@ -106,6 +115,11 @@ public EqlSearchRequest(StreamInput in) throws IOException { | |||
| 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?
| @@ -56,6 +59,13 @@ protected NamedXContentRegistry xContentRegistry() { | |||
| @Override | |||
| 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!
| @@ -106,6 +115,11 @@ public EqlSearchRequest(StreamInput in) throws IOException { | |||
| 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. |
Adds a "fields" section to the body of an eql request, identical to how a _search endpoint handles a "fields" request element.
Addresses #68115.