From 80379d6e5139ccb715694261a4c02def83a95d4f Mon Sep 17 00:00:00 2001 From: Heemin Kim Date: Mon, 10 Oct 2022 17:40:40 -0700 Subject: [PATCH 1/3] Add geojson support for XYPoint Signed-off-by: Heemin Kim --- .../index/mapper/xypoint/XYPointParser.java | 236 +++++++++++------- .../mapper/xypoint/XYPointFieldMapperIT.java | 26 ++ .../xypoint/XYPointFieldMapperTests.java | 37 +++ ...singTests.java => XYPointParserTests.java} | 75 +++++- .../rest-api-spec/test/xypoint/10_basic.yml | 135 ++++++++++ 5 files changed, 419 insertions(+), 90 deletions(-) rename src/test/java/org/opensearch/geospatial/index/mapper/xypoint/{XYPointParsingTests.java => XYPointParserTests.java} (69%) create mode 100644 src/yamlRestTest/resources/rest-api-spec/test/xypoint/10_basic.yml diff --git a/src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java b/src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java index 191ece83..94bf6d36 100644 --- a/src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java +++ b/src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java @@ -7,21 +7,28 @@ import java.io.IOException; import java.util.Collections; +import java.util.HashMap; import java.util.Objects; import org.opensearch.OpenSearchParseException; +import org.opensearch.common.geo.GeoPoint; import org.opensearch.common.xcontent.LoggingDeprecationHandler; import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.XContentSubParser; import org.opensearch.common.xcontent.support.MapXContentParser; +import org.opensearch.geometry.ShapeType; /** * Parse the value and set XYPoint represented as a String, Object, WKT, array. */ public class XYPointParser { + private static final String ERR_MSG_INVALID_TOKEN = "token [{}] not allowed"; + private static final String ERR_MSG_INVALID_FIELDS = "field must be either [x|y], or [type|coordinates]"; private static final String X_PARAMETER = "x"; private static final String Y_PARAMETER = "y"; + public static final String GEOJSON_TYPE = "type"; + public static final String GEOJSON_COORDS = "coordinates"; private static final String NULL_VALUE_PARAMETER = "null_value"; private static final Boolean TRUE = true; @@ -54,13 +61,13 @@ public static XYPoint parseXYPoint(Object value, final boolean ignoreZValue) thr } /** - * Parse the values to set the XYPoint which was represented as a String, Object, WKT or an array. - * + * Parse the values to set the XYPoint which was represented as a String, Object, WKT, Array, or GeoJson. * * * @param parser {@link XContentParser} to parse the value from @@ -71,101 +78,156 @@ public static XYPoint parseXYPoint(Object value, final boolean ignoreZValue) thr */ public static XYPoint parseXYPoint(XContentParser parser, final boolean ignoreZValue) throws IOException, OpenSearchParseException { Objects.requireNonNull(parser, "parser should not be null"); - XYPoint point = new XYPoint(); - double x = Double.NaN; - double y = Double.NaN; - - if (parser.currentToken() == XContentParser.Token.START_OBJECT) { - try (XContentSubParser subParser = new XContentSubParser(parser)) { - while (subParser.nextToken() != XContentParser.Token.END_OBJECT) { - if (subParser.currentToken() != XContentParser.Token.FIELD_NAME) { - throw new OpenSearchParseException("token [{}] not allowed", subParser.currentToken()); - } - String field = subParser.currentName(); - if (!(X_PARAMETER.equals(field) || Y_PARAMETER.equals(field))) { - throw new OpenSearchParseException("field must be either [{}] or [{}]", X_PARAMETER, Y_PARAMETER); - } - if (X_PARAMETER.equals(field)) { - subParser.nextToken(); - switch (subParser.currentToken()) { - case VALUE_NUMBER: - case VALUE_STRING: - try { - x = subParser.doubleValue(TRUE); - } catch (NumberFormatException numberFormatException) { - throw new OpenSearchParseException("[x] must be valid double value", numberFormatException); - } - break; - default: - throw new OpenSearchParseException("[x] must be a number"); - } - } - if (Y_PARAMETER.equals(field)) { - subParser.nextToken(); - switch (subParser.currentToken()) { - case VALUE_NUMBER: - case VALUE_STRING: - try { - y = subParser.doubleValue(TRUE); - } catch (NumberFormatException numberFormatException) { - throw new OpenSearchParseException("[y] must be valid double value", numberFormatException); - } - break; - default: - throw new OpenSearchParseException("[y] must be a number"); - } - } - } + switch (parser.currentToken()) { + case START_OBJECT: + parseXYPointObject(parser, point, ignoreZValue); + break; + case START_ARRAY: + parseXYPointArray(parser, point, ignoreZValue); + break; + case VALUE_STRING: + String val = parser.text(); + point.resetFromString(val, ignoreZValue); + break; + default: + throw new OpenSearchParseException("xy_point expected"); + } + return point; + } + + private static XYPoint parseXYPointObject(final XContentParser parser, final XYPoint point, final boolean ignoreZValue) + throws IOException { + try (XContentSubParser subParser = new XContentSubParser(parser)) { + if (subParser.nextToken() != XContentParser.Token.FIELD_NAME) { + throw new OpenSearchParseException(ERR_MSG_INVALID_TOKEN, subParser.currentToken()); } - if (Double.isNaN(x)) { - throw new OpenSearchParseException("field [{}] missing", X_PARAMETER); + + String field = subParser.currentName(); + if (X_PARAMETER.equals(field) || Y_PARAMETER.equals(field)) { + parseGeoPointObjectBasicFields(subParser, point); + } else if (GEOJSON_TYPE.equals(field) || GEOJSON_COORDS.equals(field)) { + parseGeoJsonFields(subParser, point, ignoreZValue); + } else { + throw new OpenSearchParseException(ERR_MSG_INVALID_FIELDS); } - if (Double.isNaN(y)) { - throw new OpenSearchParseException("field [{}] missing", Y_PARAMETER); + + if (subParser.nextToken() != XContentParser.Token.END_OBJECT) { + throw new OpenSearchParseException(ERR_MSG_INVALID_FIELDS); } - return point.reset(x, y); + + return point; } + } + + private static XYPoint parseGeoPointObjectBasicFields(final XContentParser parser, final XYPoint point) throws IOException { + HashMap data = new HashMap<>(); + for (int i = 0; i < 2; i++) { + if (i != 0) { + parser.nextToken(); + } + + if (parser.currentToken() != XContentParser.Token.FIELD_NAME) { + break; + } - if (parser.currentToken() == XContentParser.Token.START_ARRAY) { - return parseXYPointArray(parser, ignoreZValue, x, y); + String field = parser.currentName(); + if (X_PARAMETER.equals(field) == false && Y_PARAMETER.equals(field) == false) { + throw new OpenSearchParseException(ERR_MSG_INVALID_FIELDS); + } + switch (parser.nextToken()) { + case VALUE_NUMBER: + case VALUE_STRING: + try { + data.put(field, parser.doubleValue(true)); + } catch (NumberFormatException e) { + throw new OpenSearchParseException("[{}] and [{}] must be valid double values", e, X_PARAMETER, Y_PARAMETER); + } + break; + default: + throw new OpenSearchParseException("{} must be a number", field); + } } - if (parser.currentToken() == XContentParser.Token.VALUE_STRING) { - String val = parser.text(); - return point.resetFromString(val, ignoreZValue); + if (data.get(X_PARAMETER) == null) { + throw new OpenSearchParseException("field [{}] missing", X_PARAMETER); } - throw new OpenSearchParseException("Expected xy_point. But, the provided mapping is not of type xy_point"); + if (data.get(Y_PARAMETER) == null) { + throw new OpenSearchParseException("field [{}] missing", Y_PARAMETER); + } + + return point.reset(data.get(X_PARAMETER), data.get(Y_PARAMETER)); } - /** - * Parse the values to set the XYPoint which was represented as an array. - * - * @param subParser {@link XContentParser} to parse the values from an array - * @param ignoreZValue boolean parameter which decides if third coordinate needs to be ignored or not - * @param x x coordinate that will be set by parsing the value from array - * @param y y coordinate that will be set by parsing the value from array - * @return {@link XYPoint} after setting the x and y coordinates parsed from the parse - * @throws IOException - */ - private static XYPoint parseXYPointArray(XContentParser subParser, final boolean ignoreZValue, double x, double y) throws IOException { - XYPoint point = new XYPoint(); - int element = 0; - while (subParser.nextToken() != XContentParser.Token.END_ARRAY) { - if (subParser.currentToken() != XContentParser.Token.VALUE_NUMBER) { - throw new OpenSearchParseException("numeric value expected"); + private static XYPoint parseGeoJsonFields(final XContentParser parser, final XYPoint point, final boolean ignoreZValue) + throws IOException { + boolean hasTypePoint = false; + boolean hasCoordinates = false; + for (int i = 0; i < 2; i++) { + if (i != 0) { + parser.nextToken(); } - element++; - if (element == 1) { - x = subParser.doubleValue(); - } else if (element == 2) { - y = subParser.doubleValue(); - } else if (element == 3) { - XYPoint.assertZValue(ignoreZValue, subParser.doubleValue()); + + if (parser.currentToken() != XContentParser.Token.FIELD_NAME) { + if (hasTypePoint == false) { + throw new OpenSearchParseException("field [{}] missing", GEOJSON_TYPE); + } + if (hasCoordinates == false) { + throw new OpenSearchParseException("field [{}] missing", GEOJSON_COORDS); + } + } + + if (GEOJSON_TYPE.equals(parser.currentName())) { + if (parser.nextToken() != XContentParser.Token.VALUE_STRING) { + throw new OpenSearchParseException("{} must be a string", GEOJSON_TYPE); + } + + // To be consistent with geo_shape parsing, ignore case here as well. + if (ShapeType.POINT.name().equalsIgnoreCase(parser.text()) == false) { + throw new OpenSearchParseException("{} must be Point", GEOJSON_TYPE); + } + hasTypePoint = true; + } else if (GEOJSON_COORDS.equals(parser.currentName())) { + if (parser.nextToken() != XContentParser.Token.START_ARRAY) { + throw new OpenSearchParseException("{} must be an array", GEOJSON_COORDS); + } + parseXYPointArray(parser, point, ignoreZValue); + hasCoordinates = true; } else { - throw new OpenSearchParseException("[xy_point] field type does not accept more than 3 dimensions"); + throw new OpenSearchParseException(ERR_MSG_INVALID_FIELDS); } } - return point.reset(x, y); + + return point; + } + + private static XYPoint parseXYPointArray(final XContentParser parser, final XYPoint point, final boolean ignoreZValue) + throws IOException { + try (XContentSubParser subParser = new XContentSubParser(parser)) { + double x = Double.NaN; + double y = Double.NaN; + + int element = 0; + while (subParser.nextToken() != XContentParser.Token.END_ARRAY) { + if (parser.currentToken() != XContentParser.Token.VALUE_NUMBER) { + throw new OpenSearchParseException("numeric value expected"); + } + element++; + if (element == 1) { + x = parser.doubleValue(); + } else if (element == 2) { + y = parser.doubleValue(); + } else if (element == 3) { + GeoPoint.assertZValue(ignoreZValue, parser.doubleValue()); + } else { + throw new OpenSearchParseException("[xy_point] field type does not accept more than 3 values"); + } + } + + if (element < 2) { + throw new OpenSearchParseException("[xy_point] field type should have at least two dimensions"); + } + return point.reset(x, y); + } } } diff --git a/src/test/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointFieldMapperIT.java b/src/test/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointFieldMapperIT.java index f7e4c90b..6935cc9b 100644 --- a/src/test/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointFieldMapperIT.java +++ b/src/test/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointFieldMapperIT.java @@ -19,6 +19,9 @@ public class XYPointFieldMapperIT extends GeospatialRestTestCase { private static final String FIELD_X_KEY = "x"; private static final String FIELD_Y_KEY = "y"; + private static final String FIELD_GEOJSON_TYPE_KEY = "type"; + private static final String FIELD_GEOJSON_TYPE_VALUE = "Point"; + private static final String FIELD_GEOJSON_COORDINATES_KEY = "coordinates"; public void testMappingWithXYPointField() throws Exception { String indexName = GeospatialTestHelper.randomLowerCaseString(); @@ -85,6 +88,20 @@ public void testIndexWithXYPointFieldAsObjectFormat() throws Exception { deleteIndex(indexName); } + public void testIndexWithXYPointFieldAsGeoJsonFormat() throws Exception { + String indexName = GeospatialTestHelper.randomLowerCaseString(); + String fieldName = GeospatialTestHelper.randomLowerCaseString(); + createIndex(indexName, Settings.EMPTY, Map.of(fieldName, XYPointFieldMapper.CONTENT_TYPE)); + final Point point = ShapeObjectBuilder.randomPoint(randomBoolean()); + String docID = indexDocument(indexName, getDocumentWithObjectValueForXYPoint(fieldName, point)); + assertTrue("failed to index document", getIndexDocumentCount(indexName) > 0); + final Map document = getDocument(docID, indexName); + assertNotNull("failed to get indexed document", document); + String expectedValue = String.format(Locale.ROOT, "{x=%s, y=%s}", point.getX(), point.getY()); + assertEquals("failed to index xy_point", expectedValue, document.get(fieldName).toString()); + deleteIndex(indexName); + } + private String getDocumentWithWKTValueForXYPoint(String fieldName, Geometry geometry) throws Exception { return buildContentAsString(build -> build.field(fieldName, geometry.toString())); } @@ -106,4 +123,13 @@ private String getDocumentWithObjectValueForXYPoint(String fieldName, Point poin }); } + private String getDocumentWithGeoJsonValueForXYPoint(String fieldName, Point point) throws Exception { + return buildContentAsString(build -> { + build.startObject(fieldName); + build.field(FIELD_GEOJSON_TYPE_KEY, FIELD_GEOJSON_TYPE_VALUE); + build.array(FIELD_GEOJSON_COORDINATES_KEY, new double[] { point.getX(), point.getY() }); + build.endObject(); + }); + } + } diff --git a/src/test/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointFieldMapperTests.java b/src/test/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointFieldMapperTests.java index b31faff8..37369572 100644 --- a/src/test/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointFieldMapperTests.java +++ b/src/test/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointFieldMapperTests.java @@ -29,6 +29,10 @@ public class XYPointFieldMapperTests extends FieldMapperTestCase2 builder.startObject(FIELD_NAME) + .field(FIELD_GEOJSON_TYPE_KEY, FIELD_GEOJSON_TYPE_VALUE) + .array(FIELD_GEOJSON_COORDINATES_KEY, new double[] { randomDouble(), randomDouble() }) + .endObject() + ) + ); + final IndexableField[] actualFieldValues = doc.rootDoc().getFields(FIELD_NAME); + assertNotNull("FieldValue is null", actualFieldValues); + assertEquals("mismatch in field values count", 2, actualFieldValues.length); + } + public void testIndexAsArrayMultiPoints() throws IOException { int numOfPoints = randomIntBetween(MIN_NUM_POINTS, MAX_NUM_POINTS); DocumentMapper mapper = createDocumentMapper(fieldMapping(this::minimalMapping)); @@ -228,6 +247,24 @@ public void testIndexAsWKTMultiPoints() throws IOException { assertEquals("mismatch in field values count", 2 * numOfPoints, actualFieldValues.length); } + public void testIndexAsGeoJsonMultiPoints() throws IOException { + int numOfPoints = randomIntBetween(MIN_NUM_POINTS, MAX_NUM_POINTS); + DocumentMapper mapper = createDocumentMapper(fieldMapping(this::minimalMapping)); + ParsedDocument doc = mapper.parse(source(builder -> { + builder.startArray(FIELD_NAME); + for (int i = 0; i < numOfPoints; i++) { + builder.startObject() + .field(FIELD_GEOJSON_TYPE_KEY, FIELD_GEOJSON_TYPE_VALUE) + .array(FIELD_GEOJSON_COORDINATES_KEY, new double[] { randomDouble(), randomDouble() }) + .endObject(); + } + builder.endArray(); + })); + final IndexableField[] actualFieldValues = doc.rootDoc().getFields(FIELD_NAME); + assertNotNull("FieldValue is null", actualFieldValues); + assertEquals("mismatch in field values count", 2 * numOfPoints, actualFieldValues.length); + } + public void testIgnoreZValue() throws IOException { boolean z_value = randomBoolean(); DocumentMapper mapper = createDocumentMapper( diff --git a/src/test/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParsingTests.java b/src/test/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParserTests.java similarity index 69% rename from src/test/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParsingTests.java rename to src/test/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParserTests.java index a99d6541..b0f1e2a8 100644 --- a/src/test/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParsingTests.java +++ b/src/test/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParserTests.java @@ -5,6 +5,8 @@ package org.opensearch.geospatial.index.mapper.xypoint; +import static org.opensearch.common.xcontent.XContentFactory.jsonBuilder; + import java.io.IOException; import org.opensearch.OpenSearchParseException; @@ -14,7 +16,7 @@ import org.opensearch.common.xcontent.json.JsonXContent; import org.opensearch.test.OpenSearchTestCase; -public class XYPointParsingTests extends OpenSearchTestCase { +public class XYPointParserTests extends OpenSearchTestCase { private static final String FIELD_X_KEY = "x"; private static final String FIELD_Y_KEY = "y"; @@ -104,7 +106,7 @@ public void testInvalidField() throws IOException { OpenSearchParseException.class, () -> XYPointParser.parseXYPoint(parser, randomBoolean()) ); - assertEquals("Validation for invalid fields failed", "field must be either [x] or [y]", e.getMessage()); + assertEquals("Validation for invalid fields failed", "field must be either [x|y], or [type|coordinates]", e.getMessage()); } } @@ -120,7 +122,7 @@ public void testParsingInvalidObject() throws IOException { OpenSearchParseException.class, () -> XYPointParser.parseXYPoint(parser, randomBoolean()) ); - assertEquals("Validation failed for invalid x and y values", "[y] must be valid double value", e.getMessage()); + assertEquals("Validation failed for invalid x and y values", "[x] and [y] must be valid double values", e.getMessage()); // Skip the 'y' field and y coordinate XContentBuilder content1 = JsonXContent.contentBuilder(); @@ -183,4 +185,71 @@ private XContentParser xyAsWKT(double x, double y) throws IOException { parser.nextToken(); return parser; } + + public void testParserGeoPointGeoJson() throws IOException { + XYPoint xyPoint = new XYPoint(randomDouble(), randomDouble()); + double[] coordinates = { xyPoint.getX(), xyPoint.getY() }; + XContentBuilder json1 = jsonBuilder().startObject().field("type", "Point").array("coordinates", coordinates).endObject(); + try (XContentParser parser = createParser(json1)) { + parser.nextToken(); + XYPoint paredPoint = XYPointParser.parseXYPoint(parser, randomBoolean()); + assertEquals(xyPoint, paredPoint); + } + + XContentBuilder json2 = jsonBuilder().startObject().field("type", "PoInT").array("coordinates", coordinates).endObject(); + try (XContentParser parser = createParser(json2)) { + parser.nextToken(); + XYPoint paredPoint = XYPointParser.parseXYPoint(parser, randomBoolean()); + assertEquals(xyPoint, paredPoint); + } + } + + public void testParserGeoPointGeoJsonMissingField() throws IOException { + XYPoint xyPoint = new XYPoint(randomDouble(), randomDouble()); + double[] coordinates = { xyPoint.getX(), xyPoint.getY() }; + XContentBuilder missingType = jsonBuilder().startObject().array("coordinates", coordinates).endObject(); + expectParseException(missingType, "field [type] missing"); + + XContentBuilder missingCoordinates = jsonBuilder().startObject().field("type", "Point").endObject(); + expectParseException(missingCoordinates, "field [coordinates] missing"); + } + + public void testParserGeoPointGeoJsonUnknownField() throws IOException { + XYPoint xyPoint = new XYPoint(randomDouble(), randomDouble()); + double[] coordinates = { xyPoint.getX(), xyPoint.getY() }; + XContentBuilder unknownField = jsonBuilder().startObject() + .field("type", "Point") + .array("coordinates", coordinates) + .field("unknown", "value") + .endObject(); + expectParseException(unknownField, "field must be either [x|y], or [type|coordinates]"); + } + + public void testParserGeoPointGeoJsonInvalidValue() throws IOException { + XYPoint xyPoint = new XYPoint(randomDouble(), randomDouble()); + double[] coordinates = { xyPoint.getX(), xyPoint.getY() }; + XContentBuilder invalidGeoJsonType = jsonBuilder().startObject() + .field("type", "invalid") + .array("coordinates", coordinates) + .endObject(); + expectParseException(invalidGeoJsonType, "type must be Point"); + + String[] coordinatesInString = { String.valueOf(xyPoint.getX()), String.valueOf(xyPoint.getY()) }; + XContentBuilder invalideCoordinatesType = jsonBuilder().startObject() + .field("type", "Point") + .array("coordinates", coordinatesInString) + .endObject(); + expectParseException(invalideCoordinatesType, "numeric value expected"); + } + + private void expectParseException(XContentBuilder content, String errMsg) throws IOException { + try (XContentParser parser = createParser(content)) { + parser.nextToken(); + OpenSearchParseException ex = expectThrows( + OpenSearchParseException.class, + () -> XYPointParser.parseXYPoint(parser, randomBoolean()) + ); + assertEquals(errMsg, ex.getMessage()); + } + } } diff --git a/src/yamlRestTest/resources/rest-api-spec/test/xypoint/10_basic.yml b/src/yamlRestTest/resources/rest-api-spec/test/xypoint/10_basic.yml new file mode 100644 index 00000000..0f8c8c25 --- /dev/null +++ b/src/yamlRestTest/resources/rest-api-spec/test/xypoint/10_basic.yml @@ -0,0 +1,135 @@ +setup: + - do: + indices.create: + index: test_1 + body: + settings: + number_of_replicas: 0 + mappings: + properties: + geometry: + type: xy_point + +--- +"Single point test": + - do: + bulk: + refresh: true + body: + - index: + _index: test_1 + _id: 1 + - geometry: + x: 52.374081 + y: 4.912350 + - index: + _index: test_1 + _id: 2 + - geometry: "52.369219,4.901618" + - index: + _index: test_1 + _id: 3 + - geometry: [ 52.371667, 4.914722 ] + - index: + _index: test_1 + _id: 4 + - geometry: "POINT (52.371667 4.914722)" + - index: + _index: test_1 + _id: 5 + - geometry: + type: Point + coordinates: [ 52.371667, 4.914722 ] + + - do: + search: + index: test_1 + rest_total_hits_as_int: true + body: + query: + xy_shape: + geometry: + shape: + type: "envelope" + coordinates: [ [ 51, 5 ], [ 53, 3 ] ] + + - match: { hits.total: 5 } + + - do: + search: + index: test_1 + rest_total_hits_as_int: true + body: + query: + xy_shape: + geometry: + shape: + type: "envelope" + coordinates: [ [ 151, 15 ], [ 153, 13 ] ] + + - match: { hits.total: 0 } + +--- +"Multi points test": + - do: + bulk: + refresh: true + body: + - index: + _index: test_1 + _id: 1 + - geometry: + - {x: 52.374081, y: 4.912350} + - {x: 152.374081, y: 14.912350} + - index: + _index: test_1 + _id: 2 + - geometry: + - "52.369219,4.901618" + - "152.369219,14.901618" + - index: + _index: test_1 + _id: 3 + - geometry: + - [ 52.371667, 4.914722 ] + - [ 152.371667, 14.914722 ] + - index: + _index: test_1 + _id: 4 + - geometry: + - "POINT (52.371667 4.914722)" + - "POINT (152.371667 14.914722)" + - index: + _index: test_1 + _id: 5 + - geometry: + - {type: Point, coordinates: [ 52.371667, 4.914722 ]} + - {type: Point, coordinates: [ 152.371667, 14.914722 ]} + + - do: + search: + index: test_1 + rest_total_hits_as_int: true + body: + query: + xy_shape: + geometry: + shape: + type: "envelope" + coordinates: [ [ 51, 5 ], [ 53, 3 ] ] + + - match: { hits.total: 5 } + + - do: + search: + index: test_1 + rest_total_hits_as_int: true + body: + query: + xy_shape: + geometry: + shape: + type: "envelope" + coordinates: [ [ 151, 15 ], [ 153, 13 ] ] + + - match: { hits.total: 5 } From 29b5c680121134c309e166ee7cdd0a3c0840885b Mon Sep 17 00:00:00 2001 From: Heemin Kim Date: Wed, 12 Oct 2022 12:49:05 -0700 Subject: [PATCH 2/3] Uses a variable to represent a number Signed-off-by: Heemin Kim --- .../geospatial/index/mapper/xypoint/XYPointParser.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java b/src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java index 94bf6d36..96d26516 100644 --- a/src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java +++ b/src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java @@ -121,8 +121,9 @@ private static XYPoint parseXYPointObject(final XContentParser parser, final XYP } private static XYPoint parseGeoPointObjectBasicFields(final XContentParser parser, final XYPoint point) throws IOException { + final int numberOfFields = 2; HashMap data = new HashMap<>(); - for (int i = 0; i < 2; i++) { + for (int i = 0; i < numberOfFields; i++) { if (i != 0) { parser.nextToken(); } @@ -161,9 +162,10 @@ private static XYPoint parseGeoPointObjectBasicFields(final XContentParser parse private static XYPoint parseGeoJsonFields(final XContentParser parser, final XYPoint point, final boolean ignoreZValue) throws IOException { + final int numberOfFields = 2; boolean hasTypePoint = false; boolean hasCoordinates = false; - for (int i = 0; i < 2; i++) { + for (int i = 0; i < numberOfFields; i++) { if (i != 0) { parser.nextToken(); } From 6e353910f58f8e14a49106d7fa5dca1a744f325d Mon Sep 17 00:00:00 2001 From: Heemin Kim Date: Wed, 12 Oct 2022 13:34:26 -0700 Subject: [PATCH 3/3] Resolve review comments Signed-off-by: Heemin Kim --- .../index/mapper/xypoint/XYPointParser.java | 63 ++++++++++++++++--- 1 file changed, 55 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java b/src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java index 96d26516..cffcf2c5 100644 --- a/src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java +++ b/src/main/java/org/opensearch/geospatial/index/mapper/xypoint/XYPointParser.java @@ -8,10 +8,10 @@ import java.io.IOException; import java.util.Collections; import java.util.HashMap; +import java.util.Map; import java.util.Objects; import org.opensearch.OpenSearchParseException; -import org.opensearch.common.geo.GeoPoint; import org.opensearch.common.xcontent.LoggingDeprecationHandler; import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.XContentParser; @@ -40,7 +40,7 @@ public class XYPointParser { * @return {@link XYPoint} after setting the x and y coordinates parsed from the parse * @throws OpenSearchParseException */ - public static XYPoint parseXYPoint(Object value, final boolean ignoreZValue) throws OpenSearchParseException { + public static XYPoint parseXYPoint(final Object value, final boolean ignoreZValue) throws OpenSearchParseException { Objects.requireNonNull(value, "input value which needs to be parsed should not be null"); try ( @@ -76,7 +76,8 @@ public static XYPoint parseXYPoint(Object value, final boolean ignoreZValue) thr * @throws IOException * @throws OpenSearchParseException */ - public static XYPoint parseXYPoint(XContentParser parser, final boolean ignoreZValue) throws IOException, OpenSearchParseException { + public static XYPoint parseXYPoint(final XContentParser parser, final boolean ignoreZValue) throws IOException, + OpenSearchParseException { Objects.requireNonNull(parser, "parser should not be null"); XYPoint point = new XYPoint(); switch (parser.currentToken()) { @@ -91,11 +92,22 @@ public static XYPoint parseXYPoint(XContentParser parser, final boolean ignoreZV point.resetFromString(val, ignoreZValue); break; default: - throw new OpenSearchParseException("xy_point expected"); + throw new OpenSearchParseException("expecting xy_point as an array, a string, or an object format"); } return point; } + /** + * Parse point in either basic object format or GeoJson format + * + * Parser is expected to be pointing the start of the object. + * ex) Parser is pointing { in {"x": 12.3, "y": 45.6} + * + * @param parser {@link XContentParser} to parse the value from + * @param point {@link XYPoint} to be returned after setting the x and y coordinates parsed from the parse + * @return {@link XYPoint} after setting the x and y coordinates parsed from the parse + * @throws IOException + */ private static XYPoint parseXYPointObject(final XContentParser parser, final XYPoint point, final boolean ignoreZValue) throws IOException { try (XContentSubParser subParser = new XContentSubParser(parser)) { @@ -105,7 +117,7 @@ private static XYPoint parseXYPointObject(final XContentParser parser, final XYP String field = subParser.currentName(); if (X_PARAMETER.equals(field) || Y_PARAMETER.equals(field)) { - parseGeoPointObjectBasicFields(subParser, point); + parseXYPointObjectBasicFields(subParser, point); } else if (GEOJSON_TYPE.equals(field) || GEOJSON_COORDS.equals(field)) { parseGeoJsonFields(subParser, point, ignoreZValue); } else { @@ -120,9 +132,20 @@ private static XYPoint parseXYPointObject(final XContentParser parser, final XYP } } - private static XYPoint parseGeoPointObjectBasicFields(final XContentParser parser, final XYPoint point) throws IOException { + /** + * Parse point in basic object format + * + * Parser is expected to be pointing the first field of the object. + * ex) Parser is pointing x in {"x": 12.3, "y": 45.6} + * + * @param parser {@link XContentParser} to parse the value from + * @param point {@link XYPoint} to be returned after setting the x and y coordinates parsed from the parse + * @return {@link XYPoint} after setting the x and y coordinates parsed from the parse + * @throws IOException + */ + private static XYPoint parseXYPointObjectBasicFields(final XContentParser parser, final XYPoint point) throws IOException { final int numberOfFields = 2; - HashMap data = new HashMap<>(); + Map data = new HashMap<>(); for (int i = 0; i < numberOfFields; i++) { if (i != 0) { parser.nextToken(); @@ -160,6 +183,18 @@ private static XYPoint parseGeoPointObjectBasicFields(final XContentParser parse return point.reset(data.get(X_PARAMETER), data.get(Y_PARAMETER)); } + /** + * Parse point in GeoJson format + * + * Parser is expected to be pointing the first field of the object. + * ex) Parser is pointing type in {"type": "Point", "coordinates": [12.3, 45.6]} + * + * @param parser {@link XContentParser} to parse the value from + * @param point {@link XYPoint} to be returned after setting the x and y coordinates parsed from the parse + * @param ignoreZValue boolean parameter which decides if third coordinate needs to be ignored or not + * @return {@link XYPoint} after setting the x and y coordinates parsed from the parse + * @throws IOException + */ private static XYPoint parseGeoJsonFields(final XContentParser parser, final XYPoint point, final boolean ignoreZValue) throws IOException { final int numberOfFields = 2; @@ -203,6 +238,18 @@ private static XYPoint parseGeoJsonFields(final XContentParser parser, final XYP return point; } + /** + * Parse point in an array format + * + * Parser is expected to be pointing the start of the array. + * ex) Parser is pointing [ in [12.3, 45.6] + * + * @param parser {@link XContentParser} to parse the value from + * @param point {@link XYPoint} to be returned after setting the x and y coordinates parsed from the parse + * @param ignoreZValue boolean parameter which decides if third coordinate needs to be ignored or not + * @return {@link XYPoint} after setting the x and y coordinates parsed from the parse + * @throws IOException + */ private static XYPoint parseXYPointArray(final XContentParser parser, final XYPoint point, final boolean ignoreZValue) throws IOException { try (XContentSubParser subParser = new XContentSubParser(parser)) { @@ -220,7 +267,7 @@ private static XYPoint parseXYPointArray(final XContentParser parser, final XYPo } else if (element == 2) { y = parser.doubleValue(); } else if (element == 3) { - GeoPoint.assertZValue(ignoreZValue, parser.doubleValue()); + XYPoint.assertZValue(ignoreZValue, parser.doubleValue()); } else { throw new OpenSearchParseException("[xy_point] field type does not accept more than 3 values"); }