[Part 1] Add geo support by yupeng9 · Pull Request #5654 · apache/pinot

yupeng9 · 2020-07-02T21:20:46Z

Description

This PR added the following

add geo-spatial data model

The data model includes both geometry and geography, which is differentiated by a spatial reference identifier (SRID). Notably, uses SRID=4326 as the coordinate system of lat/lng per https://epsg.io/4326.

add serde

Added the serialization/deserialization from geo-spatial value to bytes with kryo library. Also added a benchmark for performance evaluation

Benchmark result: https://gist.github.com/yupeng9/8e2b081ffb372593492ebb6a41da97fd

add geospatial functions

geo constructors

ST_GEOG_FROM_TEXT
ST_GEOG_FROM_WKB
ST_GEOM_FROM_TEXT
ST_GEOM_FROM_WKB
ST_POINT
ST_POLYGON

geo measurements

ST_AREA (the geography area implementation is similar to Presto's)
ST_DISTANCE (the great circle distance implementation is similar to Presto's)
ST_GEOMETRY_TYPE

geo outputs

ST_AS_BINARY
ST_AS_TEXT

geo relationship

ST_CONTAINS
ST_EQUALS

Updates to MeetupRsvp quickstart example

Added a new location field from the longitude and latitude of the event, using an inbuilt stPoint transform function

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

No

Does this PR fix a zero-downtime upgrade introduced earlier?

No

Does this PR otherwise need attention when creating release notes? Things to consider:

New configuration options
Deprecation of configurations
Signature changes to public methods/interfaces
New plugins added or old plugins removed

Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

Yes, added a new experimental feature

Documentation

If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document

yupeng9 · 2020-07-02T21:23:22Z

...c/main/java/org/apache/pinot/broker/broker/helix/BrokerUserDefinedMessageHandlerFactory.java

plz ignore this, seems some search/replace error

kishoreg

Nicely done! Looks much simpler than I thought. Will the BYTES column for StPoint be dictionary encoded?

pinot-core/pom.xml

kishoreg · 2020-07-04T19:47:33Z

...main/java/org/apache/pinot/core/geospatial/transform/function/ConstructFromTextFunction.java

what does this function do? please add java docs

Added. This is an abstract class for implementing the geo constructor functions like StGeomFromText and StGeogFromText

kishoreg · 2020-07-04T19:48:16Z

...core/src/main/java/org/apache/pinot/core/geospatial/transform/function/StAsTextFunction.java

function -> functions

hmm, then I cannot define scalar function in this package for inbuilt transformation functions.

kishoreg · 2020-07-04T19:49:42Z

pinot-tools/src/main/java/org/apache/pinot/tools/streams/MeetupRsvpStream.java

kishoreg · 2020-07-04T19:50:05Z

pinot-tools/src/main/java/org/apache/pinot/tools/streams/MeetupRsvpStream.java

kishoreg · 2020-07-04T19:50:33Z

pinot-tools/src/main/resources/examples/stream/meetupRsvp/meetupRsvp_schema.json

Great to see this work

kishoreg · 2020-07-04T19:53:01Z

Add sample queries to the description and also update the java docs.

siddharthteotia · 2020-07-04T23:12:15Z

@yupeng9 , I would like to review this PR. Please give a day or two to go over the code.

mayankshriv · 2020-07-05T03:07:06Z

pinot-core/pom.xml

Is Eclipse license ok to add? So far we have taken Apache/MIT/Gnu.

My understanding is okay to include.
Per https://www.apache.org/legal/resolved.html#category-a, Eclipse Distribution License 1.0 can be included. And JTS is dual-licensed under Eclipse Public License 2.0 and Eclipse Distribution License 1.0 (https://github.com/locationtech/jts#license)

mayankshriv · 2020-07-05T03:09:56Z

pinot-core/src/main/java/org/apache/pinot/core/geospatial/serde/GeometrySerde.java

Tab space indicates not following Pinot code-styling.

mayankshriv · 2020-07-05T03:11:02Z

pinot-core/src/main/java/org/apache/pinot/core/geospatial/serde/GeometrySerde.java

Initialize list with size if known.

mayankshriv · 2020-07-05T03:38:55Z

pinot-core/src/main/java/org/apache/pinot/core/geospatial/serde/GeometrySerializationType.java

Please use Pinot code style (name of member variables starts with _ to avoid qualifying with this.

Sure. thanks.

mayankshriv · 2020-07-05T03:41:04Z

pinot-core/src/main/java/org/apache/pinot/core/geospatial/serde/GeometrySerializationType.java

Consider using a Static Map, if this list has a chance to grow.

This list is unlikely to grow, given the OGC geo is a well-defined standard per https://www.ogc.org/standards/sfa

mayankshriv · 2020-07-05T03:42:57Z

...main/java/org/apache/pinot/core/geospatial/transform/function/ConstructFromTextFunction.java

Utils.rethrow will preserve the original exception.

Good to see this util

mayankshriv · 2020-07-05T03:47:55Z

pinot-core/src/test/java/org/apache/pinot/core/geospatial/serde/BenchmarkSerde.java

Would be good to add the benchmark results in the PR description.

Sounds good

Added https://gist.github.com/yupeng9/8e2b081ffb372593492ebb6a41da97fd to the description

mayankshriv · 2020-07-05T03:50:23Z

pinot-tools/src/main/java/org/apache/pinot/tools/streams/MeetupRsvpStream.java

Unsure if LOGGER should be used here?

removed. it was for debugging purpose.

mayankshriv · 2020-07-05T03:50:50Z

pinot-tools/src/main/resources/examples/stream/meetupRsvp/meetupRsvp_schema.json

mayankshriv · 2020-07-05T03:51:48Z

pinot-tools/src/main/resources/log4j2.xml

This seems unrelated to this PR? Would be good to call it out in the description, along with the motivation for the change.

You are right. Reverted this since it was for local debugging purpose.

yupeng9 · 2020-07-05T05:49:37Z

@kishoreg @mayankshriv thanks for the review. Will address the comments.
@siddharthteotia No hurry. Please take your time.

yupeng9

Comments addressed

yupeng9 · 2020-07-07T05:53:43Z

pinot-core/pom.xml

My understanding is okay to include.
Per https://www.apache.org/legal/resolved.html#category-a, Eclipse Distribution License 1.0 can be included. And JTS is dual-licensed under Eclipse Public License 2.0 and Eclipse Distribution License 1.0 (https://github.com/locationtech/jts#license)

yupeng9 · 2020-07-07T06:02:22Z

pinot-core/src/main/java/org/apache/pinot/core/geospatial/serde/GeometrySerializationType.java

Sure. thanks.

yupeng9 · 2020-07-07T06:04:43Z

pinot-core/src/main/java/org/apache/pinot/core/geospatial/serde/GeometrySerializationType.java

This list is unlikely to grow, given the OGC geo is a well-defined standard per https://www.ogc.org/standards/sfa

yupeng9 · 2020-07-07T16:32:22Z

...main/java/org/apache/pinot/core/geospatial/transform/function/ConstructFromTextFunction.java

Added. This is an abstract class for implementing the geo constructor functions like StGeomFromText and StGeogFromText

yupeng9 · 2020-07-07T17:26:36Z

...main/java/org/apache/pinot/core/geospatial/transform/function/ConstructFromTextFunction.java

Good to see this util

yupeng9 · 2020-07-07T17:27:32Z

pinot-core/src/test/java/org/apache/pinot/core/geospatial/serde/BenchmarkSerde.java

Sounds good

yupeng9 · 2020-07-07T17:31:06Z

pinot-tools/src/main/java/org/apache/pinot/tools/streams/MeetupRsvpStream.java

removed. it was for debugging purpose.

yupeng9 · 2020-07-07T17:34:27Z

pinot-tools/src/main/resources/log4j2.xml

You are right. Reverted this since it was for local debugging purpose.

Jackie-Jiang · 2020-07-08T02:14:56Z

pinot-core/src/main/java/org/apache/pinot/core/geospatial/GeometryType.java

Please follow the Pinot coding convention of using underscore as the prefix for member variables. Same for other classes

Jackie-Jiang · 2020-07-08T02:15:41Z

pinot-core/src/main/java/org/apache/pinot/core/geospatial/GeometryType.java

As an example of Pinot coding convention

Suggested change

this.multitype = multitype;

_multitype = multitype;

Thanks for taking a pass, updated the style

@Jackie-Jiang any reason we set checkstyle severity to warning but not error. I saw we have such a rule for member variable, but the maven checkstyle does not fail the build

Good suggestion. We can switch it to error once we fixed all the existing code with wrong code styles. Opened issue #5675 to track this

Jackie-Jiang

High level question, why are we using JTS library to handle both geometry as well as geography? Shouldn't we use ESRI for geography?

yupeng9 · 2020-07-09T17:57:09Z

High level question, why are we using JTS library to handle both geometry as well as geography? Shouldn't we use ESRI for geography?

Good call.
There are two reasons, as I found out during prototyping:

Performance. I found the serialization performance JTS is better than ESRI. This is also confirmed by Presto project, as they are migrating from ESRI to JTS: Use JTS instead of ESRI for geometries, Parts 1 and 2 prestodb/presto#13604
Simplicity on the dependency. With one less dependency, it's easier to manage the implementation. As in my current way, the geometry to geography conversion is just a simple change of coordinate system (SRID).

The tradeoff of taking this approach is that JTS is a library for Euclidean planar linear geometry, so all the geography-related operations have to be implemented using JTS's primitives. That's why there is some lengthy logic on geography measurement functions. Those implementations are similar to what Presto is doing.

Will update the design doc to reflect this change

Jackie-Jiang · 2020-07-09T19:53:13Z

High level question, why are we using JTS library to handle both geometry as well as geography? Shouldn't we use ESRI for geography?

Good call.
There are two reasons, as I found out during prototyping:

Performance. I found the serialization performance JTS is better than ESRI. This is also confirmed by Presto project, as they are migrating from ESRI to JTS: prestodb/presto#13604

Simplicity on the dependency. With one less dependency, it's easier to manage the implementation. As in my current way, the geometry to geography conversion is just a simple change of coordinate system (SRID).

The tradeoff of taking this approach is that JTS is a library for Euclidean planar linear geometry, so all the geography-related operations have to be implemented using JTS's primitives. That's why there is some lengthy logic on geography measurement functions. Those implementations are similar to what Presto is doing.

Will update the design doc to reflect this change

For ser-de, we are using the customized serializer, so I don't think there will be performance difference between these 2 libraries?
For geography operations, we should leverage ESRI for the calculation (or are you planing to implement all geography operations yourself?).
We need to support both geometry (SRID 0) and geography (SRID 4326) operations:

If ESRI supports both geometry and geography operations, we can use only ESRI to do the calculation
If not, we should use JTS for geometry and ESRI for geography calculation

yupeng9 · 2020-07-10T04:52:51Z

High level question, why are we using JTS library to handle both geometry as well as geography? Shouldn't we use ESRI for geography?

Good call.
There are two reasons, as I found out during prototyping:

Performance. I found the serialization performance JTS is better than ESRI. This is also confirmed by Presto project, as they are migrating from ESRI to JTS: prestodb/presto#13604

Simplicity on the dependency. With one less dependency, it's easier to manage the implementation. As in my current way, the geometry to geography conversion is just a simple change of coordinate system (SRID).

The tradeoff of taking this approach is that JTS is a library for Euclidean planar linear geometry, so all the geography-related operations have to be implemented using JTS's primitives. That's why there is some lengthy logic on geography measurement functions. Those implementations are similar to what Presto is doing.
Will update the design doc to reflect this change

For ser-de, we are using the customized serializer, so I don't think there will be performance difference between these 2 libraries?

Though we use customized serializer, there could be some difference due to the internal representation of the fields, their accessor implementations. The PR I linked above shows about the 20% difference.

Another notable reason is that JTS conforms to the ISO standards better. I believe this is the primary reason that Presto community decided to move from ESRI to JTS. I suggest we take the lessons learned from them.

Lastly, many users query Pinot via the Presto connector, so it's also a desirable property that Pinot geo functions return same or similar results as Presto's for better unification.

For geography operations, we should leverage ESRI for the calculation (or are you planing to implement all geography operations yourself?).

There are not too many geographical functions, so I believe its implementation is still manageable.

We need to support both geometry (SRID 0) and geography (SRID 4326) operations:

If ESRI supports both geometry and geography operations, we can use only ESRI to do the calculation

If not, we should use JTS for geometry and ESRI for geography calculation

- add geo-spatial data model - add serde - add benchmark - add geospatial functions

…ions

yupeng9 · 2020-07-17T02:37:23Z

@Jackie-Jiang I changed the relationship functions to work only for the Geometry objects, to align with Presto's behavior. PTAL

Jackie-Jiang · 2020-07-18T17:38:03Z

pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java

  // Special type for annotation based scalar functions
-  SCALAR("scalar");
+  SCALAR("scalar"),
+  // geo constructors


(nit) Add an empty line in front, and capitalize the comment

pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java

Jackie-Jiang · 2020-07-18T17:41:30Z

pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java

+  // geo measurements
+  ST_AREA("ST_Area"),
+  ST_DISTANCE("ST_Distance"),
+  ST_GEOMETRY_TYPE("ST_GEOMETRY_TYPE"),


Suggested change

ST_GEOMETRY_TYPE("ST_GEOMETRY_TYPE"),

ST_GEOMETRY_TYPE("ST_GeometryType"),

Is there a standard function to return the SRID of the geometry? (Identify whether it is geometry or geography)

Yes, it returns the type of the geometry as a string. EG: 'ST_Linestring', 'ST_Polygon','ST_MultiPolygon' etc

Jackie-Jiang · 2020-07-18T17:47:23Z

pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java

   * <ul>
-   *   <li>For dimension, time, date time fields, support {@link DataType}: INT, LONG, FLOAT, DOUBLE, STRING</li>
+   *   <li>For dimension, time, date time fields, support {@link DataType}: INT, LONG, FLOAT, DOUBLE, STRING, BYTES</li>
   *   <li>For non-derived metric fields, support {@link DataType}: INT, LONG, FLOAT, DOUBLE</li>


Thanks for updating the javadoc. Add BYTES here as well

Jackie-Jiang · 2020-07-18T17:48:45Z

pinot-perf/pom.xml

      <groupId>org.openjdk.jmh</groupId>
      <artifactId>jmh-core</artifactId>
-      <version>1.21</version>
+      <version>${jmh.version}</version>


Move these to the root pom and specify the version there

it's already there

Jackie-Jiang · 2020-07-18T21:03:28Z

...re/src/main/java/org/apache/pinot/core/geospatial/transform/function/StDistanceFunction.java

+      return null;
+    }
+
+    validateGeographyType("ST_Distance", leftGeometry, EnumSet.of(GeometryType.POINT));


Can be simplified to geometry instanceof Point

try to reuse the same error msg template of several functions

There will be quite big performance difference, especially for per-value check

Jackie-Jiang · 2020-07-18T21:04:39Z

...re/src/main/java/org/apache/pinot/core/geospatial/transform/function/StDistanceFunction.java

+    return _results;
+  }
+
+  public static void checkLatitude(double latitude) {


Make all helper methods private

Jackie-Jiang · 2020-07-18T21:05:34Z

...core/src/main/java/org/apache/pinot/core/geospatial/transform/function/StEqualsFunction.java

+    for (int i = 0; i < projectionBlock.getNumDocs(); i++) {
+      Geometry firstGeometry = GeometrySerializer.deserialize(firstValues[i]);
+      Geometry secondGeometry = GeometrySerializer.deserialize(secondValues[i]);
+      if (GeometryUtils.isGeography(firstGeometry) || GeometryUtils.isGeography(secondGeometry)) {


Equals should work on geography as well?

yes, it should

Jackie-Jiang · 2020-07-18T21:10:25Z

...ore/src/main/java/org/apache/pinot/core/geospatial/transform/function/StPolygonFunction.java

+/**
+ * Constructor function for polygon object from text.
+ */
+public class StPolygonFunction extends ConstructFromTextFunction {


This doesn't seem right that St_Polygon is the same as ST_GeomFromText

yup, added the constraint of checking polygon type

Jackie-Jiang · 2020-07-18T21:11:06Z

...e/src/main/java/org/apache/pinot/core/operator/transform/function/BaseTransformFunction.java

+  protected static final TransformResultMetadata BYTES_SV_NO_DICTIONARY_METADATA =
+          new TransformResultMetadata(DataType.BYTES, true, false);

+  private boolean[] _booleanValuesSV;


yupeng9

@Jackie-Jiang Thanks for the detailed review. Comments addressed

yupeng9 · 2020-07-21T06:06:05Z

pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java

+  // geo measurements
+  ST_AREA("ST_Area"),
+  ST_DISTANCE("ST_Distance"),
+  ST_GEOMETRY_TYPE("ST_GEOMETRY_TYPE"),


Yes, it returns the type of the geometry as a string. EG: 'ST_Linestring', 'ST_Polygon','ST_MultiPolygon' etc

yupeng9 · 2020-07-21T06:12:59Z

pinot-core/pom.xml

      <artifactId>lucene-analyzers-common</artifactId>
      <version>${lucene.version}</version>
    </dependency>
+    <dependency>


moved it to pinot-perf project.

yupeng9 · 2020-07-21T06:16:03Z

pinot-core/src/main/java/org/apache/pinot/core/geospatial/GeometryType.java

+
+  POINT(false, "ST_Point"),
+  MULTI_POINT(true, "ST_MultiPoint"),
+  LINE_STRING(false, "ST_LineString"),


LINEAR_RING is a subtype of LINEAR_STRING

yupeng9 · 2020-07-21T06:18:38Z

pinot-core/src/main/java/org/apache/pinot/core/geospatial/GeometryType.java

+    _name = name;
+  }
+
+  public boolean isMultitype() {


not in this PR. It's useful in function like https://postgis.net/docs/ST_GeometryN.html

yupeng9 · 2020-07-21T20:18:16Z

pinot-core/src/main/java/org/apache/pinot/core/geospatial/serde/GeometrySerde.java

+/**
+ * Provides methods to efficiently serialize and deserialize geometry types.
+ */
+public class GeometrySerde extends Serializer {


They are not exactly same, in particular, the differences are:

Presto uses schema to indicate geometry vs geography info, while we encode this in the type byte.

Presto serializes additional information such as envelope to be compatible with ESRI serialization, but the serde here does not, which is simpler and faster

Added this to the comments

yupeng9 · 2020-07-22T02:43:03Z

...re/src/main/java/org/apache/pinot/core/geospatial/transform/function/StDistanceFunction.java

+      return null;
+    }
+
+    validateGeographyType("ST_Distance", leftGeometry, EnumSet.of(GeometryType.POINT));


try to reuse the same error msg template of several functions

yupeng9 · 2020-07-22T02:44:51Z

...re/src/main/java/org/apache/pinot/core/geospatial/transform/function/StDistanceFunction.java

+   * This assumes a spherical Earth, and uses the Vincenty formula. (https://en.wikipedia
+   * .org/wiki/Great-circle_distance)
+   */
+  public static double greatCircleDistance(double latitude1, double longitude1, double latitude2, double longitude2) {


yes, per https://en.wikipedia.org/wiki/Great-circle_distance

yupeng9 · 2020-07-22T02:47:39Z

...core/src/main/java/org/apache/pinot/core/geospatial/transform/function/StEqualsFunction.java

+    for (int i = 0; i < projectionBlock.getNumDocs(); i++) {
+      Geometry firstGeometry = GeometrySerializer.deserialize(firstValues[i]);
+      Geometry secondGeometry = GeometrySerializer.deserialize(secondValues[i]);
+      if (GeometryUtils.isGeography(firstGeometry) || GeometryUtils.isGeography(secondGeometry)) {


yes, it should

yupeng9 · 2020-07-22T02:52:23Z

...ore/src/main/java/org/apache/pinot/core/geospatial/transform/function/StPolygonFunction.java

+/**
+ * Constructor function for polygon object from text.
+ */
+public class StPolygonFunction extends ConstructFromTextFunction {


yup, added the constraint of checking polygon type

yupeng9 · 2020-07-22T02:53:35Z

pinot-perf/pom.xml

      <groupId>org.openjdk.jmh</groupId>
      <artifactId>jmh-core</artifactId>
-      <version>1.21</version>
+      <version>${jmh.version}</version>


it's already there

Jackie-Jiang

LGTM with some comments.
Let me know when you address the comments and ready to merge

Jackie-Jiang · 2020-07-22T18:30:08Z

pinot-core/src/main/java/org/apache/pinot/core/geospatial/GeometryType.java

+ */
+public enum GeometryType {
+
+  POINT(false, 0,"ST_Point"),


(nit) reformat

Jackie-Jiang · 2020-07-22T18:31:15Z

pinot-core/src/main/java/org/apache/pinot/core/geospatial/GeometryType.java

+  MULTI_POLYGON(true, 5,"ST_MultiPolygon"),
+  GEOMETRY_COLLECTION(true, 6,"ST_GeomCollection");
+
+  private final boolean _multitype;


(nit) _multiType? (IDE identify multitype as typo)

Jackie-Jiang · 2020-07-22T18:31:43Z

pinot-core/src/main/java/org/apache/pinot/core/geospatial/serde/GeometrySerializationType.java

+/**
+ * The geometry type used in serialization
+ */
+public enum GeometrySerializationType {


Remove this class

Jackie-Jiang · 2020-07-22T18:34:51Z

pinot-core/src/main/java/org/apache/pinot/core/geospatial/GeometryType.java

+   * @return the serialization type
+   */
+  public static GeometryType fromID(int id) {
+    switch (id) {


Keep an static GeometryType array
private static final GeometryType[] ID_TO_TYPE_MAP = new GeometryType[] {POINT, MULTI_POINT, ...};
Then you can avoid the switch branching for better performance
return ID_TO_TYPE_MAP[id];

Jackie-Jiang · 2020-07-22T18:36:44Z

pinot-core/src/main/java/org/apache/pinot/core/geospatial/serde/GeometrySerde.java

+ *  - The envelope info is not serialized
+ */
+public class GeometrySerde {
+  private static final Logger LOGGER = LoggerFactory.getLogger(GeometrySerde.class);


(nit) Remove the unused LOGGER (we don't want to log within serde as it is per-value based and can easily flood the log)

Jackie-Jiang · 2020-07-22T19:07:25Z

...-core/src/main/java/org/apache/pinot/core/geospatial/transform/function/ScalarFunctions.java

+  @ScalarFunction
+  public static byte[] stPoint(double longitude, double latitude) {
+    return GeometrySerializer
+        .serialize(GeometryUtils.GEOMETRY_FACTORY.createPoint(new Coordinate(longitude, latitude)));


(Major) Should this be GEOGRAPHY_FACTORY for longitude and latitude?

good point. Changed the argument to x,y

Jackie-Jiang · 2020-07-22T19:14:00Z

...t-core/src/main/java/org/apache/pinot/core/geospatial/transform/function/StAreaFunction.java

+    Geometry geometry;
+    for (int i = 0; i < projectionBlock.getNumDocs(); i++) {
+      geometry = GeometrySerializer.deserialize(values[i]);


Not necessary. I did some benchmark on this and there is performance difference

Jackie-Jiang · 2020-07-22T19:18:32Z

...re/src/main/java/org/apache/pinot/core/geospatial/transform/function/StDistanceFunction.java

+      return null;
+    }
+
+    validateGeographyType("ST_Distance", leftGeometry, EnumSet.of(GeometryType.POINT));


There will be quite big performance difference, especially for per-value check

Jackie-Jiang · 2020-07-22T19:22:19Z

...re/src/main/java/org/apache/pinot/core/geospatial/transform/function/StDistanceFunction.java

+        _results[i] = sphericalDistance(firstGeometry, secondGeometry);
+      } else {
+        _results[i] =
+            firstGeometry.isEmpty() || secondGeometry.isEmpty() ? null : firstGeometry.distance(secondGeometry);


I think you can return Double.NaN here to indicate empty geometry

Jackie-Jiang · 2020-07-22T19:23:34Z

...ore/src/main/java/org/apache/pinot/core/geospatial/transform/function/StPolygonFunction.java

+        Utils.rethrowException(
+            new RuntimeException(String.format("Failed to parse geometry from string: %s", argumentValues[i])));


Suggested change

Utils.rethrowException(

new RuntimeException(String.format("Failed to parse geometry from string: %s", argumentValues[i])));

throw new RuntimeException(String.format("Failed to parse geometry from string: %s", argumentValues[i]));

yupeng9 · 2020-07-23T02:45:17Z

@Jackie-Jiang thanks for taking another pass. Comments addressed, and feel free to merge

yupeng9 changed the title ~~add geo support~~ [Part 1] Add geo support Jul 2, 2020

yupeng9 commented Jul 2, 2020

View reviewed changes

kishoreg approved these changes Jul 4, 2020

View reviewed changes

mayankshriv reviewed Jul 5, 2020

View reviewed changes

yupeng9 commented Jul 7, 2020

View reviewed changes

Jackie-Jiang reviewed Jul 8, 2020

View reviewed changes

yupeng9 and others added 7 commits July 15, 2020 20:56

add geo support

5329f2f

- add geo-spatial data model - add serde - add benchmark - add geospatial functions

cleanup

e778f61

revert id/code change

b4cf2ad

more cleanup

2ac9fe3

address comments

d548d24

more styling

0ebb9c5

Change the relationship function not applicable to geographical funct…

b14d7c5

…ions

yupeng9 force-pushed the geo branch from 6a741bf to b14d7c5 Compare July 17, 2020 01:57

fix test failure

bb51f43

Jackie-Jiang reviewed Jul 18, 2020

View reviewed changes

addressed comments

b3bfad3

yupeng9 commented Jul 22, 2020

View reviewed changes

Jackie-Jiang approved these changes Jul 22, 2020

View reviewed changes

more comments

0d6285f

Jackie-Jiang merged commit 1124897 into apache:master Jul 23, 2020

	ST_GEOMETRY_TYPE("ST_GEOMETRY_TYPE"),
	ST_GEOMETRY_TYPE("ST_GeometryType"),

		Utils.rethrowException(
		new RuntimeException(String.format("Failed to parse geometry from string: %s", argumentValues[i])));

	Utils.rethrowException(
	new RuntimeException(String.format("Failed to parse geometry from string: %s", argumentValues[i])));
	throw new RuntimeException(String.format("Failed to parse geometry from string: %s", argumentValues[i]));

Conversation

yupeng9 commented Jul 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

add geo-spatial data model

add serde

add geospatial functions

Updates to MeetupRsvp quickstart example

Upgrade Notes

Release Notes

Documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kishoreg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kishoreg commented Jul 4, 2020

Uh oh!

siddharthteotia commented Jul 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yupeng9 commented Jul 5, 2020

Uh oh!

yupeng9 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

yupeng9 commented Jul 2, 2020 •

edited

Loading