Add declarative parameters to FieldMappers #58663

romseygeek · 2020-06-29T12:05:31Z

The FieldMapper infrastructure currently has a bunch of shared parameters, many of which
are only applicable to a subset of the 41 mapper implementations we ship with. Merging,
parsing and serialization of these parameters are spread around the class hierarchy, with
much repetitive boilerplate code required. It would be much easier to reason about these
things if we could declare the parameter set of each FieldMapper directly in the implementing
class, and share the parsing, merging and serialization logic instead.

This commit is a first effort at introducing a declarative parameter style. It adds a new FieldMapper
subclass, ParametrizedFieldMapper, and refactors two mappers, Boolean and Binary, to use it.
Parameters are declared on Builder classes, with the declaration including the parameter name,
whether or not it is updateable, a default value, how to parse it from mappings, and how to
extract it from another mapper at merge time. Builders have a getParameters method, which
returns a list of the declared parameters; this is then used for parsing, merging and serialization.
Merging is achieved by constructing a new Builder from the existing Mapper, and merging in
values from the merging Mapper; conflicts are all caught at this point, and if none exist then a new,
merged, Mapper can be built from the Builder. This allows all values on the Mapper to be final.

Other mappers can be gradually migrated to this new style, and once they have all been refactored
we can merge ParametrizedFieldMapper and FieldMapper entirely.

elasticmachine · 2020-06-29T12:05:33Z

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

nik9000

I like this! I left a few questions.

nik9000 · 2020-06-29T15:38:47Z

modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java

@@ -157,8 +157,6 @@ static KeywordFieldMapper createExtractQueryFieldBuilder(String name, BuilderCon
        static BinaryFieldMapper createQueryBuilderFieldBuilder(BuilderContext context) {
            BinaryFieldMapper.Builder builder = new BinaryFieldMapper.Builder(QUERY_BUILDER_FIELD_NAME);
            builder.docValues(true);
-            builder.indexOptions(IndexOptions.NONE);


Note to self - these were the defaults so they are safe to drop.

nik9000 · 2020-06-29T15:44:21Z

server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java

+
+    public static class Builder extends ParametrizedFieldMapper.Builder {
+
+        final ParametrizedFieldMapper.Parameter<Boolean> fixed


I think I'd import the inner class to make these a little shorter.

I know some folks hate importing inner classes but I don't really have hard and fast rules and think in this case it'd be easier to read.

nik9000 · 2020-06-29T15:51:58Z

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

+            return builder;
+        }
+
+        public static Parameter<Boolean> boolParam(String name, boolean updateable,


These smell a little like our settings which have grown a bit difficult to read over the years. There, I think a builder pattern would have helped deal with how many optional things there are. But here I dunno.

I think merger might need to take the current value so it can make a call on more complex stuff, right?

If a value isn't updateable do you need to define a merger at all?

Actually, looking above you use acceptsNull in a very bulder like way, which I'm cool with.

I think it'd be good to make the "for declaration" method public with javadoc. And the "for use by ParameterizedFieldMapper" methods private or at least package private. That way it is more obvious what we can call when building the mappers.

I'm sort of sad that this is bit of a parallel world to the xcontent parsers that we already have, but I know how we got here. And it is where we are. So, yeah, I'm +1 on building it.

++ to making the 'used internally only' methods private. I've renamed 'merger' to 'initializer' as that's what it does, takes an initial value from an existing mapper.

nik9000 · 2020-06-29T15:53:24Z

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

+
+        public final void merge(FieldMapper in, Conflicts conflicts) {
+            for (Parameter<?> param : getParameters()) {
+                if (param.updateable == false) {


Do you think it'd be clearer if merge checked for updateable?

Definitely, good spot. Begone, freeze

nik9000 · 2020-06-29T15:56:39Z

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

+
+        public Parameter<T> init(FieldMapper toInit) {
+            assert frozen == false;
+            this.value = merger.apply(toInit);


This makes me think merger isn't really the right name for this, actually - it is more like read.

++ I've used initializer

nik9000 · 2020-06-29T16:00:44Z

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

+            super(name);
+        }
+
+        // For testing


If you make them package private that does a pretty good job implying they are only exposed for testing.

It's used outside the package, annoyingly. I've changed it so that if you need to set it programmatically you can use a constructor parameter.

romseygeek · 2020-06-30T16:26:15Z

For some reason, the ML mappings (and only the ML mappings) are now being serialized differently, even though this PR makes no changes to ObjectMapper serialization. 🤷

nik9000 · 2020-06-30T19:11:53Z

For some reason, the ML mappings (and only the ML mappings) are now being serialized differently, even though this PR makes no changes to ObjectMapper serialization

Neat! Always good to find a new surprise.

nik9000 · 2020-06-30T19:16:44Z

server/src/test/java/org/elasticsearch/index/mapper/BinaryFieldMapperTests.java


 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;

-public class BinaryFieldMapperTests extends FieldMapperTestCase<BinaryFieldMapper.Builder> {


Without this we don't test store and stuff like that, right?

Ah! So you have the tests in ParameterizedFieldTest for things like this. Do you think it is important to also test that the field merges here too? I guess not, but I'm worried about dropping something accidentally.

The idea would be that we don't have 'common' fields any more - all mappers declare all their parameters outright (I am sharing the 'meta' parameter at the moment, but I think I'm going to stop doing that). So the tests for each field mapper should cover all the parameters that they declare, but general things like 'this parameter can't be updated so it should throw an error on merge' are tested in ParametrizedFieldTest.

nik9000 · 2020-06-30T19:17:03Z

server/src/test/java/org/elasticsearch/index/mapper/BooleanFieldMapperTests.java


 import static org.hamcrest.Matchers.containsString;

-public class BooleanFieldMapperTests extends FieldMapperTestCase<BooleanFieldMapper.Builder> {


Same here, right?

javanna

I like it too, I left a couple of questions

javanna · 2020-07-06T09:06:15Z

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

+            context.doc().add(new Field(fieldType().name(), value ? "T" : "F", Defaults.FIELD_TYPE));
+        }
+        if (stored) {
+            context.doc().add(new StoredField(fieldType().name(), value ? "T" : "F"));


seems like if the field is searchable and stored too, we end up adding two fields, while before we would only add one?

Adding a field with a FieldType that has stored set to true and adding a separate StoredField amount to the same thing. This way, we don't need to change the field type at all, and it's clearer what's happening at the point where we're adding documents.

It was odd that we were using Field to get stored fields but doc values. Field just does a bunch of stuff.

javanna · 2020-07-06T09:10:15Z

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

+            = new Parameter<>("null_value", false, null, (n, o) -> XContentMapValues.nodeBooleanValue(o), m -> toType(m).nullValue);
+        private final Parameter<Boolean> stored = Parameter.boolParam("store", false, m -> toType(m).stored, false);
+        private final Parameter<Map<String, String>> meta
+            = new Parameter<>("meta", true, Collections.emptyMap(), TypeParsers::parseMeta, m -> m.fieldType().meta());


I get confused here that stored seems to be treated differently from e.g. doc_values. Why is that?

Convenience, mainly - doc_values and index can be derived from the field type values, but stored can't be. I can change it so that doc_values and index are fields directly on the mapper as well if you think that's clearer? Certainly for things like searchable docvalues fields then we'll need to distinguish between whether or not something is indexed and whether it's searchable.

I guess I am trying to see what the difference is between stored and indexed/doc_values. I would expect them to be treated in the same way/same place. Why can't stored be derived from the field type values?

As it stands, MappedFieldType doesn't have anything that tells you if the field is stored or not.

You made a boolean hasDocValues on BinaryFieldMapper for this. Is it worth making one here too?

I'll add indexed and hasDocValues directly to the mappers as it sounds as though that will be less confusing.

javanna · 2020-07-06T09:14:24Z

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

+                Map.Entry<String, Object> entry = iterator.next();
+                final String propName = entry.getKey();
+                final Object propNode = entry.getValue();
+                if (Objects.equals("fields", propName)) {


this means that every field supports multi fields and copy_to, I think. But I think that runtime fields won't?

We can add disabling feature flags later if we need to.

Every field does support multi fields and copy_to right now. I'm happy to juggle things for runtime fields in a follow up.

romseygeek · 2020-07-07T13:40:29Z

@elasticmachine update branch

nik9000

LGTM, though I think @javanna might want another look too.

nik9000 · 2020-07-07T17:03:09Z

server/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java

@@ -332,6 +332,13 @@ public static String nodeStringValue(Object node, String defaultValue) {
        return node.toString();
    }

+    public static String nodeStringValue(Object node) {


Its probably worth adding javadoc about how this is different from Objects.toString.

nik9000 · 2020-07-07T17:04:16Z

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

+        private final Parameter<Boolean> stored = Parameter.boolParam("store", false, m -> toType(m).stored, false);
+        private final Parameter<Boolean> hasDocValues = Parameter.boolParam("doc_values", false, m -> toType(m).hasDocValues,  false);
+        private final Parameter<Map<String, String>> meta
+            = new Parameter<>("meta", true, Collections.emptyMap(), TypeParsers::parseMeta, m -> m.fieldType().meta());


Do we plan to eventually move meta over to the FieldMapper?

meta is used in field caps, which currently are based off MappedFieldType so I think it will probably stay there?

nik9000 · 2020-07-07T17:06:37Z

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

+            = new Parameter<>("null_value", false, null, (n, o) -> XContentMapValues.nodeBooleanValue(o), m -> toType(m).nullValue);
+        private final Parameter<Boolean> stored = Parameter.boolParam("store", false, m -> toType(m).stored, false);
+        private final Parameter<Map<String, String>> meta
+            = new Parameter<>("meta", true, Collections.emptyMap(), TypeParsers::parseMeta, m -> m.fieldType().meta());


You made a boolean hasDocValues on BinaryFieldMapper for this. Is it worth making one here too?

nik9000 · 2020-07-07T17:13:49Z

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

+            context.doc().add(new Field(fieldType().name(), value ? "T" : "F", Defaults.FIELD_TYPE));
+        }
+        if (stored) {
+            context.doc().add(new StoredField(fieldType().name(), value ? "T" : "F"));


It was odd that we were using Field to get stored fields but doc values. Field just does a bunch of stuff.

nik9000 · 2020-07-07T17:15:59Z

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

+                Map.Entry<String, Object> entry = iterator.next();
+                final String propName = entry.getKey();
+                final Object propNode = entry.getValue();
+                if (Objects.equals("fields", propName)) {


Every field does support multi fields and copy_to right now. I'm happy to juggle things for runtime fields in a follow up.

javanna

LGTM

The FieldMapper infrastructure currently has a bunch of shared parameters, many of which are only applicable to a subset of the 41 mapper implementations we ship with. Merging, parsing and serialization of these parameters are spread around the class hierarchy, with much repetitive boilerplate code required. It would be much easier to reason about these things if we could declare the parameter set of each FieldMapper directly in the implementing class, and share the parsing, merging and serialization logic instead. This commit is a first effort at introducing a declarative parameter style. It adds a new FieldMapper subclass, ParametrizedFieldMapper, and refactors two mappers, Boolean and Binary, to use it. Parameters are declared on Builder classes, with the declaration including the parameter name, whether or not it is updateable, a default value, how to parse it from mappings, and how to extract it from another mapper at merge time. Builders have a getParameters method, which returns a list of the declared parameters; this is then used for parsing, merging and serialization. Merging is achieved by constructing a new Builder from the existing Mapper, and merging in values from the merging Mapper; conflicts are all caught at this point, and if none exist then a new, merged, Mapper can be built from the Builder. This allows all values on the Mapper to be final. Other mappers can be gradually migrated to this new style, and once they have all been refactored we can merge ParametrizedFieldMapper and FieldMapper entirely.

cjcenizal · 2020-07-13T16:38:48Z

Maybe my comment from #59381 is best addressed here.

jtibshirani · 2020-07-14T22:24:19Z

@romseygeek this is an exciting change! While updating the fields-retrieval branch, I noticed that this makes it harder to unit test field mappers. For example, before I was able to construct a field mapper like this:

BooleanFieldMapper mapper = new BooleanFieldMapper.Builder("field")
    .nullValue(true)
    .build(context);

Now I think we can't configure the parameters directly, they can only be set by parsing a mapping definition. Do you have any ideas around making field mapper construction easier?

romseygeek · 2020-07-15T14:44:28Z

@jtibshirani I've tried out a couple of ideas here, but nothing is very satisfying - in particular, it's hard to put anything on the base ParametrizedFieldMapper.Builder object because of generic type erasure. When I was first implementing this (and making other mapper changes, eg #56747) I found that very few unit tests actually build mappers in this way, which was why I was happy to remove the builder methods. But maybe I should add them back? Or we could add back only specific setters that are needed by tests - this isn't really a public API, so I think it's fine to have a mix of setters on there.

jtibshirani · 2020-07-15T17:05:43Z

When I was first implementing this (and making other mapper changes, eg #56747) I found that very few unit tests actually build mappers in this way, which was why I was happy to remove the builder methods.

I also noticed that most unit tests don't create builders directly. However I think this just points the fact that we rely too much on integration testing (or 'pseudo unit tests') to test mapper functionality.

But maybe I should add them back? Or we could add back only specific setters that are needed by tests - this isn't really a public API, so I think it's fine to have a mix of setters on there.

This could work, I'll experiment with this as I keep the branch up to date.

romseygeek added 5 commits June 23, 2020 10:12

First go: need to fix merging with BuilderContext

9dda2f3

Merge remote-tracking branch 'origin/master' into mapper/mergebuilders

6c118f3

dedicated tests for parametrized mapper

56ad229

tests

da0aca5

precommit

6e99626

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

romseygeek requested review from nik9000, javanna and jtibshirani June 29, 2020 12:05

romseygeek self-assigned this Jun 29, 2020

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

romseygeek added 2 commits June 29, 2020 13:58

meta parsing

7326c49

Merge remote-tracking branch 'origin/master' into mapper/mergebuilders

ae14ac5

nik9000 reviewed Jun 29, 2020

View reviewed changes

romseygeek added 4 commits June 30, 2020 10:03

Use simple name in merge builder

5bb39b2

Merge remote-tracking branch 'origin/master' into mapper/mergebuilders

a31b1a6

feedback

928a15f

checkstyle

47c17e4

nik9000 reviewed Jun 30, 2020

View reviewed changes

romseygeek mentioned this pull request Jul 1, 2020

MappedFieldType should not extend FieldType #57666

Merged

romseygeek added 3 commits July 1, 2020 13:26

wut

4bf9327

Merge remote-tracking branch 'origin/master' into mapper/mergebuilders

3011064

fix object serialization; javadocs

bb35eda

javanna reviewed Jul 6, 2020

View reviewed changes

Merge branch 'master' into mapper/mergebuilders

b9dd287

nik9000 approved these changes Jul 7, 2020

View reviewed changes

romseygeek added 2 commits July 8, 2020 09:27

Merge remote-tracking branch 'origin/master' into mapper/mergebuilders

8f1853f

feedback

37ea8d6

nik9000 approved these changes Jul 8, 2020

View reviewed changes

javanna approved these changes Jul 9, 2020

View reviewed changes

romseygeek merged commit 219b7db into elastic:master Jul 9, 2020

romseygeek deleted the mapper/mergebuilders branch July 9, 2020 09:56

This was referenced Jul 10, 2020

Parametrized mapper should not fail on silently unsupported parameters #59359

Closed

Continue to accept unused 'universal' params in <8.0 indexes #59381

Merged

romseygeek added the >breaking label Jul 13, 2020

jakelandis mentioned this pull request Feb 22, 2021

DRAFT [META] REST Compatible API V7 completeness #68905

Closed

romseygeek removed the >breaking label Jun 30, 2021

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

masseyke mentioned this pull request Aug 9, 2021

Update Deprecation Info API checks for 8.0 #42404

Closed

80 tasks


		public static class Builder extends ParametrizedFieldMapper.Builder {

		final ParametrizedFieldMapper.Parameter<Boolean> fixed


		import static org.hamcrest.Matchers.containsString;

		public class BooleanFieldMapperTests extends FieldMapperTestCase<BooleanFieldMapper.Builder> {

Add declarative parameters to FieldMappers #58663

Add declarative parameters to FieldMappers #58663

Conversation

romseygeek commented Jun 29, 2020

elasticmachine commented Jun 29, 2020

nik9000 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

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

romseygeek commented Jun 30, 2020

nik9000 commented Jun 30, 2020

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romseygeek commented Jul 7, 2020

nik9000 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

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

cjcenizal commented Jul 13, 2020 • edited Loading

jtibshirani commented Jul 14, 2020

romseygeek commented Jul 15, 2020

jtibshirani commented Jul 15, 2020

cjcenizal commented Jul 13, 2020 •

edited

Loading