Improved error handling for spatial points #7120
Conversation
Instead of failing with `java.util.NoSuchElementException` we should fail with proper error types and error messages when the user provides syntactically invalid points
pontusmelke
changed the title
While we wait for properly handle points in bolt we should at least fail somewhat gracefully instead of the hard crash we currently do.
|
|
||
| test("should fail properly if unknown coordinate system") { | ||
| a [SyntaxException] shouldBe thrownBy(executeWithAllPlanners("RETURN point({params}) as point", | ||
| "params" -> Map("y" -> 1.0, "crs" -> "WGS-1337"))) |
There was a problem hiding this comment.
Shouldn't this test have an x coordinate? The failure condition is the invalid value of the crs parameter, isn't it?
There was a problem hiding this comment.
I guess a better option would be to skip the y coordinate, and just supply an illegal crs value.
pontusmelke
force-pushed
the
3.0-points-error-handling
branch
from
f831b40
to
91d8e98
Compare
| @@ -666,6 +666,18 @@ class SemanticErrorAcceptanceTest extends ExecutionEngineFunSuite { | |||
| "A single relationship type must be specified for MERGE (line 1, column 9 (offset: 8))") | |||
| } | |||
|
|
|||
| test("give a nice error message when missing crs in cartesian point") { | |||
| executeAndEnsureError("RETURN point({x: 2.3, y: 4.5}) as point", | |||
| "A cartesian point must contain a 'crs' (coordinate reference system) (line 1, column 14 (offset: 13))") | |||
There was a problem hiding this comment.
I don't understand the error. Without a CRS, this will get WGS84 which makes it a geographic point, not a cartesian point. Then the error should be 'A geographic point needs latitude and longitude', right?
There was a problem hiding this comment.
Is that how you see it, is the names of the coordinates or the crs the most important thing? Before this change all we got was a java.util.NoSuchElementException?
I would think if a user put in an x and a y there the intention is to use a cartesian point not a geographic point don't you think?
There was a problem hiding this comment.
I changed my mind. I think it would be better to keep your error message, and make the xy/latlng the primary criteria for geo/cartesian, which is what I assume you thought was the case. I think your approach is better, but it requires that we no longer assume WGS84 default for missing crs, but have two defaults, one for xy and one for latlng. That change does not need to be made as part of this PR either, as it is just a small refinement to the case of missing crs.
point( {x: 0.1, y: 1.1})should not fail withjava.util.NoSuchElementExceptionbut have a proper error message and typepoint({x: 0.1, y: 1.1, crs: "NOT_EXISING"})should fail with proper error message and type instead of being ascala.MatchErrorpoint({x: 1, y: 2, crs: "cartesian"})should be allowed and not produce ajava.lang.ClassCastException