Skip to content

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

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

Merged
merged 3 commits into from
Jun 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -169,3 +169,48 @@ setup:

- match: { hits.hits.0.fields.integer.0: 42 }
- is_false: hits.hits.1.fields.integer

---
"Test disable _source loading":
- do:
indices.create:
index: test
body:
settings:
number_of_shards: 1
mappings:
properties:
keyword:
type: keyword
integer:
type: integer
store: true

- do:
index:
index: test
id: 1
refresh: true
body:
keyword: "x"
integer: 42

- do:
Copy link
Member

Choose a reason for hiding this comment

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

I tend to stick refresh on the index request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 this is nicer, especially when there's only one index request.

search:
index: test
body:
fields: [ keyword ]
_source: false

- match: { hits.hits.0.fields.keyword.0: "x" }

- do:
search:
index: test
body:
fields: [ keyword ]
stored_fields: [ integer ]
_source: false

- match: { hits.hits.0.fields.keyword.0: "x" }
- match: { hits.hits.0.fields.integer.0: 42 }
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
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 Expand Up @@ -130,7 +131,7 @@ public void execute(SearchContext context) {
}
}
}
boolean loadSource = context.sourceRequested();
boolean loadSource = context.sourceRequested() || context.fetchFieldsContext() != null;
if (storedToRequestedFields.isEmpty()) {
// empty list specified, default to disable _source if no explicit indication
fieldsVisitor = new FieldsVisitor(loadSource);
Expand Down
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;
}
}
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