Move merge compatibility logic from MappedFieldType to FieldMapper by romseygeek · Pull Request #56915 · elastic/elasticsearch

romseygeek · 2020-05-18T14:36:52Z

Merging logic is currently split between FieldMapper, with its merge() method, and
MappedFieldType, which checks for merging compatibility. The compatibility checks
are called from a third class, MappingMergeValidator. This makes it difficult to reason
about what is or is not compatible in updates, and even what is in fact updateable - we
have a number of tests that check compatibility on changes in mapping configuration
that are not in fact possible.

This commit refactors the compatibility logic so that it all sits on FieldMapper, and
makes it called at merge time. It adds a new FieldMapperTestCase base class that
FieldMapper tests can extend, and moves the compatibility testing machinery from
FieldTypeTestCase to here.

Relates to #56814

elasticmachine · 2020-05-18T14:37:20Z

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

jtibshirani

It's great you're cleaning up FieldMapper! I'm excited about the plan in #56814.

I was wondering if we could simplify the method structure more by combining the compatibility checks and merging together. This seems natural to me -- for each option you validate if it can be changed, and then apply the change if possible. Subclasses would only need to override a single method.

A new call structure would look something like this:
FieldMapper#merge
-> FieldMapper#mergeSharedOptions (handles both compatibility checks and updates field type, copy to, etc.)
-> FieldMapper#mergeOptions (overridden by subclasses to perform both compatibility checks and updates options)

This could be done as a follow-up refactor if we wanted to keep the current PR simple, as a straight reshuffling of methods.

jtibshirani · 2020-05-18T17:28:18Z

test/framework/src/main/java/org/elasticsearch/index/mapper/FieldMapperTestCase.java

+
+    protected abstract T newBuilder();
+
+    private static class BogusFieldType extends MappedFieldType {


It seems like we could reuse the existing test classes MockFieldMapper and FakeFieldType here.

jtibshirani · 2020-05-18T17:35:09Z

test/framework/src/main/java/org/elasticsearch/index/mapper/FieldTypeTestCase.java

-        }
-    }
-
-    public void testFreeze() {


It looks like this test was lost in the refactor. Do we have plans to replace it or make it no longer necessary?

The next step will be to decouple FieldType and MappedFieldType entirely, so there won't need to be a freeze() method here at all.

romseygeek · 2020-05-18T18:33:25Z

A new call structure would look something like this:

I like this a lot, it would make things a lot clearer. Let's do it in a follow-up to keep this PR reasonably contained?

jtibshirani

I left a last round of comments.

Let's do it in a follow-up to keep this PR reasonably contained?

Sounds good to me.

jtibshirani · 2020-05-18T21:58:49Z

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

-        }
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () ->
+            mapperService.merge("type", new CompressedXContent(Strings.toString(update)), MapperService.MergeReason.MAPPING_UPDATE));
+        assertThat(e.getMessage(), containsString("mapper [foo] of different type, current_type [long], merged_type [double]"));


To me the error message "mapper [foo] cannot be changed from type [long] to [double]" is a lot clearer, we could prefer that original wording.

On a related note, it looks like MappedFieldType#checkTypeName is now unused and can be removed.

jtibshirani · 2020-05-18T22:06:17Z

test/framework/src/main/java/org/elasticsearch/index/mapper/FieldTypeTestCase.java

-            ft1 = createNamedDefaultFieldType();
-            ft2 = createNamedDefaultFieldType();
-            modifier.modify(ft2);
-            assertFieldTypeNotEquals(modifier.property, ft1, ft2);


I just noticed we've lost test coverage for the equals method, which is used in FieldTypeLookup. This is too bad, I wonder if we could we keep these modifiers around or switch to using EqualsHashCodeTestUtils#checkEqualsAndHashCode.

With luck we'll be able to simplify a lot of the MapperService lookup stuff once things have been rearranged, but for now I agree we still need these tests. I've modified FieldTypeTestCase to allow testing of subtypes with modifiers, using EqualsHashCodeTestUtils.

jtibshirani · 2020-05-18T22:37:12Z

test/framework/src/main/java/org/elasticsearch/index/mapper/FieldMapperTestCase.java

            FieldMapper mapper = (FieldMapper) newBuilder().build(context);
-            FieldMapper toMerge = new FieldMapper("bogus", new BogusFieldType(), new BogusFieldType(),
-                SETTINGS, FieldMapper.MultiFields.empty(), FieldMapper.CopyTo.empty()) {
+            FieldMapper toMerge = new FieldMapper("bogus", new MockFieldMapper.FakeFieldType(),


I think this could just be FIeldMapper toMerge = new MockFieldMapper("bogus") ?

It could, thanks!

romseygeek · 2020-05-19T13:41:58Z

A new call structure would look something like this:

This turned out to make the final patch slightly smaller and easier to review, so I added it in. Thanks for the suggestion @jtibshirani !

jtibshirani

I left some comments about field type tests that were deleted. I think they need to be restored, but once that's done it looks ready to merge.

jtibshirani · 2020-05-19T16:32:31Z

...t/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeWithDocValuesFieldTypeTests.java

-import org.elasticsearch.index.mapper.MappedFieldType;
-import org.junit.Before;
-
-public class GeoShapeWithDocValuesFieldTypeTests extends FieldTypeTestCase {


Should this test have been deleted?

It should not! Have restored it.

jtibshirani · 2020-05-19T16:34:10Z

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

+    /**
+     * Merge type-specific options and check for incompatible settings in mappings to be merged
+     */
+    protected abstract void mergeOptions(FieldMapper other, List<String> conflicts);


Great that this is abstract, I like how it encourages implementors to think through what can be updated vs. not.

jtibshirani · 2020-05-19T16:39:07Z

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

+            throw new IllegalArgumentException("mapper [" + fieldType.name() + "] cannot be changed from type ["
+            + contentType() + "] to [" + mergeWith.getClass().getSimpleName() + "]");
+        }
+        merged.mergeSharedOptions((FieldMapper)mergeWith, conflicts);


Small comment, calling mergeOptions directly from here could be nice:

FieldMapper fieldMapper = (FieldMapper) mergeWith; merged.mergeSharedOptions(fieldMapper, conflicts); merged.mergeOptions(fieldMapper, conflicts);

++, I've rearranged things here.

jtibshirani · 2020-05-19T16:40:21Z

...es/mapper-extras/src/test/java/org/elasticsearch/index/mapper/ScaledFloatFieldTypeTests.java

    }

-    @Before
-    public void setupProperties() {


I think we dropped this modifier accidentally.

scaling_factor can't be updated, so we'd never be in a position when FieldTypeLookup would be comparing and old and new types with different scaling factors.

jtibshirani · 2020-05-19T16:41:40Z

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

-
-import java.util.Arrays;
-
-public class CompletionFieldTypeTests extends FieldTypeTestCase {


Should this have been deleted?

Again, none of these values can be updated in a merge, so we don't need an equality check.

…56915) Merging logic is currently split between FieldMapper, with its merge() method, and MappedFieldType, which checks for merging compatibility. The compatibility checks are called from a third class, MappingMergeValidator. This makes it difficult to reason about what is or is not compatible in updates, and even what is in fact updateable - we have a number of tests that check compatibility on changes in mapping configuration that are not in fact possible. This commit refactors the compatibility logic so that it all sits on FieldMapper, and makes it called at merge time. It adds a new FieldMapperTestCase base class that FieldMapper tests can extend, and moves the compatibility testing machinery from FieldTypeTestCase to here. Relates to #56814

I think this is a left-over from #56915 where a change in assertion message didn't make it to this very rare-case assertion.

…57088) I think this is a left-over from #56915 where a change in assertion message didn't make it to this very rare-case assertion.

Move merge compatibility logic from MappedFieldType to FieldMapper

a49d77c

romseygeek self-assigned this May 18, 2020

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

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

romseygeek added the >breaking-java label May 18, 2020

romseygeek added 5 commits May 18, 2020 15:56

checkstyle

a1e8418

Exception types in parent-join

941c15b

icu error message

41e79d5

better icu_collation_keyword update tests

b7da9e8

compare types using contentType, not class

0b0852b

jtibshirani reviewed May 18, 2020

View reviewed changes

test; feedback

b59cedc

jtibshirani reviewed May 18, 2020

View reviewed changes

romseygeek added 4 commits May 19, 2020 12:33

Equas/hashcode tests for MappedFieldType

eae93a9

mapping message

4ce12a6

test; jobmanager pattern match

e03d4e0

Simplify merging methods

e9fb0d5

romseygeek added 3 commits May 19, 2020 14:59

rawtypes

ed92cd1

Merge remote-tracking branch 'origin/master' into mapper/merge-checks

a9cf7d5

oops

ee7c14a

jtibshirani approved these changes May 19, 2020

View reviewed changes

romseygeek added 3 commits May 19, 2020 18:25

feedback

7e58b1a

Do sub-mapper checks before replacing fieldtype

056482a

sigh

5a1770a

Merge remote-tracking branch 'origin/master' into mapper/merge-checks

53dd899

romseygeek merged commit f82d74b into elastic:master May 20, 2020

romseygeek deleted the mapper/merge-checks branch May 20, 2020 08:32

original-brownbear mentioned this pull request May 23, 2020

Fix Test Failure from Incorrect Mapping Conflict Assertion #57085

Merged

original-brownbear added a commit that referenced this pull request May 23, 2020

Fix Test Failure from Incorrect Mapping Conflict Assertion (#57085)

05e2743

I think this is a left-over from #56915 where a change in assertion message didn't make it to this very rare-case assertion.

original-brownbear mentioned this pull request May 23, 2020

Fix Test Failure from Incorrect Mapping Conflict Assertion (#57085) #57088

Merged

romseygeek mentioned this pull request May 26, 2020

Rework FieldMapper and MappedFieldType #56814

Closed

10 tasks

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


		protected abstract T newBuilder();

		private static class BogusFieldType extends MappedFieldType {


		import java.util.Arrays;

		public class CompletionFieldTypeTests extends FieldTypeTestCase {

Conversation

romseygeek commented May 18, 2020

Uh oh!

elasticmachine commented May 18, 2020

Uh oh!

jtibshirani 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!

romseygeek commented May 18, 2020

Uh oh!

jtibshirani 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!

romseygeek commented May 19, 2020

Uh oh!

jtibshirani 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants