Refactor how to determine if a field is metafield #57378

mayya-sharipova · 2020-05-29T18:31:53Z

Before to determine if a field is meta-field, a static method of MapperService
isMetadataField was used. This method was using an outdated static list
of meta-fields.

This PR instead changes this method to the instance method that
is also aware of meta-fields in all registered plugins.

Related #38373, #41656
Closes #24422

elasticmachine · 2020-05-29T18:31:55Z

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

Before to determine if a field is meta-field, a static method of MapperService isMetadataField was used. This method was using an outdated static list of meta-fields. This PR instead changes this method to the instance method that is also aware of meta-fields in all registered plugins. Related elastic#38373, elastic#41656 Closes elastic#24422

cbuescher

Thanks @mayya-sharipova, I did a first pass and it looks great I think. Left a few questions/comments but nothing big I think. One general question I have around the goals of this PR wrt testing: in your description you write that

This PR instead changes this method to the instance method that
is also aware of meta-fields in all registered plugins.

Is there any way to test (unit or IT test) we really pick up meta-fields from other plugins this way? Or is this already covered, e.g. by yml test in the mapper-size plugin that would break if we wouldn't use the mapperRegistry but e.g. only the core built-in meta-fields?

server/src/main/java/org/elasticsearch/search/SearchHit.java

...ercolator/src/main/java/org/elasticsearch/percolator/PercolatorMatchedSlotSubFetchPhase.java

server/src/internalClusterTest/java/org/elasticsearch/search/fields/SearchFieldsIT.java

server/src/main/java/org/elasticsearch/search/SearchHit.java

cbuescher · 2020-06-02T16:36:09Z

server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java

        if (fieldsVisitor == null) {
-            return new SearchHit(docId, null, null, null);
-
+            return new SearchHit(docId, null, docFields, metaFields);


I'd probably prefer it if we continue using null arguments and remove earlier declaration of maps (move it down to where they are needed?)

cbuescher · 2020-06-02T16:38:34Z

server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java

@@ -278,11 +251,17 @@ private SearchHit createNestedSearchHit(SearchContext context,
            source = null;
        }

-        Map<String, DocumentField> searchFields = null;
+        Map<String, DocumentField> docFields = emptyMap();


I'd prefer modifiable maps right here and remove L261/262 instead. Just for readability

Indeed, readability will be improved with this suggestion. But I think it is more important not to create unnecessary HashMap object for every nested hit, that's why I set it to emptyMap(). I can also set it to null as it was before. What do you think?

cbuescher · 2020-06-02T16:42:08Z

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

                    if (hitField == null) {
                        hitField = new DocumentField(field, new ArrayList<>(2));
-                        hit.setField(field, hitField);
+                        // docValues field is put under "fields" even if they are meta-data fields


I was under the impression docValues are put under "documentFields", not meta? Am I missing sth here? Maybe rephrase?

cbuescher · 2020-06-02T16:45:25Z

server/src/test/java/org/elasticsearch/index/get/DocumentFieldTests.java

            String metaField = randomValueOtherThanMany(field -> field.equals(TypeFieldMapper.NAME)
-                    || field.equals(IndexFieldMapper.NAME) || field.equals(IdFieldMapper.NAME),
-                () -> randomFrom(MapperService.getAllMetaFields()));
+                    || field.equals(IndexFieldMapper.NAME) || field.equals(IdFieldMapper.NAME)


Do you know why we exclude these random field types here? I don't remember atm, if you do (bc. you added some cases) maybe a comment would help my future me understanding this better.

cbuescher · 2020-06-02T17:02:05Z

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

+        if (IndicesModule.getBuiltInMetadataFields().contains(fieldName)) {
+            return true;
+        }
+        // adding _size field as meta-field for bwc


Since this sticks out as odd, maybe extend the comment by briefly explaining the cases where we need this. As far as I understand this should only be the case when a pre-7.3 node uses the size mapper plugin and sends these fields jumbled with the other document fields, right?

mayya-sharipova · 2020-06-03T22:02:44Z

@cbuescher Thank you for the review. I have tried to address your comments in 3a56fd2. Please continue to review when you have time.

Is there any way to test (unit or IT test) we really pick up meta-fields from other plugins this way?

I have added a test RankFeatureMetaFieldMapperTests:: testDocumentParsingFailsOnMetaField that uses MapperExtrasPlugin, where document parsing fails on a meta-field _feature from this plugin.

mayya-sharipova · 2020-06-04T14:31:55Z

@elasticmachine run elasticsearch-ci/bwc

…ring

cbuescher

Hi @mayya-sharipova, thanks for the update and adressing the previous review comments. I just have one tiny ask around comments of the new tests, but no need for another round.

cbuescher · 2020-06-04T16:14:44Z

...per-extras/src/test/java/org/elasticsearch/index/mapper/RankFeatureMetaFieldMapperTests.java

@@ -55,4 +57,14 @@ public void testBasics() throws Exception {
        assertEquals(mapping, mapper.mappingSource().toString());
        assertNotNull(mapper.metadataMapper(RankFeatureMetaFieldMapper.class));
    }
+
+    public void testDocumentParsingFailsOnMetaField() throws Exception {


Great way to test!
nit: could you add a very brief comment explaining why this proves the metadata field from the plugin was correctly registered? Makes it easier to later track why tests are there and what they check.

@cbuescher Thank you for the review.
The last comment addressed in ac6a4bc

Before to determine if a field is meta-field, a static method of MapperService isMetadataField was used. This method was using an outdated static list of meta-fields. This PR instead changes this method to the instance method that is also aware of meta-fields in all registered plugins. Related elastic#38373, elastic#41656 Closes elastic#24422

Before to determine if a field is meta-field, a static method of MapperService isMetadataField was used. This method was using an outdated static list of meta-fields. This PR instead changes this method to the instance method that is also aware of meta-fields in all registered plugins. Related #38373, #41656 Closes #24422

mayya-sharipova added >non-issue :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.9.0 labels May 29, 2020

elasticmachine added the Team:Search Meta label for search team label May 29, 2020

mayya-sharipova requested a review from cbuescher May 29, 2020 18:32

mayya-sharipova mentioned this pull request May 29, 2020

Refactor DocumentField to add metafield info #56790

Closed

mayya-sharipova force-pushed the metafield-refactoring branch from 0e39f1b to 3de6272 Compare May 29, 2020 19:26

cbuescher reviewed Jun 2, 2020

View reviewed changes

Address Christoph's feedback

3a56fd2

mayya-sharipova added 2 commits June 4, 2020 10:49

Merge remote-tracking branch 'upstream/master' into metafield-refacto…

a5a6b3f

…ring

Correct AggregationsTests to exclude random fields from top_hits

b6d3364

cbuescher approved these changes Jun 4, 2020

View reviewed changes

Add comment to test: testDocumentParsingFailsOnMetaField

ac6a4bc

mayya-sharipova merged commit b68bd78 into elastic:master Jun 5, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor how to determine if a field is metafield #57378

Refactor how to determine if a field is metafield #57378

mayya-sharipova commented May 29, 2020

elasticmachine commented May 29, 2020

cbuescher left a comment

cbuescher Jun 2, 2020

cbuescher Jun 2, 2020

mayya-sharipova Jun 3, 2020

cbuescher Jun 2, 2020

cbuescher Jun 2, 2020

cbuescher Jun 2, 2020

mayya-sharipova commented Jun 3, 2020

mayya-sharipova commented Jun 4, 2020

cbuescher left a comment

cbuescher Jun 4, 2020

mayya-sharipova Jun 4, 2020

Refactor how to determine if a field is metafield #57378

Refactor how to determine if a field is metafield #57378

Conversation

mayya-sharipova commented May 29, 2020

elasticmachine commented May 29, 2020

cbuescher left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayya-sharipova commented Jun 3, 2020

mayya-sharipova commented Jun 4, 2020

cbuescher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment