[DRAFT] Remove builder instance from TextFieldMapper #1
Conversation
Gradle Check (Jenkins) Run Completed with:
|
| @@ -1165,28 +1182,4 @@ public static Query createPhrasePrefixQuery( | |||
| return spanQuery.build(); | |||
| } | |||
|
|
|||
| @Override | |||
| protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException { | |||
There was a problem hiding this comment.
how will toXContent work as you have removed this method?
There was a problem hiding this comment.
was reading through the code, looks like doXContentBody in ParametrizedFieldMapper might be sufficient as it is iterating over all the params. Trying to understand why Builder has been retained in the first place if it is not access from anywhere except when doXContentBody is called which there is no need to override.
There was a problem hiding this comment.
Yes, the method doXContentBody in ParametrizedFieldMapper is iterating over all the params. So the serialized object has all the fields. But when I removed the doXContentBody method from TextFieldMapper the test testBWCSerialization failed. The failure occurred because the sequence of keys in the json string was different.
Expected:
{
"_doc": {
"properties": {
"field": {
"type": "text",
"fields": {
"subfield": {
"type": "long"
}
},
"fielddata": true
}
}
}
}
Actual
{
"_doc": {
"properties": {
"field": {
"type": "text",
"fielddata": true,
"fields": {
"subfield": {
"type": "long"
}
}
}
}
}
}
Normally in a json the order of keys should not matter but the serialized json string changes in this case and there might be some dependencies on this.
So I will add back the doXContentBody method but will call the getMergeBuilder() method to get the builder rather using the builder instance in the TextFieldMapper
There was a problem hiding this comment.
but as per existing comment in the code and this test, can you check the history, why we might have to retain the order. Which BWC is this and how it can fail in prod.
There was a problem hiding this comment.
There is some assertion logic in MapperService where if the index mapping versions are the same then the serialized form of the mappings should also be identical. This is where the order of the fields is important to ensure BWC with old serialization.
Gradle Check (Jenkins) Run Completed with:
|
[DO NOT MERGE]
Description
[Describe what this change achieves]
Issues Resolved
opensearch-project#7001
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.