Skip to content

Commit b8bb8c6

Browse files
costincostin
committed
EQL: Avoid filtering on tiebreakers (#63415)
Do not filter by tiebreaker while searching sequence matches as it's not monotonic and thus can filter out valid data. Add handling for data 'near' the boundary that has the same timestamp but different tie-breaker and thus can be just outside the window. Fix #62781 Relates #63215 (cherry picked from commit 36f8346) (cherry picked from commit 72a2ce8) (cherry picked from commit 2ab5f22)
1 parent 967ba89 commit b8bb8c6

File tree

10 files changed

+218
-99
lines changed

10 files changed

+218
-99
lines changed

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

Lines changed: 6 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -6,48 +6,38 @@
66

77
package org.elasticsearch.xpack.eql.execution.assembler;
88

9-
import org.elasticsearch.index.query.BoolQueryBuilder;
109
import org.elasticsearch.index.query.RangeQueryBuilder;
1110
import org.elasticsearch.search.builder.SearchSourceBuilder;
1211
import org.elasticsearch.xpack.eql.execution.search.Ordinal;
1312
import org.elasticsearch.xpack.eql.execution.search.QueryRequest;
13+
import org.elasticsearch.xpack.eql.execution.search.RuntimeUtils;
1414

15-
import static org.elasticsearch.index.query.QueryBuilders.boolQuery;
1615
import static org.elasticsearch.index.query.QueryBuilders.rangeQuery;
1716

1817
/**
1918
* Ranged or boxed query. Provides a beginning or end to the current query.
2019
* The query moves between them through search_after.
21-
*
20+
*
2221
* Note that the range is not set at once on purpose since each query tends to have
2322
* its own number of results separate from the others.
24-
* As such, each query starts where it lefts to reach the current in-progress window
23+
* As such, each query starts from where it left off to reach the current in-progress window
2524
* as oppose to always operating with the exact same window.
2625
*/
2726
public class BoxedQueryRequest implements QueryRequest {
2827

2928
private final RangeQueryBuilder timestampRange;
30-
private final RangeQueryBuilder tiebreakerRange;
3129

3230
private final SearchSourceBuilder searchSource;
3331

3432
private Ordinal from, to;
3533
private Ordinal after;
3634

37-
public BoxedQueryRequest(QueryRequest original, String timestamp, String tiebreaker) {
35+
public BoxedQueryRequest(QueryRequest original, String timestamp) {
3836
searchSource = original.searchSource();
3937

4038
// setup range queries and preserve their reference to simplify the update
4139
timestampRange = rangeQuery(timestamp).timeZone("UTC").format("epoch_millis");
42-
BoolQueryBuilder filter = boolQuery().filter(timestampRange);
43-
if (tiebreaker != null) {
44-
tiebreakerRange = rangeQuery(tiebreaker);
45-
filter.filter(tiebreakerRange);
46-
} else {
47-
tiebreakerRange = null;
48-
}
49-
// add ranges to existing query
50-
searchSource.query(filter.must(searchSource.query()));
40+
RuntimeUtils.addFilter(timestampRange, searchSource);
5141
}
5242

5343
@Override
@@ -69,9 +59,6 @@ public void nextAfter(Ordinal ordinal) {
6959
public BoxedQueryRequest from(Ordinal begin) {
7060
from = begin;
7161
timestampRange.gte(begin != null ? begin.timestamp() : null);
72-
if (tiebreakerRange != null) {
73-
tiebreakerRange.gte(begin != null ? begin.tiebreaker() : null);
74-
}
7562
return this;
7663
}
7764

@@ -85,14 +72,10 @@ public Ordinal from() {
8572

8673
/**
8774
* Sets the upper boundary for the query (inclusive).
88-
* Can be removed (when the query in unbounded) through null.
8975
*/
9076
public BoxedQueryRequest to(Ordinal end) {
9177
to = end;
9278
timestampRange.lte(end != null ? end.timestamp() : null);
93-
if (tiebreakerRange != null) {
94-
tiebreakerRange.lte(end != null ? end.tiebreaker() : null);
95-
}
9679
return this;
9780
}
9881

@@ -104,4 +87,4 @@ public String toString() {
10487
private static String string(Ordinal o) {
10588
return o != null ? o.toString() : "<none>";
10689
}
107-
}
90+
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ public Executable assemble(List<List<Attribute>> listOfKeys,
7272
if (query instanceof EsQueryExec) {
7373
SearchSourceBuilder source = ((EsQueryExec) query).source(session);
7474
QueryRequest original = () -> source;
75-
BoxedQueryRequest boxedRequest = new BoxedQueryRequest(original, timestampName, tiebreakerName);
75+
BoxedQueryRequest boxedRequest = new BoxedQueryRequest(original, timestampName);
7676
Criterion<BoxedQueryRequest> criterion =
7777
new Criterion<>(i, boxedRequest, keyExtractors, tsExtractor, tbExtractor, i == 0 && descending);
7878
criteria.add(criterion);

‎x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/search/Ordinal.java‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ public Ordinal(long timestamp, Comparable<Object> tiebreaker) {
1717
this.timestamp = timestamp;
1818
this.tiebreaker = tiebreaker;
1919
}
20-
20+
2121
public long timestamp() {
2222
return timestamp;
2323
}
@@ -36,11 +36,11 @@ public boolean equals(Object obj) {
3636
if (this == obj) {
3737
return true;
3838
}
39-
39+
4040
if (obj == null || getClass() != obj.getClass()) {
4141
return false;
4242
}
43-
43+
4444
Ordinal other = (Ordinal) obj;
4545
return Objects.equals(timestamp, other.timestamp)
4646
&& Objects.equals(tiebreaker, other.tiebreaker);
@@ -81,7 +81,23 @@ public boolean between(Ordinal left, Ordinal right) {
8181
return (compareTo(left) <= 0 && compareTo(right) >= 0) || (compareTo(right) <= 0 && compareTo(left) >= 0);
8282
}
8383

84+
public boolean before(Ordinal other) {
85+
return compareTo(other) < 0;
86+
}
87+
88+
public boolean beforeOrAt(Ordinal other) {
89+
return compareTo(other) <= 0;
90+
}
91+
92+
public boolean after(Ordinal other) {
93+
return compareTo(other) > 0;
94+
}
95+
96+
public boolean afterOrAt(Ordinal other) {
97+
return compareTo(other) >= 0;
98+
}
99+
84100
public Object[] toArray() {
85101
return tiebreaker != null ? new Object[] { timestamp, tiebreaker } : new Object[] { timestamp };
86102
}
87-
}
103+
}

‎x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/search/RuntimeUtils.java‎

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,9 @@
1111
import org.elasticsearch.action.search.SearchRequest;
1212
import org.elasticsearch.action.search.SearchResponse;
1313
import org.elasticsearch.client.Client;
14+
import org.elasticsearch.index.query.BoolQueryBuilder;
15+
import org.elasticsearch.index.query.QueryBuilder;
16+
import org.elasticsearch.search.SearchHit;
1417
import org.elasticsearch.search.aggregations.Aggregation;
1518
import org.elasticsearch.search.builder.SearchSourceBuilder;
1619
import org.elasticsearch.xpack.eql.EqlIllegalArgumentException;
@@ -27,11 +30,14 @@
2730
import org.elasticsearch.xpack.ql.index.IndexResolver;
2831

2932
import java.util.ArrayList;
33+
import java.util.Arrays;
3034
import java.util.Collections;
3135
import java.util.LinkedHashSet;
3236
import java.util.List;
3337
import java.util.Set;
3438

39+
import static org.elasticsearch.index.query.QueryBuilders.boolQuery;
40+
3541
public final class RuntimeUtils {
3642

3743
static final Logger QUERY_LOG = LogManager.getLogger(QueryClient.class);
@@ -48,10 +54,10 @@ static void logSearchResponse(SearchResponse response, Logger logger) {
4854
aggsNames.append(aggs.get(i).getName() + (i + 1 == aggs.size() ? "" : ", "));
4955
}
5056

51-
logger.trace("Got search response [hits {} {}, {} aggregations: [{}], {} failed shards, {} skipped shards, "
52-
+ "{} successful shards, {} total shards, took {}, timed out [{}]]", response.getHits().getTotalHits().relation.toString(),
53-
response.getHits().getTotalHits().value, aggs.size(), aggsNames, response.getFailedShards(), response.getSkippedShards(),
54-
response.getSuccessfulShards(), response.getTotalShards(), response.getTook(), response.isTimedOut());
57+
logger.trace("Got search response [hits {}, {} aggregations: [{}], {} failed shards, {} skipped shards, "
58+
+ "{} successful shards, {} total shards, took {}, timed out [{}]]", response.getHits().getTotalHits(), aggs.size(),
59+
aggsNames, response.getFailedShards(), response.getSkippedShards(), response.getSuccessfulShards(),
60+
response.getTotalShards(), response.getTook(), response.isTimedOut());
5561
}
5662

5763
public static List<HitExtractor> createExtractor(List<FieldExtraction> fields, EqlConfiguration cfg) {
@@ -62,7 +68,7 @@ public static List<HitExtractor> createExtractor(List<FieldExtraction> fields, E
6268
}
6369
return extractors;
6470
}
65-
71+
6672
public static HitExtractor createExtractor(FieldExtraction ref, EqlConfiguration cfg) {
6773
if (ref instanceof SearchHitFieldRef) {
6874
SearchHitFieldRef f = (SearchHitFieldRef) ref;
@@ -92,7 +98,7 @@ public static HitExtractor createExtractor(FieldExtraction ref, EqlConfiguration
9298

9399
throw new EqlIllegalArgumentException("Unexpected value reference {}", ref.getClass());
94100
}
95-
101+
96102

97103
public static SearchRequest prepareRequest(Client client,
98104
SearchSourceBuilder source,
@@ -105,4 +111,34 @@ public static SearchRequest prepareRequest(Client client,
105111
includeFrozen ? IndexResolver.FIELD_CAPS_FROZEN_INDICES_OPTIONS : IndexResolver.FIELD_CAPS_INDICES_OPTIONS)
106112
.request();
107113
}
108-
}
114+
115+
public static List<SearchHit> searchHits(SearchResponse response) {
116+
return Arrays.asList(response.getHits().getHits());
117+
}
118+
119+
// optimized method that adds filter to existing bool queries without additional wrapping
120+
// additionally checks whether the given query exists for safe decoration
121+
public static SearchSourceBuilder addFilter(QueryBuilder filter, SearchSourceBuilder source) {
122+
BoolQueryBuilder bool = null;
123+
QueryBuilder query = source.query();
124+
125+
if (query instanceof BoolQueryBuilder) {
126+
bool = (BoolQueryBuilder) query;
127+
if (filter != null && bool.filter().contains(filter) == false) {
128+
bool.filter(filter);
129+
}
130+
}
131+
else {
132+
bool = boolQuery();
133+
if (query != null) {
134+
bool.filter(query);
135+
}
136+
if (filter != null) {
137+
bool.filter(filter);
138+
}
139+
140+
source.query(bool);
141+
}
142+
return source;
143+
}
144+
}

‎x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/search/SourceGenerator.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,8 @@ public static SearchSourceBuilder sourceBuilder(QueryContainer container, QueryB
7676
}
7777
}
7878

79+
optimize(container, source);
80+
7981
return source;
8082
}
8183

@@ -94,7 +96,7 @@ private static void sorting(QueryContainer container, SearchSourceBuilder source
9496
sortBuilder = fieldSort(fa.name())
9597
.missing(as.missing().position())
9698
.unmappedType(fa.dataType().esType());
97-
99+
98100
if (fa.isNested()) {
99101
FieldSortBuilder fieldSort = fieldSort(fa.name())
100102
.missing(as.missing().position())
@@ -134,8 +136,6 @@ private static void sorting(QueryContainer container, SearchSourceBuilder source
134136
}
135137

136138
private static void optimize(QueryContainer query, SearchSourceBuilder builder) {
137-
if (query.shouldTrackHits()) {
138-
builder.trackTotalHits(true);
139-
}
139+
builder.trackTotalHits(query.shouldTrackHits());
140140
}
141-
}
141+
}

0 commit comments

Comments
 (0)