-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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.
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
Geo: Add GeoJson parser to libs/geo classes #41575
Geo: Add GeoJson parser to libs/geo classes #41575
Conversation
Adds GeoJson parser for Geometry classes defined in libs/geo. This should allow us to bypass ShapeBuilders in geosql and is the first step in ShapeBuilder refactoring. The provided functionality should be sufficient for geosql parsing, but additional functionality will be required for complete parity with ShapeBuilder parser - we need to add handling of mapper parameters in WKT parser as well. That will be added in a follow up PR. Relates elastic#40908
Pinging @elastic/es-analytics-geo |
@elasticmachine run elasticsearch-ci/1 |
1 similar comment
@elasticmachine run elasticsearch-ci/1 |
…bs-geo-geojson-parser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks good. I am still digging through the tests :)
@@ -33,4 +35,8 @@ | |||
LINEARRING, // not serialized by itself in WKT or WKB | |||
ENVELOPE, // not part of the actual WKB spec | |||
CIRCLE; // not part of the actual WKB spec | |||
|
|||
public static ShapeType forName(String geoshapename) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we camelCase that variable name?
|
||
@Override | ||
public String visit(LinearRing ring) { | ||
throw new UnsupportedOperationException("line ring cannot be serialized using GeoJson"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/line ring/linearRing?
/** | ||
* Parses supplied XContent into Geometry | ||
*/ | ||
static Geometry parse(XContentParser parser, BaseGeoShapeFieldMapper shapeMapper) throws IOException, ParseException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to remove the dependency on a field mapper here. maybe require the three attributes explicitly
Adds GeoJson parser for Geometry classes defined in libs/geo. Relates elastic#40908 and elastic#29872
Adds GeoJson parser for Geometry classes defined in libs/geo. Relates elastic#40908 and elastic#29872
Adds GeoJson parser for Geometry classes defined in libs/geo. Relates elastic#40908 and elastic#29872
Adds GeoJson parser for Geometry classes defined in libs/geo. This
should allow us to bypass ShapeBuilders in geosql and is the first
step in ShapeBuilder refactoring. The provided functionality should
be sufficient for geosql parsing, but additional functionality will
be required for complete parity with ShapeBuilder parser - we need
to add handling of mapper parameters in WKT parser as well. That
will be added in a follow up PR.
Relates #40908 and #29872