diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index f391b0c9a7833..47afb8adb42bf 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -604,19 +604,34 @@ public DocValuesLoader docValuesLoader(LeafReader leafReader, int[] docIdsInLeaf loaders.add(loader); } } - return docId -> { + if (loaders.isEmpty()) { + return null; + } + return new ObjectDocValuesLoader(loaders); + } + + private class ObjectDocValuesLoader implements DocValuesLoader { + private final List loaders; + + private ObjectDocValuesLoader(List loaders) { + this.loaders = loaders; + } + + @Override + public boolean advanceToDoc(int docId) throws IOException { + boolean anyLeafHasDocValues = false; for (SourceLoader.SyntheticFieldLoader.DocValuesLoader docValueLoader : loaders) { boolean leafHasValue = docValueLoader.advanceToDoc(docId); - hasValue |= leafHasValue; + anyLeafHasDocValues |= leafHasValue; } - /* - * Important and kind of sneaky note: this will return true - * if there were any values loaded from stored fields as - * well. That *is* how we "wake up" objects that contain just - * stored field. - */ - return hasValue; - }; + hasValue |= anyLeafHasDocValues; + return anyLeafHasDocValues; + } + } + + @Override + public boolean hasValue() { + return hasValue; } @Override @@ -626,7 +641,9 @@ public void write(XContentBuilder b) throws IOException { } startSyntheticField(b); for (SourceLoader.SyntheticFieldLoader field : fields) { - field.write(b); + if (field.hasValue()) { + field.write(b); + } } b.endObject(); hasValue = false; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SortedNumericDocValuesSyntheticFieldLoader.java b/server/src/main/java/org/elasticsearch/index/mapper/SortedNumericDocValuesSyntheticFieldLoader.java index 0ad69a0498a6c..6c479ed94bc5d 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/SortedNumericDocValuesSyntheticFieldLoader.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/SortedNumericDocValuesSyntheticFieldLoader.java @@ -63,6 +63,11 @@ public DocValuesLoader docValuesLoader(LeafReader reader, int[] docIdsInLeaf) th return loader; } + @Override + public boolean hasValue() { + return values.count() > 0; + } + @Override public void write(XContentBuilder b) throws IOException { switch (values.count()) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SortedSetDocValuesSyntheticFieldLoader.java b/server/src/main/java/org/elasticsearch/index/mapper/SortedSetDocValuesSyntheticFieldLoader.java index 321d1620c41b6..49c692dc73c75 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/SortedSetDocValuesSyntheticFieldLoader.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/SortedSetDocValuesSyntheticFieldLoader.java @@ -67,6 +67,11 @@ public DocValuesLoader docValuesLoader(LeafReader reader, int[] docIdsInLeaf) th return loader; } + @Override + public boolean hasValue() { + return values.count() > 0; + } + @Override public void write(XContentBuilder b) throws IOException { switch (values.count()) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SourceLoader.java b/server/src/main/java/org/elasticsearch/index/mapper/SourceLoader.java index 70111f1e48ffe..e7d4f54abd1bb 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/SourceLoader.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/SourceLoader.java @@ -106,8 +106,18 @@ public Set requiredStoredFields() { @Override public Leaf leaf(LeafReader reader, int[] docIdsInLeaf) throws IOException { - SyntheticFieldLoader.DocValuesLoader leaf = loader.docValuesLoader(reader, docIdsInLeaf); - return (fieldsVisitor, docId) -> { + return new SyntheticLeaf(loader.docValuesLoader(reader, docIdsInLeaf)); + } + + private class SyntheticLeaf implements Leaf { + private final SyntheticFieldLoader.DocValuesLoader docValuesLoader; + + private SyntheticLeaf(SyntheticFieldLoader.DocValuesLoader docValuesLoader) { + this.docValuesLoader = docValuesLoader; + } + + @Override + public BytesReference source(FieldsVisitor fieldsVisitor, int docId) throws IOException { if (fieldsVisitor != null) { for (Map.Entry> e : fieldsVisitor.fields().entrySet()) { SyntheticFieldLoader.StoredFieldLoader loader = storedFieldLoaders.get(e.getKey()); @@ -116,16 +126,19 @@ public Leaf leaf(LeafReader reader, int[] docIdsInLeaf) throws IOException { } } } + if (docValuesLoader != null) { + docValuesLoader.advanceToDoc(docId); + } // TODO accept a requested xcontent type try (XContentBuilder b = new XContentBuilder(JsonXContent.jsonXContent, new ByteArrayOutputStream())) { - if (leaf.advanceToDoc(docId)) { + if (loader.hasValue()) { loader.write(b); } else { b.startObject().endObject(); } return BytesReference.bytes(b); } - }; + } } } @@ -175,6 +188,11 @@ public DocValuesLoader docValuesLoader(LeafReader leafReader, int[] docIdsInLeaf return null; } + @Override + public boolean hasValue() { + return false; + } + @Override public void write(XContentBuilder b) {} }; @@ -192,6 +210,8 @@ public void write(XContentBuilder b) {} */ DocValuesLoader docValuesLoader(LeafReader leafReader, int[] docIdsInLeaf) throws IOException; + boolean hasValue(); + /** * Write values for this document. */ diff --git a/server/src/main/java/org/elasticsearch/index/mapper/StringStoredFieldFieldLoader.java b/server/src/main/java/org/elasticsearch/index/mapper/StringStoredFieldFieldLoader.java index 0cd65d0ee3059..2c74afce43321 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/StringStoredFieldFieldLoader.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/StringStoredFieldFieldLoader.java @@ -39,6 +39,11 @@ public void load(List values) { this.values = values; } + @Override + public boolean hasValue() { + return values != null && values.isEmpty() == false; + } + @Override public void write(XContentBuilder b) throws IOException { if (values == null || values.isEmpty()) { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java index 4a4bea2c02c3a..c0b61a21a0938 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java @@ -23,6 +23,7 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.nullValue; public class ObjectMapperTests extends MapperServiceTestCase { @@ -516,4 +517,43 @@ public void testSubobjectsCannotBeUpdatedOnRoot() throws IOException { ); assertEquals("the [subobjects] parameter can't be updated for the object mapping [_doc]", exception.getMessage()); } + + /** + * Makes sure that an empty object mapper returns {@code null} from + * {@link SourceLoader.SyntheticFieldLoader#docValuesLoader}. This + * is important because it allows us to skip whole chains of empty + * fields when loading synthetic {@code _source}. + */ + public void testSyntheticSourceDocValuesEmpty() throws IOException { + DocumentMapper mapper = createDocumentMapper(mapping(b -> b.startObject("o").field("type", "object").endObject())); + ObjectMapper o = (ObjectMapper) mapper.mapping().getRoot().getMapper("o"); + assertThat(o.syntheticFieldLoader().docValuesLoader(null, null), nullValue()); + assertThat(mapper.mapping().getRoot().syntheticFieldLoader().docValuesLoader(null, null), nullValue()); + } + + /** + * Makes sure that an object mapper containing only fields without + * doc values returns {@code null} from + * {@link SourceLoader.SyntheticFieldLoader#docValuesLoader}. This + * is important because it allows us to skip whole chains of empty + * fields when loading synthetic {@code _source}. + */ + public void testSyntheticSourceDocValuesFieldWithout() throws IOException { + DocumentMapper mapper = createDocumentMapper(mapping(b -> { + b.startObject("o").startObject("properties"); + { + b.startObject("kwd"); + { + b.field("type", "keyword"); + b.field("doc_values", false); + b.field("store", true); + } + b.endObject(); + } + b.endObject().endObject(); + })); + ObjectMapper o = (ObjectMapper) mapper.mapping().getRoot().getMapper("o"); + assertThat(o.syntheticFieldLoader().docValuesLoader(null, null), nullValue()); + assertThat(mapper.mapping().getRoot().syntheticFieldLoader().docValuesLoader(null, null), nullValue()); + } } diff --git a/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java b/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java index bd17327ce36f4..576045db004c6 100644 --- a/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java +++ b/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java @@ -725,6 +725,11 @@ public DocValuesLoader docValuesLoader(LeafReader reader, int[] docIdsInLeaf) th return new AggregateDocValuesLoader(); } + @Override + public boolean hasValue() { + return metricHasValue.isEmpty() == false; + } + @Override public void write(XContentBuilder b) throws IOException { if (metricHasValue.isEmpty()) { diff --git a/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java b/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java index a070b828aa32b..257fe97de34ca 100644 --- a/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java +++ b/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java @@ -311,6 +311,11 @@ protected String contentType() { @Override public SourceLoader.SyntheticFieldLoader syntheticFieldLoader() { + String value = fieldType().value(); + ; + if (value == null) { + return SourceLoader.SyntheticFieldLoader.NOTHING; + } return new SourceLoader.SyntheticFieldLoader() { @Override public Stream> storedFieldLoaders() { @@ -319,19 +324,14 @@ public Stream> storedFieldLoaders() { @Override public DocValuesLoader docValuesLoader(LeafReader reader, int[] docIdsInLeaf) { - /* - * If there is a value we need to enable objects containing these - * fields. We could build something special for fields that are - * always "on", but constant_keyword fields are rare enough that - * having an extra doc values loader that always returns `true` - * isn't a big performance hit and gets the job done. - */ - if (fieldType().value == null) { - return null; - } return docId -> true; } + @Override + public boolean hasValue() { + return true; + } + @Override public void write(XContentBuilder b) throws IOException { if (fieldType().value != null) {