Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL: Switch server side for geosql from ShapeBuilder to libs/geo #41692

Merged
merged 4 commits into from
May 2, 2019
+62 −38
Merged

SQL: Switch server side for geosql from ShapeBuilder to libs/geo #41692

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7de52b2
SQL: Switch server side for geosql from ShapeBuilder to libs/geo
imotov Apr 28, 2019
1d006b8
Replace ElasticsearchParseException with SqlIllegalArgumentException
imotov May 2, 2019
2934482
Add parsing value to the error message
imotov May 2, 2019
f4ed851
Merge remote-tracking branch 'elastic/geosql' into geosql-switch-to-l…
imotov May 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 12 additions & 15 deletions x-pack/plugin/sql/qa/src/main/resources/ogc/ogc.sql-spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,28 +61,25 @@ SELECT fid, UCASE(ST_GeometryType(boundary)) type FROM named_places ORDER BY fid
selectMapNeatLinesProps
SELECT fid, UCASE(ST_GeometryType(neatline)) type FROM map_neatlines ORDER BY fid;

// AwaitsFix https://github.com/elastic/elasticsearch/issues/40908
// selectLakesXY
// SELECT fid, ST_X(shore) x, ST_Y(shore) y FROM lakes ORDER BY fid;
selectLakesXY
SELECT fid, ST_X(shore) x, ST_Y(shore) y FROM lakes ORDER BY fid;
selectRoadSegmentsXY
SELECT fid, ST_X(centerline) x, ST_Y(centerline) y FROM road_segments ORDER BY fid;
selectDividedRoutesXY
SELECT fid, ST_X(centerlines) x, ST_Y(centerlines) y FROM divided_routes ORDER BY fid;
// AwaitsFix https://github.com/elastic/elasticsearch/issues/40908
// selectForestsXY
// SELECT fid, ST_X(boundary) x, ST_Y(boundary) y FROM forests ORDER BY fid;
selectForestsXY
SELECT fid, ST_X(boundary) x, ST_Y(boundary) y FROM forests ORDER BY fid;
selectBridgesPositionsXY
SELECT fid, ST_X(position) x, ST_Y(position) y FROM bridges ORDER BY fid;
selectStreamsXY
SELECT fid, ST_X(centerline) x, ST_Y(centerline) y FROM streams ORDER BY fid;
selectBuildingsXY
SELECT fid, ST_X(position) x, ST_Y(position) y FROM buildings ORDER BY fid;
// AwaitsFix https://github.com/elastic/elasticsearch/issues/40908
// selectBuildingsFootprintsXY
// SELECT fid, ST_X(footprint) x, ST_Y(footprint) y FROM buildings ORDER BY fid;
// selectPondsXY
// SELECT fid, ST_X(shores) x, ST_Y(shores) y FROM ponds ORDER BY fid;
// selectNamedPlacesXY
// SELECT fid, ST_X(boundary) x, ST_Y(boundary) y FROM named_places ORDER BY fid;
// selectMapNeatLinesXY
// SELECT fid, ST_X(neatline) x, ST_Y(neatline) y FROM map_neatlines ORDER BY fid;
selectBuildingsFootprintsXY
SELECT fid, ST_X(footprint) x, ST_Y(footprint) y FROM buildings ORDER BY fid;
selectPondsXY
SELECT fid, ST_X(shores) x, ST_Y(shores) y FROM ponds ORDER BY fid;
selectNamedPlacesXY
SELECT fid, ST_X(boundary) x, ST_Y(boundary) y FROM named_places ORDER BY fid;
selectMapNeatLinesXY
SELECT fid, ST_X(neatline) x, ST_Y(neatline) y FROM map_neatlines ORDER BY fid;
70 changes: 50 additions & 20 deletions ...ql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/GeoShape.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,19 @@
*/
package org.elasticsearch.xpack.sql.expression.function.scalar.geo;

import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.geo.GeoUtils;
import org.elasticsearch.common.geo.builders.PointBuilder;
import org.elasticsearch.common.geo.builders.ShapeBuilder;
import org.elasticsearch.common.geo.parsers.ShapeParser;
import org.elasticsearch.common.geo.GeometryParser;
import org.elasticsearch.common.io.stream.NamedWriteable;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.common.xcontent.ToXContentFragment;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.json.JsonXContent;
import org.elasticsearch.geo.geometry.Circle;
import org.elasticsearch.geo.geometry.Geometry;
import org.elasticsearch.geo.geometry.GeometryCollection;
Expand All @@ -26,9 +30,12 @@
import org.elasticsearch.geo.geometry.Point;
import org.elasticsearch.geo.geometry.Polygon;
import org.elasticsearch.geo.geometry.Rectangle;
import org.elasticsearch.geo.utils.WellKnownText;
import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;

import java.io.IOException;
import java.io.InputStream;
import java.text.ParseException;
import java.util.Objects;

/**
Expand All @@ -41,41 +48,49 @@ public class GeoShape implements ToXContentFragment, NamedWriteable {

public static final String NAME = "geo";

private final ShapeBuilder<?, ?, ?> shapeBuilder;
private final Geometry shape;

public GeoShape(double lon, double lat) {
shapeBuilder = new PointBuilder(lon, lat);
shape = new Point(lat, lon);
}

public GeoShape(Object value) throws IOException {
shapeBuilder = ShapeParser.parse(value);
try {
shape = parse(value);
} catch (ParseException ex) {
throw new ElasticsearchParseException("cannot load shape", ex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to wrap it in a new exception here? And if it's the case shouldn't this be an Sql exception?
Also can we have some tests to check those cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to wrap it in a new exception here? And if it's the case shouldn't this be an Sql exception?

I wrapped it here for consistency with the next constructor which cannot throw anything besides IOException, I will rewrap the it into SqlIllegalArgumentException that sounds like a better idea.

Also can we have some tests to check those cases?

We are already testing this cases in StWkttosqlProcessorTests.

}
}

public GeoShape(StreamInput in) throws IOException {
shapeBuilder = ShapeParser.parse(in.readString());
try {
shape = parse(in.readString());
} catch (ParseException ex) {
throw new ElasticsearchParseException("cannot load shape", ex);
}
}

@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeString(shapeBuilder.toWKT());
out.writeString(WellKnownText.toWKT(shape));
}

@Override
public String toString() {
return shapeBuilder.toWKT();
return WellKnownText.toWKT(shape);
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
return builder.value(shapeBuilder.toWKT());
return builder.value(WellKnownText.toWKT(shape));
}

public Geometry toGeometry() {
return shapeBuilder.buildGeometry();
return shape;
}

public Point firstPoint() {
return shapeBuilder.buildGeometry().visit(new GeometryVisitor<Point, RuntimeException>() {
return shape.visit(new GeometryVisitor<Point, RuntimeException>() {
@Override
public Point visit(Circle circle) {
return new Point(circle.getLat(), circle.getLon(), circle.hasAlt() ? circle.getAlt() : Double.NaN);
Expand Down Expand Up @@ -149,16 +164,16 @@ public String getGeometryType() {
}

public static double distance(GeoShape shape1, GeoShape shape2) {
if (shape1.shapeBuilder instanceof PointBuilder == false) {
if (shape1.shape instanceof Point == false) {
throw new SqlIllegalArgumentException("distance calculation is only supported for points; received [{}]", shape1);
}
if (shape2.shapeBuilder instanceof PointBuilder == false) {
if (shape2.shape instanceof Point == false) {
throw new SqlIllegalArgumentException("distance calculation is only supported for points; received [{}]", shape2);
}
double srcLat = ((PointBuilder) shape1.shapeBuilder).latitude();
double srcLon = ((PointBuilder) shape1.shapeBuilder).longitude();
double dstLat = ((PointBuilder) shape2.shapeBuilder).latitude();
double dstLon = ((PointBuilder) shape2.shapeBuilder).longitude();
double srcLat = ((Point) shape1.shape).getLat();
double srcLon = ((Point) shape1.shape).getLon();
double dstLat = ((Point) shape2.shape).getLat();
double dstLon = ((Point) shape2.shape).getLon();
return GeoUtils.arcDistance(srcLat, srcLon, dstLat, dstLon);
}

Expand All @@ -171,17 +186,32 @@ public boolean equals(Object o) {
return false;
}
GeoShape geoShape = (GeoShape) o;
return shapeBuilder.equals(geoShape.shapeBuilder);
return shape.equals(geoShape.shape);
}

@Override
public int hashCode() {
return Objects.hash(shapeBuilder);
return Objects.hash(shape);
}

@Override
public String getWriteableName() {
return NAME;
}

private static Geometry parse(Object value) throws IOException, ParseException {
XContentBuilder content = JsonXContent.contentBuilder();
content.startObject();
content.field("value", value);
content.endObject();

try (InputStream stream = BytesReference.bytes(content).streamInput();
XContentParser parser = JsonXContent.jsonXContent.createParser(
NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, stream)) {
parser.nextToken(); // start object
parser.nextToken(); // field name
parser.nextToken(); // field value
return GeometryParser.parse(parser, true, true, true);
}
}
}
3 changes: 0 additions & 3 deletions ...st/java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/GeoProcessorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,6 @@ public void testApplyGetXY() throws Exception {
new GeoShape("geometrycollection (point (20.0 10.0),point (1.0 2.0))")));
}

// That doesn't work correctly at the moment because shape builder changes the order or points in polygons, so the second point
// sometimes becomes the first
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/40908")
public void testApplyGetXYToPolygons() throws Exception {
assertEquals(3.0, new GeoProcessor(GeoOperation.X).process(new GeoShape("polygon ((3.0 1.0, 4.0 2.0, 4.0 3.0, 3.0 1.0))")));
assertEquals(1.0, new GeoProcessor(GeoOperation.Y).process(new GeoShape("polygon ((3.0 1.0, 4.0 2.0, 4.0 3.0, 3.0 1.0))")));
Expand Down