Add assertion that all field mapper names are interned #87238
Conversation
Follow up to elastic#86301. Assert that all field names are interned so we don't lose this optimization and fix `FieldAliasMapper.name` for it not to trip.
original-brownbear
added
>non-issue
:Search Foundations/Mapping
|
Pinging @elastic/es-search (Team:Search) |
| assert mapper.simpleName() == mapper.simpleName().intern(); | ||
| if (mapper instanceof ObjectMapper) { | ||
| ((ObjectMapper) mapper).mappers.forEach(MappingLookup::assertNamesInterned); | ||
| } |
There was a problem hiding this comment.
thanks a lot for working on this. Maybe silly question, but could this be instead a new test method added to MapperTestCase instead of an assertion?
There was a problem hiding this comment.
I figured that a test will always only cover a portion of our parsing (and wouldn't have caught the alias field case fixed here for example) while an assertion gives us full coverage?
There was a problem hiding this comment.
@romseygeek do you have opinions here? Do you mind giving this a quick look?
There was a problem hiding this comment.
I think an assertion is fine, all of the MapperTestCase tests will build a MappingLookup so this assertion will trip there as well.
| assert mapper.simpleName() == mapper.simpleName().intern(); | ||
| if (mapper instanceof ObjectMapper) { | ||
| ((ObjectMapper) mapper).mappers.forEach(MappingLookup::assertNamesInterned); | ||
| } |
There was a problem hiding this comment.
thanks a lot for working on this. Maybe silly question, but could this be instead a new test method added to MapperTestCase instead of an assertion?
|
Thanks Luca! |
Follow up to #86301. Assert that all field names are interned so we don't
lose this optimization and fix
FieldAliasMapper.namefor it not to trip.