Handle legacy mappings with placeholder fields by ywelsch · Pull Request #85059 · elastic/elasticsearch

ywelsch · 2022-03-17T11:04:49Z

As part of #81210 we would like to add support for handling legacy (Elasticsearch 5 and 6) mappings in newer Elasticsearch versions. The idea is to import old mappings "as-is" into Elasticsearch 8, and adapt the mapper parsers so that they can handle those old mappings. Only a select subset of the legacy mapping will actually be parsed, and fields that are neither known to newer ES version nor supported for search will be mapped as "placeholder fields", i.e., they are still represented as fields in the system so that they can give proper error messages when queried by a user.

Fields that are supported:

field data types that support doc values only fields
- normalizer on keyword fields and date formats on date fields are on supported in so far as they behave similarly across versions. In case they are not, these fields are now updateable on legacy indices so that they can be "fixed" by user.
object fields
nested fields in limited form (not supporting nested queries)
- add tests / checks in follow-up PR
multi fields
field aliases
metadata fields
runtime fields (auto-import to be added for future versions)

5.x indices with mappings that have multiple mapping types are collapsed together on a best-effort basis before they are imported.

Relates #81210

fix compiel

elasticmachine · 2022-03-28T08:20:47Z

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

…ersion indexCreatedVersion)

romseygeek

I'm happy with the Mapper changes now, I think. Can we have some tests checking serialization of placeholder mappers with unknown fields?

romseygeek · 2022-04-21T13:39:20Z

...gin/vectors/src/main/java/org/elasticsearch/xpack/vectors/mapper/DenseVectorFieldMapper.java

        (n, c) -> new Builder(n, c.indexVersionCreated()),
-        notInMultiFields(CONTENT_TYPE)
+        notInMultiFields(CONTENT_TYPE),
+        Version.CURRENT.minimumCompatibilityVersion()


Given that there are a few of these multi-parameter calls, maybe we should add a super constructor that takes the Builder and verifier functions and passes Version.CURRENT.minimumCompatibilityVersion() as a default value?

ok, fixed in 07234b1

romseygeek · 2022-04-21T13:39:55Z

...a/repository-old-versions/src/test/java/org/elasticsearch/oldrepos/DocValueOnlyFieldsIT.java

-        mappingsBuilder.startObject("date").field("type", "date").field("format", "yyyy/MM/dd").endObject();
-        mappingsBuilder.endObject().endObject();
-        putMappingsRequest.setJsonEntity(Strings.toString(mappingsBuilder));
-        assertOK(client().performRequest(putMappingsRequest));


Can you clarify why this needs to be removed?

This is now automated by the mapping conversion. Prior to this PR, you had to manually define the mapping after importing a legacy index in order to access the fields in that index (the original mapping would only be imported to _meta/legacy-mappings section).

ywelsch · 2022-04-25T09:20:07Z

I'm happy with the Mapper changes now, I think. Can we have some tests checking serialization of placeholder mappers with unknown fields?

I've added such a test in ad99173 in addition to the ones in OldMappingsIT

romseygeek

I left a couple of nits, but LGTM otherwise. Thanks for your patience on this @ywelsch!

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

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

romseygeek · 2022-04-25T09:28:16Z

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

-                    "include_in_all",
-                    "[include_in_all] is deprecated, the _all field have been removed in this version"
-                );
+                if (parserContext.indexVersionCreated().isLegacyIndexVersion() == false) {


Do we need to guard this with a version check? I think it will still be true even for older mappings, in that we won't generate an _all field type that you can search against even if the underlying lucene field exists in the index?

Good spot. In an earlier iteration, I was pondering on adding _all support for legacy indices, hence the version check. I've removed it now.

ywelsch · 2022-04-25T13:13:38Z

@elasticmachine run elasticsearch-ci/part-1 (unrelated failure)

javanna

I left some nits and some questions, no blockers on my end. LGTM, thanks for all the iterations.

javanna · 2022-04-26T07:23:41Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadataVerifier.java

     */
-    private void checkMappingsCompatibility(IndexMetadata indexMetadata) {
+    @Nullable
+    public Mapping checkMappingsCompatibility(IndexMetadata indexMetadata) {


nit: I find it slightly confusing that this check method is now returning the mappings. I can see how it is convenient to return the mappings obtained from the merge operation. Would it make sense renaming the method to adapt it to the updated behaviour?

changed in 07a2602

javanna · 2022-04-26T07:35:51Z

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

+     * Return a map of the non-legacy mappers that have been registered. The
     * returned map uses the type of the field as a key.
     */
    public Map<String, Mapper.TypeParser> getMapperParsers() {


I think that this method is only used in tests now: is that ok or do we need to adapt tests as a follow-up?

I've adapted the tests and removed it in 4c86d3c

javanna · 2022-04-26T07:37:54Z

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

+ * so that the original mapping can be preserved and proper exception messages can
+ * be provided when accessing these fields.
+ */
+public class PlaceHolderFieldMapper extends FieldMapper {


I tend to get confused with naming here, and maybe it is just me: would a different name like UnknownLegacyFieldMapper better represent what this mapper is used for? Shall we make it final also?

I prefer the PlaceHolder terminology, as we might know the field type (e.g. it's a completion field, or a search_as_you_type field, which the current version/implementation knows about), but we just not chose to support for legacy indices. I'm open for changing it if we find a better name, but also happy with the current one.
Regarding "final" modifier, it's more of a personal preference, but I like to avoid adding too many restrictive modifiers in our codebase, as we're not developing a library (unlike Lucene), and it feels unnecessary doing so (putting extra focus on things that should not matter).

javanna · 2022-04-26T07:45:54Z

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

+            this(builderFunction, contextValidator, Version.CURRENT.minimumIndexCompatibilityVersion());
+        }
+
+        public TypeParser(


I believe this one could be made private

Changed in f163412

javanna · 2022-04-26T07:48:48Z

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

+            this(builderFunction, (n, c) -> {}, Version.CURRENT.minimumIndexCompatibilityVersion());
+        }
+
+        public TypeParser(BiFunction<String, MappingParserContext, Builder> builderFunction, Version minimumCompatibilityVersion) {


would it help to add javadocs here to clarify when to use which constructor? as far as I understand, the constructor that takes the version is the one called by all the mappers that are supported on legacy indices, all the rest remains the same?

addressed in f163412

javanna · 2022-04-26T08:19:24Z

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

            validate();
        }

+        protected void handleUnknownParam(String propName, Object propNode) {


should we rename it to something that removes any doubt that this is only called for legacy indices? handleLegacyindexUnknownParam ?

I've renamed the method to handleUnknownParamOnLegacyIndex in f163412

javanna · 2022-04-26T08:24:04Z

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

+
+        @Override
+        public boolean supportsVersion(Version indexCreatedVersion) {
+            return true;


this I find a bit surprising especially given that field aliases were added with 6.4 . I wonder if this is never called, in which case maybe we should throw exception instead, or if it could rely on the default impl instead.

Even though field aliases were only added in ES 6.4, they could be used with older indices as well (i.e. you had a 5.x index for example that you kept using in 6.4 and now added the field alias to the mapping of that 5.x index). As field aliases are more of a runtime property (e.g. just like runtime fields, which you could also use on indices created before the release of runtime fields) and have no connection to the actual data, I see no reason to limit their use.

javanna · 2022-04-26T08:26:31Z

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

+
+        @Override
+        public boolean supportsVersion(Version indexCreatedVersion) {
+            return true;


I wonder if overriding these two supportsVersion is needed. Maybe it does because metadata fields are always supported and they don't take a version in like FieldMapper does?

This was never called, it mainly resulted from the fact that MetadataFieldMapper.TypeParser was extending Mapper.TypeParser (with no good reason). I separated the two in b25369e, which means we no longer have to have this method here.

javanna · 2022-04-26T08:27:08Z

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

+
+        @Override
+        public boolean supportsVersion(Version indexCreatedVersion) {
+            return true;


here too, I wonder what this means. Does this get called, or does this simply mean "objects are always supported regardless of the version"?

The latter (objects are always supported regardless of the version). This gets called indeed, e.g. when you have an object below an object.

ywelsch · 2022-04-26T12:41:20Z

Thank you for the reviews @romseygeek and @javanna!

Placeholder fields

a70bf43

elasticsearchmachine added the v8.2.0 label Mar 17, 2022

ywelsch mentioned this pull request Mar 18, 2022

Snapshots as simple archives #81210

Closed

32 tasks

ywelsch added 9 commits March 22, 2022 14:49

Merge remote-tracking branch 'elastic/master' into placeholder-fields

20fd0c1

o

d66e928

fix compiel

add tests

1523ca1

add tests

2b0395b

more foxes

42e3911

tests

5e1ea96

Merge remote-tracking branch 'elastic/master' into placeholder-fields

114dd7f

Merge remote-tracking branch 'elastic/master' into placeholder-fields

2a33e8e

fix tests

3e579ec

ywelsch added >non-issue :Search Foundations/Mapping Index mappings, including merging and defining field types labels Mar 28, 2022

ywelsch marked this pull request as ready for review March 28, 2022 08:20

elasticmachine added the Team:Search Meta label for search team label Mar 28, 2022

second mapping only supported on 5x

fbd4c4f

ywelsch requested review from javanna and romseygeek March 28, 2022 09:46

salvatore-campagna added v8.3.0 and removed v8.2.0 labels Mar 30, 2022

romseygeek mentioned this pull request Apr 1, 2022

Placeholder fields - alternative impl #85600

Closed

ywelsch added 7 commits April 19, 2022 17:04

Merge remote-tracking branch 'elastic/master' into placeholder-fields

b761fd1

move legacy mapping handling to factory

cdf33dd

Merge remote-tracking branch 'elastic/master' into placeholder-fields

d49e1bb

fix compilation

89e8bfd

wrong class

5f01d4a

rename TypeParser.supportsLegacyField to TypeParser.supportsVersion(V…

6fd1366

…ersion indexCreatedVersion)

move unknownParams downwards

713dbc2

Merge remote-tracking branch 'elastic/master' into placeholder-fields

880fbe6

romseygeek reviewed Apr 21, 2022

View reviewed changes

ywelsch added 3 commits April 25, 2022 10:27

Merge remote-tracking branch 'elastic/master' into placeholder-fields

f75f5d2

place holder tests

ad99173

new default constructor

07234b1

ywelsch requested a review from romseygeek April 25, 2022 09:20

romseygeek approved these changes Apr 25, 2022

View reviewed changes

ywelsch added 2 commits April 25, 2022 13:28

feedback

68408c8

fix test

cb213b1

ywelsch added 2 commits April 25, 2022 15:47

disalbing warnings is hard

2482c26

Merge remote-tracking branch 'elastic/master' into placeholder-fields

c677101

javanna approved these changes Apr 26, 2022

View reviewed changes

ywelsch added 6 commits April 26, 2022 11:32

separate TypeParser interface for Metadata mappers

b25369e

Remove getMapperParsers method

4c86d3c

javadocs / rename

f163412

spotless

0b1157b

rename checkMappingsCompatibility

07a2602

Merge remote-tracking branch 'elastic/master' into placeholder-fields

5b85b55

ywelsch merged commit 4e41c5f into elastic:master Apr 26, 2022

ywelsch mentioned this pull request Apr 29, 2022

Allow field retrieval on placeholder fields #86289

Merged

ywelsch mentioned this pull request May 10, 2022

Add text field support to archive indices #86591

Merged

Conversation

ywelsch commented Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Mar 28, 2022

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywelsch commented Apr 25, 2022

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywelsch commented Apr 25, 2022

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywelsch commented Apr 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ywelsch commented Mar 17, 2022 •

edited

Loading