[RFC] - Support GeoJson Point type format in GeoPoint and XYPoint field #152
Description
Update: It will be mostly just GeoJson Point type format support for both GeoPoint and XYPoint. There won't be code sharing between GeoPoint and XYPoint implementation to reduce code duplication as it requires OpenSearch core package to have awareness of geospatial plugin which is not ideal.
The purpose of this RFC (request for comments) is to gather community feedback on a new proposal to support GeoJson Point type format in GeoPoint and XYPoint field.
Related issue opensearch-project/OpenSearch#3775
GeoJson Point Type
Point type in GeoJson is one of GeoJson type which is used to represent a single position. Point coordinates are in x, y order (easting, northing for projected coordinates, longitude, and latitude for geographic coordinates):
{
"type": "Point",
"coordinates": [41.12, -71.34]
}
Indexing
Below is an example of indexing GeoPoint and XYPoint field using GeoJson Point type format
GeoPoint
Point expressed as a GeoJson Point object.
PUT indexing_geopoint
{
"mappings": {
"properties": {
"point": {
"type": "geo_point"
}
}
}
}
PUT indexing_geopoint/_doc/1
{
"text": "Point as an object using GeoJSON format",
"point": {
"type": "Point",
"coordinates": [41.12, -71.34]
}
}
XYPoint
Point expressed as a GeoJson Point object.
PUT indexing_xypoint
{
"mappings": {
"properties": {
"geometry": {
"type": "xy_point"
}
}
}
}
PUT indexing_xypoint/_doc/1
{
"text": "Point as an object using GeoJSON format",
"geometry": {
"type": "Point",
"coordinates": [41.12, -71.34]
}
}
Implementation
Current
XYPointFieldType and GeoPointFieldType has PointParser. When generating PointParser instance, it passes function parameter using a static method in XYPointParser and GeoUtils respectively. PointParser has parsing logic to handle the outer part of contents and the static method in XYPointParser and GeoUtils has parsing logic of each format for a point. The static methods in GeoUtils and XYPointParser have duplicated logic in them.
Proposal
We will introduce a PointUtils which will have a parsing method for each types which are common for both GeoPoint and XYPoint: Object, GeoJson, and Array. GeoUtils and XYPointParser will have the top level parsing logic and calls PointUtils to handle final data format.
This will remove code duplication between GeoUtils and XYPointParser. This approach also minimize the code change required compared to the other alternatives.
Red box in the following diagram indicate either modified or newly created classes.
Alternatives
1. Adds common point parser class
We will introduce a new CommonPointParser class. CommonPointParser will have the top level parsing logic which is generic to any point type. The CommonPointParser will delegate all type specific tasks to methods of which GeoPointParser and XYPointParser will override.
This change will reduce duplicated code in XYPoint type parser and GeoPoint type parser. However, one of the comments that I received is that GeoUtils having dependency on GeoPointParser doesn’t look right. Also, during implementation, this approach makes the code more complex than the proposed option without gaining much in reducing code duplication.
Red box in the following diagram represent either modified or newly created classes.
2. Adds GeoJson Point type parsing logic in both GeoUtils and XYPointParser
We can just modify existing parsing method in each of GeoUtils and XYPointParser to parse GeoJson point type. Even if the logic of parsing GeoJson is simple, it takes same mount of code change as proposed approach to embed the logic in existing parsing logic.
3. Use existing GeoJson or GeoJsonParser
We could reuse existing GeoJson class or GeoJsonParser class to parse GeoJson point type. However, Current implementation in GeoJson class and GeoJsonParser class parse any type of GeoJson format. Therefore, we cannot fail fast when the data is not GeoJson Point type which is the only GeoJson type we are going to support. There is no separate method only to parse GeoJson Point type in those classes as well. Also, the code duplication issue will remain same as before because other parsing logics will stay in GeoUtils and XYPointParser.
