Move FieldMapper#valueFetcher to MappedFieldType #62974

romseygeek · 2020-09-28T17:41:38Z

For runtime fields, we will want to do all search-time interaction with
a field definition via a MappedFieldType, rather than a FieldMapper, to
avoid interfering with the logic of document parsing. Currently, fetching
values for runtime scripts and for building top hits responses need to
call a method on FieldMapper. This commit moves this method to
MappedFieldType, incidentally simplifying the current call sites and freeing
us up to implement runtime fields as pure MappedFieldType objects.

elasticmachine · 2020-09-28T17:41:40Z

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

romseygeek · 2020-09-28T17:43:14Z

Some more refactoring from me :). We are getting some fairly unwieldy telescoping MappedFieldType constructors now, and there are also quite a few places where we end up sharing object parsing logic between the FieldMapper and the MappedFieldType which I think can be cleaned up, but I wanted to keep this PR as simple as possible and so this is just doing the absolute basics. Further improvements will follow.

romseygeek · 2020-09-28T17:46:53Z

modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapper.java

@@ -122,7 +122,7 @@ Builder nullValue(double nullValue) {
        @Override
        public ScaledFloatFieldMapper build(BuilderContext context) {
            ScaledFloatFieldType type = new ScaledFloatFieldType(buildFullName(context), indexed.getValue(), stored.getValue(),
-                hasDocValues.getValue(), meta.getValue(), scalingFactor.getValue());
+                hasDocValues.getValue(), meta.getValue(), scalingFactor.getValue(), nullValue.getValue());


These telescoping constructors are getting too complicated now - one idea I have for a follow-up is to add a FieldCharacteristics wrapper object that takes the (indexed, docvales, stored, meta) tuple, analogous to TextSearchInfo, and give both of these 'holder' objects constructors that take Parameter sets directly, so that you can go FieldCharacteristics.build(index, hasDocValues, stored, meta) directly in build and remove some of the ceremony.

romseygeek · 2020-09-28T17:49:45Z

...nalysis-icu/src/main/java/org/elasticsearch/index/mapper/ICUCollationKeywordFieldMapper.java


        public CollationFieldType(String name, boolean isSearchable, boolean isStored, boolean hasDocValues,
-                                  Collator collator, Map<String, String> meta) {
+                                  Collator collator, String nullValue, int ignoreAbove, Map<String, String> meta) {
            super(name, isSearchable, isStored, hasDocValues, TextSearchInfo.SIMPLE_MATCH_ONLY, meta);


I think we can wrap this up more nicely as well in a followup, passing a Function<Object, String> parsing object here that is shared with the FieldMapper impl so that we don't need to a) pass nullValue and ignoreAbove and b) duplicate the relevant logic.

And indeed, we can probably add a factory function to SourceValueFetcher that takes a parsing function as well and further reduce the boilerplate.

I tried this out when initially designing the change, and it proved difficult to share the parsing logic. I wrote up the details in this draft PR: #56473. But I would love to discuss further if you find a way forward!

javanna

I left some questions, thanks for taking care of this change!

...s/mapper-extras/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java

.../mapper-murmur3/src/main/java/org/elasticsearch/index/mapper/murmur3/Murmur3FieldMapper.java

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

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

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

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

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

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

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

javanna · 2020-09-29T15:01:57Z

...-flattened/src/main/java/org/elasticsearch/xpack/flattened/mapper/FlatObjectFieldMapper.java

+
+        @Override
+        public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
+            throw new UnsupportedOperationException();  // TODO can we implement this?


I was wondering the same, or even if we should do so. when would this field type be looked up compared to the other one that implements valueFetcher?

This is a good question, I'll propose an idea in a follow-up PR.

romseygeek · 2020-09-29T19:54:28Z

Have updated @javanna and ready for another go-round.

javanna

LGTM

For runtime fields, we will want to do all search-time interaction with a field definition via a MappedFieldType, rather than a FieldMapper, to avoid interfering with the logic of document parsing. Currently, fetching values for runtime scripts and for building top hits responses need to call a method on FieldMapper. This commit moves this method to MappedFieldType, incidentally simplifying the current call sites and freeing us up to implement runtime fields as pure MappedFieldType objects.

jtibshirani · 2020-10-05T17:45:13Z

modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/TokenCountFieldMapper.java

@@ -129,20 +130,6 @@ protected void parseCreateField(ParseContext context) throws IOException {
        );
    }

-    @Override


After this change, I think value parsing will always fail -- since it uses NumberFieldType, it will attempt to parse a piece of text as a number.

jtibshirani · 2020-10-05T17:46:15Z

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

                    continue;
                }
-
-                if (mapper instanceof FieldAliasMapper) {


Very happy we removed this 🎉

jtibshirani · 2020-10-05T17:47:01Z

Thanks @romseygeek for this refactor! Overall it turned out nicely -- it feels better conceptually for field types to own value fetching, since they're in charge of all other search-time operations.

jtibshirani · 2020-10-06T18:08:33Z

modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeatureFieldMapper.java

+            if (format != null) {
+                throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
+            }
+            return new SourceValueFetcher(name(), mapperService, false) {


One other change I noticed -- previously we always passed the result of FieldMapper#parsesArrayValue instead of hard-coding the booleans. Now there is nothing really tying this to parsesArrayValue and it could be easily overlooked. I wonder how we could make this more robust.

An update: I opened #63354 with a small improvement.

When constructing a value fetcher, the 'parsesArrayValue' flag must match `FieldMapper#parsesArrayValue`. However there is nothing in code or tests to help enforce this. This PR reworks the value fetcher constructors so that `parsesArrayValue` is 'false' by default. Just as for `FieldMapper#parsesArrayValue`, field types must explicitly set it to true and ensure the behavior is covered by tests. Follow-up to elastic#62974.

When constructing a value fetcher, the 'parsesArrayValue' flag must match `FieldMapper#parsesArrayValue`. However there is nothing in code or tests to help enforce this. This PR reworks the value fetcher constructors so that `parsesArrayValue` is 'false' by default. Just as for `FieldMapper#parsesArrayValue`, field types must explicitly set it to true and ensure the behavior is covered by tests. Follow-up to #62974.

When constructing a value fetcher, the 'parsesArrayValue' flag must match `FieldMapper#parsesArrayValue`. However there is nothing in code or tests to help enforce this. This PR reworks the value fetcher constructors so that `parsesArrayValue` is 'false' by default. Just as for `FieldMapper#parsesArrayValue`, field types must explicitly set it to true and ensure the behavior is covered by tests. Follow-up to elastic#62974.

When constructing a value fetcher, the 'parsesArrayValue' flag must match `FieldMapper#parsesArrayValue`. However there is nothing in code or tests to help enforce this. This PR reworks the value fetcher constructors so that `parsesArrayValue` is 'false' by default. Just as for `FieldMapper#parsesArrayValue`, field types must explicitly set it to true and ensure the behavior is covered by tests. Follow-up to #62974.

romseygeek added 4 commits September 28, 2020 11:14

WIP

5b74dde

Move valueFetcher() from FieldMapper to MappedFieldType

5b13f4c

Merge remote-tracking branch 'origin/master' into mapper/valuefetcher-ft

0f750b9

tests

d450600

romseygeek added :Search Foundations/Mapping Index mappings, including merging and defining field types >refactoring v8.0.0 v7.10.0 labels Sep 28, 2020

romseygeek requested review from javanna and jtibshirani September 28, 2020 17:41

romseygeek self-assigned this Sep 28, 2020

elasticmachine added the Team:Search Meta label for search team label Sep 28, 2020

romseygeek added 2 commits September 29, 2020 08:58

test jar hell; unsigned long null values

1d4ec99

spotless

4cfc51d

romseygeek commented Sep 29, 2020

View reviewed changes

const keyword tests

3a7494a

javanna reviewed Sep 29, 2020

View reviewed changes

romseygeek added 3 commits September 29, 2020 17:18

Merge remote-tracking branch 'origin/master' into mapper/valuefetcher-ft

ab26af5

feedback

477b48b

Merge remote-tracking branch 'origin/master' into mapper/valuefetcher-ft

11c4fb1

compilation

adc8b9a

javanna approved these changes Sep 30, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into mapper/valuefetcher-ft

1d25eeb

romseygeek merged commit ce649d0 into elastic:master Oct 4, 2020

romseygeek deleted the mapper/valuefetcher-ft branch October 4, 2020 09:47

jtibshirani reviewed Oct 5, 2020

View reviewed changes

jtibshirani reviewed Oct 6, 2020

View reviewed changes

jtibshirani mentioned this pull request Oct 6, 2020

Make array value parsing flag more robust. #63354

Merged

This was referenced Oct 7, 2020

Small fixes to flattened field value fetching. #63443

Merged

Follow-up improvements to field retrieval. #60985

Closed

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

Move FieldMapper#valueFetcher to MappedFieldType #62974

Move FieldMapper#valueFetcher to MappedFieldType #62974

romseygeek commented Sep 28, 2020

elasticmachine commented Sep 28, 2020

romseygeek commented Sep 28, 2020

romseygeek Sep 28, 2020

romseygeek Sep 28, 2020

romseygeek Sep 28, 2020

jtibshirani Oct 5, 2020 •

edited

Loading

javanna left a comment

javanna Sep 29, 2020

jtibshirani Oct 5, 2020

romseygeek commented Sep 29, 2020

javanna left a comment

jtibshirani Oct 5, 2020

jtibshirani Oct 5, 2020

jtibshirani commented Oct 5, 2020

jtibshirani Oct 6, 2020 •

edited

Loading

Move FieldMapper#valueFetcher to MappedFieldType #62974

Move FieldMapper#valueFetcher to MappedFieldType #62974

Conversation

romseygeek commented Sep 28, 2020

elasticmachine commented Sep 28, 2020

romseygeek commented Sep 28, 2020

romseygeek Sep 28, 2020

Choose a reason for hiding this comment

romseygeek Sep 28, 2020

Choose a reason for hiding this comment

romseygeek Sep 28, 2020

Choose a reason for hiding this comment

jtibshirani Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

javanna Sep 29, 2020

Choose a reason for hiding this comment

jtibshirani Oct 5, 2020

Choose a reason for hiding this comment

romseygeek commented Sep 29, 2020

javanna left a comment

Choose a reason for hiding this comment

jtibshirani Oct 5, 2020

Choose a reason for hiding this comment

jtibshirani Oct 5, 2020

Choose a reason for hiding this comment

jtibshirani commented Oct 5, 2020

jtibshirani Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

jtibshirani Oct 5, 2020 •

edited

Loading

jtibshirani Oct 6, 2020 •

edited

Loading