Add support for spatial point values and distance comparisons #94

Andy2003 · 2020-07-16T09:41:06Z

This PR add support for spatials and distance comparisons.

Now you can rund queries like:

query {
  person(location:{longitude: 1, latitude: 2 }){
    name
    location {
      crs
      longitude
      latitude
      height
    }
  }
}

or

{
  person(filter:{ location_distance_gte: { distance: 3, point: {longitude: 1, latitude:2, height: 3}}}){
    name
  }
}

resolves #11

neo4j-graphql#11)

michael-simons · 2020-07-27T12:01:59Z

FYI Cypher-DSL 2020.0.1 with your PR should be on central soonish.

<dependency>
        <groupId>org.neo4j</groupId>
        <artifactId>neo4j-cypher-dsl</artifactId>
        <version>2020.0.1</version>
</dependency>

jexp · 2020-08-05T14:58:09Z

src/main/kotlin/org/neo4j/graphql/Predicates.kt

                    if (op != null) {
-                        return ExpressionPredicate(definedField.name, op, value, definedField)
+                        if (definedField.type.isNeo4jType()) {


can we extract the block into a method?

jexp · 2020-08-05T14:59:28Z

src/main/kotlin/org/neo4j/graphql/Predicates.kt

-            "$not$variable.${name.quote()} ${op.op} \$$paramName"
+            val appendField = if (nestedField != null) {
+                paramName += "_$nestedField"
+                ".$nestedField"


paramNames can't contain dots ?
is there a better way than modfiying the paramName?

they can, but it breaks the "style" so I've chosen to use underscore

they can't, since neo4j will try to resolve everything behind the . as property of a scalar type

jexp · 2020-08-05T15:04:03Z

src/main/kotlin/org/neo4j/graphql/handler/filter/OptimizedFilterHandler.kt

-                val condition = conditionField.op.conditionCreator(prop, parameter)
-                filterParams[parameter.name] = conditionField.queryField.value.toJavaValue()
-                where = conditionAdder(where, condition)
+                if (conditionField.fieldDefinition.type.isNeo4jType()


we have a lot of places where larger chunks of code related to spatial / distance are now spread through the code-base like here, would be there any way of extracting them/putting them together so that the implementation doesn't leak into all these places?

jexp · 2020-08-05T15:04:51Z

src/main/kotlin/org/neo4j/graphql/handler/filter/OptimizedFilterHandler.kt

+                } else {
+                    val parameter = Cypher.parameter(variablePrefix + "_" + conditionField.queryField.name)
+                    var name = conditionField.fieldDefinition.name
+                    if (appendObjectFieldName) {


also more boolean conditions like this one that are passed through which make the code much harder to understand and maintain.

esp. as they only communicate the what but not the why and what does it mean.

it would also be good if we found a way to co-locate these code-chunks for the different operations (mostly expressions) with the enums / expression types that were originally meant for keeping that functionality isolated.

jexp · 2020-08-05T15:06:48Z

src/main/kotlin/org/neo4j/graphql/handler/filter/OptimizedFilterHandler.kt

-                                queriedFields.remove(queryFieldName)?.let { (index, objectField) ->
-                                    Predicate(predicate, objectField, definedField, index)
+                                queriedFields[queryFieldName]?.let { (index, objectField) ->
+                                    if (predicate.requireParam xor (objectField.value !is NullValue)) {


first time I've seen an XOR in real code. esp. xor with negations is kinda ouch

jexp · 2020-08-05T15:07:02Z

src/main/kotlin/org/neo4j/graphql/handler/filter/OptimizedFilterHandler.kt

-                                    Predicate(predicate, objectField, definedField, index)
+                                queriedFields[queryFieldName]?.let { (index, objectField) ->
+                                    if (predicate.requireParam xor (objectField.value !is NullValue)) {
+                                        // if we got a value but the predicate requires none


can we turn the comment into code instead?

I did this before the xor but the condition was even uglier than the xor. Do you really don't like the xor so much.
In my opinion this line is even worse:

if ((predicate.requireParam && objectField.value is NullValue) || (!predicate.requireParam && objectField.value !is NullValue))

jexp · 2020-08-05T15:08:48Z

src/test/kotlin/org/neo4j/graphql/utils/AsciiDocTestSuite.kt

-            is Float -> v.toDouble(); is Int -> v.toLong(); else -> v
+            is Float -> v.toDouble()
+            is Int -> v.toLong()
+            is List<*> -> v.map { fixNumber(it) }


probably for Iterable/Sequence?

jexp · 2020-08-05T15:12:29Z

src/test/resources/augmentation-tests.adoc

@@ -214,6 +229,42 @@ type _Neo4jLocalTime {
  second: Int
 }

+type _Neo4jPoint {


I wonder if we can minimize that.

Either by splitting it into 2 point types.
And removing / limiting a bit of the capabilities (e.g. coordinate systems) ?

/cc @johnymontana

This looks to match the point type used in neo4j-graphql.js, which is good. I don't think we want multiple GraphQL point types, and instead we should match the Neo4j database point type.

I think the issue @jexp is getting at here is that there are many optional fields on the point type. This stems from the fact that there aren't separate database types for 2D, or 3D - instead they are the same type. So to match that behavior the GraphQL type needs to have these optional fields. One thing that could be done in the future to simplify this is to use a GraphQL union input type (coming to the GraphQL spec soon) for the mutations and filters, which would have a LatLon, xyz, etc types wrapped up in a point union input type which I think would make it more clear what the various options for specifying a point type are.

In most cases I think users are interested in latitude, longitude with the geographic CRS, which is the default.

jexp

Please see if we can do anything about my comments.

Otherwise it's ok if we merge it but we need to make sure the code stays maintainable and we're not just slapping more if conditionals and booleans into all the places.

jexp

Looks great, thanks so much!

Add support for spatial point values and distance comparisons (resolves

a223a26

neo4j-graphql#11)

Andy2003 requested a review from jexp July 16, 2020 09:41

Update neo4j-opencypher-dsl dependency

04407ab

jexp reviewed Aug 5, 2020

View reviewed changes

jexp requested changes Aug 5, 2020

View reviewed changes

adjustments after review

9aca686

Andy2003 requested a review from jexp August 5, 2020 17:31

jexp approved these changes Aug 6, 2020

View reviewed changes

jexp merged commit 2ad5915 into neo4j-graphql:master Aug 6, 2020

Andy2003 deleted the feature/GH11-support-spatial branch August 27, 2020 09:11

Add support for spatial point values and distance comparisons #94

Add support for spatial point values and distance comparisons #94

Uh oh!

Conversation

Andy2003 commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-simons commented Jul 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jexp left a comment

Choose a reason for hiding this comment

Uh oh!

jexp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Andy2003 commented Jul 16, 2020 •

edited

Loading