Several issues around the flat object type

[bugmakerrrrrr]

Is your feature request related to a problem? Please describe

In #6507, we add the flat object type. In current implementation, we use two stages to process the flat_object field in the document. First we use the JsonToStringXContentParser to collect all the keys and values (keyList, valueList and valueAndPathList) in the field and convert to XContentParser for return. The Lucene fields are then constructed by parsing the fields in the XContentParser.

OpenSearch/server/src/main/java/org/opensearch/common/xcontent/JsonToStringXContentParser.java

Lines 80 to 85 in d6bda7d

	builder.field(this.fieldTypeName, new HashSet<>(keyList));
	builder.field(this.fieldTypeName + VALUE_SUFFIX, new HashSet<>(valueList));
	builder.field(this.fieldTypeName + VALUE_AND_PATH_SUFFIX, new HashSet<>(valueAndPathList));
	builder.endObject();
	String jString = XContentHelper.convertToJson(BytesReference.bytes(builder), false, MediaTypeRegistry.JSON);
	return JsonXContent.jsonXContent.createParser(this.xContentRegistry, this.deprecationHandler, String.valueOf(jString));

For a field of flat_object type in a document, the following internal fields will be created by default:

• root StringField and SortedSetDocValuesField for each subfield key(prefiexed by root field name);
• value StringField and SortedSetDocValuesField for each subfield value;
• valueAndPath StringField and SortedSetDocValuesField for each subfield;
• _field_name StringField for each value and valueAndPath.

PUT test
{
  "mappings": {
    "properties": {
      "field1": {
        "properties": {
          "field2": {
            "type": "flat_object"
          }
        }
      }
    }
  }
}

PUT test/_bulk
{"index": {}}
{"field1": {"field2": {"a": "1", "b": "2"}}}

For example, the request above generates the fields listed below.

There are several issues around the flat object field.

1. If a subfield in the flat_object field suffixed by VALUE_SUFFIX (._value) or VALUE_AND_PATH_SUFFIX (._valueAndPath), some extra unexpected field may be created.

OpenSearch/server/src/main/java/org/opensearch/index/mapper/FlatObjectFieldMapper.java

Lines 660 to 669 in d6bda7d

	if (valueType.equals(VALUE_SUFFIX)) {
	if (valueFieldMapper != null) {
	valueFieldMapper.addField(context, value);
	}
	}
	if (valueType.equals(VALUE_AND_PATH_SUFFIX)) {
	if (valueAndPathFieldMapper != null) {
	valueAndPathFieldMapper.addField(context, value);
	}
	}

2. We use '=' to concat subfield key and value, if a subfield key contains '=', the prefix query may return wrong results.

3. The root fields is confusing and unnecessary. AFAIK, the root field is use to execute exist query and build fielddata, but it doesn't be generated correctly. For example, if we have document {"field1": {"field2": {"field3": {"a": "1", "b": "2"}}}}, and field2 is flat_object field. After processed, the root fields contains values field1.field2.a, field1.field2.b and field1.field2.field3. The exist query of field1.field2.field3.a doesn't return correct result. On the other hand, I don't know is there any meaning to aggregate or sort on the subfield keys. In fact, I don't think that we need to support aggs on flat_object field, it's a object, not a scalar value. If we do need, then we should aggregate on the subfield values, not the subfield keys. Of course, it still makes sense to aggregate subfields, we can utilize the valueAndPath field to support this.

4. Creating _field_name field for value and valueAndPath is meaningless. The _field_name field is used by exist query, we just need to create it for each full leaf path of subfield.

5. The value of SortedSetDocValuesField of value and valueAndPath has unnecessary prefix. When create SortedSetDocValuesField, we use root field name as the prefix of value.

6. Two-stage processing is unnecessary. In the process of converting to JSON strings, we use a lot of additional resources, this is really unnecessary, we can add the corresponding field to parse context during the process.

Describe the solution you'd like

1. Use one-stage processing;
2. Remove the FlatObjecField;
3. Support the aggs on the subfield, but not the root field, which means the fielddata is not supported on the root field but the subfield;
4. For the indices created after 2.18.0, remove the prefix of the value of SortedSetDocValuesField.

In addition, I have no good idea to fix the issue 2, any suggestions about this or the overall issue are welcome.

Related component

Search

Describe alternatives you've considered

No response

Additional context

No response

[bugmakerrrrrr]

@msfroh @kkewwei you might be interested in this :)

[kkewwei]

@msfroh @kkewwei you might be interested in this :)

@bugmakerrrrrr Most of the optimizations make sense to me. @msfroh , how do you think?

I also agree with the fifth point, and mentioned in the pr #14383 (comment)

[msfroh]

2. We use '=' to concat subfield key and value, if a subfield key contains '=', the prefix query may return wrong results.

Note that if we want to change the delimiter, we need to be careful about backward compatibility.

I believe we can tell what version of OpenSearch was used to create an index. Maybe we can continue to use the old behavior on indices created before the change, while supporting the new behavior only indices created on newer versions.

[msfroh]

@bugmakerrrrrr Having read your PR, I really like your improvements and suggestions. Combined with @kkewwei's work in #14383, I think it might be worth it to "fork" the existing flat object code to allow us to make the backward-incompatible changes only on new indices.

Some of your improvements (like getting rid of JsonToStringXParser) can be done in a backwards-compatible way. So, maybe we isolate those changes. Other changes, like removing unnecessary prefixes, using the field names field, not using = as a delimiter, etc. could be moved into a new class and we could mark the old class as deprecated. I believe that we only guarantee backward compatibility from the last 2.x release to 3.0, so the deprecated implementation could be removed in 3.0.

[bugmakerrrrrr]

@msfroh make sense to me. I'll create a new PR to make some bwc improvements firstly.