From 92daa76e5c31b51513afb54b12f627d83190778e Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Thu, 28 Mar 2024 01:19:53 +0100 Subject: [PATCH] Move _ignored to be fetched via value fetcher --- docs/reference/search/profile.asciidoc | 22 +++++++++ .../rest-api-spec/test/30_inner_hits.yml | 21 ++++---- .../rest-api-spec/test/search/370_profile.yml | 49 ++++++++++--------- .../fetch/subphase/FetchFieldsPhase.java | 35 +++++++++---- .../fetch/subphase/StoredFieldsPhase.java | 5 +- 5 files changed, 89 insertions(+), 43 deletions(-) diff --git a/docs/reference/search/profile.asciidoc b/docs/reference/search/profile.asciidoc index 5b63929934770..3fed14231808c 100644 --- a/docs/reference/search/profile.asciidoc +++ b/docs/reference/search/profile.asciidoc @@ -197,6 +197,17 @@ The API returns the following result: "stored_fields": ["_id", "_routing", "_source"] }, "children": [ + { + "type" : "FetchFieldsPhase", + "description" : "", + "time_in_nanos" : 238762, + "breakdown" : { + "process_count" : 5, + "process" : 227914, + "next_reader" : 10848, + "next_reader_count" : 1 + } + }, { "type": "FetchSourcePhase", "description": "", @@ -1043,6 +1054,17 @@ And here is the fetch profile: "stored_fields": ["_id", "_routing", "_source"] }, "children": [ + { + "type" : "FetchFieldsPhase", + "description" : "", + "time_in_nanos" : 238762, + "breakdown" : { + "process_count" : 5, + "process" : 227914, + "next_reader" : 10848, + "next_reader_count" : 1 + } + }, { "type": "FetchSourcePhase", "description": "", diff --git a/modules/parent-join/src/yamlRestTest/resources/rest-api-spec/test/30_inner_hits.yml b/modules/parent-join/src/yamlRestTest/resources/rest-api-spec/test/30_inner_hits.yml index fab161baf1750..74a9b069fd56b 100644 --- a/modules/parent-join/src/yamlRestTest/resources/rest-api-spec/test/30_inner_hits.yml +++ b/modules/parent-join/src/yamlRestTest/resources/rest-api-spec/test/30_inner_hits.yml @@ -120,8 +120,8 @@ teardown: --- profile fetch: - skip: - version: ' - 7.15.99' - reason: fetch profiling implemented in 7.16.0 + version: ' - 8.13.99' + reason: _ignored is now retrieved via FetchFieldsPhase, previously via StoredFieldsPhase - do: search: @@ -141,15 +141,16 @@ profile fetch: - gt: { profile.shards.0.fetch.breakdown.load_stored_fields_count: 0 } - gt: { profile.shards.0.fetch.breakdown.load_stored_fields: 0 } - match: { profile.shards.0.fetch.debug.stored_fields: [_id, _routing, _source] } - - length: { profile.shards.0.fetch.children: 3 } - - match: { profile.shards.0.fetch.children.0.type: FetchSourcePhase } - - gt: { profile.shards.0.fetch.children.0.breakdown.next_reader_count: 0 } - - gt: { profile.shards.0.fetch.children.0.breakdown.next_reader: 0 } - - gt: { profile.shards.0.fetch.children.0.breakdown.next_reader_count: 0 } - - gt: { profile.shards.0.fetch.children.0.breakdown.next_reader: 0 } - - match: { profile.shards.0.fetch.children.1.type: InnerHitsPhase } + - length: { profile.shards.0.fetch.children: 4 } + - match: { profile.shards.0.fetch.children.0.type: FetchFieldsPhase } + - match: { profile.shards.0.fetch.children.1.type: FetchSourcePhase } - gt: { profile.shards.0.fetch.children.1.breakdown.next_reader_count: 0 } - gt: { profile.shards.0.fetch.children.1.breakdown.next_reader: 0 } - gt: { profile.shards.0.fetch.children.1.breakdown.next_reader_count: 0 } - gt: { profile.shards.0.fetch.children.1.breakdown.next_reader: 0 } - - match: { profile.shards.0.fetch.children.2.type: StoredFieldsPhase } + - match: { profile.shards.0.fetch.children.2.type: InnerHitsPhase } + - gt: { profile.shards.0.fetch.children.2.breakdown.next_reader_count: 0 } + - gt: { profile.shards.0.fetch.children.2.breakdown.next_reader: 0 } + - gt: { profile.shards.0.fetch.children.2.breakdown.next_reader_count: 0 } + - gt: { profile.shards.0.fetch.children.2.breakdown.next_reader: 0 } + - match: { profile.shards.0.fetch.children.3.type: StoredFieldsPhase } diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/370_profile.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/370_profile.yml index 0ead7b87f8acf..7b3fb0d1a2f70 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/370_profile.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/370_profile.yml @@ -53,8 +53,8 @@ fetch fields: --- fetch source: - skip: - version: ' - 8.5.99' - reason: stored fields phase added in 8.6 + version: ' - 8.13.99' + reason: _ignored is now retrieved via FetchFieldsPhase, previously via StoredFieldsPhase - do: search: @@ -71,20 +71,21 @@ fetch source: - gt: { profile.shards.0.fetch.breakdown.load_stored_fields_count: 0 } - gt: { profile.shards.0.fetch.breakdown.load_stored_fields: 0 } - match: { profile.shards.0.fetch.debug.stored_fields: [_id, _routing, _source] } - - length: { profile.shards.0.fetch.children: 2 } - - match: { profile.shards.0.fetch.children.0.type: FetchSourcePhase } - - gt: { profile.shards.0.fetch.children.0.breakdown.next_reader_count: 0 } - - gt: { profile.shards.0.fetch.children.0.breakdown.next_reader: 0 } - - gt: { profile.shards.0.fetch.children.0.breakdown.next_reader_count: 0 } - - gt: { profile.shards.0.fetch.children.0.breakdown.next_reader: 0 } - - match: { profile.shards.0.fetch.children.0.debug.fast_path: 1 } - - match: { profile.shards.0.fetch.children.1.type: StoredFieldsPhase } + - length: { profile.shards.0.fetch.children: 3 } + - match: { profile.shards.0.fetch.children.0.type: FetchFieldsPhase } + - match: { profile.shards.0.fetch.children.1.type: FetchSourcePhase } + - gt: { profile.shards.0.fetch.children.1.breakdown.next_reader_count: 0 } + - gt: { profile.shards.0.fetch.children.1.breakdown.next_reader: 0 } + - gt: { profile.shards.0.fetch.children.1.breakdown.next_reader_count: 0 } + - gt: { profile.shards.0.fetch.children.1.breakdown.next_reader: 0 } + - match: { profile.shards.0.fetch.children.1.debug.fast_path: 1 } + - match: { profile.shards.0.fetch.children.2.type: StoredFieldsPhase } --- fetch nested source: - skip: - version: ' - 8.5.99' - reason: stored fields phase added in 8.6 + version: ' - 8.13.99' + reason: _ignored is now retrieved via FetchFieldsPhase, previously via StoredFieldsPhase - do: indices.create: @@ -135,24 +136,25 @@ fetch nested source: - gt: { profile.shards.0.fetch.breakdown.load_stored_fields_count: 0 } - gt: { profile.shards.0.fetch.breakdown.load_stored_fields: 0 } - match: { profile.shards.0.fetch.debug.stored_fields: [_id, _routing, _source] } - - length: { profile.shards.0.fetch.children: 3 } - - match: { profile.shards.0.fetch.children.0.type: FetchSourcePhase } - - gt: { profile.shards.0.fetch.children.0.breakdown.next_reader_count: 0 } - - gt: { profile.shards.0.fetch.children.0.breakdown.next_reader: 0 } - - gt: { profile.shards.0.fetch.children.0.breakdown.next_reader_count: 0 } - - gt: { profile.shards.0.fetch.children.0.breakdown.next_reader: 0 } - - match: { profile.shards.0.fetch.children.1.type: InnerHitsPhase } + - length: { profile.shards.0.fetch.children: 4 } + - match: { profile.shards.0.fetch.children.0.type: FetchFieldsPhase } + - match: { profile.shards.0.fetch.children.1.type: FetchSourcePhase } - gt: { profile.shards.0.fetch.children.1.breakdown.next_reader_count: 0 } - gt: { profile.shards.0.fetch.children.1.breakdown.next_reader: 0 } - gt: { profile.shards.0.fetch.children.1.breakdown.next_reader_count: 0 } - gt: { profile.shards.0.fetch.children.1.breakdown.next_reader: 0 } - - match: { profile.shards.0.fetch.children.2.type: StoredFieldsPhase } + - match: { profile.shards.0.fetch.children.2.type: InnerHitsPhase } + - gt: { profile.shards.0.fetch.children.2.breakdown.next_reader_count: 0 } + - gt: { profile.shards.0.fetch.children.2.breakdown.next_reader: 0 } + - gt: { profile.shards.0.fetch.children.2.breakdown.next_reader_count: 0 } + - gt: { profile.shards.0.fetch.children.2.breakdown.next_reader: 0 } + - match: { profile.shards.0.fetch.children.3.type: StoredFieldsPhase } --- disabling stored fields removes fetch sub phases: - skip: - version: ' - 7.15.99' - reason: fetch profiling implemented in 7.16.0 + version: ' - 8.13.99' + reason: _ignored is now retrieved via FetchFieldsPhase, previously via StoredFieldsPhase - do: search: @@ -163,7 +165,8 @@ disabling stored fields removes fetch sub phases: - match: { hits.hits.0._index: test } - match: { profile.shards.0.fetch.debug.stored_fields: [] } - - is_false: profile.shards.0.fetch.children + - length: { profile.shards.0.fetch.children: 1 } + - match: { profile.shards.0.fetch.children.0.type: FetchFieldsPhase } --- dfs knn vector profiling: 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 b46f2752642a5..6836bab58d664 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 @@ -10,6 +10,7 @@ import org.apache.lucene.index.LeafReaderContext; import org.elasticsearch.common.document.DocumentField; +import org.elasticsearch.index.mapper.IgnoredFieldMapper; import org.elasticsearch.search.SearchHit; import org.elasticsearch.search.fetch.FetchContext; import org.elasticsearch.search.fetch.FetchSubPhase; @@ -17,6 +18,7 @@ import org.elasticsearch.search.fetch.StoredFieldsSpec; import java.io.IOException; +import java.util.Collections; import java.util.Map; /** @@ -25,33 +27,48 @@ * and returns them as document fields. */ public final class FetchFieldsPhase implements FetchSubPhase { + @Override public FetchSubPhaseProcessor getProcessor(FetchContext fetchContext) { FetchFieldsContext fetchFieldsContext = fetchContext.fetchFieldsContext(); - if (fetchFieldsContext == null) { - return null; - } - FieldFetcher fieldFetcher = FieldFetcher.create(fetchContext.getSearchExecutionContext(), fetchFieldsContext.fields()); + final FieldFetcher fieldFetcher = fetchFieldsContext == null + ? null + : FieldFetcher.create(fetchContext.getSearchExecutionContext(), fetchFieldsContext.fields()); + // TODO do we need to explicitly handle for the case when _ignored is explicitly requested? That's redundant? + FieldFetcher fieldFetcherMetadataFields = FieldFetcher.create( + fetchContext.getSearchExecutionContext(), + Collections.singletonList(new FieldAndFormat(IgnoredFieldMapper.NAME, null)) + ); return new FetchSubPhaseProcessor() { @Override public void setNextReader(LeafReaderContext readerContext) { - fieldFetcher.setNextReader(readerContext); + if (fieldFetcher != null) { + fieldFetcher.setNextReader(readerContext); + } + fieldFetcherMetadataFields.setNextReader(readerContext); } @Override public StoredFieldsSpec storedFieldsSpec() { - return fieldFetcher.storedFieldsSpec(); + if (fieldFetcher != null) { + return fieldFetcher.storedFieldsSpec(); + } + return StoredFieldsSpec.NO_REQUIREMENTS; } @Override public void process(HitContext hitContext) throws IOException { - Map documentFields = fieldFetcher.fetch(hitContext.source(), hitContext.docId()); SearchHit hit = hitContext.hit(); - for (Map.Entry entry : documentFields.entrySet()) { - hit.setDocumentField(entry.getKey(), entry.getValue()); + if (fieldFetcher != null) { + Map documentFields = fieldFetcher.fetch(hitContext.source(), hitContext.docId()); + for (Map.Entry entry : documentFields.entrySet()) { + hit.setDocumentField(entry.getKey(), entry.getValue()); + } } + Map metaFields = fieldFetcherMetadataFields.fetch(hitContext.source(), hitContext.docId()); + hit.addDocumentFields(Collections.emptyMap(), metaFields); } }; } diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/StoredFieldsPhase.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/StoredFieldsPhase.java index d6950df962433..d115e854775a8 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/subphase/StoredFieldsPhase.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/subphase/StoredFieldsPhase.java @@ -55,7 +55,6 @@ boolean hasValue(Map> loadedFields) { private static final List METADATA_FIELDS = List.of( new StoredField("_routing", RoutingFieldMapper.FIELD_TYPE, true), - new StoredField("_ignored", IgnoredFieldMapper.FIELD_TYPE, true), // pre-6.0 indexes can return a _type field, this will be valueless in modern indexes and ignored new StoredField("_type", LegacyTypeFieldMapper.FIELD_TYPE, true) ); @@ -76,6 +75,10 @@ public FetchSubPhaseProcessor getProcessor(FetchContext fetchContext) { if (SourceFieldMapper.NAME.equals(field) == false) { Collection fieldNames = sec.getMatchingFieldNames(field); for (String fieldName : fieldNames) { + if (fieldName.equals(IgnoredFieldMapper.NAME)) { + // _ignored is now fetched via FetchFieldsPhase + continue; + } MappedFieldType ft = sec.getFieldType(fieldName); if (ft.isStored() == false) { continue;