Skip to content

Acquire searcher in field caps only when needed #126137

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Acquire searcher in field caps only when needed #126137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91e3b3b
Acquire search in field caps only when needed
javanna Apr 2, 2025
b46e340
Update docs/changelog/126137.yaml
javanna Apr 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/126137.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 126137
summary: Acquire searcher in field caps only when needed
area: Search
type: bug
issues: []
50 changes: 11 additions & 39 deletions server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesFetcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,12 @@

import org.elasticsearch.cluster.metadata.MappingMetadata;
import org.elasticsearch.core.Booleans;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.index.IndexService;
import org.elasticsearch.index.engine.Engine;
import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.index.mapper.RuntimeField;
import org.elasticsearch.index.query.MatchAllQueryBuilder;
import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.index.query.SearchExecutionContext;
import org.elasticsearch.index.query.QueryRewriteContext;
import org.elasticsearch.index.shard.IndexShard;
import org.elasticsearch.index.shard.ShardId;
import org.elasticsearch.indices.IndicesService;
Expand Down Expand Up @@ -65,28 +63,11 @@ FieldCapabilitiesIndexResponse fetch(
) throws IOException {
final IndexService indexService = indicesService.indexServiceSafe(shardId.getIndex());
final IndexShard indexShard = indexService.getShard(shardId.getId());
final Engine.Searcher searcher;
if (alwaysMatches(indexFilter)) {
// no need to open a searcher if we aren't filtering, but make sure we are reading from an up-to-dated shard
indexShard.readAllowed();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what the purpose of this is, I think we can drop it as well, can't we?

searcher = null;
} else {
searcher = indexShard.acquireSearcher(Engine.CAN_MATCH_SEARCH_SOURCE);
}
try (searcher) {
return doFetch(
task,
shardId,
fieldNameFilter,
filters,
fieldTypes,
indexFilter,
nowInMillis,
runtimeFields,
indexService,
searcher
);
}
return doFetch(task, shardId, fieldNameFilter, filters, fieldTypes, indexFilter, nowInMillis, runtimeFields, indexService);
}

private FieldCapabilitiesIndexResponse doFetch(
Expand All @@ -98,22 +79,13 @@ private FieldCapabilitiesIndexResponse doFetch(
QueryBuilder indexFilter,
long nowInMillis,
Map<String, Object> runtimeFields,
IndexService indexService,
@Nullable Engine.Searcher searcher
IndexService indexService
) throws IOException {
final SearchExecutionContext searchExecutionContext = indexService.newSearchExecutionContext(
shardId.id(),
0,
searcher,
() -> nowInMillis,
null,
runtimeFields
);
var indexMode = searchExecutionContext.getIndexSettings().getMode();
if (searcher != null && canMatchShard(shardId, indexFilter, nowInMillis, searchExecutionContext) == false) {
QueryRewriteContext queryRewriteContext = indexService.newQueryRewriteContext(() -> nowInMillis, runtimeFields, null);
var indexMode = indexService.getIndexSettings().getMode();
if (canMatchShard(shardId, indexFilter, nowInMillis, queryRewriteContext) == false) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is what I missed: while this method takes a query rewrite context, it can indirectly call convertToSearchExecutionContext which performs additional actions, and only does something when the original argument was a SearchExecutionContext. That means a searcher may be needed, but not necessarily.

return new FieldCapabilitiesIndexResponse(shardId.getIndexName(), null, Collections.emptyMap(), false, indexMode);
}

final MappingMetadata mapping = indexService.getMetadata().mapping();
String indexMappingHash;
if (includeEmptyFields || enableFieldHasValue == false) {
Expand All @@ -135,7 +107,7 @@ private FieldCapabilitiesIndexResponse doFetch(
}
task.ensureNotCancelled();
final Map<String, IndexFieldCapabilities> responseMap = retrieveFieldCaps(
searchExecutionContext,
queryRewriteContext,
fieldNameFilter,
filters,
fieldTypes,
Expand All @@ -150,7 +122,7 @@ private FieldCapabilitiesIndexResponse doFetch(
}

static Map<String, IndexFieldCapabilities> retrieveFieldCaps(
SearchExecutionContext context,
QueryRewriteContext context,
Predicate<String> fieldNameFilter,
String[] filters,
String[] types,
Expand Down Expand Up @@ -236,20 +208,20 @@ private static boolean canMatchShard(
ShardId shardId,
QueryBuilder indexFilter,
long nowInMillis,
SearchExecutionContext searchExecutionContext
QueryRewriteContext queryRewriteContext
) throws IOException {
assert alwaysMatches(indexFilter) == false : "should not be called for always matching [" + indexFilter + "]";
assert nowInMillis != 0L;
ShardSearchRequest searchRequest = new ShardSearchRequest(shardId, nowInMillis, AliasFilter.EMPTY);
searchRequest.source(new SearchSourceBuilder().query(indexFilter));
return SearchService.queryStillMatchesAfterRewrite(searchRequest, searchExecutionContext);
return SearchService.queryStillMatchesAfterRewrite(searchRequest, queryRewriteContext);
}

private static boolean alwaysMatches(QueryBuilder indexFilter) {
return indexFilter == null || indexFilter instanceof MatchAllQueryBuilder;
}

private static Predicate<MappedFieldType> buildFilter(String[] filters, String[] fieldTypes, SearchExecutionContext context) {
private static Predicate<MappedFieldType> buildFilter(String[] filters, String[] fieldTypes, QueryRewriteContext context) {
// security filters don't exclude metadata fields
Predicate<MappedFieldType> fcf = null;
if (fieldTypes.length > 0) {
Expand Down
19 changes: 19 additions & 0 deletions server/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.elasticsearch.index.mapper.MapperBuilderContext;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.mapper.MappingLookup;
import org.elasticsearch.index.mapper.NestedLookup;
import org.elasticsearch.index.mapper.SourceFieldMapper;
import org.elasticsearch.index.mapper.TextFieldMapper;
import org.elasticsearch.plugins.internal.rewriter.QueryRewriteInterceptor;
Expand Down Expand Up @@ -465,4 +466,22 @@ public void setQueryRewriteInterceptor(QueryRewriteInterceptor queryRewriteInter
this.queryRewriteInterceptor = queryRewriteInterceptor;
}

public boolean isMetadataField(String field) {
return mapperService.isMetadataField(field);
}

public boolean isMultiField(String field) {
if (runtimeMappings.containsKey(field)) {
return false;
}
return mapperService.isMultiField(field);
}

public NestedLookup nestedLookup() {
return mappingLookup.nestedLookup();
}

public boolean hasMappings() {
return mappingLookup.hasMappings();
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these methods rely on state that's already available in query rewrite context, there's no good reason to expose them only in SearchExecutionContext, that's more of a leftover from before we were able to rewrite based on mappings only.

}
20 changes: 0 additions & 20 deletions server/src/main/java/org/elasticsearch/index/query/SearchExecutionContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.mapper.MappingLookup;
import org.elasticsearch.index.mapper.MappingParserContext;
import org.elasticsearch.index.mapper.NestedLookup;
import org.elasticsearch.index.mapper.ParsedDocument;
import org.elasticsearch.index.mapper.SourceLoader;
import org.elasticsearch.index.mapper.SourceToParse;
Expand Down Expand Up @@ -372,32 +371,13 @@ public ParsedDocument parseDocument(SourceToParse source) throws DocumentParsing
return mapperService.documentParser().parseDocument(source, mappingLookup);
}

public NestedLookup nestedLookup() {
return mappingLookup.nestedLookup();
}

public boolean hasMappings() {
return mappingLookup.hasMappings();
}

/**
* Returns true if the field identified by the provided name is mapped, false otherwise
*/
public boolean isFieldMapped(String name) {
return fieldType(name) != null;
}

public boolean isMetadataField(String field) {
return mapperService.isMetadataField(field);
}

public boolean isMultiField(String field) {
if (runtimeMappings.containsKey(field)) {
return false;
}
return mapperService.isMultiField(field);
}

public Iterable<MappedFieldType> dimensionFields() {
List<MappedFieldType> dimensionFields = new ArrayList<>();
for (var mapper : mapperService.mappingLookup().fieldMappers()) {
Expand Down
Loading