Remove type handling in LeafFieldsLookup#64041
Conversation
|
Pinging @elastic/es-search (:Search/Mapping) |
| final DocumentMapper mapper = mapperService.documentMapper(); | ||
| if (mapper != null) { | ||
| values.add(mapper.type()); | ||
| } |
There was a problem hiding this comment.
Is this what you meant @romseygeek ? Are we sure that we can drop this? Looks like a breaking change to me, wondering if it's worth leaving it around although it is constant, not sure.
There was a problem hiding this comment.
It is a breaking change, but I think it comes under the general umbrella of types removal so probably doesn't need a specific call-out. If you try and use _type in a script in 7x you get a deprecation warning so I think it's fine to remove it.
There was a problem hiding this comment.
the problem here may be that the field type does come up by name, but then it fails to retrieve values from lucene, or is that ok?
There was a problem hiding this comment.
I see what you mean. Maybe we should be removing the special-casing in MapperService.fieldType() in master as well, given that it's deprecated generally in 7.x. @jtibshirani what do you think?
There was a problem hiding this comment.
I just caught up on the code and was surprised to see we still have TypeFieldType in master. Is there a reason we added this special logic instead of removing TypeFieldType entirely, which would disallow all search operations on _type?
There was a problem hiding this comment.
I think I added this as a stop-gap while backporting the various type field deprecations. I'll open a PR to remove it entirely in master.
There was a problem hiding this comment.
One question I had -- does removing this have an implication for our API version compatibility efforts? I'm not up-to-date on our collaborations there.
There was a problem hiding this comment.
Version compatibility only applies to the 'shape' of the request, as I understand it. So we need to support type queries, but we don't need to support queries against the field _type (otherwise we'd never be able to deprecate types at all...)
There was a problem hiding this comment.
thanks for the input. It looks like this can go in then, and as a follow-up we will also remove TypeFieldType completely, which sounds like a good plan to me.
Access to type in scripts is deprecated in 7.x, so we don't need to support it in master(8.x).