Skip to content

Commit

Permalink
Revert change (#90674) (#103865) (#103933)
Browse files Browse the repository at this point in the history 
Reverts #90674

The revert is not perfectly clean as there are some minor adjustments to
account for later changes.

This is in contrast with:
#103858

closes: #103011
  • Loading branch information
@benwtrent
benwtrent committed Jan 4, 2024
1 parent cd8e1e6 commit 8b9f084
Show file tree
Hide file tree
Showing 10 changed files with 127 additions and 97 deletions.
5 changes: 5 additions & 0 deletions docs/changelog/103865.yaml
View file
@@ -0,0 +1,5 @@
pr: 103865
summary: Revert change
area: Mapping
type: bug
issues: []
32 changes: 12 additions & 20 deletions server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java
View file
Expand Up @@ -250,8 +250,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 @@ -484,20 +484,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 @@ -519,8 +512,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 @@ -557,13 +550,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 @@ -606,7 +598,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 @@ -615,8 +607,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 @@ -625,7 +616,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
48 changes: 24 additions & 24 deletions server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java
View file
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<>(),
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
26 changes: 21 additions & 5 deletions server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java
View file
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
6 changes: 5 additions & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java
View file
Expand Up @@ -118,7 +118,7 @@ public static 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 @@ -1732,6 +1732,10 @@ public NumberFieldType fieldType() {
return (NumberFieldType) super.fieldType();
}

public NumberType type() {
return type;
}

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

0 comments on commit 8b9f084

Please sign in to comment.