From 2c7279db4689c526608aac2f832a32a73edfaa0f Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Tue, 16 Jun 2020 12:01:00 -0700 Subject: [PATCH 1/3] For the fields fetch phase, avoid reloading stored fields. This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute (plural). This way, we can make sure that the stored fields (including _source) are only loaded once per hit as part of FetchPhase. --- .../elasticsearch/search/SearchService.java | 5 ++- .../search/fetch/FetchPhase.java | 3 +- .../fetch/subphase/FetchFieldsContext.java | 26 ++++++++++++--- .../fetch/subphase/FetchFieldsPhase.java | 32 +++++-------------- 4 files changed, 35 insertions(+), 31 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/SearchService.java b/server/src/main/java/org/elasticsearch/search/SearchService.java index f5f86623f1124..7bc7aef2f49ce 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchService.java +++ b/server/src/main/java/org/elasticsearch/search/SearchService.java @@ -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(); diff --git a/server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java b/server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java index d20f5155ac8b2..1cb1ce50b78f5 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java @@ -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; + fieldsVisitor = new FieldsVisitor(loadSource); } else if (storedFieldsContext.fetchFields() == false) { // disable stored fields entirely fieldsVisitor = null; diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsContext.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsContext.java index 84a390531204b..79b33003da788 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsContext.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsContext.java @@ -18,6 +18,9 @@ */ package org.elasticsearch.search.fetch.subphase; +import org.elasticsearch.index.mapper.DocumentMapper; +import org.elasticsearch.index.mapper.MapperService; + import java.util.List; /** @@ -25,13 +28,26 @@ */ public class FetchFieldsContext { - private final List fields; + private FieldValueRetriever fieldValueRetriever; + + public static FetchFieldsContext create(String indexName, + MapperService mapperService, + List 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 fields) { - this.fields = fields; + private FetchFieldsContext(FieldValueRetriever fieldValueRetriever) { + this.fieldValueRetriever = fieldValueRetriever; } - public List fields() { - return this.fields; + public FieldValueRetriever fieldValueRetriever() { + return fieldValueRetriever; } } diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java index 50513dee9c898..e14cf24726dac 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java @@ -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; @@ -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 ignoredFields = getIgnoredFields(hit); - Map documentFields = fieldValueRetriever.retrieve(sourceLookup, ignoredFields); - for (Map.Entry entry : documentFields.entrySet()) { - hit.setDocumentField(entry.getKey(), entry.getValue()); - } + Set ignoredFields = getIgnoredFields(hit); + Map documentFields = fieldValueRetriever.retrieve(sourceLookup, ignoredFields); + for (Map.Entry entry : documentFields.entrySet()) { + hit.setDocumentField(entry.getKey(), entry.getValue()); } } From b1ff79aa51408e581b9b993bbf043ff91b35d106 Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Tue, 16 Jun 2020 14:21:34 -0700 Subject: [PATCH 2/3] Make sure to load _source when stored_fields are requested. --- .../test/search/330_fetch_fields.yml | 48 +++++++++++++++++++ .../search/fetch/FetchPhase.java | 2 +- 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml index 02c3b2c592a88..b0e42b3439724 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml @@ -169,3 +169,51 @@ 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 + body: + keyword: "x" + integer: 42 + + - do: + indices.refresh: + index: [ test ] + + - do: + 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 } diff --git a/server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java b/server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java index 1cb1ce50b78f5..18241a5b462fb 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java @@ -131,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); From 02c5c599ddd2269417588657423c770430bfc5af Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Tue, 16 Jun 2020 14:54:34 -0700 Subject: [PATCH 3/3] Small simplification to YAML test. --- .../resources/rest-api-spec/test/search/330_fetch_fields.yml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml index b0e42b3439724..c521123f278ed 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml @@ -190,14 +190,11 @@ setup: index: index: test id: 1 + refresh: true body: keyword: "x" integer: 42 - - do: - indices.refresh: - index: [ test ] - - do: search: index: test