-
Notifications
You must be signed in to change notification settings - Fork 25.6k
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.
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
Conversation
FieldCapabilitiesFetcher acquires a searcher to perform basic checks that don't actually require a searcher to be performed, as they are purely mappings driven. Instead, we can reuse the existing QueryRewriteContext to perform such checks and entirely avoid acquiring a searcher.
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
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(); |
There was a problem hiding this comment.
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?
|
||
public boolean hasMappings() { | ||
return mappingLookup.hasMappings(); | ||
} |
There was a problem hiding this comment.
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.
Hi @javanna, I've created a changelog YAML for you. |
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) { |
There was a problem hiding this comment.
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.
FieldCapabilitiesFetcher acquires a searcher to perform basic checks that don't actually require a searcher to be performed, as they are purely mappings driven. Instead, we can reuse the existing QueryRewriteContext to perform such checks and entirely avoid acquiring a searcher.