Skip to content

Commit 5d1bca3

Browse files
Make NestedHelper a utility class (#118071)
Noticed instantiating these instances taking a visible and unexpected amount of CPU in profiles (probably from bootstrapping the lambda/callsite for the predicate). This fixes the logic to effectively disappear from profiling and makes it easier to reason about as well by removing the indirect use of the search context and just explicitly passing it around. No need to instantiate instances of this thing either, escape analysis probably isn't able to remove it because of the recursive instance method calls.
1 parent 9d35053 commit 5d1bca3

File tree

7 files changed

+185
-179
lines changed

7 files changed

+185
-179
lines changed

server/src/main/java/org/elasticsearch/index/query/NestedQueryBuilder.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -321,8 +321,7 @@ public static <E extends Exception> Query toQuery(
321321

322322
// ToParentBlockJoinQuery requires that the inner query only matches documents
323323
// in its child space
324-
NestedHelper nestedHelper = new NestedHelper(context.nestedLookup(), context::isFieldMapped);
325-
if (nestedHelper.mightMatchNonNestedDocs(innerQuery, path)) {
324+
if (NestedHelper.mightMatchNonNestedDocs(innerQuery, path, context)) {
326325
innerQuery = Queries.filtered(innerQuery, mapper.nestedTypeFilter());
327326
}
328327

server/src/main/java/org/elasticsearch/index/search/NestedHelper.java

Lines changed: 26 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -21,61 +21,53 @@
2121
import org.apache.lucene.search.Query;
2222
import org.apache.lucene.search.TermInSetQuery;
2323
import org.apache.lucene.search.TermQuery;
24-
import org.elasticsearch.index.mapper.NestedLookup;
2524
import org.elasticsearch.index.mapper.NestedObjectMapper;
26-
27-
import java.util.function.Predicate;
25+
import org.elasticsearch.index.query.SearchExecutionContext;
2826

2927
/** Utility class to filter parent and children clauses when building nested
3028
* queries. */
3129
public final class NestedHelper {
3230

33-
private final NestedLookup nestedLookup;
34-
private final Predicate<String> isMappedFieldPredicate;
35-
36-
public NestedHelper(NestedLookup nestedLookup, Predicate<String> isMappedFieldPredicate) {
37-
this.nestedLookup = nestedLookup;
38-
this.isMappedFieldPredicate = isMappedFieldPredicate;
39-
}
31+
private NestedHelper() {}
4032

4133
/** Returns true if the given query might match nested documents. */
42-
public boolean mightMatchNestedDocs(Query query) {
34+
public static boolean mightMatchNestedDocs(Query query, SearchExecutionContext searchExecutionContext) {
4335
if (query instanceof ConstantScoreQuery) {
44-
return mightMatchNestedDocs(((ConstantScoreQuery) query).getQuery());
36+
return mightMatchNestedDocs(((ConstantScoreQuery) query).getQuery(), searchExecutionContext);
4537
} else if (query instanceof BoostQuery) {
46-
return mightMatchNestedDocs(((BoostQuery) query).getQuery());
38+
return mightMatchNestedDocs(((BoostQuery) query).getQuery(), searchExecutionContext);
4739
} else if (query instanceof MatchAllDocsQuery) {
4840
return true;
4941
} else if (query instanceof MatchNoDocsQuery) {
5042
return false;
5143
} else if (query instanceof TermQuery) {
5244
// We only handle term(s) queries and range queries, which should already
5345
// cover a high majority of use-cases
54-
return mightMatchNestedDocs(((TermQuery) query).getTerm().field());
46+
return mightMatchNestedDocs(((TermQuery) query).getTerm().field(), searchExecutionContext);
5547
} else if (query instanceof TermInSetQuery tis) {
5648
if (tis.getTermsCount() > 0) {
57-
return mightMatchNestedDocs(tis.getField());
49+
return mightMatchNestedDocs(tis.getField(), searchExecutionContext);
5850
} else {
5951
return false;
6052
}
6153
} else if (query instanceof PointRangeQuery) {
62-
return mightMatchNestedDocs(((PointRangeQuery) query).getField());
54+
return mightMatchNestedDocs(((PointRangeQuery) query).getField(), searchExecutionContext);
6355
} else if (query instanceof IndexOrDocValuesQuery) {
64-
return mightMatchNestedDocs(((IndexOrDocValuesQuery) query).getIndexQuery());
56+
return mightMatchNestedDocs(((IndexOrDocValuesQuery) query).getIndexQuery(), searchExecutionContext);
6557
} else if (query instanceof final BooleanQuery bq) {
6658
final boolean hasRequiredClauses = bq.clauses().stream().anyMatch(BooleanClause::isRequired);
6759
if (hasRequiredClauses) {
6860
return bq.clauses()
6961
.stream()
7062
.filter(BooleanClause::isRequired)
7163
.map(BooleanClause::query)
72-
.allMatch(this::mightMatchNestedDocs);
64+
.allMatch(f -> mightMatchNestedDocs(f, searchExecutionContext));
7365
} else {
7466
return bq.clauses()
7567
.stream()
7668
.filter(c -> c.occur() == Occur.SHOULD)
7769
.map(BooleanClause::query)
78-
.anyMatch(this::mightMatchNestedDocs);
70+
.anyMatch(f -> mightMatchNestedDocs(f, searchExecutionContext));
7971
}
8072
} else if (query instanceof ESToParentBlockJoinQuery) {
8173
return ((ESToParentBlockJoinQuery) query).getPath() != null;
@@ -85,7 +77,7 @@ public boolean mightMatchNestedDocs(Query query) {
8577
}
8678

8779
/** Returns true if a query on the given field might match nested documents. */
88-
boolean mightMatchNestedDocs(String field) {
80+
private static boolean mightMatchNestedDocs(String field, SearchExecutionContext searchExecutionContext) {
8981
if (field.startsWith("_")) {
9082
// meta field. Every meta field behaves differently, eg. nested
9183
// documents have the same _uid as their parent, put their path in
@@ -94,50 +86,50 @@ boolean mightMatchNestedDocs(String field) {
9486
// we might add a nested filter when it is nor required.
9587
return true;
9688
}
97-
if (isMappedFieldPredicate.test(field) == false) {
89+
if (searchExecutionContext.isFieldMapped(field) == false) {
9890
// field does not exist
9991
return false;
10092
}
101-
return nestedLookup.getNestedParent(field) != null;
93+
return searchExecutionContext.nestedLookup().getNestedParent(field) != null;
10294
}
10395

10496
/** Returns true if the given query might match parent documents or documents
10597
* that are nested under a different path. */
106-
public boolean mightMatchNonNestedDocs(Query query, String nestedPath) {
98+
public static boolean mightMatchNonNestedDocs(Query query, String nestedPath, SearchExecutionContext searchExecutionContext) {
10799
if (query instanceof ConstantScoreQuery) {
108-
return mightMatchNonNestedDocs(((ConstantScoreQuery) query).getQuery(), nestedPath);
100+
return mightMatchNonNestedDocs(((ConstantScoreQuery) query).getQuery(), nestedPath, searchExecutionContext);
109101
} else if (query instanceof BoostQuery) {
110-
return mightMatchNonNestedDocs(((BoostQuery) query).getQuery(), nestedPath);
102+
return mightMatchNonNestedDocs(((BoostQuery) query).getQuery(), nestedPath, searchExecutionContext);
111103
} else if (query instanceof MatchAllDocsQuery) {
112104
return true;
113105
} else if (query instanceof MatchNoDocsQuery) {
114106
return false;
115107
} else if (query instanceof TermQuery) {
116-
return mightMatchNonNestedDocs(((TermQuery) query).getTerm().field(), nestedPath);
108+
return mightMatchNonNestedDocs(searchExecutionContext, ((TermQuery) query).getTerm().field(), nestedPath);
117109
} else if (query instanceof TermInSetQuery tis) {
118110
if (tis.getTermsCount() > 0) {
119-
return mightMatchNonNestedDocs(tis.getField(), nestedPath);
111+
return mightMatchNonNestedDocs(searchExecutionContext, tis.getField(), nestedPath);
120112
} else {
121113
return false;
122114
}
123115
} else if (query instanceof PointRangeQuery) {
124-
return mightMatchNonNestedDocs(((PointRangeQuery) query).getField(), nestedPath);
116+
return mightMatchNonNestedDocs(searchExecutionContext, ((PointRangeQuery) query).getField(), nestedPath);
125117
} else if (query instanceof IndexOrDocValuesQuery) {
126-
return mightMatchNonNestedDocs(((IndexOrDocValuesQuery) query).getIndexQuery(), nestedPath);
118+
return mightMatchNonNestedDocs(((IndexOrDocValuesQuery) query).getIndexQuery(), nestedPath, searchExecutionContext);
127119
} else if (query instanceof final BooleanQuery bq) {
128120
final boolean hasRequiredClauses = bq.clauses().stream().anyMatch(BooleanClause::isRequired);
129121
if (hasRequiredClauses) {
130122
return bq.clauses()
131123
.stream()
132124
.filter(BooleanClause::isRequired)
133125
.map(BooleanClause::query)
134-
.allMatch(q -> mightMatchNonNestedDocs(q, nestedPath));
126+
.allMatch(q -> mightMatchNonNestedDocs(q, nestedPath, searchExecutionContext));
135127
} else {
136128
return bq.clauses()
137129
.stream()
138130
.filter(c -> c.occur() == Occur.SHOULD)
139131
.map(BooleanClause::query)
140-
.anyMatch(q -> mightMatchNonNestedDocs(q, nestedPath));
132+
.anyMatch(q -> mightMatchNonNestedDocs(q, nestedPath, searchExecutionContext));
141133
}
142134
} else {
143135
return true;
@@ -146,7 +138,7 @@ public boolean mightMatchNonNestedDocs(Query query, String nestedPath) {
146138

147139
/** Returns true if a query on the given field might match parent documents
148140
* or documents that are nested under a different path. */
149-
boolean mightMatchNonNestedDocs(String field, String nestedPath) {
141+
private static boolean mightMatchNonNestedDocs(SearchExecutionContext searchExecutionContext, String field, String nestedPath) {
150142
if (field.startsWith("_")) {
151143
// meta field. Every meta field behaves differently, eg. nested
152144
// documents have the same _uid as their parent, put their path in
@@ -155,9 +147,10 @@ boolean mightMatchNonNestedDocs(String field, String nestedPath) {
155147
// we might add a nested filter when it is nor required.
156148
return true;
157149
}
158-
if (isMappedFieldPredicate.test(field) == false) {
150+
if (searchExecutionContext.isFieldMapped(field) == false) {
159151
return false;
160152
}
153+
var nestedLookup = searchExecutionContext.nestedLookup();
161154
String nestedParent = nestedLookup.getNestedParent(field);
162155
if (nestedParent == null || nestedParent.startsWith(nestedPath) == false) {
163156
// the field is not a sub field of the nested path

server/src/main/java/org/elasticsearch/search/DefaultSearchContext.java

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -444,10 +444,9 @@ public void preProcess() {
444444
public Query buildFilteredQuery(Query query) {
445445
List<Query> filters = new ArrayList<>();
446446
NestedLookup nestedLookup = searchExecutionContext.nestedLookup();
447-
NestedHelper nestedHelper = new NestedHelper(nestedLookup, searchExecutionContext::isFieldMapped);
448447
if (nestedLookup != NestedLookup.EMPTY
449-
&& nestedHelper.mightMatchNestedDocs(query)
450-
&& (aliasFilter == null || nestedHelper.mightMatchNestedDocs(aliasFilter))) {
448+
&& NestedHelper.mightMatchNestedDocs(query, searchExecutionContext)
449+
&& (aliasFilter == null || NestedHelper.mightMatchNestedDocs(aliasFilter, searchExecutionContext))) {
451450
filters.add(Queries.newNonNestedFilter(searchExecutionContext.indexVersionCreated()));
452451
}
453452

server/src/main/java/org/elasticsearch/search/vectors/KnnVectorQueryBuilder.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -481,10 +481,9 @@ protected Query doToQuery(SearchExecutionContext context) throws IOException {
481481
}
482482
parentBitSet = context.bitsetFilter(parentFilter);
483483
if (filterQuery != null) {
484-
NestedHelper nestedHelper = new NestedHelper(context.nestedLookup(), context::isFieldMapped);
485484
// We treat the provided filter as a filter over PARENT documents, so if it might match nested documents
486485
// we need to adjust it.
487-
if (nestedHelper.mightMatchNestedDocs(filterQuery)) {
486+
if (NestedHelper.mightMatchNestedDocs(filterQuery, context)) {
488487
// Ensure that the query only returns parent documents matching `filterQuery`
489488
filterQuery = Queries.filtered(filterQuery, parentFilter);
490489
}

0 commit comments

Comments
 (0)