Fetch meta fields in FetchFieldsPhase using ValueFetcher #106325

salvatore-campagna · 2024-03-13T18:27:02Z

Here we extract the logic to populate metadata fields such as _ignored, _routing, _size and the deprecated _type into FetchFieldsPhase so that we can use the ValueFetcher interface to retrieve field values. This allows us to fetch values no matter if the Mapper uses stored or doc values.

This is a refactoring required for #101373

elasticsearchmachine · 2024-03-13T18:28:20Z

Pinging @elastic/es-search (Team:Search)

elasticsearchmachine · 2024-03-13T18:28:20Z

Pinging @elastic/es-storage-engine (Team:StorageEngine)

javanna

Thanks for opening this separate PR, I left some comments.

javanna · 2024-03-14T09:18:25Z

server/src/main/java/org/elasticsearch/search/fetch/subphase/StoredFieldsPhase.java

@@ -77,7 +68,7 @@ public FetchSubPhaseProcessor getProcessor(FetchContext fetchContext) {
                    Collection<String> fieldNames = sec.getMatchingFieldNames(field);
                    for (String fieldName : fieldNames) {
                        MappedFieldType ft = sec.getFieldType(fieldName);
-                        if (ft.isStored() == false) {
+                        if (ft.isStored() == false || sec.isMetadataField(fieldName)) {


Given that StoredField is a private record, and we create it within this class, I think we should simply skip creating an instance for metadata fields, instead of creating it and later ignoring it?

If the field is a metadata field we hit the if and execute continue which means we do not create the stored field and we do not add it to storedFields. So I think we are not creating an instance od StoredField for metadata fields. So it looks to me that we already skip the creation of the stored field. Am I wrong?

I guess one could argue that later the check on storedField.isMetadataField == false is not necessary because of this since all stored fields we create are not metadata fields, right?

You are correct, I think we should now be able to remove the isMetadataField member from the private StoredField record. There is also a conditional to exclude the _source from retrieval, that can and should be now removed.

javanna · 2024-03-14T09:22:31Z

server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchMetadataFieldsPhase.java

+    @Override
+    public FetchSubPhaseProcessor getProcessor(FetchContext fetchContext) {
+        StoredFieldsContext storedFieldsContext = fetchContext.storedFieldsContext();
+        if (storedFieldsContext == null || storedFieldsContext.fetchFields() == false) {


I see that MetadataFetcher below uses valueFetchers to retrieve values, and that is great. That means that this phase should no longer be exposed to how a certain field is stored, it just retrieves metadata fields via value fetchers.

That should be straightforward for metadata fields which get returned by default, because they are not explicitly requested and instead of being retrieve from stored fields they go via value fetchers.

Metadata fields that are requested explicitly are requested in different ways: via fields, which goes through FetchFieldsPhase, and via stored_fields which currently goes through StoredFieldsPhase. It seems to me that you intend to move the latter and not the former to this new phase. In fact FetchFieldsPhase already goes through value fetchers, so requesting fields:_ignored would become equivalent to retrieving it via stored_fields:_ignored in how the field is retrieved, but it would be returned in a different section in the search response?

What is odd here is that for this phase to return metadata fields via value fetchers, they need to be requested via stored_fields. That tells me that it may make sense to unify this new phase with the existing FetchFieldsPhase, and add the notion of extracting meta fields requested via stored fields context. That would also require less code duplication, or am I missing something?

javanna · 2024-03-14T09:46:39Z

server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchMetadataFieldsPhase.java

+        };
+    }
+
+    private static List<FieldAndFormat> getAdditionalFields(final StoredFieldsContext storedFieldsContext) {


I don't understand the purpose of this method, shouldn't we extract only metadata fields that are explicitly requested via stored_fields. What do we do instead? Why the explicit mention of _size?

javanna · 2024-03-14T09:48:09Z

server/src/main/java/org/elasticsearch/search/fetch/subphase/MetadataFetcher.java

+import java.util.Map;
+import java.util.stream.Stream;
+
+public class MetadataFetcher {


given that this is the only way to retrieve metadata fields in the new phase, I don't see why we need a separate component for it. A big portion of it is what FetchFieldsPhase already does, so maybe we should unify the two and try to reuse what is already available? What is the need for a new component / phase ? The only difference is where fields are returned in the search response?

…ields

salvatore-campagna · 2024-03-14T16:52:41Z

server/src/main/java/org/elasticsearch/index/mapper/NestedValueFetcher.java

                Source.fromMap(filteredSource, source.sourceContentType()),
                doc
            );

            Map<String, Object> nestedEntry = new HashMap<>();
-            for (DocumentField field : fetchResult.values()) {
+            for (DocumentField field : fetchResult.documentFields().values()) {


Here I am using just the document fields because there is no one of the metadata fields used in the response at the top level that need to be used in a nested context.

server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldFetcher.java

…a fields" This reverts commit 99f129d.

If fields are not allowed, later on we are not able to retrieve the corresponding MappedFieldType which results in a null pointer exception. Filtering such fields happens by means of the QueryRewriteContext#allowedFields which prevents fetching mappings if the field predicate evaluates to false.

While looking at #106325, which moves fetching of metadata fields out of the StoredFieldsPhase, I noticed some small adjustments that we can make to FieldsVisitor, CustomFieldsVisitor and StoredFieldsPhase. These are not functional changes, the only goal is to make things simpler and clearer, hopefully. - add test coverage for situation where _routing is provided with docs, hence returned by default make a stronger connection between CustomFieldsVisitor and FieldsVisitor around fields that are treated differently (_ignored, _routing, _id and _source) - explicitly exclude _id from StoredFieldsPhase like we already do for _source as it's retrieved separately - move the _source exclusion in StoredFieldsPhase to after calling getMatchingFieldNames, so that patterns that match _source still exclude it

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/520_fetch_fields.yml

javanna

I found two small things that need addressing, but it's real close. Thanks!

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/get/120_stored_fields_ignored.yml

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/520_fetch_fields.yml

server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java

javanna · 2024-04-11T09:34:30Z

server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java

+                final Set<String> matchingFieldNames = searchExecutionContext.getMatchingFieldNames(storedField);
+                for (final String matchingFieldName : matchingFieldNames) {
+                    final MappedFieldType fieldType = searchExecutionContext.getFieldType(matchingFieldName);
+                    if (SourceFieldMapper.NAME.equals(matchingFieldName) == false


this conditional should be before getting the field type: there is no need to retrieve the field type for source and id, and for any other field that is not a metadata field.

javanna · 2024-04-11T09:36:06Z

server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java

+                    if (SourceFieldMapper.NAME.equals(matchingFieldName) == false
+                        && IdFieldMapper.NAME.equals(matchingFieldName) == false
+                        && searchExecutionContext.isMetadataField(matchingFieldName)
+                        && fieldType.isStored()) {


can you add a comment around using isStored here to reproduce the previous behaviour we had in StoredFieldsPhase. This is purely for bw comp. I think one way to improve this is merging #107086 later.

…card in stored fields phase

…via wildcard in stored fields phase" This reverts commit 4ace7a0.

javanna

LGTM

We have moved fetching of metadata fields to FetchFieldsPhase with elastic#106325 . For backwards compatibility, metadata fields requested via `stored_fields` are returned even when they match a wildcard expression, which differs from how patterns are resolved via `fields`. Instead of calling getMatchingFieldNames and getFieldType twice per field, this distinction is now included in FieldFetcher#create via a predicate

We have moved fetching of metadata fields to FetchFieldsPhase with #106325 . For backwards compatibility, metadata fields requested via `stored_fields` are returned even when they match a wildcard expression, which differs from how patterns are resolved via `fields`. Instead of calling getMatchingFieldNames and getFieldType twice per field, this distinction is now included in FieldFetcher#create via a predicate

refactor: extract meta fields fetching in separate fetch sub-phase

7f93d0a

elasticsearchmachine added v8.14.0 needs:triage Requires assignment of a team area label labels Mar 13, 2024

salvatore-campagna added :Search/Search Search-related issues that do not fall into other categories >refactoring :StorageEngine/Logs You know, for Logs and removed needs:triage Requires assignment of a team area label labels Mar 13, 2024

elasticsearchmachine added Team:Search Meta label for search team Team:StorageEngine labels Mar 13, 2024

salvatore-campagna added 2 commits March 13, 2024 19:30

fix: inline return value of storedFieldSpec

1006059

fix: check stored fields not fetch fields

535cada

javanna reviewed Mar 14, 2024

View reviewed changes

salvatore-campagna added 3 commits March 14, 2024 17:45

refactor: just use FetchFieldsPhase to fetch both fields and stored_f…

c840da4

…ields

fix: update skip version

6c31e57

fix: simplify code

503439f

salvatore-campagna changed the title ~~Extract meta fields fetching in separate fetch sub-phase using ValueFecther~~ Fetch meta fields in FetchFieldsPhase using ValueFetcher Mar 14, 2024

salvatore-campagna commented Mar 14, 2024

View reviewed changes

fix: prevent a NPE when retrieving the field type for metadata fields

99f129d

salvatore-campagna commented Mar 18, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldFetcher.java Outdated Show resolved Hide resolved

salvatore-campagna self-assigned this Mar 18, 2024

salvatore-campagna added 8 commits March 18, 2024 12:36

Revert "fix: prevent a NPE when retrieving the field type for metadat…

ba0d9e7

…a fields" This reverts commit 99f129d.

fix: fetch _source using ValueFetcher

9bb8dbc

fix: return stored fields with their correct type not as strings

9fa44ca

fix: process just metadata fields

64dbaed

fix: restore original exception message

cff6343

fix: hadling of alias fields

d335005

nit: improve readability

45c5bb9

fix: fetch fields and stored fields

4290eac

salvatore-campagna mentioned this pull request Apr 5, 2024

Fetching _seq_no via fields results in 5xx error #107136

Closed

salvatore-campagna added 7 commits April 5, 2024 12:09

fix: always return default metadata fields

6687daa

fix: remove unnecessary null check

48f35fb

fix: remove skip version

ce16a81

fix: remove reason as it is sligthly different for ccs

ac592e9

Merge branch 'main' into refactor/meta-fields-sub-fetch

7c33c62

fix: _ignored fetched through stored_fields

06697a5

fix: do not fetch metadata fields in stored fields phase

f87dbf4

salvatore-campagna commented Apr 8, 2024

View reviewed changes

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/520_fetch_fields.yml Show resolved Hide resolved

javanna requested changes Apr 11, 2024

View reviewed changes

salvatore-campagna added 9 commits April 12, 2024 10:09

fix: remove check on stored_fields for get api

c6d84fc

fix: exclude source and id before getting the field type

cf81e82

test: add missing test requesting none stored_fields and a few fields

26b2463

fix: include a method to know if metadata fields are fetched via wild…

4ace7a0

…card in stored fields phase

Merge branch 'main' into refactor/meta-fields-sub-fetch

65bd1ec

fix: skip test in v7 since it was http status 500 instead of 400

f8ed186

Revert "fix: include a method to know if metadata fields are fetched …

db1a8cb

…via wildcard in stored fields phase" This reverts commit 4ace7a0.

fix: usae HashSet instead of TreeSet

56ea97a

note: clarify why we need to check field is stored

866ce95

javanna approved these changes Apr 15, 2024

View reviewed changes

salvatore-campagna merged commit 4dfcb08 into elastic:main Apr 15, 2024
14 checks passed

salvatore-campagna added a commit to eyalkoren/elasticsearch that referenced this pull request Apr 15, 2024

fix: restore original after merging elastic#106325

c2232c9

javanna mentioned this pull request Apr 15, 2024

Avoid resolving metadata fields twice in FetchFieldsPhase #107474

Merged

salvatore-campagna mentioned this pull request Apr 16, 2024

Only retrive required stored fields #107543

Closed

javanna mentioned this pull request May 2, 2024

Avoid resolving metadata fields twice in FetchFieldsPhase #108204

Closed

benwtrent mentioned this pull request Jul 9, 2024

Join field called "_type" is duplicated in the _search results #110438

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch meta fields in FetchFieldsPhase using ValueFetcher #106325

Fetch meta fields in FetchFieldsPhase using ValueFetcher #106325

salvatore-campagna commented Mar 13, 2024 •

edited

Loading

elasticsearchmachine commented Mar 13, 2024

elasticsearchmachine commented Mar 13, 2024

javanna left a comment

javanna Mar 14, 2024

salvatore-campagna Mar 14, 2024 •

edited

Loading

javanna Apr 3, 2024

javanna Mar 14, 2024

javanna Mar 14, 2024

javanna Mar 14, 2024

salvatore-campagna Mar 14, 2024

javanna left a comment

javanna Apr 11, 2024

javanna Apr 11, 2024

javanna left a comment

Fetch meta fields in FetchFieldsPhase using ValueFetcher #106325

Fetch meta fields in FetchFieldsPhase using ValueFetcher #106325

Conversation

salvatore-campagna commented Mar 13, 2024 • edited Loading

elasticsearchmachine commented Mar 13, 2024

elasticsearchmachine commented Mar 13, 2024

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvatore-campagna Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

salvatore-campagna commented Mar 13, 2024 •

edited

Loading

salvatore-campagna Mar 14, 2024 •

edited

Loading