Skip to content

Commit 1ab15af

Browse files
krocky-cookykrocky-cooky
authored
Fix copy_to functionality for geo_point fields with object/array values (opensearch-project#20542)
This change enables copy_to functionality for geo_point fields when they are represented as objects or arrays. Previously, copy_to would fail for geo_point fields because the parser consumed tokens during the initial parse, leaving nothing for the copy_to operation. The fix serializes complex field values (objects/arrays) to bytes before parsing, allowing the same data to be parsed multiple times for both the original field and copy_to targets. Changes: - Add parseFieldWithCopyTo() method to handle field parsing with copy_to support for both simple and complex value types - Add parseChildToBytes() helper to serialize current parser state - Extend parseCopyFields() to accept byte array for reparsing - Add test case for geo_point with copy_to attribute Signed-off-by: krocky-cooky <kurotaku9679.sub@gmail.com> Signed-off-by: Takuo Kuroki <kurotaku9679.sub@gmail.com>
1 parent 564cbee commit 1ab15af

File tree

4 files changed

+141
-6
lines changed

4 files changed

+141
-6
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
3737
- Changing opensearch.cgroups.hierarchy.override causes java.lang.SecurityException exception ([#20565](https://github.com/opensearch-project/OpenSearch/pull/20565))
3838
- Fix CriteriaBasedCodec to work with delegate codec. ([#20442](https://github.com/opensearch-project/OpenSearch/pull/20442))
3939
- Fix WLM workload group creation failing due to updated_at clock skew ([#20486](https://github.com/opensearch-project/OpenSearch/pull/20486))
40+
- Fix copy_to functionality for geo_point fields with object/array values ([#20542](https://github.com/opensearch-project/OpenSearch/pull/20542))
4041
- Fix SLF4J component error ([#20587](https://github.com/opensearch-project/OpenSearch/pull/20587))
4142
- Service does not start on Windows with OpenJDK ([#20615](https://github.com/opensearch-project/OpenSearch/pull/20615))
4243
- Update RemoteClusterStateCleanupManager to performed batched deletions of stale ClusterMetadataManifests and address deletion timeout issues ([#20566](https://github.com/opensearch-project/OpenSearch/pull/20566))

‎server/src/main/java/org/opensearch/index/mapper/DocumentParser.java‎

Lines changed: 62 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,13 +37,16 @@
3737
import org.opensearch.OpenSearchParseException;
3838
import org.opensearch.Version;
3939
import org.opensearch.cluster.metadata.IndexMetadata;
40+
import org.opensearch.common.CheckedBiConsumer;
4041
import org.opensearch.common.collect.Tuple;
4142
import org.opensearch.common.settings.Settings;
4243
import org.opensearch.common.time.DateFormatter;
4344
import org.opensearch.common.xcontent.LoggingDeprecationHandler;
4445
import org.opensearch.common.xcontent.XContentHelper;
4546
import org.opensearch.core.common.Strings;
47+
import org.opensearch.core.common.bytes.BytesReference;
4648
import org.opensearch.core.xcontent.MediaType;
49+
import org.opensearch.core.xcontent.XContentBuilder;
4750
import org.opensearch.core.xcontent.XContentParser;
4851
import org.opensearch.index.IndexSettings;
4952
import org.opensearch.index.mapper.DynamicTemplate.XContentFieldType;
@@ -766,8 +769,7 @@ private static void parseObjectOrField(ParseContext context, Mapper mapper) thro
766769
if (mapper instanceof ObjectMapper objectMapper) {
767770
parseObjectMapper(context, objectMapper);
768771
} else if (mapper instanceof FieldMapper fieldMapper) {
769-
fieldMapper.parse(context);
770-
parseCopyFields(context, fieldMapper.copyTo().copyToFields());
772+
parseFieldWithCopyTo(context, fieldMapper);
771773
} else if (mapper instanceof FieldAliasMapper) {
772774
throw new IllegalArgumentException("Cannot write to a field alias [" + mapper.name() + "].");
773775
} else {
@@ -847,8 +849,7 @@ private static void processExistingMapperForDisableObjects(ParseContext context,
847849
throws IOException {
848850
switch (existingMapper) {
849851
case FieldMapper fm -> {
850-
fm.parse(context);
851-
parseCopyFields(context, fm.copyTo().copyToFields());
852+
parseFieldWithCopyTo(context, fm);
852853
}
853854
case ObjectMapper om -> {
854855
// Even with disable_objects, we can have nested ObjectMappers for explicitly defined sub-objects
@@ -1339,7 +1340,30 @@ private static void parseDynamicValue(
13391340
}
13401341

13411342
/** Creates instances of the fields that the current field should be copied to */
1343+
private static void parseCopyFields(ParseContext context, List<String> copyToFields, byte[] sourceBytes) throws IOException {
1344+
parseCopyFieldsInternal(context, copyToFields, (copyToContext, field) -> {
1345+
XContentParser parser = copyToContext.parser();
1346+
try (
1347+
XContentParser innerParser = parser.contentType()
1348+
.xContent()
1349+
.createParser(parser.getXContentRegistry(), parser.getDeprecationHandler(), sourceBytes)
1350+
) {
1351+
innerParser.nextToken();
1352+
ParseContext parserSwitchedContext = copyToContext.switchParser(innerParser);
1353+
parseCopy(field, parserSwitchedContext);
1354+
}
1355+
});
1356+
}
1357+
13421358
private static void parseCopyFields(ParseContext context, List<String> copyToFields) throws IOException {
1359+
parseCopyFieldsInternal(context, copyToFields, (copyToContext, field) -> parseCopy(field, copyToContext));
1360+
}
1361+
1362+
private static void parseCopyFieldsInternal(
1363+
ParseContext context,
1364+
List<String> copyToFields,
1365+
CheckedBiConsumer<ParseContext, String, IOException> copyAction
1366+
) throws IOException {
13431367
if (!context.isWithinCopyTo() && copyToFields.isEmpty() == false) {
13441368
context = context.createCopyToContext();
13451369
for (String field : copyToFields) {
@@ -1353,13 +1377,13 @@ private static void parseCopyFields(ParseContext context, List<String> copyToFie
13531377
}
13541378
}
13551379
assert targetDoc != null;
1356-
final ParseContext copyToContext;
1380+
ParseContext copyToContext;
13571381
if (targetDoc == context.doc()) {
13581382
copyToContext = context;
13591383
} else {
13601384
copyToContext = context.switchDoc(targetDoc);
13611385
}
1362-
parseCopy(field, copyToContext);
1386+
copyAction.accept(copyToContext, field);
13631387
}
13641388
}
13651389
}
@@ -1530,6 +1554,38 @@ private static Mapper getMapper(final ParseContext context, ObjectMapper objectM
15301554
return objectMapper.getMapper(subfields[subfields.length - 1]);
15311555
}
15321556

1557+
private static byte[] parseChildToBytes(ParseContext context) throws IOException {
1558+
XContentParser parser = context.parser();
1559+
try (XContentBuilder builder = XContentBuilder.builder(parser.contentType().xContent())) {
1560+
builder.copyCurrentStructure(parser);
1561+
return BytesReference.toBytes(BytesReference.bytes(builder));
1562+
}
1563+
}
1564+
1565+
private static void parseFieldWithCopyTo(ParseContext context, FieldMapper fieldMapper) throws IOException {
1566+
XContentParser.Token token = context.parser().currentToken();
1567+
if ((token == XContentParser.Token.START_ARRAY || token == XContentParser.Token.START_OBJECT)
1568+
&& !fieldMapper.copyTo().copyToFields().isEmpty()) {
1569+
byte[] childBytes = parseChildToBytes(context);
1570+
// After parseChildToBytes, the original parser has consumed the full structure.
1571+
// Parse the field using a fresh parser over the captured bytes.
1572+
XContentParser parser = context.parser();
1573+
try (
1574+
XContentParser innerParser = parser.contentType()
1575+
.xContent()
1576+
.createParser(parser.getXContentRegistry(), parser.getDeprecationHandler(), childBytes)
1577+
) {
1578+
innerParser.nextToken();
1579+
ParseContext innerContext = context.switchParser(innerParser);
1580+
fieldMapper.parse(innerContext);
1581+
}
1582+
parseCopyFields(context, fieldMapper.copyTo().copyToFields(), childBytes);
1583+
} else {
1584+
fieldMapper.parse(context);
1585+
parseCopyFields(context, fieldMapper.copyTo().copyToFields());
1586+
}
1587+
}
1588+
15331589
// Throws exception if no dynamic templates found but `dynamic` is set to strict_allow_templates
15341590
@SuppressWarnings("rawtypes")
15351591
private static Mapper.Builder findTemplateBuilder(

‎server/src/main/java/org/opensearch/index/mapper/ParseContext.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -700,6 +700,15 @@ public final ParseContext createNestedContext(String fullPath) {
700700
return switchDoc(doc);
701701
}
702702

703+
public final ParseContext switchParser(final XContentParser parser) {
704+
return new FilterParseContext(this) {
705+
@Override
706+
public XContentParser parser() {
707+
return parser;
708+
}
709+
};
710+
}
711+
703712
/**
704713
* Return a new context that has the provided document as the current document.
705714
*/

‎server/src/test/java/org/opensearch/index/mapper/DocumentParserTests.java‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3505,4 +3505,73 @@ public void testParseNonDynamicArrayWithNullValuesDisableObjects() throws Except
35053505
// Null fields should not be created
35063506
assertNull("Null field should not be created", doc.rootDoc().getField("mixed_nulls.null_field"));
35073507
}
3508+
3509+
public void testGeoPointWithCopyTo() throws Exception {
3510+
DocumentMapper mapper = createDocumentMapper(mapping(b -> {
3511+
b.startObject("point");
3512+
{
3513+
b.field("type", "geo_point");
3514+
b.field("copy_to", "location_copy");
3515+
}
3516+
b.endObject();
3517+
b.startObject("location_copy").field("type", "geo_point").endObject();
3518+
}));
3519+
3520+
ParsedDocument doc = mapper.parse(source(b -> { b.startObject("point").field("lat", 40.71).field("lon", 74.00).endObject(); }));
3521+
3522+
// Verify that the geo_point field itself exists
3523+
assertNotNull(doc.rootDoc().getField("point"));
3524+
3525+
// Verify that the field was copied to the copy_to target
3526+
IndexableField[] copiedFields = doc.rootDoc().getFields("location_copy");
3527+
assertNotNull(copiedFields);
3528+
assertTrue(copiedFields.length > 0);
3529+
}
3530+
3531+
public void testGeoPointArrayWithCopyTo() throws Exception {
3532+
DocumentMapper mapper = createDocumentMapper(mapping(b -> {
3533+
b.startObject("point");
3534+
{
3535+
b.field("type", "geo_point");
3536+
b.field("copy_to", "location_copy");
3537+
}
3538+
b.endObject();
3539+
b.startObject("location_copy").field("type", "geo_point").endObject();
3540+
}));
3541+
3542+
ParsedDocument doc = mapper.parse(source(b -> { b.array("point", 74.00, 40.71); }));
3543+
3544+
// Verify that the geo_point field itself exists
3545+
assertNotNull(doc.rootDoc().getField("point"));
3546+
3547+
// Verify that the field was copied to the copy_to target
3548+
IndexableField[] copiedFields = doc.rootDoc().getFields("location_copy");
3549+
assertNotNull(copiedFields);
3550+
assertTrue(copiedFields.length > 0);
3551+
}
3552+
3553+
public void testGeoPointArrayWithMultipleCopyTo() throws Exception {
3554+
DocumentMapper mapper = createDocumentMapper(mapping(b -> {
3555+
b.startObject("point");
3556+
{
3557+
b.field("type", "geo_point");
3558+
b.array("copy_to", "location_copy1", "location_copy2");
3559+
}
3560+
b.endObject();
3561+
b.startObject("location_copy1").field("type", "geo_point").endObject();
3562+
b.startObject("location_copy2").field("type", "geo_point").endObject();
3563+
}));
3564+
3565+
ParsedDocument doc = mapper.parse(source(b -> { b.startObject("point").field("lat", 40.71).field("lon", 74.00).endObject(); }));
3566+
3567+
assertNotNull(doc.rootDoc().getField("point"));
3568+
3569+
IndexableField[] copy1Fields = doc.rootDoc().getFields("location_copy1");
3570+
assertNotNull(copy1Fields);
3571+
assertTrue(copy1Fields.length > 0);
3572+
3573+
IndexableField[] copy2Fields = doc.rootDoc().getFields("location_copy2");
3574+
assertNotNull(copy2Fields);
3575+
assertTrue(copy2Fields.length > 0);
3576+
}
35083577
}

0 commit comments

Comments
 (0)