From a1d27ce4f05757b69a447953573de8fdbc3f452a Mon Sep 17 00:00:00 2001 From: Parker Timmins Date: Fri, 9 May 2025 14:49:15 -0500 Subject: [PATCH 1/2] Do not respect synthetic_source_keep=arrays if type parses arrays (#127796) Types that parse arrays directly should not need to store values in _ignored_source if synthetic_source_keep=arrays. Since they have custom handling of arrays, it provides no benefit to store in _ignored_source when there are multiple values of the type. (cherry picked from commit c04a9569fea360f0d2b6fcde666f7362a54dcac2) --- docs/changelog/127796.yaml | 6 ++ .../mapping/types/geo-point.asciidoc | 14 +++- .../index/mapper/DocumentParser.java | 2 +- .../GeoPointFieldBlockLoaderTests.java | 79 ++----------------- 4 files changed, 23 insertions(+), 78 deletions(-) create mode 100644 docs/changelog/127796.yaml diff --git a/docs/changelog/127796.yaml b/docs/changelog/127796.yaml new file mode 100644 index 0000000000000..c87e777f83d40 --- /dev/null +++ b/docs/changelog/127796.yaml @@ -0,0 +1,6 @@ +pr: 127796 +summary: Do not respect synthetic_source_keep=arrays if type parses arrays +area: Mapping +type: enhancement +issues: + - 126155 diff --git a/docs/reference/mapping/types/geo-point.asciidoc b/docs/reference/mapping/types/geo-point.asciidoc index 0958997d3fb00..8157a0d31bb35 100644 --- a/docs/reference/mapping/types/geo-point.asciidoc +++ b/docs/reference/mapping/types/geo-point.asciidoc @@ -220,7 +220,10 @@ any issues, but features in technical preview are not subject to the support SLA of official GA features. Synthetic source may sort `geo_point` fields (first by latitude and then -longitude) and reduces them to their stored precision. For example: +longitude) and reduces them to their stored precision. Additionally, unlike most +types, arrays of `geo_point` fields will not preserve order if +`synthetic_source_keep` is set to `arrays`. For example: + [source,console,id=synthetic-source-geo-point-example] ---- PUT idx @@ -236,15 +239,18 @@ PUT idx }, "mappings": { "properties": { - "point": { "type": "geo_point" } + "point": { + "type": "geo_point", + "synthetic_source_keep": "arrays" + } } } } PUT idx/_doc/1 { "point": [ - {"lat":-90, "lon":-80}, - {"lat":10, "lon":30} + {"lat":10, "lon":30}, + {"lat":-90, "lon":-80} ] } ---- 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 1fc964ea7d0e6..7f779760aefdf 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -460,7 +460,7 @@ static void parseObjectOrField(DocumentParserContext context, Mapper mapper) thr if (context.canAddIgnoredField() && (fieldMapper.syntheticSourceMode() == FieldMapper.SyntheticSourceMode.FALLBACK || sourceKeepMode == Mapper.SourceKeepMode.ALL - || (sourceKeepMode == Mapper.SourceKeepMode.ARRAYS && context.inArrayScope()) + || (sourceKeepMode == Mapper.SourceKeepMode.ARRAYS && context.inArrayScope() && parsesArrayValue(mapper) == false) || (context.isWithinCopyTo() == false && context.isCopyToDestinationField(mapper.fullPath())))) { context = context.addIgnoredFieldFromContext( IgnoredSourceFieldMapper.NameValue.fromContext(context, fieldMapper.fullPath(), null) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/blockloader/GeoPointFieldBlockLoaderTests.java b/server/src/test/java/org/elasticsearch/index/mapper/blockloader/GeoPointFieldBlockLoaderTests.java index ebb751fb18eee..fdb1d89175fef 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/blockloader/GeoPointFieldBlockLoaderTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/blockloader/GeoPointFieldBlockLoaderTests.java @@ -17,7 +17,6 @@ import org.elasticsearch.index.mapper.MappedFieldType; import java.nio.ByteOrder; -import java.util.ArrayList; import java.util.Comparator; import java.util.List; import java.util.Map; @@ -30,20 +29,12 @@ public GeoPointFieldBlockLoaderTests(BlockLoaderTestCase.Params params) { @Override @SuppressWarnings("unchecked") - protected Object expected(Map fieldMapping, Object value, TestContext testContext) { - var extractedFieldValues = (ExtractedFieldValues) value; - var values = extractedFieldValues.values(); - - var rawNullValue = fieldMapping.get("null_value"); - - GeoPoint nullValue; - if (rawNullValue == null) { - nullValue = null; - } else if (rawNullValue instanceof String s) { - nullValue = convert(s, null, false); - } else { - throw new IllegalStateException("Unexpected null_value format"); - } + protected Object expected(Map fieldMapping, Object values, TestContext testContext) { + var nullValue = switch (fieldMapping.get("null_value")) { + case String s -> convert(s, null, false); + case null -> null; + default -> throw new IllegalStateException("Unexpected null_value format"); + }; if (params.preference() == MappedFieldType.FieldExtractPreference.DOC_VALUES && hasDocValues(fieldMapping, true)) { if (values instanceof List == false) { @@ -80,9 +71,6 @@ protected Object expected(Map fieldMapping, Object value, TestCo if (syntheticSourceKeep.equals("all")) { return exactValuesFromSource(values, nullValue, false); } - if (syntheticSourceKeep.equals("arrays") && extractedFieldValues.documentHasObjectArrays()) { - return exactValuesFromSource(values, nullValue, false); - } // synthetic source and doc_values are present if (hasDocValues(fieldMapping, true)) { @@ -117,61 +105,6 @@ private Object exactValuesFromSource(Object value, GeoPoint nullValue, boolean n return maybeFoldList(resultList); } - private record ExtractedFieldValues(Object values, boolean documentHasObjectArrays) {} - - @Override - protected Object getFieldValue(Map document, String fieldName) { - var extracted = new ArrayList<>(); - var documentHasObjectArrays = processLevel(document, fieldName, extracted, false); - - if (extracted.size() == 1) { - return new ExtractedFieldValues(extracted.get(0), documentHasObjectArrays); - } - - return new ExtractedFieldValues(extracted, documentHasObjectArrays); - } - - @SuppressWarnings("unchecked") - private boolean processLevel(Map level, String field, ArrayList extracted, boolean documentHasObjectArrays) { - if (field.contains(".") == false) { - var value = level.get(field); - processLeafLevel(value, extracted); - return documentHasObjectArrays; - } - - var nameInLevel = field.split("\\.")[0]; - var entry = level.get(nameInLevel); - if (entry instanceof Map m) { - return processLevel((Map) m, field.substring(field.indexOf('.') + 1), extracted, documentHasObjectArrays); - } - if (entry instanceof List l) { - for (var object : l) { - processLevel((Map) object, field.substring(field.indexOf('.') + 1), extracted, true); - } - return true; - } - - assert false : "unexpected document structure"; - return false; - } - - private void processLeafLevel(Object value, ArrayList extracted) { - if (value instanceof List l) { - if (l.size() > 0 && l.get(0) instanceof Double) { - // this must be a single point in array form - // we'll put it into a different form here to make our lives a bit easier while implementing `expected` - extracted.add(Map.of("type", "point", "coordinates", l)); - } else { - // this is actually an array of points but there could still be points in array form inside - for (var arrayValue : l) { - processLeafLevel(arrayValue, extracted); - } - } - } else { - extracted.add(value); - } - } - @SuppressWarnings("unchecked") private GeoPoint convert(Object value, GeoPoint nullValue, boolean needsMultifieldAdjustment) { if (value == null) { From f8eca90224075f1dc9ed94ca565dc1bf9c07ceb0 Mon Sep 17 00:00:00 2001 From: Parker Timmins Date: Fri, 9 May 2025 16:22:50 -0500 Subject: [PATCH 2/2] Broke some stuff while merging --- .../GeoPointFieldBlockLoaderTests.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/blockloader/GeoPointFieldBlockLoaderTests.java b/server/src/test/java/org/elasticsearch/index/mapper/blockloader/GeoPointFieldBlockLoaderTests.java index fdb1d89175fef..86547849b3234 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/blockloader/GeoPointFieldBlockLoaderTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/blockloader/GeoPointFieldBlockLoaderTests.java @@ -30,11 +30,16 @@ public GeoPointFieldBlockLoaderTests(BlockLoaderTestCase.Params params) { @Override @SuppressWarnings("unchecked") protected Object expected(Map fieldMapping, Object values, TestContext testContext) { - var nullValue = switch (fieldMapping.get("null_value")) { - case String s -> convert(s, null, false); - case null -> null; - default -> throw new IllegalStateException("Unexpected null_value format"); - }; + var rawNullValue = fieldMapping.get("null_value"); + + GeoPoint nullValue; + if (rawNullValue == null) { + nullValue = null; + } else if (rawNullValue instanceof String s) { + nullValue = convert(s, null, false); + } else { + throw new IllegalStateException("Unexpected null_value format"); + } if (params.preference() == MappedFieldType.FieldExtractPreference.DOC_VALUES && hasDocValues(fieldMapping, true)) { if (values instanceof List == false) {