EQL: Add Head/Tail pipe support by costin · Pull Request #58536 · elastic/elasticsearch

costin · 2020-06-25T12:02:45Z

Introduce pipe support, in particular head and tail
(which can also be chained).

Replaces #58326

Introduce pipe support, in particular head and tail (which can also be chained).

elasticmachine · 2020-06-25T12:02:47Z

Pinging @elastic/es-ql (:Query Languages/EQL)

costin · 2020-06-25T12:07:03Z

The main idea behind this PR is to support head/tail through a limit (w/ offset). Further more when dealing with tail, data needs to be returned in descending format however since ES does not offer a search_before construct, this is implemented by making non-base queries in a sequence to be ASC and then looked at in reverse.

The code can be further polished, in particular the query side and progression of the keys (so its handling doesn't leak inside the ExecutionManager) however I opted to do that in a follow-up PR, when chasing internal pagination (as that's when the keys will be picked up by the same queries as it advances through the data).

Add Head and Tail to simplify verification and future identification of said nodes.

costin · 2020-06-25T21:38:34Z

docs/reference/eql/eql-search-api.asciidoc

          1607252647000
-        ]
+        ],
+        "fields": {


@jrodewig is there a way to ignore the "fields" field? This data is used internally and it might be that in the future, we'll remove it from the final response (so only _source is included).

You can use the filter_path query param to exclude fields from the output.
You can add the query param using a //TEST comment so it's not visible to users.

This should fail gracefully. The request won't return an error if fields is later removed from the response.

Here's an example with a basic query:

[source,console] ---- GET /sec_logs/_eql/search { "query": """ process where process.name == "cmd.exe" """ } ---- // TEST[s/search/search\?filter_path\=\-\*\.events\.\*fields/]

A sequence query will need to use a different filter because the response format is different.

[source,console] ---- GET /my_index/_eql/search { "query": """ sequence by agent.id [ file where file.name == "cmd.exe" and agent.id != "my_user" ] [ process where stringContains(process.path, "regsvr32") ] """ } ---- // TEST[s/search/search\?filter_path\=\-\*\.sequences\.events\.\*fields/]

For some reason this didn't seem to work consistently so I left things in a bit of a mixed state, namely the fields tag is still in there but I also added the TESTRESPONSE; the idea being that the user should not depend on whether fields (or even sort) are in the response.
Can you please take a look at it after this PR gets merged?

Sure thing. Thanks!

astefan

LGTM. Left some comments.

astefan · 2020-06-26T07:51:19Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/assembler/Criterion.java

    private final HitExtractor tiebreakerExtractor;

+    // search after markers
+    private Object[] startMarker;


Maybe name these with the plural variant, since they are arrays?

I had them as such but in case of a timestamp there's only one marker. Plus the fact that it's one or multiple fields is an implementation detail in the end.
Hence why talking a marker as an entity (which currently happens to be an array of objects) seems better.

astefan · 2020-06-26T08:00:54Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/assembler/Criterion.java

+            tiebreaker = tiebreaker(hit);
+        }
+
+        return tiebreaker != null ? new Object[] { timestamp, tiebreaker } : new Object[] { timestamp };


Since the markers can only be a combination of timestamp and tiebreaker OR only the timestamp, maybe abstract these two variants away in a TiebreakerMarker and TimestampMarker or something around these lines?

I think the whole marker bit can be encapsulated in the Criterion itself, I plan to revisit this when working on the pagination.

astefan · 2020-06-26T08:30:39Z

.../plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/assembler/KeyAndOrdinal.java

+
+    @Override
+    public String toString() {
+        return key + "[" + timestamp + "][" + (tiebreaker != null ? Objects.toString(tiebreaker) : "") + "]";


If the tiebreaker is not set, why still logging the square brackets?

To indicate there is no tiebreaker - I find the message more consistent to be [..][] vs [..]

Myself, I wouldn't mind to have [] when there is no tiebreaker.

How about specifying that there is no tiebreaker? (ie [...][no tiebreaker])

That's quite verbose...

...lugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/assembler/SequenceRuntime.java

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/parser/LogicalPlanBuilder.java

astefan · 2020-06-26T09:28:12Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/planner/QueryFolder.java

    }
-}
+
+    abstract static class QueryFoldingRule<SubPlan extends UnaryExec> extends FoldingRule<SubPlan> {


Is this being used anywhere?

Thanks. Used it for the existing rules

x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/optimizer/OptimizerTests.java

matriv

Looks good overall, left some minor comments, but additionally can we have some unit tests for the new optimizer rules: SortByLimit, PushDownOrderBy and SkipQueryOnLimitZero? (unless I missed something :) )

matriv · 2020-06-26T10:35:37Z

.../plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/assembler/KeyAndOrdinal.java

+
+    @Override
+    public String toString() {
+        return key + "[" + timestamp + "][" + (tiebreaker != null ? Objects.toString(tiebreaker) : "") + "]";


Myself, I wouldn't mind to have [] when there is no tiebreaker.

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/parser/LogicalPlanBuilder.java

matriv · 2020-06-26T12:10:47Z

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/plan/physical/SequenceExec.java

                        Attribute timestamp,
-                        Attribute tiebreaker) {
-        this(source, combine(matches, until), combine(keys, singletonList(untilKeys)), timestamp, tiebreaker);
+                        Attribute tiebreaker, OrderDirection direction) {


Minor: code formatting for consistency:

Suggested change

Attribute tiebreaker, OrderDirection direction) {

Attribute tiebreaker,

OrderDirection direction) {

matriv · 2020-06-26T12:11:40Z

.../plugin/eql/src/main/java/org/elasticsearch/xpack/eql/querydsl/container/QueryContainer.java

        this.attributes = attributes;
        this.trackHits = trackHits;
        this.includeFrozen = includeFrozen;
+


minor: empty line

Introduce pipe support, in particular head and tail (which can also be chained). (cherry picked from commit 4521ca3) (cherry picked from commit d6731d6)

EQL: Add Head/Tail pipe support

c930e3d

Introduce pipe support, in particular head and tail (which can also be chained).

costin added the :Analytics/EQL EQL querying label Jun 25, 2020

costin requested review from aleksmaus, astefan, matriv and rw-access June 25, 2020 12:02

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Jun 25, 2020

costin added 2 commits June 25, 2020 20:58

Fix failing tests

4551c77

Add Head and Tail to simplify verification and future identification of said nodes.

Update docs

86181b9

costin commented Jun 25, 2020

View reviewed changes

fix checkstyle

40ada65

astefan approved these changes Jun 26, 2020

View reviewed changes

matriv reviewed Jun 26, 2020

View reviewed changes

Address feedback

f7d3e1b

costin merged commit 4521ca3 into elastic:master Jun 27, 2020

costin deleted the eql/head-tail branch June 27, 2020 06:08

costin added a commit that referenced this pull request Jun 27, 2020

EQL: Add Head/Tail pipe support (#58536)

3c81b91

Introduce pipe support, in particular head and tail (which can also be chained). (cherry picked from commit 4521ca3) (cherry picked from commit d6731d6)

costin mentioned this pull request Jun 27, 2020

EQL: Add head/tail support #58326

Closed

jrodewig mentioned this pull request Jun 29, 2020

[DOCS] EQL: Document head and tail pipes #58673

Merged

costin mentioned this pull request Jul 2, 2020

Implement EQL Pipes in EQL Plugin #49627

Closed

7 tasks

costin mentioned this pull request Jul 16, 2020

EQL - query language for event data in Elasticsearch #59686

Closed

18 tasks

	Attribute tiebreaker, OrderDirection direction) {
	Attribute tiebreaker,
	OrderDirection direction) {

Conversation

costin commented Jun 25, 2020

Uh oh!

elasticmachine commented Jun 25, 2020

Uh oh!

costin commented Jun 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants