[Rest Api Compatibility] Ignore use_field_mapping option for docvalue #74435
Conversation
previously removed in elastic#55622 use_field_mapping option can be used on doc value format under rest api compatibility. The value itself is ignored (replaced with null) as it is a default behaviour to use field mapping format.
pgomulka
added
:Search/Search
elasticmachine
added
Team:Search
|
Pinging @elastic/es-search (Team:Search) |
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| @@ -104,7 +104,6 @@ tasks.named("yamlRestCompatTest").configure { | |||
| 'mtermvectors/11_basic_with_types/Basic tests for multi termvector get', | |||
| 'mtermvectors/21_deprecated_with_types/Deprecated camel case and _ parameters should fail in Term Vectors query', | |||
| 'mtermvectors/30_mix_typeless_typeful/mtermvectors without types on an index that has types', | |||
| 'search/10_source_filtering/docvalue_fields with default format', //use_field_mapping change | |||
There was a problem hiding this comment.
the 7.x test asserts about the deprecation warning and the right format (declared on mapping) being used
| return null; | ||
| } else { | ||
| String text = p.text(); | ||
| if (text.equals("use_field_mapping")) { |
There was a problem hiding this comment.
Small comment, could use USE_DEFAULT_FORMAT constant here.
| /** | ||
| * Wrapper around a field name and the format that should be used to | ||
| * display values of this field. | ||
| */ | ||
| public final class FieldAndFormat implements Writeable, ToXContentObject { | ||
| private static final String USE_DEFAULT_FORMAT = "use_field_mapping"; |
There was a problem hiding this comment.
This class is also used by the fields option, in addition to docvalue_fields, which means we're allowing use_field_mapping to be set on 7.x on the fields option, even though it was never allowed before. This is unfortunate, but seems like an okay compromise to me.
There was a problem hiding this comment.
good point, I was not aware of this.. I don't think the parser has the information about the parent of the current key/value..
The only alternative would be to use two different parsers, but I am not sure if this is not an overdo.
Especially because this is meant to be for rest backwards compatibility. It would be surprising to see someone start using "use_field_mapping" with rest api compatibility for the fields when it was not allowed in 7.x..
Also it would not do anything anyway.
There was a problem hiding this comment.
I also feel like this is the right trade-off to keep the code simple.
previously removed in #55622 use_field_mapping option can be used on doc
value format under rest api compatibility.
The value itself is ignored (replaced with null) as it is a default
behaviour to use field mapping format.
gradle check?