From f785d0ff15020535c2e17176565e48918cd0b74c Mon Sep 17 00:00:00 2001 From: Kostas Krikellas <131142368+kkrik-es@users.noreply.github.com> Date: Wed, 9 Oct 2024 10:22:39 +0300 Subject: [PATCH 1/6] Skip storing ignored source for single-element leaf arrays (#113937) * Minimize storing array source * restrict to fields * revert changes for `addIgnoredFieldFromContext` * fix test * spotless * count nulls (cherry picked from commit f79705d9b6c528a96cb5badaa251377525cbd628) --- .../index/mapper/DocumentParser.java | 39 ++++++++++++++----- .../index/mapper/DocumentParserContext.java | 6 +++ .../mapper/IgnoredSourceFieldMapperTests.java | 32 +++++++++++++++ .../index/mapper/MapperTestCase.java | 6 ++- 4 files changed, 73 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index 39b6a5b4faf7a..634a017269164 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -799,17 +799,30 @@ private static void parseNonDynamicArray( String fullPath = context.path().pathAsText(arrayFieldName); // Check if we need to record the array source. This only applies to synthetic source. + boolean canRemoveSingleLeafElement = false; if (context.canAddIgnoredField()) { - boolean objectRequiresStoringSource = mapper instanceof ObjectMapper objectMapper - && (getSourceKeepMode(context, objectMapper.sourceKeepMode()) == Mapper.SourceKeepMode.ALL - || (getSourceKeepMode(context, objectMapper.sourceKeepMode()) == Mapper.SourceKeepMode.ARRAYS - && objectMapper instanceof NestedObjectMapper == false)); - boolean fieldWithFallbackSyntheticSource = mapper instanceof FieldMapper fieldMapper - && fieldMapper.syntheticSourceMode() == FieldMapper.SyntheticSourceMode.FALLBACK; - boolean fieldWithStoredArraySource = mapper instanceof FieldMapper fieldMapper - && getSourceKeepMode(context, fieldMapper.sourceKeepMode()) != Mapper.SourceKeepMode.NONE; + Mapper.SourceKeepMode mode = Mapper.SourceKeepMode.NONE; + boolean objectWithFallbackSyntheticSource = false; + if (mapper instanceof ObjectMapper objectMapper) { + mode = getSourceKeepMode(context, objectMapper.sourceKeepMode()); + objectWithFallbackSyntheticSource = (mode == Mapper.SourceKeepMode.ALL + || (mode == Mapper.SourceKeepMode.ARRAYS && objectMapper instanceof NestedObjectMapper == false)); + } + boolean fieldWithFallbackSyntheticSource = false; + boolean fieldWithStoredArraySource = false; + if (mapper instanceof FieldMapper fieldMapper) { + mode = getSourceKeepMode(context, fieldMapper.sourceKeepMode()); + fieldWithFallbackSyntheticSource = fieldMapper.syntheticSourceMode() == FieldMapper.SyntheticSourceMode.FALLBACK; + fieldWithStoredArraySource = mode != Mapper.SourceKeepMode.NONE; + } boolean copyToFieldHasValuesInDocument = context.isWithinCopyTo() == false && context.isCopyToDestinationField(fullPath); - if (objectRequiresStoringSource + + canRemoveSingleLeafElement = mapper instanceof FieldMapper + && mode == Mapper.SourceKeepMode.ARRAYS + && fieldWithFallbackSyntheticSource == false + && copyToFieldHasValuesInDocument == false; + + if (objectWithFallbackSyntheticSource || fieldWithFallbackSyntheticSource || fieldWithStoredArraySource || copyToFieldHasValuesInDocument) { @@ -829,20 +842,28 @@ private static void parseNonDynamicArray( XContentParser parser = context.parser(); XContentParser.Token token; + int elements = 0; while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { if (token == XContentParser.Token.START_OBJECT) { + elements = Integer.MAX_VALUE; parseObject(context, lastFieldName); } else if (token == XContentParser.Token.START_ARRAY) { + elements = Integer.MAX_VALUE; parseArray(context, lastFieldName); } else if (token == XContentParser.Token.VALUE_NULL) { + elements++; parseNullValue(context, lastFieldName); } else if (token == null) { throwEOFOnParseArray(arrayFieldName, context); } else { assert token.isValue(); + elements++; parseValue(context, lastFieldName); } } + if (elements <= 1 && canRemoveSingleLeafElement) { + context.removeLastIgnoredField(fullPath); + } postProcessDynamicArrayMapping(context, lastFieldName); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java index eebe95e260dcf..ac236e5a7e5fd 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java @@ -296,6 +296,12 @@ public final void addIgnoredField(IgnoredSourceFieldMapper.NameValue values) { } } + final void removeLastIgnoredField(String name) { + if (ignoredFieldValues.isEmpty() == false && ignoredFieldValues.getLast().name().equals(name)) { + ignoredFieldValues.removeLast(); + } + } + /** * Return the collection of values for fields that have been ignored so far. */ diff --git a/server/src/test/java/org/elasticsearch/index/mapper/IgnoredSourceFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/IgnoredSourceFieldMapperTests.java index 8c65424fb8560..205ff08c397b2 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/IgnoredSourceFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/IgnoredSourceFieldMapperTests.java @@ -532,6 +532,38 @@ public void testIndexStoredArraySourceRootValueArrayDisabled() throws IOExceptio {"bool_value":true,"int_value":[10,20,30]}""", syntheticSource); } + public void testIndexStoredArraySourceSingleLeafElement() throws IOException { + DocumentMapper documentMapper = createMapperServiceWithStoredArraySource(syntheticSourceMapping(b -> { + b.startObject("int_value").field("type", "integer").endObject(); + })).documentMapper(); + var syntheticSource = syntheticSource(documentMapper, b -> b.array("int_value", new int[] { 10 })); + assertEquals("{\"int_value\":10}", syntheticSource); + ParsedDocument doc = documentMapper.parse(source(syntheticSource)); + assertNull(doc.rootDoc().getField("_ignored_source")); + } + + public void testIndexStoredArraySourceSingleLeafElementAndNull() throws IOException { + DocumentMapper documentMapper = createMapperServiceWithStoredArraySource(syntheticSourceMapping(b -> { + b.startObject("value").field("type", "keyword").endObject(); + })).documentMapper(); + var syntheticSource = syntheticSource(documentMapper, b -> b.array("value", new String[] { "foo", null })); + assertEquals("{\"value\":[\"foo\",null]}", syntheticSource); + } + + public void testIndexStoredArraySourceSingleObjectElement() throws IOException { + DocumentMapper documentMapper = createMapperServiceWithStoredArraySource(syntheticSourceMapping(b -> { + b.startObject("path").startObject("properties"); + { + b.startObject("int_value").field("type", "integer").endObject(); + } + b.endObject().endObject(); + })).documentMapper(); + var syntheticSource = syntheticSource(documentMapper, b -> { + b.startArray("path").startObject().field("int_value", 10).endObject().endArray(); + }); + assertEquals("{\"path\":[{\"int_value\":10}]}", syntheticSource); + } + public void testFieldStoredArraySourceRootValueArray() throws IOException { DocumentMapper documentMapper = createMapperService(syntheticSourceMapping(b -> { b.startObject("int_value").field("type", "integer").field(Mapper.SYNTHETIC_SOURCE_KEEP_PARAM, "arrays").endObject(); diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java index bae2363e90002..bd9ff10edf4a6 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java @@ -1596,7 +1596,11 @@ public void testSyntheticSourceKeepArrays() throws IOException { buildInput.accept(builder); builder.endObject(); String expected = Strings.toString(builder); - assertThat(syntheticSource(mapperAll, buildInput), equalTo(expected)); + String actual = syntheticSource(mapperAll, buildInput); + // Check for single-element array, the array source is not stored in this case. + if (expected.replace("[", "").replace("]", "").equals(actual) == false) { + assertThat(actual, equalTo(expected)); + } } @Override From 20f27818a1ef2ff412bdb51ced433d385f915baa Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Wed, 9 Oct 2024 10:47:16 +0300 Subject: [PATCH 2/6] fix list api --- .../org/elasticsearch/index/mapper/DocumentParserContext.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java index ac236e5a7e5fd..ad22f4917cb79 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java @@ -297,8 +297,8 @@ public final void addIgnoredField(IgnoredSourceFieldMapper.NameValue values) { } final void removeLastIgnoredField(String name) { - if (ignoredFieldValues.isEmpty() == false && ignoredFieldValues.getLast().name().equals(name)) { - ignoredFieldValues.removeLast(); + if (ignoredFieldValues.isEmpty() == false && ignoredFieldValues.get(ignoredFieldValues.size() - 1).name().equals(name)) { + ignoredFieldValues.remove(ignoredFieldValues.size() - 1); } } From 28ec176e506cad5e3f3ac3ef5c84b87537ef368a Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Wed, 9 Oct 2024 12:19:14 +0300 Subject: [PATCH 3/6] fix tests --- .../java/org/elasticsearch/index/mapper/MapperTestCase.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java index bd9ff10edf4a6..fb91e1dd7803e 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java @@ -1598,7 +1598,7 @@ public void testSyntheticSourceKeepArrays() throws IOException { String expected = Strings.toString(builder); String actual = syntheticSource(mapperAll, buildInput); // Check for single-element array, the array source is not stored in this case. - if (expected.replace("[", "").replace("]", "").equals(actual) == false) { + if (expected.contains("[") == false) { assertThat(actual, equalTo(expected)); } } From 0ca937988fced537bce340a045f9da18cfdd0495 Mon Sep 17 00:00:00 2001 From: Kostas Krikellas <131142368+kkrik-es@users.noreply.github.com> Date: Wed, 9 Oct 2024 12:49:17 +0300 Subject: [PATCH 4/6] Update MapperTestCase.java --- .../java/org/elasticsearch/index/mapper/MapperTestCase.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java index fb91e1dd7803e..863399240d550 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java @@ -1598,7 +1598,7 @@ public void testSyntheticSourceKeepArrays() throws IOException { String expected = Strings.toString(builder); String actual = syntheticSource(mapperAll, buildInput); // Check for single-element array, the array source is not stored in this case. - if (expected.contains("[") == false) { + if (expected.contains("[") && expected.replace("[", "").replace("]", "").equals(actual) == false) { assertThat(actual, equalTo(expected)); } } From 2b87e43af59299cc32221558919c7dee91ab13ba Mon Sep 17 00:00:00 2001 From: Kostas Krikellas <131142368+kkrik-es@users.noreply.github.com> Date: Wed, 9 Oct 2024 12:54:21 +0300 Subject: [PATCH 5/6] Update MapperTestCase.java --- .../java/org/elasticsearch/index/mapper/MapperTestCase.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java index 863399240d550..ee437cee11876 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java @@ -1598,7 +1598,7 @@ public void testSyntheticSourceKeepArrays() throws IOException { String expected = Strings.toString(builder); String actual = syntheticSource(mapperAll, buildInput); // Check for single-element array, the array source is not stored in this case. - if (expected.contains("[") && expected.replace("[", "").replace("]", "").equals(actual) == false) { + if (actual.contains("[") && expected.replace("[", "").replace("]", "").equals(actual) == false) { assertThat(actual, equalTo(expected)); } } From 2c313dda40311b8f98dd87ba721c2f86dd77e834 Mon Sep 17 00:00:00 2001 From: Kostas Krikellas <131142368+kkrik-es@users.noreply.github.com> Date: Wed, 9 Oct 2024 13:09:34 +0300 Subject: [PATCH 6/6] Update MapperTestCase.java --- .../org/elasticsearch/index/mapper/MapperTestCase.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java index ee437cee11876..61ffbfe1fee37 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java @@ -1586,7 +1586,7 @@ public void testSyntheticSourceKeepArrays() throws IOException { b.endObject(); })); - int elementCount = randomIntBetween(1, 5); + int elementCount = randomIntBetween(2, 5); CheckedConsumer buildInput = (XContentBuilder builder) -> { example.buildInputArray(builder, elementCount); }; @@ -1597,10 +1597,7 @@ public void testSyntheticSourceKeepArrays() throws IOException { builder.endObject(); String expected = Strings.toString(builder); String actual = syntheticSource(mapperAll, buildInput); - // Check for single-element array, the array source is not stored in this case. - if (actual.contains("[") && expected.replace("[", "").replace("]", "").equals(actual) == false) { - assertThat(actual, equalTo(expected)); - } + assertThat(actual, equalTo(expected)); } @Override