Skip to content
Closed
Show file tree
Hide file tree
Changes from 16 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
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;

public class IndexSortIT extends ESIntegTestCase {
private static final XContentBuilder TEST_MAPPING = createTestMapping();
Expand All @@ -28,6 +29,9 @@ private static XContentBuilder createTestMapping() {
try {
return jsonBuilder()
.startObject()
.startObject("runtime")
.startObject("alias").field("type", "alias").field("path", "numeric").endObject()
.endObject()
.startObject("properties")
.startObject("date")
.field("type", "date")
Expand Down Expand Up @@ -79,6 +83,18 @@ public void testIndexSort() {
flushAndRefresh();
ensureYellow();
assertSortedSegments("test", indexSort);

Exception e = expectThrows(Exception.class, () ->
prepareCreate("test_with_runtime_sort")
.setSettings(Settings.builder()
.put(indexSettings())
.put("index.number_of_shards", "1")
.put("index.number_of_replicas", "1")
.putList("index.sort.field", "alias")
)
.setMapping(TEST_MAPPING)
.get());
assertThat(e.getMessage(), equalTo("Cannot use alias [alias] as an index sort field"));
}

public void testInvalidIndexSort() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ abstract class AbstractScriptFieldType<LeafFactory> extends MappedFieldType impl
}

@Override
public final MappedFieldType asMappedFieldType() {
public final MappedFieldType asMappedFieldType(Function<String, MappedFieldType> lookup) {
return this;
}

Expand Down Expand Up @@ -257,8 +257,4 @@ private static Script parseScript(String name, Mapper.TypeParser.ParserContext p
return script;
}
}

static <T> Function<FieldMapper, T> initializerNotSupported() {
return mapper -> { throw new UnsupportedOperationException(); };
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

package org.elasticsearch.index.mapper;

import org.elasticsearch.common.xcontent.XContentBuilder;

import java.io.IOException;
import java.util.List;
import java.util.Objects;
import java.util.function.Function;

public class AliasRuntimeField implements RuntimeField {

public static final String CONTENT_TYPE = "alias";

private static class Builder extends RuntimeField.Builder {

final FieldMapper.Parameter<String> path = FieldMapper.Parameter.stringParam(
"path",
true,
initializerNotSupported(),
null
).setValidator(
s -> {
if (s == null) {
throw new MapperParsingException("Missing required parameter [path]");
}
}
);

protected Builder(String name) {
super(name);
}

@Override
protected List<FieldMapper.Parameter<?>> getParameters() {
return List.of(path);
}

@Override
protected RuntimeField createRuntimeField(Mapper.TypeParser.ParserContext parserContext) {
return new AliasRuntimeField(name, path.get());
}
}

public static final RuntimeField.Parser PARSER = new RuntimeField.Parser(Builder::new);

private final String name;
private final String path;

public AliasRuntimeField(String name, String path) {
this.name = name;
this.path = path;
if (Objects.equals(name, path)) {
throw new MapperParsingException("Invalid path [" + path + "] for alias [" + path + "]: an alias cannot refer to itself");
}
}

@Override
public void doXContentBody(XContentBuilder builder, Params params) throws IOException {
builder.field("path", path);
}

@Override
public MappedFieldType asMappedFieldType(Function<String, MappedFieldType> lookup) {
MappedFieldType ft = lookup.apply(path);
if (ft == null) {
throw new MapperParsingException("Cannot resolve alias [" + name + "]: path [" + path + "] does not exist in mappings");
}
return ft;
}

@Override
public String name() {
return name;
}

@Override
public String typeName() {
return CONTENT_TYPE;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -875,7 +875,7 @@ private static Mapper getLeafMapper(final ParseContext context,
String fieldPath = context.path().pathAsText(fieldName);
RuntimeField runtimeField = context.root().getRuntimeField(fieldPath);
if (runtimeField != null) {
return new NoOpFieldMapper(subfields[subfields.length - 1], runtimeField.asMappedFieldType().name());
return new NoOpFieldMapper(subfields[subfields.length - 1], runtimeField.name());
}
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,21 +68,29 @@ final class FieldTypeLookup {
}
this.maxParentPathDots = maxParentPathDots;

Map<String, MappedFieldType> resolvedRuntimeMappers = new HashMap<>();
Map<String, DynamicFieldType> resolvedDynamicMappers = new HashMap<>();
for (FieldAliasMapper fieldAliasMapper : fieldAliasMappers) {
String aliasName = fieldAliasMapper.name();
String path = fieldAliasMapper.path();
MappedFieldType fieldType = fullNameToFieldType.get(path);
fullNameToFieldType.put(aliasName, fieldType);
if (fieldType instanceof DynamicFieldType) {
dynamicFieldTypes.put(aliasName, (DynamicFieldType) fieldType);
RuntimeField aliasField = new AliasRuntimeField(aliasName, path);
MappedFieldType resolved = aliasField.asMappedFieldType(fullNameToFieldType::get);
Copy link
Member

Choose a reason for hiding this comment

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

This means that the internal representation of an alias field as it existed so far is the same as the runtime alias field, but this has no effect on how it gets serialized and sent out throughout the nodes and stored in the cluster state, correct?

resolvedRuntimeMappers.put(aliasName, resolved);
if (resolved instanceof DynamicFieldType) {
resolvedDynamicMappers.put(aliasName, (DynamicFieldType) resolved);
}
}

for (RuntimeField runtimeField : runtimeFields) {
MappedFieldType runtimeFieldType = runtimeField.asMappedFieldType();
//this will override concrete fields with runtime fields that have the same name
Copy link
Member

Choose a reason for hiding this comment

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

did you remove this comment on purpose?

fullNameToFieldType.put(runtimeFieldType.name(), runtimeFieldType);
MappedFieldType resolved = runtimeField.asMappedFieldType(fullNameToFieldType::get);
Copy link
Member

Choose a reason for hiding this comment

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

thinking out loud: if we use asMappedFieldType only for aliases, given that all existing runtime fields return this, should we consider adding the notion of alias to RuntimeField through a method that returns a String and return by default the same as the field name, and when it differs look it up directly in FieldTypeLookup? Maybe it is cryptic, not too sure, but it would make it more specific to aliases (which is both good and bad I guess), would remove the need to delegate to the runtime field to look up the aliased field type. I do wonder if it should be the responsibility of the field type to look up the aliases field among all fields. We may or may not need something more generic at some point in the future but we can always refactor things later when needed?

Copy link
Member

Choose a reason for hiding this comment

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

one more thing: fullNameToFieldType does not include the previously resolved alias field mapper, so we are sure that an alias runtime field never points to an alias field defined under properties?

resolvedRuntimeMappers.put(runtimeField.name(), resolved);
if (resolved instanceof DynamicFieldType) {
resolvedDynamicMappers.put(runtimeField.name(), (DynamicFieldType) resolved);
}
}

this.fullNameToFieldType.putAll(resolvedRuntimeMappers);
this.dynamicFieldTypes.putAll(resolvedDynamicMappers);
}

private static int dotCount(String path) {
Expand All @@ -103,6 +111,9 @@ MappedFieldType get(String field) {
if (fieldType != null) {
return fieldType;
}

// If the mapping contains fields that support dynamic sub-key lookup, check
// if this could correspond to a keyed field of the form 'path_to_field.path_to_key'.
return getDynamicField(field);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ default XContentBuilder toXContent(XContentBuilder builder, Params params) throw
* Exposes the {@link MappedFieldType} backing this runtime field, used to execute queries, run aggs etc.
* @return the {@link MappedFieldType} backing this runtime field
*/
MappedFieldType asMappedFieldType();
MappedFieldType asMappedFieldType(Function<String, MappedFieldType> lookup);
Copy link
Contributor

Choose a reason for hiding this comment

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

For my knowledge, will only runtime aliases override this method? Or do we have other plans to use it?

Also small comment, we could add javadoc for lookup.

Copy link
Member

Choose a reason for hiding this comment

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

Back when I added the method, I thought we were going to use it for the "object" runtime field, possibly changing the signature to return a list of field types. It turns out I won't need it though, as we went for using the DynamicFieldType interface instead. What happens instead is that I don't know what to return here for the object runtime field, because it does not have a corresponding mapped field type, I think. I was looking at this method too wondering whether it still makes sense. I admit I need to dive deeper on this PR to form a clearer opinion.

Copy link
Member

Choose a reason for hiding this comment

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

I am not super happy about this method. I added it as I thought we needed it for the object runtime field, but it turns out we won't use it and we have so far all implementors returning this which is not useful. This change of signature makes it a bit harder to follow, as it tries to do something specific to aliases (looking up the field type) in a generic way, by introducing a function argument, and this is to prevent a field type from depending on FieldTypeLookup directly, I assume.

Question: do we need the lookup to be dynamic or could it be done at creation time? We still rebuild all mappings at every mapping update don't we?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not super happy about this method. I added it as I thought we needed it for the object runtime field, but it turns out we won't use it...

After thinking about this more, having some method like this makes sense to me in general. It seems like RuntimeField is a counterpart to FieldMapper in that it contains a mapping definition and is able to produce a MappedFieldType. I wonder if we could rename the method to fieldType to better fit with FieldMapper?

As for the function argument, so far I don't see a cleaner approach. To me it's not too strange that runtime fields have access to the concrete field types here. @romseygeek pointed out that we've brainstormed new types of runtime fields that refer to concrete fields -- such as one that combines the contents of multiple text fields like a virtual copy_to.


/**
* For runtime fields the {@link RuntimeField.Parser} returns directly the {@link MappedFieldType}.
Expand Down Expand Up @@ -103,6 +103,10 @@ private void validate() {
throw new IllegalArgumentException("runtime field [" + name + "] does not support [copy_to]");
}
}

protected static <T> Function<FieldMapper, T> initializerNotSupported() {
return mapper -> { throw new UnsupportedOperationException(); };
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -394,8 +394,10 @@ public boolean isFieldMapped(String name) {
}

private MappedFieldType fieldType(String name) {
MappedFieldType fieldType = runtimeMappings.get(name);
return fieldType == null ? mappingLookup.getFieldType(name) : fieldType;
if (runtimeMappings.containsKey(name)) {
return runtimeMappings.get(name);
}
return mappingLookup.getFieldType(name);
Copy link
Member

Choose a reason for hiding this comment

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

do we gain anything with this change?

}

public ObjectMapper getObjectMapper(String name) {
Expand Down Expand Up @@ -664,14 +666,15 @@ private static Map<String, MappedFieldType> parseRuntimeMappings(Map<String, Obj
if (runtimeMappings.isEmpty()) {
return Collections.emptyMap();
}
Map<String, RuntimeField> runtimeFields = RuntimeField.parseRuntimeFields(new HashMap<>(runtimeMappings),
mapperService.parserContext(), false);
Map<String, MappedFieldType> runtimeFieldTypes = new HashMap<>();
Map<String, RuntimeField> runtimeFields
= RuntimeField.parseRuntimeFields(new HashMap<>(runtimeMappings), mapperService.parserContext(), false);
MappingLookup lookup = mapperService.mappingLookup();
Map<String, MappedFieldType> resolvedFields = new HashMap<>();

for (RuntimeField runtimeField : runtimeFields.values()) {
MappedFieldType fieldType = runtimeField.asMappedFieldType();
runtimeFieldTypes.put(fieldType.name(), fieldType);
resolvedFields.put(runtimeField.name(), runtimeField.asMappedFieldType(lookup::getFieldType));
}
return Collections.unmodifiableMap(runtimeFieldTypes);
return resolvedFields;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.elasticsearch.common.inject.AbstractModule;
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.index.mapper.AliasRuntimeField;
import org.elasticsearch.index.mapper.BinaryFieldMapper;
import org.elasticsearch.index.mapper.BooleanFieldMapper;
import org.elasticsearch.index.mapper.CompletionFieldMapper;
Expand Down Expand Up @@ -149,6 +150,7 @@ private static Map<String, RuntimeField.Parser> getRuntimeFields(List<MapperPlug
runtimeParsers.put(DateFieldMapper.CONTENT_TYPE, DateScriptFieldType.PARSER);
runtimeParsers.put(KeywordFieldMapper.CONTENT_TYPE, KeywordScriptFieldType.PARSER);
runtimeParsers.put(GeoPointFieldMapper.CONTENT_TYPE, GeoPointScriptFieldType.PARSER);
runtimeParsers.put(AliasRuntimeField.CONTENT_TYPE, AliasRuntimeField.PARSER);

for (MapperPlugin mapperPlugin : mapperPlugins) {
for (Map.Entry<String, RuntimeField.Parser> entry : mapperPlugin.getRuntimeFields().entrySet()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,26 +62,20 @@ public void testAliasThatRefersToAlias() {
FieldAliasMapper alias = new FieldAliasMapper("alias", "alias", "field");
FieldAliasMapper invalidAlias = new FieldAliasMapper("invalid-alias", "invalid-alias", "alias");

MappingLookup mappers = createMappingLookup(
Exception e = expectThrows(MapperParsingException.class, () -> createMappingLookup(
singletonList(field),
emptyList(),
Arrays.asList(alias, invalidAlias),
emptyList()
);
alias.validate(mappers);
));

MapperParsingException e = expectThrows(MapperParsingException.class, () -> {
invalidAlias.validate(mappers);
});

assertEquals("Invalid [path] value [alias] for field alias [invalid-alias]: an alias" +
" cannot refer to another alias.", e.getMessage());
assertEquals("Cannot resolve alias [invalid-alias]: path [alias] does not exist in mappings", e.getMessage());
}

public void testAliasThatRefersToItself() {
FieldAliasMapper invalidAlias = new FieldAliasMapper("invalid-alias", "invalid-alias", "invalid-alias");

MapperParsingException e = expectThrows(MapperParsingException.class, () -> {
Exception e = expectThrows(MapperParsingException.class, () -> {
MappingLookup mappers = createMappingLookup(
emptyList(),
emptyList(),
Expand All @@ -91,8 +85,8 @@ public void testAliasThatRefersToItself() {
invalidAlias.validate(mappers);
});

assertEquals("Invalid [path] value [invalid-alias] for field alias [invalid-alias]: an alias" +
" cannot refer to itself.", e.getMessage());
assertEquals("Invalid path [invalid-alias] for alias [invalid-alias]: an alias" +
" cannot refer to itself", e.getMessage());
}

public void testAliasWithNonExistentPath() {
Expand All @@ -108,8 +102,7 @@ public void testAliasWithNonExistentPath() {
invalidAlias.validate(mappers);
});

assertEquals("Invalid [path] value [non-existent] for field alias [invalid-alias]: an alias" +
" must refer to an existing field in the mappings.", e.getMessage());
assertEquals("Cannot resolve alias [invalid-alias]: path [non-existent] does not exist in mappings", e.getMessage());
}

public void testFieldAliasWithNestedScope() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

package org.elasticsearch.index.mapper;

import org.elasticsearch.index.mapper.flattened.FlattenedFieldMapper;

import java.io.IOException;

public class RuntimeAliasTests extends MapperServiceTestCase {

public void testSimpleAlias() throws IOException {
MapperService mapperService = createMapperService(topMapping(b -> {
b.startObject("runtime");
{
b.startObject("alias-to-field").field("type", "alias").field("path", "field").endObject();
}
b.endObject();
b.startObject("properties");
{
b.startObject("field").field("type", "keyword").endObject();
}
b.endObject();
}));

MappedFieldType aliased = mapperService.mappingLookup().getFieldType("alias-to-field");
assertEquals("field", aliased.name());
assertEquals(KeywordFieldMapper.KeywordFieldType.class, aliased.getClass());
}

public void testInvalidAlias() {
Exception e = expectThrows(MapperParsingException.class, () -> createMapperService(topMapping(b -> {
b.startObject("runtime");
{
b.startObject("alias-to-field").field("type", "alias").field("path", "field").endObject();
}
b.endObject();
})));
assertEquals("Cannot resolve alias [alias-to-field]: path [field] does not exist in mappings", e.getMessage());
}

public void testDynamicLookup() throws IOException {
MapperService mapperService = createMapperService(topMapping(b -> {
b.startObject("runtime");
{
b.startObject("dynamic-alias").field("type", "alias").field("path", "flattened").endObject();
}
b.endObject();
b.startObject("properties");
{
b.startObject("flattened").field("type", "flattened").endObject();
}
b.endObject();
}));

MappedFieldType dynamic = mapperService.fieldType("flattened.key");
Copy link
Contributor

Choose a reason for hiding this comment

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

Small comment, we could check the subfield paths too (KeyedFlattenedFieldType#key)?

assertEquals("flattened._keyed", dynamic.name());
MappedFieldType aliased = mapperService.fieldType("dynamic-alias.key");
assertNotNull(aliased);
assertEquals("flattened._keyed", aliased.name());
FlattenedFieldMapper.KeyedFlattenedFieldType keyed = (FlattenedFieldMapper.KeyedFlattenedFieldType) aliased;
assertEquals("key", keyed.key());
}

}
Loading