Support geopoint type (#309)

* Added changes from POC PR Signed-off-by: Guian Gumpac <[email protected]> * Added geopoint parser for value factory Signed-off-by: Guian Gumpac <[email protected]> * Fixed old tests Signed-off-by: Guian Gumpac <[email protected]> * Fixed all old tests Signed-off-by: Guian Gumpac <[email protected]> * Removed irrelevant changes Signed-off-by: Guian Gumpac <[email protected]> * Removed irrelevant changes Signed-off-by: Guian Gumpac <[email protected]> * Added integration tests Signed-off-by: Guian Gumpac <[email protected]> * Fixed not throwing exception for geojson Signed-off-by: Guian Gumpac <[email protected]> * Made GeoPointValue an ExprTupleValue to enable access to lat and lon Signed-off-by: Guian Gumpac <[email protected]> * Fixed checkstyle Signed-off-by: Guian Gumpac <[email protected]> * Added unit tests and removed unnecessary code Signed-off-by: Guian Gumpac <[email protected]> * reverted irrelevant changes Signed-off-by: Guian Gumpac <[email protected]> * Added to docs Signed-off-by: Guian Gumpac <[email protected]> * Fixed doc typo Signed-off-by: Guian Gumpac <[email protected]> * Test doctests Signed-off-by: Guian Gumpac <[email protected]> * Test doctests Signed-off-by: Guian Gumpac <[email protected]> * Remove geopoint data from doctests Signed-off-by: Guian Gumpac <[email protected]> * Fixed doctest failure Signed-off-by: Guian Gumpac <[email protected]> * Fixed doctest failure Signed-off-by: Guian Gumpac <[email protected]> * Fix doctest clean up. Signed-off-by: Yury-Fridlyand <[email protected]> * Minimized doc example Signed-off-by: Guian Gumpac <[email protected]> * Fixed doctest Signed-off-by: Guian Gumpac <[email protected]> * Fixed doctest Signed-off-by: Guian Gumpac <[email protected]> * Fixed doctest Signed-off-by: Guian Gumpac <[email protected]> * Fixed doctest Signed-off-by: Guian Gumpac <[email protected]> * Fixed reordering of results Signed-off-by: Guian Gumpac <[email protected]> * Added more rows to doctests and removed IOException: Signed-off-by: Guian Gumpac <[email protected]> * Fixed doctest Signed-off-by: Guian Gumpac <[email protected]> * Fixed doctest Signed-off-by: Guian Gumpac <[email protected]> * Addressed PR comments Signed-off-by: Guian Gumpac <[email protected]> --------- Signed-off-by: Guian Gumpac <[email protected]> Signed-off-by: Yury-Fridlyand <[email protected]> Co-authored-by: Yury-Fridlyand <[email protected]>
Bit-Quill · Jul 27, 2023 · 937f82b · 937f82b
1 parent 430d7a9
commit 937f82b
Show file tree

Hide file tree

Showing 19 changed files with 262 additions and 78 deletions.
diff --git a/docs/user/dql/metadata.rst b/docs/user/dql/metadata.rst
@@ -35,7 +35,7 @@ Example 1: Show All Indices Information
 SQL query::
 
     os> SHOW TABLES LIKE '%'
-    fetched rows / total rows = 9/9
+    fetched rows / total rows = 10/10
     +----------------+---------------+-----------------+--------------+-----------+------------+--------------+-------------+-----------------------------+------------------+
     | TABLE_CAT      | TABLE_SCHEM   | TABLE_NAME      | TABLE_TYPE   | REMARKS   | TYPE_CAT   | TYPE_SCHEM   | TYPE_NAME   | SELF_REFERENCING_COL_NAME   | REF_GENERATION   |
     |----------------+---------------+-----------------+--------------+-----------+------------+--------------+-------------+-----------------------------+------------------|
@@ -44,6 +44,7 @@ SQL query::
     | docTestCluster | null          | accounts        | BASE TABLE   | null      | null       | null         | null        | null                        | null             |
     | docTestCluster | null          | apache          | BASE TABLE   | null      | null       | null         | null        | null                        | null             |
     | docTestCluster | null          | books           | BASE TABLE   | null      | null       | null         | null        | null                        | null             |
+    | docTestCluster | null          | geopoint        | BASE TABLE   | null      | null       | null         | null        | null                        | null             |
     | docTestCluster | null          | nested          | BASE TABLE   | null      | null       | null         | null        | null                        | null             |
     | docTestCluster | null          | nyc_taxi        | BASE TABLE   | null      | null       | null         | null        | null                        | null             |
     | docTestCluster | null          | people          | BASE TABLE   | null      | null       | null         | null        | null                        | null             |

diff --git a/docs/user/general/datatypes.rst b/docs/user/general/datatypes.rst
@@ -103,6 +103,8 @@ The table below list the mapping between OpenSearch Data Type, OpenSearch SQL Da
 +-----------------+---------------------+-----------+
 | nested          | array               | STRUCT    |
 +-----------------+---------------------+-----------+
+| geo_point       | geo_point           | STRUCT    |
++-----------------+---------------------+-----------+
 
 Notes:
 * Not all the OpenSearch SQL Type has correspond OpenSearch Type. e.g. data and time. To use function which required such data type, user should explicitly convert the data type.
@@ -456,3 +458,18 @@ A boolean can be represented by constant value ``TRUE`` or ``FALSE``. Besides, c
     |--------+---------+---------------------------+----------------------------|
     | True   | False   | True                      | False                      |
     +--------+---------+---------------------------+----------------------------+
+
+Geopoint Data Types
+==================
+
+A geopoint has a latitude and a longitude property. Although OpenSearch `supports multiple formats <https://opensearch.org/docs/2.3/opensearch/supported-field-types/geo-point/>`_, the SQL plugin currently only supports the format :code:`{"lat": number, "lon": number}`. The geopoint object can be queried or lat and lon can be specified using dot notation. For example, ::
+
+    os> SELECT geo_point_object, geo_point_object.lat, geo_point_object.lon FROM geopoint;
+    fetched rows / total rows = 3/3
+    +----------------------------------+------------------------+------------------------+
+    | geo_point_object                 | geo_point_object.lat   | geo_point_object.lon   |
+    |----------------------------------+------------------------+------------------------|
+    | {'lat': 40.71, 'lon': 74.0}      | 40.71                  | 74.0                   |
+    | {'lat': -33.852, 'lon': 151.216} | -33.852                | 151.216                |
+    | null                             | null                   | null                   |
+    +----------------------------------+------------------------+------------------------+
diff --git a/doctest/test_data/geopoint.json b/doctest/test_data/geopoint.json
@@ -0,0 +1,3 @@
+{"geo_point_object": {"lat": 40.71, "lon": 74.00}}
+{"geo_point_object": {"lat": -33.852, "lon": 151.216}}
+{"geo_point_object": null}
diff --git a/doctest/test_docs.py b/doctest/test_docs.py
@@ -28,6 +28,7 @@
 APACHE = "apache"
 WILDCARD = "wildcard"
 NESTED = "nested"
+GEOPOINT = "geopoint"
 DATASOURCES = ".ql-datasources"
 
 
@@ -97,6 +98,7 @@ def set_up_test_indices(test):
     load_file("apache.json", index_name=APACHE)
     load_file("wildcard.json", index_name=WILDCARD)
     load_file("nested_objects.json", index_name=NESTED)
+    load_file("geopoint.json", index_name=GEOPOINT)
     load_file("datasources.json", index_name=DATASOURCES)
 
 
@@ -126,7 +128,7 @@ def set_up(test):
 
 def tear_down(test):
     # drop leftover tables after each test
-    test_data_client.indices.delete(index=[ACCOUNTS, EMPLOYEES, PEOPLE, ACCOUNT2, NYC_TAXI, BOOKS, APACHE, WILDCARD, NESTED], ignore_unavailable=True)
+    test_data_client.indices.delete(index=[ACCOUNTS, EMPLOYEES, PEOPLE, ACCOUNT2, NYC_TAXI, BOOKS, APACHE, WILDCARD, NESTED, GEOPOINT], ignore_unavailable=True)
 
 
 docsuite = partial(doctest.DocFileSuite,

diff --git a/doctest/test_mapping/geopoint.json b/doctest/test_mapping/geopoint.json
@@ -0,0 +1,9 @@
+{
+  "mappings": {
+    "properties": {
+      "geo_point_object": {
+        "type": "geo_point"
+      }
+    }
+  }
+}
diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java
@@ -681,7 +681,11 @@ public enum Index {
     NESTED_WITH_NULLS(TestsConstants.TEST_INDEX_NESTED_WITH_NULLS,
         "multi_nested",
         getNestedTypeIndexMapping(),
-        "src/test/resources/nested_with_nulls.json");
+        "src/test/resources/nested_with_nulls.json"),
+    GEOPOINT(TestsConstants.TEST_INDEX_GEOPOINT,
+        "geopoint",
+        getMappingFile("geo_point_mapping.json"),
+        "src/test/resources/geo_point.json");
 
     private final String name;
     private final String type;

diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/TestsConstants.java b/integ-test/src/test/java/org/opensearch/sql/legacy/TestsConstants.java
@@ -60,6 +60,7 @@ public class TestsConstants {
   public final static String TEST_INDEX_WILDCARD = TEST_INDEX + "_wildcard";
   public final static String TEST_INDEX_MULTI_NESTED_TYPE = TEST_INDEX + "_multi_nested";
   public final static String TEST_INDEX_NESTED_WITH_NULLS = TEST_INDEX + "_nested_with_nulls";
+  public final static String TEST_INDEX_GEOPOINT = TEST_INDEX + "_geopoint";
   public final static String DATASOURCES = ".ql-datasources";
 
   public final static String DATE_FORMAT = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'";

diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/GeoPointIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/GeoPointIT.java
@@ -0,0 +1,120 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package org.opensearch.sql.sql;
+
+import org.json.JSONObject;
+import org.junit.Test;
+import org.opensearch.sql.legacy.SQLIntegTestCase;
+
+import java.util.Map;
+
+import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_GEOPOINT;
+import static org.opensearch.sql.util.MatcherUtils.rows;
+import static org.opensearch.sql.util.MatcherUtils.verifyDataRows;
+
+public class GeoPointIT extends SQLIntegTestCase {
+  @Override
+  protected void init() throws Exception {
+    loadIndex(Index.GEOPOINT);
+  }
+
+  @Test
+  public void test_geo_point() {
+    String query = "SELECT geo_point_object FROM " + TEST_INDEX_GEOPOINT;
+    JSONObject result = executeJdbcRequest(query);
+    verifyDataRows(result,
+        rows(new JSONObject(Map.of(
+                "lat", 40.71,
+                "lon", 74))),
+        rows(new JSONObject(Map.of(
+            "lat", -33.85253637358241,
+            "lon", 151.21652352950258))),
+        rows(JSONObject.NULL)
+    );
+  }
+
+  @Test
+  public void test_geo_point_unsupported_format() {
+    String query = "SELECT geo_point_geohash FROM " + TEST_INDEX_GEOPOINT;
+    Exception exception = assertThrows(RuntimeException.class,
+        () -> executeJdbcRequest(query));
+
+    assertTrue(exception.getMessage().contains(
+            "  \"error\": {\n" +
+            "    \"reason\": \"There was internal problem at backend\",\n" +
+            "    \"details\": \"geo point must be in format of {\\\"lat\\\": number, \\\"lon\\\": number}\",\n" +
+            "    \"type\": \"IllegalStateException\"\n" +
+            "  }"
+    ));
+  }
+
+  @Test
+  public void test_geo_point_in_objects() {
+    String query = "SELECT object.geo_point_object FROM " + TEST_INDEX_GEOPOINT;
+    JSONObject result = executeJdbcRequest(query);
+    verifyDataRows(result,
+        rows(
+            (new JSONObject(Map.of(
+                "lat", 40.71,
+                "lon", 74)))),
+        rows(new JSONObject(Map.of(
+            "lat", -33.85253637358241,
+            "lon", 151.21652352950258))),
+        rows(JSONObject.NULL)
+    );
+  }
+
+  @Test
+  public void test_geo_point_lat_in_objects() {
+    String query = "SELECT object.geo_point_object.lat FROM " + TEST_INDEX_GEOPOINT;
+    JSONObject result = executeJdbcRequest(query);
+    verifyDataRows(result,
+        rows(40.71),
+        rows( -33.85253637358241),
+        rows(JSONObject.NULL)
+    );
+  }
+
+  @Test
+  public void test_geo_point_lat_and_lon() {
+    String query = "SELECT geo_point_object.lat, geo_point_object.lon FROM " + TEST_INDEX_GEOPOINT;
+    JSONObject result = executeJdbcRequest(query);
+    verifyDataRows(result,
+        rows(40.71, 74),
+        rows(-33.85253637358241, 151.21652352950258),
+        rows(JSONObject.NULL, JSONObject.NULL)
+    );
+  }
+
+  @Test
+  public void test_geo_point_object_with_lat_and_lon() {
+    String query = "SELECT geo_point_object, geo_point_object.lat," +
+        " geo_point_object.lon FROM " + TEST_INDEX_GEOPOINT;
+    JSONObject result = executeJdbcRequest(query);
+    verifyDataRows(result,
+        rows(new JSONObject(Map.of(
+            "lat", 40.71,
+            "lon", 74)),
+            40.71, 74),
+        rows(new JSONObject(Map.of(
+            "lat", -33.85253637358241,
+            "lon", 151.21652352950258)),
+            -33.85253637358241, 151.21652352950258),
+        rows(JSONObject.NULL, JSONObject.NULL, JSONObject.NULL)
+    );
+  }
+
+  @Test
+  public void test_geo_point_lat_in_functions() {
+    String query = "SELECT ABS(geo_point_object.lat) FROM " + TEST_INDEX_GEOPOINT;
+    JSONObject result = executeJdbcRequest(query);
+    verifyDataRows(result,
+        rows(40.71),
+        rows(33.85253637358241),
+        rows(JSONObject.NULL)
+    );
+  }
+}
diff --git a/integ-test/src/test/resources/geo_point.json b/integ-test/src/test/resources/geo_point.json
@@ -0,0 +1,6 @@
+{"index":{"_id":"1"}}
+{"geo_point_object": {"lat": 40.71, "lon": 74.00}, "object": {"geo_point_object": {"lat": 40.71, "lon": 74.00}}, "geo_point_string": "40.71, 74.00", "geo_point_geohash": "txhxegj0uyp3", "geo_point_array": [74.00, 40.71], "geo_point_string_point": "POINT (74.00 40.71)", "geo_point_geojson": {"type": "Point", "coordinates": [74.00, 40.71]}}
+{"index":{"_id":"2"}}
+{"geo_point_object": {"lat": -33.85253637358241, "lon": 151.21652352950258}, "object": {"geo_point_object": {"lat": -33.85253637358241, "lon": 151.21652352950258}},"geo_point_string": "-33.85356510743158, 151.22222172610114", "geo_point_geohash": "txhxegj0uyp3", "geo_point_array": [74.00, 40.71], "geo_point_string_point": "POINT (74.00 40.71)", "geo_point_geojson": {"type": "Point", "coordinates": [74.00, 40.71]}}
+{"index":{"_id":"3"}}
+{"geo_point_object": null, "object": {"geo_point_object": null},"geo_point_string": null, "geo_point_geohash": null, "geo_point_array": null, "geo_point_string_point": null, "geo_point_geojson": null}
diff --git a/integ-test/src/test/resources/indexDefinitions/geo_point_mapping.json b/integ-test/src/test/resources/indexDefinitions/geo_point_mapping.json
@@ -0,0 +1,32 @@
+{
+  "mappings": {
+    "properties": {
+      "geo_point_object": {
+        "type": "geo_point"
+      },
+      "object": {
+        "type": "object",
+        "properties": {
+          "geo_point_object": {
+            "type": "geo_point"
+          }
+        }
+      },
+      "geo_point_string": {
+        "type": "geo_point"
+      },
+      "geo_point_geohash": {
+        "type": "geo_point"
+      },
+      "geo_point_array": {
+        "type": "geo_point"
+      },
+      "geo_point_string_point": {
+        "type": "geo_point"
+      },
+      "geo_point_geojson": {
+        "type": "geo_point"
+      }
+    }
+  }
+}
diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java
@@ -46,7 +46,7 @@ public enum MappingType {
     ScaledFloat("scaled_float", ExprCoreType.DOUBLE),
     Double("double", ExprCoreType.DOUBLE),
     Boolean("boolean", ExprCoreType.BOOLEAN);
-    // TODO: ranges, geo shape, point, shape
+    // TODO: ranges, geo shape, shape
 
     private final String name;
 

diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchGeoPointType.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchGeoPointType.java
@@ -8,7 +8,11 @@
 
 import static org.opensearch.sql.data.type.ExprCoreType.UNKNOWN;
 
+import java.util.HashMap;
+import java.util.Map;
 import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import org.opensearch.sql.data.type.ExprCoreType;
 
 /**
  * The type of a geo_point value. See
@@ -22,8 +26,15 @@ public class OpenSearchGeoPointType extends OpenSearchDataType {
   private OpenSearchGeoPointType() {
     super(MappingType.GeoPoint);
     exprCoreType = UNKNOWN;
+    this.properties = new HashMap<>();
+    this.properties.put("lat", new OpenSearchDataType(ExprCoreType.DOUBLE));
+    this.properties.put("lon", new OpenSearchDataType(ExprCoreType.DOUBLE));
   }
 
+  @Getter
+  @EqualsAndHashCode.Exclude
+  Map<String, OpenSearchDataType> properties;
+
   public static OpenSearchGeoPointType of() {
     return OpenSearchGeoPointType.instance;
   }

diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/OpenSearchJsonContent.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/OpenSearchJsonContent.java
@@ -126,28 +126,35 @@ public Object objectValue() {
   @Override
   public Pair<Double, Double> geoValue() {
     final JsonNode value = value();
-    if (value.has("lat") && value.has("lon")) {
-      Double lat = 0d;
-      Double lon = 0d;
+    Double lat = null;
+    Double lon = null;
+
+    if (value.has("lat")) {
       try {
         lat = extractDoubleValue(value.get("lat"));
       } catch (Exception exception) {
         throw new IllegalStateException(
             "latitude must be number value, but got value: " + value.get(
                 "lat"));
       }
+    }
+
+    if (value.has("lon")) {
       try {
         lon = extractDoubleValue(value.get("lon"));
       } catch (Exception exception) {
         throw new IllegalStateException(
             "longitude must be number value, but got value: " + value.get(
                 "lon"));
       }
-      return Pair.of(lat, lon);
-    } else {
-      throw new IllegalStateException("geo point must in format of {\"lat\": number, \"lon\": "
+    }
+
+    if (lat == null && lon == null) {
+      throw new IllegalStateException("geo point must be in format of {\"lat\": number, \"lon\": "
           + "number}");
     }
+
+    return Pair.of(lat, lon);
   }
 
   /**