Revert changes in AbstractPointGeometryFieldMapper

The change made in AbstractPointGeometryFieldMapper class with commit 0503897 introduced a performace degradation during point data indexing. Reverting it therefore. Signed-off-by: Heemin Kim <[email protected]>
opensearch-project · Nov 15, 2022 · 8818409 · 8818409
1 parent b43e985
commit 8818409
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 52 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -75,6 +75,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Bump `opencensus-contrib-http-util` from 0.18.0 to 0.31.1 ([#3633](https://github.com/opensearch-project/OpenSearch/pull/3633))
 - Bump `geoip2` from 3.0.1 to 3.0.2 ([#5103](https://github.com/opensearch-project/OpenSearch/pull/5103))
 ### Changed
+- Revert changes in AbstractPointGeometryFieldMapper ([#5246](https://github.com/opensearch-project/OpenSearch/pull/5246))
 ### Deprecated
 ### Removed
 ### Fixed

diff --git a/server/src/main/java/org/opensearch/index/mapper/AbstractPointGeometryFieldMapper.java b/server/src/main/java/org/opensearch/index/mapper/AbstractPointGeometryFieldMapper.java
@@ -36,14 +36,11 @@
 import org.opensearch.common.CheckedBiFunction;
 import org.opensearch.common.Explicit;
 import org.opensearch.common.ParseField;
-import org.opensearch.common.bytes.BytesReference;
+import org.opensearch.common.geo.GeoPoint;
 import org.opensearch.common.geo.GeometryFormat;
 import org.opensearch.common.geo.GeometryParser;
-import org.opensearch.common.xcontent.DeprecationHandler;
 import org.opensearch.common.xcontent.LoggingDeprecationHandler;
-import org.opensearch.common.xcontent.NamedXContentRegistry;
 import org.opensearch.common.xcontent.XContentBuilder;
-import org.opensearch.common.xcontent.XContentFactory;
 import org.opensearch.common.xcontent.XContentParser;
 import org.opensearch.geometry.Geometry;
 import org.opensearch.geometry.Point;
@@ -245,7 +242,6 @@ default boolean isNormalizable(double coord) {
      * @opensearch.internal
      */
     public static class PointParser<P extends ParsedPoint> extends Parser<List<P>> {
-        private static final int MAX_NUMBER_OF_VALUES_IN_ARRAY_FORMAT = 3;
         /**
          * Note that this parser is only used for formatting values.
          */
@@ -285,27 +281,32 @@ private P process(P in) {
 
         @Override
         public List<P> parse(XContentParser parser) throws IOException, ParseException {
+
             if (parser.currentToken() == XContentParser.Token.START_ARRAY) {
-                parser.nextToken();
-                if (parser.currentToken() == XContentParser.Token.VALUE_NUMBER) {
-                    XContentBuilder xContentBuilder = reconstructArrayXContent(parser);
-                    try (
-                        XContentParser subParser = createParser(
-                            parser.getXContentRegistry(),
-                            parser.getDeprecationHandler(),
-                            xContentBuilder
-                        );
-                    ) {
-                        return Collections.singletonList(process(objectParser.apply(subParser, pointSupplier.get())));
+                XContentParser.Token token = parser.nextToken();
+                P point = pointSupplier.get();
+                ArrayList<P> points = new ArrayList<>();
+                if (token == XContentParser.Token.VALUE_NUMBER) {
+                    double x = parser.doubleValue();
+                    parser.nextToken();
+                    double y = parser.doubleValue();
+                    token = parser.nextToken();
+                    if (token == XContentParser.Token.VALUE_NUMBER) {
+                        GeoPoint.assertZValue(ignoreZValue, parser.doubleValue());
+                    } else if (token != XContentParser.Token.END_ARRAY) {
+                        throw new OpenSearchParseException("field type does not accept > 3 dimensions");
                     }
+
+                    point.resetCoords(x, y);
+                    points.add(process(point));
                 } else {
-                    ArrayList<P> points = new ArrayList<>();
-                    while (parser.currentToken() != XContentParser.Token.END_ARRAY) {
-                        points.add(process(objectParser.apply(parser, pointSupplier.get())));
-                        parser.nextToken();
+                    while (token != XContentParser.Token.END_ARRAY) {
+                        points.add(process(objectParser.apply(parser, point)));
+                        point = pointSupplier.get();
+                        token = parser.nextToken();
                     }
-                    return points;
                 }
+                return points;
             } else if (parser.currentToken() == XContentParser.Token.VALUE_NULL) {
                 if (nullValue == null) {
                     return null;
@@ -317,37 +318,6 @@ public List<P> parse(XContentParser parser) throws IOException, ParseException {
             }
         }
 
-        private XContentParser createParser(
-            NamedXContentRegistry namedXContentRegistry,
-            DeprecationHandler deprecationHandler,
-            XContentBuilder xContentBuilder
-        ) throws IOException {
-            XContentParser subParser = xContentBuilder.contentType()
-                .xContent()
-                .createParser(namedXContentRegistry, deprecationHandler, BytesReference.bytes(xContentBuilder).streamInput());
-            subParser.nextToken();
-            return subParser;
-        }
-
-        private XContentBuilder reconstructArrayXContent(XContentParser parser) throws IOException {
-            XContentBuilder builder = XContentFactory.jsonBuilder().startArray();
-            int numberOfValuesAdded = 0;
-            while (parser.currentToken() != XContentParser.Token.END_ARRAY) {
-                if (parser.currentToken() != XContentParser.Token.VALUE_NUMBER) {
-                    throw new OpenSearchParseException("numeric value expected");
-                }
-                builder.value(parser.doubleValue());
-                parser.nextToken();
-
-                // Allows one more value to be added so that the error case can be handled by a parser
-                if (++numberOfValuesAdded > MAX_NUMBER_OF_VALUES_IN_ARRAY_FORMAT) {
-                    break;
-                }
-            }
-            builder.endArray();
-            return builder;
-        }
-
         @Override
         public Object format(List<P> points, String format) {
             List<Object> result = new ArrayList<>();