Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert change (#90674) #103865

Merged
merged 8 commits into from
Jan 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/103865.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 103865
summary: Revert change
area: Mapping
type: bug
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -251,8 +251,8 @@ static Mapping createDynamicUpdate(DocumentParserContext context) {
return null;
}
RootObjectMapper.Builder rootBuilder = context.updateRoot();
context.getDynamicMappers()
.forEach((name, builders) -> builders.forEach(builder -> rootBuilder.addDynamic(name, null, builder, context)));
context.getDynamicMappers().forEach(mapper -> rootBuilder.addDynamic(mapper.name(), null, mapper, context));

for (RuntimeField runtimeField : context.getDynamicRuntimeFields()) {
rootBuilder.addRuntimeField(runtimeField);
}
Expand Down Expand Up @@ -485,20 +485,13 @@ private static void parseObjectDynamic(DocumentParserContext context, String cur
// not dynamic, read everything up to end object
context.parser().skipChildren();
} else {
Mapper.Builder dynamicObjectBuilder = null;
Mapper dynamicObjectMapper;
if (context.dynamic() == ObjectMapper.Dynamic.RUNTIME) {
// with dynamic:runtime all leaf fields will be runtime fields unless explicitly mapped,
// hence we don't dynamically create empty objects under properties, but rather carry around an artificial object mapper
dynamicObjectMapper = new NoOpObjectMapper(currentFieldName, context.path().pathAsText(currentFieldName));
} else {
dynamicObjectBuilder = DynamicFieldsBuilder.findTemplateBuilderForObject(context, currentFieldName);
if (dynamicObjectBuilder == null) {
dynamicObjectBuilder = new ObjectMapper.Builder(currentFieldName, ObjectMapper.Defaults.SUBOBJECTS).enabled(
ObjectMapper.Defaults.ENABLED
);
}
dynamicObjectMapper = dynamicObjectBuilder.build(context.createDynamicMapperBuilderContext());
dynamicObjectMapper = DynamicFieldsBuilder.createDynamicObjectMapper(context, currentFieldName);
}
if (context.parent().subobjects() == false) {
if (dynamicObjectMapper instanceof NestedObjectMapper) {
Expand All @@ -520,8 +513,8 @@ private static void parseObjectDynamic(DocumentParserContext context, String cur
}

}
if (context.dynamic() != ObjectMapper.Dynamic.RUNTIME && dynamicObjectBuilder != null) {
context.addDynamicMapper(dynamicObjectMapper.name(), dynamicObjectBuilder);
if (context.dynamic() != ObjectMapper.Dynamic.RUNTIME) {
context.addDynamicMapper(dynamicObjectMapper);
}
if (dynamicObjectMapper instanceof NestedObjectMapper && context.isWithinCopyTo()) {
throwOnCreateDynamicNestedViaCopyTo(dynamicObjectMapper, context);
Expand Down Expand Up @@ -558,13 +551,12 @@ private static void parseArrayDynamic(DocumentParserContext context, String curr
if (context.dynamic() == ObjectMapper.Dynamic.FALSE) {
context.parser().skipChildren();
} else {
Mapper.Builder objectBuilderFromTemplate = DynamicFieldsBuilder.findTemplateBuilderForObject(context, currentFieldName);
if (objectBuilderFromTemplate == null) {
Mapper objectMapperFromTemplate = DynamicFieldsBuilder.createObjectMapperFromTemplate(context, currentFieldName);
if (objectMapperFromTemplate == null) {
parseNonDynamicArray(context, currentFieldName, currentFieldName);
} else {
Mapper objectMapperFromTemplate = objectBuilderFromTemplate.build(context.createDynamicMapperBuilderContext());
if (parsesArrayValue(objectMapperFromTemplate)) {
context.addDynamicMapper(objectMapperFromTemplate.name(), objectBuilderFromTemplate);
context.addDynamicMapper(objectMapperFromTemplate);
context.path().add(currentFieldName);
parseObjectOrField(context, objectMapperFromTemplate);
context.path().remove();
Expand Down Expand Up @@ -607,7 +599,7 @@ private static void postProcessDynamicArrayMapping(DocumentParserContext context
if (context.indexSettings().getIndexVersionCreated().onOrAfter(DYNAMICALLY_MAP_DENSE_VECTORS_INDEX_VERSION)) {
final MapperBuilderContext builderContext = context.createDynamicMapperBuilderContext();
final String fullFieldName = builderContext.buildFullName(fieldName);
final List<Mapper.Builder> mappers = context.getDynamicMappers(fullFieldName);
final List<Mapper> mappers = context.getDynamicMappers(fullFieldName);
if (mappers == null
|| context.isFieldAppliedFromTemplate(fullFieldName)
|| context.isCopyToField(fullFieldName)
Expand All @@ -616,8 +608,7 @@ private static void postProcessDynamicArrayMapping(DocumentParserContext context
// Anything that is NOT a number or anything that IS a number but not mapped to `float` should NOT be mapped to dense_vector
|| mappers.stream()
.anyMatch(
m -> m instanceof NumberFieldMapper.Builder == false
|| ((NumberFieldMapper.Builder) m).type != NumberFieldMapper.NumberType.FLOAT
m -> m instanceof NumberFieldMapper == false || ((NumberFieldMapper) m).type() != NumberFieldMapper.NumberType.FLOAT
)) {
return;
}
Expand All @@ -626,7 +617,8 @@ private static void postProcessDynamicArrayMapping(DocumentParserContext context
fieldName,
context.indexSettings().getIndexVersionCreated()
);
context.updateDynamicMappers(fullFieldName, builder);
DenseVectorFieldMapper denseVectorFieldMapper = builder.build(builderContext);
context.updateDynamicMappers(fullFieldName, List.of(denseVectorFieldMapper));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand Down Expand Up @@ -84,9 +84,9 @@ protected void addDoc(LuceneDocument doc) {
private final MappingParserContext mappingParserContext;
private final SourceToParse sourceToParse;
private final Set<String> ignoredFields;
private final Map<String, List<Mapper.Builder>> dynamicMappers;
private final Map<String, List<Mapper>> dynamicMappers;
private final Set<String> newFieldsSeen;
private final Map<String, ObjectMapper.Builder> dynamicObjectMappers;
private final Map<String, ObjectMapper> dynamicObjectMappers;
private final List<RuntimeField> dynamicRuntimeFields;
private final DocumentDimensions dimensions;
private final ObjectMapper parent;
Expand All @@ -102,9 +102,9 @@ private DocumentParserContext(
MappingParserContext mappingParserContext,
SourceToParse sourceToParse,
Set<String> ignoreFields,
Map<String, List<Mapper.Builder>> dynamicMappers,
Map<String, List<Mapper>> dynamicMappers,
Set<String> newFieldsSeen,
Map<String, ObjectMapper.Builder> dynamicObjectMappers,
Map<String, ObjectMapper> dynamicObjectMappers,
List<RuntimeField> dynamicRuntimeFields,
String id,
Field version,
Expand Down Expand Up @@ -166,9 +166,9 @@ protected DocumentParserContext(
mappingParserContext,
source,
new HashSet<>(),
new LinkedHashMap<>(),
new HashMap<>(),
new HashSet<>(),
new LinkedHashMap<>(),
new HashMap<>(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ it makes sense to go back to the original hashmap, which we had before.

new ArrayList<>(),
null,
null,
Expand Down Expand Up @@ -304,29 +304,29 @@ public boolean isCopyToField(String name) {
/**
* Add a new mapper dynamically created while parsing.
*/
public final void addDynamicMapper(String fullName, Mapper.Builder builder) {
public final void addDynamicMapper(Mapper mapper) {
// eagerly check object depth limit here to avoid stack overflow errors
if (builder instanceof ObjectMapper.Builder) {
MappingLookup.checkObjectDepthLimit(indexSettings().getMappingDepthLimit(), fullName);
if (mapper instanceof ObjectMapper) {
MappingLookup.checkObjectDepthLimit(indexSettings().getMappingDepthLimit(), mapper.name());
}

// eagerly check field name limit here to avoid OOM errors
// only check fields that are not already mapped or tracked in order to avoid hitting field limit too early via double-counting
// note that existing fields can also receive dynamic mapping updates (e.g. constant_keyword to fix the value)
if (mappingLookup.getMapper(fullName) == null
&& mappingLookup.objectMappers().containsKey(fullName) == false
&& newFieldsSeen.add(fullName)) {
if (mappingLookup.getMapper(mapper.name()) == null
&& mappingLookup.objectMappers().containsKey(mapper.name()) == false
&& newFieldsSeen.add(mapper.name())) {
mappingLookup.checkFieldLimit(indexSettings().getMappingTotalFieldsLimit(), newFieldsSeen.size());
}
if (builder instanceof ObjectMapper.Builder objectMapper) {
dynamicObjectMappers.put(fullName, objectMapper);
if (mapper instanceof ObjectMapper objectMapper) {
dynamicObjectMappers.put(objectMapper.name(), objectMapper);
// dynamic object mappers may have been obtained from applying a dynamic template, in which case their definition may contain
// sub-fields as well as sub-objects that need to be added to the mappings
for (Mapper.Builder submapper : objectMapper.subBuilders()) {
for (Mapper submapper : objectMapper.mappers.values()) {
// we could potentially skip the step of adding these to the dynamic mappers, because their parent is already added to
// that list, and what is important is that all of the intermediate objects are added to the dynamic object mappers so that
// they can be looked up once sub-fields need to be added to them. For simplicity, we treat these like any other object
addDynamicMapper(fullName + "." + submapper.name, submapper);
addDynamicMapper(submapper);
}
}

Expand All @@ -336,7 +336,7 @@ public final void addDynamicMapper(String fullName, Mapper.Builder builder) {
// dynamically mapped objects when the incoming document defines no sub-fields in them:
// 1) by default, they would be empty containers in the mappings, is it then important to map them?
// 2) they can be the result of applying a dynamic template which may define sub-fields or set dynamic, enabled or subobjects.
dynamicMappers.computeIfAbsent(fullName, k -> new ArrayList<>()).add(builder);
dynamicMappers.computeIfAbsent(mapper.name(), k -> new ArrayList<>()).add(mapper);
}

/**
Expand All @@ -345,8 +345,8 @@ public final void addDynamicMapper(String fullName, Mapper.Builder builder) {
* Consists of a all {@link Mapper}s that will need to be added to their respective parent {@link ObjectMapper}s in order
* to become part of the resulting dynamic mapping update.
*/
public final Map<String, List<Mapper.Builder>> getDynamicMappers() {
return dynamicMappers;
public final List<Mapper> getDynamicMappers() {
return dynamicMappers.values().stream().flatMap(List::stream).toList();
}

/**
Expand All @@ -355,13 +355,13 @@ public final Map<String, List<Mapper.Builder>> getDynamicMappers() {
* @param fieldName Full field name with dot-notation.
* @return List of Mappers or null
*/
public final List<Mapper.Builder> getDynamicMappers(String fieldName) {
public final List<Mapper> getDynamicMappers(String fieldName) {
return dynamicMappers.get(fieldName);
}

public void updateDynamicMappers(String name, Mapper.Builder mapper) {
public void updateDynamicMappers(String name, List<Mapper> mappers) {
dynamicMappers.remove(name);
dynamicMappers.put(name, List.of(mapper));
mappers.forEach(this::addDynamicMapper);
}

/**
Expand All @@ -371,7 +371,7 @@ public void updateDynamicMappers(String name, Mapper.Builder mapper) {
* Holds a flat set of object mappers, meaning that an object field named <code>foo.bar</code> can be looked up directly with its
* dotted name.
*/
final ObjectMapper.Builder getDynamicObjectMapper(String name) {
final ObjectMapper getDynamicObjectMapper(String name) {
return dynamicObjectMappers.get(name);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,25 @@ void createDynamicFieldFromValue(final DocumentParserContext context, String nam
}
}

/**
* Returns a dynamically created object mapper, eventually based on a matching dynamic template.
*/
static Mapper createDynamicObjectMapper(DocumentParserContext context, String name) {
Mapper mapper = createObjectMapperFromTemplate(context, name);
return mapper != null
? mapper
: new ObjectMapper.Builder(name, ObjectMapper.Defaults.SUBOBJECTS).enabled(ObjectMapper.Defaults.ENABLED)
.build(context.createDynamicMapperBuilderContext());
}

/**
* Returns a dynamically created object mapper, based exclusively on a matching dynamic template, null otherwise.
*/
static Mapper createObjectMapperFromTemplate(DocumentParserContext context, String name) {
Mapper.Builder templateBuilder = findTemplateBuilderForObject(context, name);
return templateBuilder == null ? null : templateBuilder.build(context.createDynamicMapperBuilderContext());
}

/**
* Creates a dynamic string field based on a matching dynamic template.
* No field is created in case there is no matching dynamic template.
Expand Down Expand Up @@ -234,10 +253,7 @@ private static boolean applyMatchingTemplate(
return true;
}

/**
* Returns a dynamically created object builder, based exclusively on a matching dynamic template, null otherwise.
*/
static Mapper.Builder findTemplateBuilderForObject(DocumentParserContext context, String name) {
private static Mapper.Builder findTemplateBuilderForObject(DocumentParserContext context, String name) {
DynamicTemplate.XContentFieldType matchType = DynamicTemplate.XContentFieldType.OBJECT;
DynamicTemplate dynamicTemplate = context.findDynamicTemplate(name, matchType);
if (dynamicTemplate == null) {
Expand Down Expand Up @@ -293,7 +309,7 @@ private static final class Concrete implements Strategy {

void createDynamicField(Mapper.Builder builder, DocumentParserContext context) throws IOException {
Mapper mapper = builder.build(context.createDynamicMapperBuilderContext());
context.addDynamicMapper(mapper.name(), builder);
context.addDynamicMapper(mapper);
parseField.accept(context, mapper);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ public static final class Builder extends FieldMapper.Builder {
private final Parameter<Map<String, String>> meta = Parameter.metaParam();

private final ScriptCompiler scriptCompiler;
public final NumberType type;
private final NumberType type;

private boolean allowMultipleValues = true;
private final IndexVersion indexCreatedVersion;
Expand Down Expand Up @@ -1817,6 +1817,10 @@ public NumberFieldType fieldType() {
return (NumberFieldType) super.fieldType();
}

public NumberType type() {
return type;
}

@Override
protected String contentType() {
return fieldType().type.typeName();
Expand Down
Loading