Skip to content
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

For the fields fetch phase, avoid reloading stored fields. #58196

Merged
merged 3 commits into from
Jun 17, 2020
Merged

For the fields fetch phase, avoid reloading stored fields. #58196

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2c7279d
For the fields fetch phase, avoid reloading stored fields.
jtibshirani Jun 16, 2020
b1ff79a
Make sure to load _source when stored_fields are requested.
jtibshirani Jun 16, 2020
02c5c59
Small simplification to YAML test.
jtibshirani Jun 16, 2020
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: 4 additions & 1 deletion server/src/main/java/org/elasticsearch/search/SearchService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -937,7 +937,10 @@ private void parseSource(DefaultSearchContext context, SearchSourceBuilder sourc
context.docValuesContext(new FetchDocValuesContext(docValueFields));
}
if (source.fetchFields() != null) {
context.fetchFieldsContext(new FetchFieldsContext(source.fetchFields()));
String indexName = context.indexShard().shardId().getIndexName();
FetchFieldsContext fetchFieldsContext = FetchFieldsContext.create(
indexName, context.mapperService(), source.fetchFields());
context.fetchFieldsContext(fetchFieldsContext);
}
if (source.highlighter() != null) {
HighlightBuilder highlightBuilder = source.highlighter();
Expand Down
3 changes: 2 additions & 1 deletion server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ public void execute(SearchContext context) {
if (!context.hasScriptFields() && !context.hasFetchSourceContext()) {
context.fetchSourceContext(new FetchSourceContext(true));
}
fieldsVisitor = new FieldsVisitor(context.sourceRequested());
boolean loadSource = context.sourceRequested() || context.fetchFieldsContext() != null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have the same logic in the else below or do we expect users to not use stored_fields in conjunction with the new fields option ?

Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be safest to add it below too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, thanks for catching this. I'll push an update.

fieldsVisitor = new FieldsVisitor(loadSource);
} else if (storedFieldsContext.fetchFields() == false) {
// disable stored fields entirely
fieldsVisitor = null;
Expand Down
26 changes: 21 additions & 5 deletions server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,36 @@
*/
package org.elasticsearch.search.fetch.subphase;

import org.elasticsearch.index.mapper.DocumentMapper;
import org.elasticsearch.index.mapper.MapperService;

import java.util.List;

/**
* The context needed to retrieve fields.
*/
public class FetchFieldsContext {

private final List<FieldAndFormat> fields;
private FieldValueRetriever fieldValueRetriever;

public static FetchFieldsContext create(String indexName,
MapperService mapperService,
List<FieldAndFormat> fields) {
DocumentMapper documentMapper = mapperService.documentMapper();
if (documentMapper.sourceMapper().enabled() == false) {
throw new IllegalArgumentException("Unable to retrieve the requested [fields] since _source is " +
"disabled in the mappings for index [" + indexName + "]");
}

FieldValueRetriever fieldValueRetriever = FieldValueRetriever.create(mapperService, fields);
return new FetchFieldsContext(fieldValueRetriever);
}

public FetchFieldsContext(List<FieldAndFormat> fields) {
this.fields = fields;
private FetchFieldsContext(FieldValueRetriever fieldValueRetriever) {
this.fieldValueRetriever = fieldValueRetriever;
}

public List<FieldAndFormat> fields() {
return this.fields;
public FieldValueRetriever fieldValueRetriever() {
return fieldValueRetriever;
}
}
32 changes: 8 additions & 24 deletions server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,7 @@

package org.elasticsearch.search.fetch.subphase;

import org.apache.lucene.index.LeafReaderContext;
import org.apache.lucene.index.ReaderUtil;
import org.elasticsearch.common.document.DocumentField;
import org.elasticsearch.index.mapper.DocumentMapper;
import org.elasticsearch.index.mapper.IgnoredFieldMapper;
import org.elasticsearch.search.SearchHit;
import org.elasticsearch.search.fetch.FetchSubPhase;
Expand All @@ -40,33 +37,20 @@
public final class FetchFieldsPhase implements FetchSubPhase {

@Override
public void hitsExecute(SearchContext context, SearchHit[] hits) {
public void hitExecute(SearchContext context, HitContext hitContext) {
FetchFieldsContext fetchFieldsContext = context.fetchFieldsContext();
if (fetchFieldsContext == null || fetchFieldsContext.fields().isEmpty()) {
if (fetchFieldsContext == null) {
return;
}

DocumentMapper documentMapper = context.mapperService().documentMapper();
if (documentMapper.sourceMapper().enabled() == false) {
throw new IllegalArgumentException("Unable to retrieve the requested [fields] since _source is " +
"disabled in the mappings for index [" + context.indexShard().shardId().getIndexName() + "]");
}

SearchHit hit = hitContext.hit();
SourceLookup sourceLookup = context.lookup().source();
FieldValueRetriever fieldValueRetriever = FieldValueRetriever.create(
context.mapperService(),
fetchFieldsContext.fields());

for (SearchHit hit : hits) {
int readerIndex = ReaderUtil.subIndex(hit.docId(), context.searcher().getIndexReader().leaves());
LeafReaderContext readerContext = context.searcher().getIndexReader().leaves().get(readerIndex);
sourceLookup.setSegmentAndDocument(readerContext, hit.docId());
FieldValueRetriever fieldValueRetriever = fetchFieldsContext.fieldValueRetriever();

Set<String> ignoredFields = getIgnoredFields(hit);
Map<String, DocumentField> documentFields = fieldValueRetriever.retrieve(sourceLookup, ignoredFields);
for (Map.Entry<String, DocumentField> entry : documentFields.entrySet()) {
hit.setDocumentField(entry.getKey(), entry.getValue());
}
Set<String> ignoredFields = getIgnoredFields(hit);
Map<String, DocumentField> documentFields = fieldValueRetriever.retrieve(sourceLookup, ignoredFields);
for (Map.Entry<String, DocumentField> entry : documentFields.entrySet()) {
hit.setDocumentField(entry.getKey(), entry.getValue());
}
}

Expand Down