Improve error handling for geometry deserialization edge cases

[jagill]

Certain geometries could be constructed (via GeometryFromText), but
could not be deserialized: it raised non-PrestoException exceptions that
bubbled up as non-catchable GenericInternalExceptions. This is a bad
user experience! This change catches those and raise catchable
PrestoExceptions.

== RELEASE NOTES ==

General Changes
* Improve error handling for geometry deserialization edge cases.

[jagill]

cc @mbasmanova @aweisberg

[aweisberg]

Two comments about untested error paths. The one in GeometryType appears redundant with the wrapping and throwing done in the function it calls. Is there a case for having it twice?

GeometrySerializationType.getForCode throws IllegalArgumentException, is that worth catching, wrapping, and rethrowing?

[aweisberg]

Is this error path tested exercised by any test? Looks like deserialize catches the GeometryException and wraps it so is this necessary?

[aweisberg]

When I ran TestGeometrySerialization I didn't see this exercised in the debugger.

[aweisberg]

I'm not advocating for removal of the error handling if you can trace the code and see where it might throw the exception. I was just asking if it was possible to put together a test case so I could see that the exception is indeed thrown by it and it's not superflous exception handling.

Do what you think is best.

[aweisberg]

+1

[jagill]

Offline conversation with Ariel: Testing those exception paths is rather difficult with the current testing set up, but we know those exceptions can be thrown (and the cases that can throw them), so it's better to catch the exceptions and raise PrestoExceptions.

@mbasmanova any comments? Or does this look good to you?

[mbasmanova]

@jagill Would you share a sample error message?

[jagill]

@mbasmanova : Error messages for the test examples:
Invalid WKT: Invalid number of points in LineString (found 1 - must be 0 or >= 2)
Invalid WKT: Points of LinearRing do not form a closed linestring
Error constructing Polygon: shell is empty but holes are not

We can put them in the test cases explicitly, but these messages comes from the upstream libraries, which might change them between releases, leading to spurious test breakages. It won't be too hard to fix those breakages, if we think having the messages tested is important.

[mbasmanova]

@jagill Thank you, James.