fixes bug in slotted when checking if property IS NULL on null node

slotted rewriter incorrectly rewrote IsNull(Property(_)) * was Not(NullCheck(NodePropertyExistsLate(_))) * now Or(IsNull(Variable), NodePropertyExistsLate(_))
neo4j · Aug 23, 2018 · 1b3846b · 1b3846b
1 parent 9b2d920
commit 1b3846b
Show file tree

Hide file tree

Showing 2 changed files with 73 additions and 11 deletions.
diff --git a/...-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/ExpressionAcceptanceTest.scala b/...-suite/src/test/scala/org/neo4j/internal/cypher/acceptance/ExpressionAcceptanceTest.scala
@@ -24,6 +24,63 @@ import org.neo4j.internal.cypher.acceptance.CypherComparisonSupport._
 
 class ExpressionAcceptanceTest extends ExecutionEngineFunSuite with CypherComparisonSupport {
 
+  test("property existence checks, on node") {
+    createNode("exists" -> 1)
+    val result = executeWith(
+      expectSucceed = Configs.Interpreted,
+      query =
+        "MATCH (n) " +
+          "RETURN n.missing IS NULL," +
+          "       n.missing IS NOT NULL," +
+          "       exists(n.missing)," +
+          "       n.exists IS NULL," +
+          "       n.exists IS NOT NULL," +
+          "       exists(n.exists)")
+    result.toList should equal(List(Map(
+      "n.missing IS NULL" -> true,
+      "n.missing IS NOT NULL" -> false,
+      "exists(n.missing)" -> false,
+      "n.exists IS NULL" -> false,
+      "n.exists IS NOT NULL" -> true,
+      "exists(n.exists)" -> true)))
+  }
+
+  test("property existence checks, on optional non-null node") {
+    createNode("exists" -> 1)
+    val result = executeWith(
+      expectSucceed = Configs.Interpreted,
+      query =
+        "OPTIONAL MATCH (n) " +
+          "RETURN n.missing IS NULL," +
+          "       n.missing IS NOT NULL," +
+          "       exists(n.missing)," +
+          "       n.exists IS NULL," +
+          "       n.exists IS NOT NULL," +
+          "       exists(n.exists)")
+    result.toList should equal(List(Map(
+      "n.missing IS NULL" -> true,
+      "n.missing IS NOT NULL" -> false,
+      "exists(n.missing)" -> false,
+      "n.exists IS NULL" -> false,
+      "n.exists IS NOT NULL" -> true,
+      "exists(n.exists)" -> true)))
+  }
+
+  test("property existence checks, on optional null node") {
+    val result = executeWith(
+      expectSucceed = Configs.Interpreted,
+      query =
+        "OPTIONAL MATCH (n) " +
+          "RETURN n.missing IS NULL," +
+          // "       n.missing IS NOT NULL," + // Do not test. In 3.3 IS NOT NULL is incorrectly rewritten to Exists. Will be fixed in 3.5.
+          "       exists(n.missing)",
+      expectedDifferentResults = Configs.Empty)
+    result.toList should equal(List(Map(
+      "n.missing IS NULL" -> true,
+      // "n.missing IS NOT NULL" -> false, // Do not test. In 3.3 IS NOT NULL is incorrectly rewritten to Exists. Will be fixed in 3.5.
+      "exists(n.missing)" -> null)))
+  }
+
   test("should handle map projection with property selectors") {
     createNode("foo" -> 1, "bar" -> "apa")
 

diff --git a/...src/main/scala/org/neo4j/cypher/internal/compatibility/v3_3/runtime/SlottedRewriter.scala b/...src/main/scala/org/neo4j/cypher/internal/compatibility/v3_3/runtime/SlottedRewriter.scala
@@ -190,12 +190,23 @@ class SlottedRewriter(tokenContext: TokenContext) {
       case idFunction: FunctionInvocation if idFunction.function == frontendAst.functions.Exists =>
         idFunction.args.head match {
           case Property(Variable(key), PropertyKeyName(propKey)) =>
-            checkIfPropertyExists(pipelineInformation, key, propKey)
+            val slot = pipelineInformation(key)
+            val propExpression = checkIfPropertyExists(pipelineInformation, key, propKey, slot)
+            if (slot.nullable)
+              NullCheck(slot.offset, propExpression)
+            else
+              propExpression
           case _ => idFunction // Don't know how to specialize this
         }
 
-      case e@IsNull(Property(Variable(key), PropertyKeyName(propKey))) =>
-        Not(checkIfPropertyExists(pipelineInformation, key, propKey))(e.position)
+      case e@IsNull(Property(variable@Variable(key), PropertyKeyName(propKey))) =>
+        val slot = pipelineInformation(key)
+        val propertyExists = checkIfPropertyExists(pipelineInformation, key, propKey, slot)
+        val notPropertyExists = Not(propertyExists)(e.position)
+        if (slot.nullable)
+          Or(IsNull(variable)(e.position), notPropertyExists)(e.position)
+        else
+          notPropertyExists
 
       case _: ShortestPathExpression =>
         throw new CantCompileQueryException(s"Expressions with shortestPath functions not yet supported in slot allocation")
@@ -209,11 +220,10 @@ class SlottedRewriter(tokenContext: TokenContext) {
     topDown(rewriter = innerRewriter, stopper = stopAtOtherLogicalPlans(thisPlan))
   }
 
-  private def checkIfPropertyExists(pipelineInformation: PipelineInformation, key: String, propKey: String) = {
-    val slot = pipelineInformation(key)
+  private def checkIfPropertyExists(pipelineInformation: PipelineInformation, key: String, propKey: String, slot: Slot) = {
     val maybeToken = tokenContext.getOptPropertyKeyId(propKey)
 
-    val propExpression = (slot, maybeToken) match {
+    (slot, maybeToken) match {
       case (LongSlot(offset, _, typ, name), Some(token)) if typ == CTNode =>
         NodePropertyExists(offset, token, s"$name.$propKey")
 
@@ -228,11 +238,6 @@ class SlottedRewriter(tokenContext: TokenContext) {
 
       case _ => throw new CantCompileQueryException(s"Expressions on object other then nodes and relationships are not yet supported")
     }
-
-    if (slot.nullable)
-      NullCheck(slot.offset, propExpression)
-    else
-      propExpression
   }
 
   private def stopAtOtherLogicalPlans(thisPlan: LogicalPlan): (AnyRef) => Boolean = {