Fix a type check that is always false #27726
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
| @@ -618,16 +618,16 @@ private static void parseNullValue(ParseContext context, ObjectMapper parentMapp | |||
|
|
|||
| private static Mapper.Builder<?,?> createBuilderFromFieldType(final ParseContext context, MappedFieldType fieldType, String currentFieldName) { | |||
| Mapper.Builder builder = null; | |||
| if (fieldType instanceof StringFieldType) { | |||
There was a problem hiding this comment.
Moving my original comment from #27706 over here.
This change also makes sense to me, but I'd like @jpountz or @jimczi to also comment on this because fixing it might change the current behaviour in unexpected ways or there might be other side effects to consider.
It looks like we never reached the TextFieldType-case in the past and I'm wondering under which circumstance we might have wanted to.
There was a problem hiding this comment.
Ping, any chance someone would be able to take a look at this one?
|
I've resolved the conflicts on this one. |
|
@rneatherway thanks a lot, I started looking into this again. The change itself makes total sense but as I mentioned earlier we will need to get a better overview about its effects on currently existing mappings. Unfortunately that hasn't happend yet, thats why this is still in the review queue. |
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
|
@jpountz I think we briefly talked about this one and the fix makes sense, but we were not sure about what this means for backwards compatibility. Maybe we should fix it at least in master? Adding you and //cc @elastic/es-search-aggs in case somebody else has an opinion on this. |
cbuescher
added
:Search Foundations/Mapping
|
This is a good fix indeed. It is due to the fact that we used the same name for a parent class of the |
|
@rneatherway sorry for the long delay here, seems like we're all set. Could you remove the whole "instanceof StringFieldType" check as @jpountz suggested, then we can get this into master and 6.x. |
DocumentParser: The checks for Text and Keyword were masked by the earlier check for String, which they are child classes of. As String field types are no longer supported, this check can be removed.
|
Sure, amended the commit and adjusted the description. |
There was a problem hiding this comment.
@rneatherway thanks a lot, LGTM. Will kick of final tests and merge once they passed.
|
@elasticmachine test this please |
|
@elasticmachine retest this please |
DocumentParser: The checks for Text and Keyword were masked by the earlier check for String, which they are child classes of. As String field types are no longer supported, this check can be removed.
|
@rneatherway its merged now, thanks again for raising this and the other issues, much appreciated. |
|
Glad to get it in, I know it can be a bit unclear with some of these issues exactly what the right change is. I'll make this my final shameless plug for turning on PR integration at https://lgtm.com/projects/g/elastic/elasticsearch/ci/ :-) That way you have a chance to catch these when they are introduced and it is easier to see what the fix is from the changes that are being made at the time. |
DocumentParser: The checks for Text and Keyword were masked by the
earlier check for String, which they are child classes of. I changed
the order to have the more specific checks first so that they are all
now possible.
Separated for more review from #27706