From 572cd125c940e2a8813122b48596a84be4a5cc64 Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Wed, 29 Apr 2020 13:03:57 -0700 Subject: [PATCH] Resolve field aliases and multi-fields. (#55889) This commit adds the capability to `FieldTypeLookup` to retrieve a field's paths in the _source. When retrieving a field's values, we consult these source paths to make sure we load the relevant values. This allows us to handle requests for field aliases and multi-fields. We also retrieve values that were copied into the field through copy_to. To me this is what users would expect out of the API, and it's consistent with what comes back from `docvalues_fields` and `stored_fields`. However it does add some complexity, and was not something flagged as important from any of the clients I spoke to about this API. I'm looking for feedback on this point. Relates to #55363. --- .../index/mapper/FieldTypeLookup.java | 52 +++++- .../index/mapper/MapperService.java | 8 + .../fetch/subphase/FieldValueRetriever.java | 70 +++++--- .../index/mapper/FieldTypeLookupTests.java | 60 +++++++ .../subphase/FieldValueRetrieverTests.java | 156 ++++++++++++++---- .../index/mapper/MockFieldMapper.java | 21 +++ 6 files changed, 318 insertions(+), 49 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java index 2a444fa4f5987..835d4a485ca1a 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -24,6 +24,7 @@ import org.elasticsearch.common.regex.Regex; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; @@ -38,20 +39,32 @@ class FieldTypeLookup implements Iterable { final CopyOnWriteHashMap fullNameToFieldType; private final CopyOnWriteHashMap aliasToConcreteName; + + /** + * A map from field name to all fields whose content has been copied into it + * through copy_to. A field only be present in the map if some other field + * has listed it as a target of copy_to. + * + * For convenience, the set of copied fields includes the field itself. + */ + private final CopyOnWriteHashMap> fieldToCopiedFields; private final DynamicKeyFieldTypeLookup dynamicKeyLookup; FieldTypeLookup() { fullNameToFieldType = new CopyOnWriteHashMap<>(); aliasToConcreteName = new CopyOnWriteHashMap<>(); + fieldToCopiedFields = new CopyOnWriteHashMap<>(); dynamicKeyLookup = new DynamicKeyFieldTypeLookup(); } private FieldTypeLookup(CopyOnWriteHashMap fullNameToFieldType, CopyOnWriteHashMap aliasToConcreteName, + CopyOnWriteHashMap> fieldToCopiedFields, DynamicKeyFieldTypeLookup dynamicKeyLookup) { this.fullNameToFieldType = fullNameToFieldType; this.aliasToConcreteName = aliasToConcreteName; + this.fieldToCopiedFields = fieldToCopiedFields; this.dynamicKeyLookup = dynamicKeyLookup; } @@ -66,6 +79,7 @@ public FieldTypeLookup copyAndAddAll(Collection fieldMappers, CopyOnWriteHashMap fullName = this.fullNameToFieldType; CopyOnWriteHashMap aliases = this.aliasToConcreteName; + CopyOnWriteHashMap> sourcePaths = this.fieldToCopiedFields; Map dynamicKeyMappers = new HashMap<>(); for (FieldMapper fieldMapper : fieldMappers) { @@ -80,6 +94,17 @@ public FieldTypeLookup copyAndAddAll(Collection fieldMappers, if (fieldMapper instanceof DynamicKeyFieldMapper) { dynamicKeyMappers.put(fieldName, (DynamicKeyFieldMapper) fieldMapper); } + + for (String targetField : fieldMapper.copyTo().copyToFields()) { + Set sourcePath = sourcePaths.get(targetField); + if (sourcePath == null) { + sourcePaths = sourcePaths.copyAndPut(targetField, Set.of(targetField, fieldName)); + } else if (sourcePath.contains(fieldName) == false) { + Set newSourcePath = new HashSet<>(sourcePath); + newSourcePath.add(fieldName); + sourcePaths = sourcePaths.copyAndPut(targetField, Collections.unmodifiableSet(newSourcePath)); + } + } } for (FieldAliasMapper fieldAliasMapper : fieldAliasMappers) { @@ -93,7 +118,7 @@ public FieldTypeLookup copyAndAddAll(Collection fieldMappers, } DynamicKeyFieldTypeLookup newDynamicKeyLookup = this.dynamicKeyLookup.copyAndAddAll(dynamicKeyMappers, aliases); - return new FieldTypeLookup(fullName, aliases, newDynamicKeyLookup); + return new FieldTypeLookup(fullName, aliases, sourcePaths, newDynamicKeyLookup); } /** @@ -129,6 +154,31 @@ public Set simpleMatchToFullName(String pattern) { return fields; } + /** + * Given a field, returns its possible paths in the _source. + * + * For most fields, the source path is the same as the field itself. However + * there are some exceptions: + * - The 'source path' for a field alias is its target field. + * - For a multi-field, the source path is the parent field. + * - One field's content could have been copied to another through copy_to. + */ + public Set sourcePaths(String field) { + String resolvedField = aliasToConcreteName.getOrDefault(field, field); + + int lastDotIndex = resolvedField.lastIndexOf('.'); + if (lastDotIndex > 0) { + String parentField = resolvedField.substring(0, lastDotIndex); + if (fullNameToFieldType.containsKey(parentField)) { + resolvedField = parentField; + } + } + + return fieldToCopiedFields.containsKey(resolvedField) + ? fieldToCopiedFields.get(resolvedField) + : Set.of(resolvedField); + } + @Override public Iterator iterator() { Iterator concreteFieldTypes = fullNameToFieldType.values().iterator(); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 68ba92e9fa140..2ab7d5f4afedd 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -574,6 +574,14 @@ public Set simpleMatchToFullName(String pattern) { return fieldTypes.simpleMatchToFullName(pattern); } + /** + * Given a field name, returns its possible paths in the _source. For example, + * the 'source path' for a multi-field is the path to its parent field. + */ + public Set sourcePath(String fullName) { + return fieldTypes.sourcePaths(fullName); + } + /** * Returns all mapped field types. */ diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldValueRetriever.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldValueRetriever.java index 9c1e74983c171..e9d1b88a9199f 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldValueRetriever.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldValueRetriever.java @@ -21,10 +21,11 @@ import org.elasticsearch.common.document.DocumentField; import org.elasticsearch.common.xcontent.support.XContentMapValues; -import org.elasticsearch.index.mapper.DocumentMapper; +import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.search.lookup.SourceLookup; +import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; @@ -32,39 +33,60 @@ import java.util.Map; import java.util.Set; +/** + * A helper class to {@link FetchFieldsPhase} that's initialized with a list of field patterns to fetch. + * Then given a specific document, it can retrieve the corresponding fields from the document's source. + */ public class FieldValueRetriever { - private final Set fields; + private final List fieldContexts; + private final Set sourcePaths; public static FieldValueRetriever create(MapperService mapperService, Collection fieldPatterns) { - Set fields = new HashSet<>(); - DocumentMapper documentMapper = mapperService.documentMapper(); + List fields = new ArrayList<>(); + Set sourcePaths = new HashSet<>(); for (String fieldPattern : fieldPatterns) { - if (documentMapper.objectMappers().containsKey(fieldPattern)) { - continue; - } Collection concreteFields = mapperService.simpleMatchToFullName(fieldPattern); - fields.addAll(concreteFields); + for (String field : concreteFields) { + MappedFieldType fieldType = mapperService.fieldType(field); + + if (fieldType != null) { + Set sourcePath = mapperService.sourcePath(field); + fields.add(new FieldContext(field, sourcePath)); + sourcePaths.addAll(sourcePath); + } + } } - return new FieldValueRetriever(fields); + + return new FieldValueRetriever(fields, sourcePaths); } - private FieldValueRetriever(Set fields) { - this.fields = fields; + private FieldValueRetriever(List fieldContexts, Set sourcePaths) { + this.fieldContexts = fieldContexts; + this.sourcePaths = sourcePaths; } @SuppressWarnings("unchecked") public Map retrieve(SourceLookup sourceLookup) { Map result = new HashMap<>(); - Map sourceValues = extractValues(sourceLookup, this.fields); + Map sourceValues = extractValues(sourceLookup, sourcePaths); + + for (FieldContext fieldContext : fieldContexts) { + String field = fieldContext.fieldName; + Set sourcePath = fieldContext.sourcePath; - for (Map.Entry entry : sourceValues.entrySet()) { - String field = entry.getKey(); - Object value = entry.getValue(); - List values = value instanceof List - ? (List) value - : List.of(value); + List values = new ArrayList<>(); + for (String path : sourcePath) { + Object value = sourceValues.get(path); + if (value != null) { + if (value instanceof List) { + values.addAll((List) value); + } else { + values.add(value); + } + } + } result.put(field, new DocumentField(field, values)); } return result; @@ -74,7 +96,7 @@ public Map retrieve(SourceLookup sourceLookup) { * For each of the provided paths, return its value in the source. Note that in contrast with * {@link SourceLookup#extractRawValues}, array and object values can be returned. */ - private static Map extractValues(SourceLookup sourceLookup, Collection paths) { + private static Map extractValues(SourceLookup sourceLookup, Set paths) { Map result = new HashMap<>(paths.size()); for (String path : paths) { Object value = XContentMapValues.extractValue(path, sourceLookup); @@ -84,4 +106,14 @@ private static Map extractValues(SourceLookup sourceLookup, Coll } return result; } + + private static class FieldContext { + final String fieldName; + final Set sourcePath; + + FieldContext(String fieldName, Set sourcePath) { + this.fieldName = fieldName; + this.sourcePath = sourcePath; + } + } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java b/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java index 6fcf2f70df79e..76518f804b767 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java @@ -30,6 +30,7 @@ import java.util.Collection; import java.util.Iterator; import java.util.List; +import java.util.Set; import static java.util.Collections.emptyList; @@ -150,6 +151,65 @@ public void testSimpleMatchToFullName() { assertTrue(names.contains("barometer")); } + public void testSourcePathWithMultiFields() { + MappedFieldType ft = new MockFieldMapper.FakeFieldType(); + Mapper.BuilderContext context = new Mapper.BuilderContext( + MockFieldMapper.dummySettings, new ContentPath()); + + MockFieldMapper field = new MockFieldMapper.Builder("field", ft, ft) + .addMultiField(new MockFieldMapper.Builder("field.subfield1", ft, ft)) + .addMultiField(new MockFieldMapper.Builder("field.subfield2", ft, ft)) + .build(context); + + FieldTypeLookup lookup = new FieldTypeLookup(); + lookup = lookup.copyAndAddAll(newList(field), emptyList()); + + assertEquals(Set.of("field"), lookup.sourcePaths("field")); + assertEquals(Set.of("field"), lookup.sourcePaths("field.subfield1")); + assertEquals(Set.of("field"), lookup.sourcePaths("field.subfield2")); + } + + public void testSourcePathWithAliases() { + MappedFieldType ft = new MockFieldMapper.FakeFieldType(); + Mapper.BuilderContext context = new Mapper.BuilderContext( + MockFieldMapper.dummySettings, new ContentPath()); + + MockFieldMapper field = new MockFieldMapper.Builder("field", ft, ft) + .addMultiField(new MockFieldMapper.Builder("field.subfield", ft, ft)) + .build(context); + + FieldAliasMapper alias1 = new FieldAliasMapper("alias1", "alias1", "field"); + FieldAliasMapper alias2 = new FieldAliasMapper("alias2", "alias2", "field.subfield"); + + FieldTypeLookup lookup = new FieldTypeLookup(); + lookup = lookup.copyAndAddAll(newList(field), newList(alias1, alias2)); + + assertEquals(Set.of("field"), lookup.sourcePaths("alias1")); + assertEquals(Set.of("field"), lookup.sourcePaths("alias2")); + } + + public void testSourcePathsWithCopyTo() { + MappedFieldType ft = new MockFieldMapper.FakeFieldType(); + Mapper.BuilderContext context = new Mapper.BuilderContext( + MockFieldMapper.dummySettings, new ContentPath()); + + MockFieldMapper field = new MockFieldMapper.Builder("field", ft, ft) + .addMultiField(new MockFieldMapper.Builder("field.subfield1", ft, ft)) + .build(context); + + MockFieldMapper otherField = new MockFieldMapper.Builder("other_field", ft, ft) + .copyTo(new FieldMapper.CopyTo.Builder() + .add("field") + .build()) + .build(context); + + FieldTypeLookup lookup = new FieldTypeLookup(); + lookup = lookup.copyAndAddAll(newList(field, otherField), emptyList()); + + assertEquals(Set.of("other_field", "field"), lookup.sourcePaths("field")); + assertEquals(Set.of("other_field", "field"), lookup.sourcePaths("field.subfield1")); + } + public void testIteratorImmutable() { MockFieldMapper f1 = new MockFieldMapper("foo"); FieldTypeLookup lookup = new FieldTypeLookup(); diff --git a/server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldValueRetrieverTests.java b/server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldValueRetrieverTests.java index 99ebf1a8807d1..92aa7eeacde4f 100644 --- a/server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldValueRetrieverTests.java +++ b/server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldValueRetrieverTests.java @@ -28,7 +28,6 @@ import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.search.lookup.SourceLookup; import org.elasticsearch.test.ESSingleNodeTestCase; -import org.junit.Before; import java.io.IOException; import java.util.List; @@ -38,29 +37,9 @@ import static org.hamcrest.Matchers.hasItems; public class FieldValueRetrieverTests extends ESSingleNodeTestCase { - private MapperService mapperService; - - @Before - public void createMapperService() throws IOException { - XContentBuilder mapping = XContentFactory.jsonBuilder().startObject() - .startObject("properties") - .startObject("field").field("type", "keyword").endObject() - .startObject("integer_field").field("type", "integer").endObject() - .startObject("float_range").field("type", "float_range").endObject() - .startObject("object") - .startObject("properties") - .startObject("field").field("type", "keyword").endObject() - .endObject() - .endObject() - .startObject("field_that_does_not_match").field("type", "keyword").endObject() - .endObject() - .endObject(); - - IndexService indexService = createIndex("index", Settings.EMPTY, mapping); - this.mapperService = indexService.mapperService(); - } public void testLeafValues() throws IOException { + MapperService mapperService = createMapperService(); XContentBuilder source = XContentFactory.jsonBuilder().startObject() .array("field", "first", "second") .startObject("object") @@ -68,7 +47,7 @@ public void testLeafValues() throws IOException { .endObject() .endObject(); - Map fields = retrieveFields(source, List.of("field", "object.field")); + Map fields = retrieveFields(mapperService, source, List.of("field", "object.field")); assertThat(fields.size(), equalTo(2)); DocumentField field = fields.get("field"); @@ -83,6 +62,7 @@ public void testLeafValues() throws IOException { } public void testObjectValues() throws IOException { + MapperService mapperService = createMapperService(); XContentBuilder source = XContentFactory.jsonBuilder().startObject() .startObject("float_range") .field("gte", 0.0) @@ -90,7 +70,7 @@ public void testObjectValues() throws IOException { .endObject() .endObject(); - Map fields = retrieveFields(source, "float_range"); + Map fields = retrieveFields(mapperService, source, "float_range"); assertThat(fields.size(), equalTo(1)); DocumentField rangeField = fields.get("float_range"); @@ -100,6 +80,7 @@ public void testObjectValues() throws IOException { } public void testFieldNamesWithWildcard() throws IOException { + MapperService mapperService = createMapperService();; XContentBuilder source = XContentFactory.jsonBuilder().startObject() .array("field", "first", "second") .field("integer_field", "third") @@ -108,7 +89,7 @@ public void testFieldNamesWithWildcard() throws IOException { .endObject() .endObject(); - Map fields = retrieveFields(source, "*field"); + Map fields = retrieveFields(mapperService, source, "*field"); assertThat(fields.size(), equalTo(3)); DocumentField field = fields.get("field"); @@ -127,7 +108,104 @@ public void testFieldNamesWithWildcard() throws IOException { assertThat(objectField.getValues(), hasItems("fourth")); } + + public void testFieldAliases() throws IOException { + XContentBuilder mapping = XContentFactory.jsonBuilder().startObject() + .startObject("properties") + .startObject("field").field("type", "keyword").endObject() + .startObject("alias_field") + .field("type", "alias") + .field("path", "field") + .endObject() + .endObject() + .endObject(); + + IndexService indexService = createIndex("index", Settings.EMPTY, mapping); + MapperService mapperService = indexService.mapperService(); + + XContentBuilder source = XContentFactory.jsonBuilder().startObject() + .field("field", "value") + .endObject(); + + Map fields = retrieveFields(mapperService, source, "alias_field"); + assertThat(fields.size(), equalTo(1)); + + DocumentField field = fields.get("alias_field"); + assertNotNull(field); + assertThat(field.getValues().size(), equalTo(1)); + assertThat(field.getValues(), hasItems("value")); + + fields = retrieveFields(mapperService, source, "*field"); + assertThat(fields.size(), equalTo(2)); + assertTrue(fields.containsKey("alias_field")); + assertTrue(fields.containsKey("field")); + } + + public void testMultiFields() throws IOException { + XContentBuilder mapping = XContentFactory.jsonBuilder().startObject() + .startObject("properties") + .startObject("field") + .field("type", "integer") + .startObject("fields") + .startObject("keyword").field("type", "keyword").endObject() + .endObject() + .endObject() + .endObject() + .endObject(); + + IndexService indexService = createIndex("index", Settings.EMPTY, mapping); + MapperService mapperService = indexService.mapperService(); + + XContentBuilder source = XContentFactory.jsonBuilder().startObject() + .field("field", 42) + .endObject(); + + Map fields = retrieveFields(mapperService, source, "field.keyword"); + assertThat(fields.size(), equalTo(1)); + + DocumentField field = fields.get("field.keyword"); + assertNotNull(field); + assertThat(field.getValues().size(), equalTo(1)); + assertThat(field.getValues(), hasItems(42)); + + fields = retrieveFields(mapperService, source, "field*"); + assertThat(fields.size(), equalTo(2)); + assertTrue(fields.containsKey("field")); + assertTrue(fields.containsKey("field.keyword")); + } + + public void testCopyTo() throws IOException { + XContentBuilder mapping = XContentFactory.jsonBuilder().startObject() + .startObject("properties") + .startObject("field") + .field("type", "keyword") + .endObject() + .startObject("other_field") + .field("type", "integer") + .field("copy_to", "field") + .endObject() + .endObject() + .endObject(); + + IndexService indexService = createIndex("index", Settings.EMPTY, mapping); + MapperService mapperService = indexService.mapperService(); + + XContentBuilder source = XContentFactory.jsonBuilder().startObject() + .array("field", "one", "two", "three") + .array("other_field", 1, 2, 3) + .endObject(); + + Map fields = retrieveFields(mapperService, source, "field"); + assertThat(fields.size(), equalTo(1)); + + DocumentField field = fields.get("field"); + assertNotNull(field); + assertThat(field.getValues().size(), equalTo(6)); + assertThat(field.getValues(), hasItems("one", "two", "three", 1, 2, 3)); + } + public void testObjectFields() throws IOException { + MapperService mapperService = createMapperService();; XContentBuilder source = XContentFactory.jsonBuilder().startObject() .array("field", "first", "second") .startObject("object") @@ -135,19 +213,39 @@ public void testObjectFields() throws IOException { .endObject() .endObject(); - Map fields = retrieveFields(source, "object"); + Map fields = retrieveFields(mapperService, source, "object"); assertFalse(fields.containsKey("object")); } - private Map retrieveFields(XContentBuilder source, String fieldPattern) { - return retrieveFields(source, List.of(fieldPattern)); + private Map retrieveFields(MapperService mapperService, XContentBuilder source, String fieldPattern) { + return retrieveFields(mapperService, source, List.of(fieldPattern)); } - private Map retrieveFields(XContentBuilder source, List fieldPatterns) { + private Map retrieveFields(MapperService mapperService, XContentBuilder source, List fieldPatterns) { SourceLookup sourceLookup = new SourceLookup(); sourceLookup.setSource(BytesReference.bytes(source)); FieldValueRetriever fetchFieldsLookup = FieldValueRetriever.create(mapperService, fieldPatterns); return fetchFieldsLookup.retrieve(sourceLookup); } + + + public MapperService createMapperService() throws IOException { + XContentBuilder mapping = XContentFactory.jsonBuilder().startObject() + .startObject("properties") + .startObject("field").field("type", "keyword").endObject() + .startObject("integer_field").field("type", "integer").endObject() + .startObject("float_range").field("type", "float_range").endObject() + .startObject("object") + .startObject("properties") + .startObject("field").field("type", "keyword").endObject() + .endObject() + .endObject() + .startObject("field_that_does_not_match").field("type", "keyword").endObject() + .endObject() + .endObject(); + + IndexService indexService = createIndex("index", Settings.EMPTY, mapping); + return indexService.mapperService(); + } } diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MockFieldMapper.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MockFieldMapper.java index 311e3dd625e40..08830205f464c 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MockFieldMapper.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MockFieldMapper.java @@ -44,6 +44,14 @@ public MockFieldMapper(String fullName, MappedFieldType fieldType) { MultiFields.empty(), new CopyTo.Builder().build()); } + public MockFieldMapper(String fullName, + MappedFieldType fieldType, + MultiFields multifields, + CopyTo copyTo) { + super(findSimpleName(fullName), setName(fullName, fieldType), setName(fullName, fieldType), dummySettings, + multifields, copyTo); + } + static MappedFieldType setName(String fullName, MappedFieldType fieldType) { fieldType.setName(fullName); return fieldType; @@ -95,4 +103,17 @@ protected void parseCreateField(ParseContext context) throws IOException { protected void mergeOptions(FieldMapper other, List conflicts) { } + + public static class Builder extends FieldMapper.Builder { + protected Builder(String name, MappedFieldType fieldType, MappedFieldType defaultFieldType) { + super(name, fieldType, defaultFieldType); + builder = this; + } + + @Override + public MockFieldMapper build(BuilderContext context) { + MultiFields multiFields = multiFieldsBuilder.build(this, context); + return new MockFieldMapper(name(), fieldType, multiFields, copyTo); + } + } }