SQL: Switch server side for geosql from ShapeBuilder to libs/geo #41692
Conversation
Switches the server side of the geosql processing from using ShapeBuilder to libs/geo geometry objects. Relates to elastic#29872
imotov
added
:Analytics/Geo
|
Pinging @elastic/es-analytics-geo |
|
Pinging @elastic/es-search |
| try { | ||
| shape = parse(value); | ||
| } catch (ParseException ex) { | ||
| throw new ElasticsearchParseException("cannot load shape", ex); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 { | ||
| try { | ||
| shape = parse(in.readString()); | ||
| } catch (ParseException ex) { | ||
| throw new ElasticsearchParseException("cannot load shape", ex); | ||
| throw new SqlIllegalArgumentException("cannot load geo_shape", ex); |
There was a problem hiding this comment.
why not adding the read value here too? (save in.readString() to a var)
|
@elasticmachine run elasticsearch-ci/1 |
Switches the server side of the geosql processing from using
ShapeBuilder to libs/geo geometry objects.
Relates to #29872