Fixes #336 - Patterns that re-use a pattern node can produce non-exis…

…ting matches

community/cypher/CHANGES.txt

@@ -1,5 +1,9 @@
+1.9.M03
+-------
+o Fixes #336 - Patterns that re-use a pattern node can produce non-existing matches
+
 1.9.M02
--------------------
+-------
 o The traversal pattern matcher now supports variable length relationship patterns
 o Fixes #946 - HAS(...) fails with ThisShouldNotHappenException for some patterns
 o Made Cypher better at keeping a numeric type. Adding two integers now returns an integer, and not a double.

community/cypher/src/main/scala/org/neo4j/cypher/internal/pipes/matching/EndPoint.scala

@@ -33,9 +33,15 @@ final case class EndPoint(name: String) extends Trail {
 
   def toSteps(id: Int) = None
 
-  protected[matching] def decompose(p: Seq[PropertyContainer], r: Map[String, Any]) =
+  protected[matching] def decompose(p: Seq[PropertyContainer], mapSoFar: Map[String, Any]) =
     if (p.size == 1) {
-      Iterator((Seq.empty, r ++ Map(name -> p.head)))
+      val existingValue = mapSoFar.get(name)
+      val endNode = p.head
+
+      existingValue match {
+        case Some(existing) if endNode != existing => Iterator()
+        case _                                     => Iterator((Seq.empty, mapSoFar ++ Map(name -> endNode)))
+      }
     } else {
       Iterator()
     }

community/cypher/src/main/scala/org/neo4j/cypher/internal/pipes/matching/SingleStepTrail.scala

@@ -26,15 +26,15 @@ import org.neo4j.cypher.internal.symbols.{RelationshipType, NodeType, SymbolTabl
 
 final case class SingleStepTrail(next: Trail,
                                  dir: Direction,
-                                 rel: String,
+                                 relName: String,
                                  typ: Seq[String],
                                  start: String,
                                  relPred: Option[Predicate],
                                  nodePred: Option[Predicate],
                                  pattern: Pattern) extends Trail {
   def end = next.end
 
-  def pathDescription = next.pathDescription ++ Seq(rel, end)
+  def pathDescription = next.pathDescription ++ Seq(relName, end)
 
   def toSteps(id: Int) = {
     val steps = next.toSteps(id + 1)
@@ -50,14 +50,22 @@ final case class SingleStepTrail(next: Trail,
     if (p.size < 2) {
       Iterator()
     } else {
-      val r = p.tail.head
-      val n = p.head
-      val newMap = m + (rel -> r) + (start -> n)
-      next.decompose(p.tail.tail, newMap)
+      val thisRel = p.tail.head
+      val thisNode = p.head
+
+      val a = m.get(relName)
+      val b = m.get(start)
+
+      if ((a.nonEmpty && a.get != thisRel)||(b.nonEmpty && b.get != thisNode)) {
+        Iterator()
+      } else {
+        val newMap = m + (relName -> thisRel) + (start -> thisNode)
+        next.decompose(p.tail.tail, newMap)
+      }
     }
 
   def symbols(table: SymbolTable): SymbolTable =
-    next.symbols(table).add(start, NodeType()).add(rel, RelationshipType())
+    next.symbols(table).add(start, NodeType()).add(relName, RelationshipType())
 
   def contains(target: String): Boolean = next.contains(target) || target == end
 
@@ -73,7 +81,7 @@ final case class SingleStepTrail(next: Trail,
       case x      => typ.mkString(":", "|", "")
     }
 
-    "(%s)%s-[%s%s]-%s%s".format(start, left, rel, t, right, next.toString)
+    "(%s)%s-[%s%s]-%s%s".format(start, left, relName, t, right, next.toString)
   }
 
 

community/cypher/src/main/scala/org/neo4j/cypher/internal/pipes/matching/VariableLengthStepTrail.scala

@@ -62,35 +62,42 @@ final case class VariableLengthStepTrail(next: Trail,
     left = x._2
 
     var result: Seq[(Seq[PropertyContainer], Map[String, Any])] = Seq.empty
-    val map = MutableMaps.create(m)
-    map += (start -> p.head)
 
-    var validRelationship = checkPath(curr)
+    val newNode = p.head
+    val oldNode = m.get(start)
 
-    while (validRelationship &&
-           idx <= max.getOrElse(idx) &&
-           left.nonEmpty) {
+    if (oldNode.isEmpty || oldNode.get == newNode) {
+      val map = MutableMaps.create(m)
 
-      val currentPath = curr :+ left.head
-      map += (path -> PathImpl(currentPath:_*))
+      map += (start -> newNode)
 
-      relIterator.foreach {
-        key => map += (key -> currentPath.filter(_.isInstanceOf[Relationship]))
-      }
+      var validRelationship = checkPath(curr)
+
+      while (validRelationship &&
+             idx <= max.getOrElse(idx) &&
+             left.nonEmpty) {
+
+        val currentPath = curr :+ left.head
+        map += (path -> PathImpl(currentPath: _*))
 
-      //Add this result to the stack
-      //if our downstreams trail doesn't return anything,
-      //we'll also not return anything
-      result = result ++ next.decompose(left, map.toMap)
+        relIterator.foreach {
+          key => map += (key -> currentPath.filter(_.isInstanceOf[Relationship]))
+        }
 
-      //Get more stuff from the remaining path
-      idx += 1
+        //Add this result to the stack
+        //if our downstreams trail doesn't return anything,
+        //we'll also not return anything
+        result = result ++ next.decompose(left, map.toMap)
 
-      val x = p.splitAt(idx * 2)
-      curr = x._1
-      left = x._2
+        //Get more stuff from the remaining path
+        idx += 1
 
-      validRelationship = checkPath(curr)
+        val x = p.splitAt(idx * 2)
+        curr = x._1
+        left = x._2
+
+        validRelationship = checkPath(curr)
+      }
     }
 
     result.toIterator

community/cypher/src/test/scala/org/neo4j/cypher/docgen/WithTest.scala

@@ -19,7 +19,6 @@
  */
 package org.neo4j.cypher.docgen
 
-import org.neo4j.cypher.internal.commands.expressions.Literal
 import org.junit.Test
 import org.neo4j.graphdb.Node
 import org.junit.Assert._

community/cypher/src/test/scala/org/neo4j/cypher/internal/executionplan/builders/TrailDecomposeTest.scala

@@ -58,6 +58,64 @@ class TrailDecomposeTest extends GraphDatabaseTestBase with Assertions with Buil
     assert(resultMap === List(Map("a" -> nodeA, "b" -> nodeB, "c" -> nodeC, "link1" -> rel1, "link2" -> rel2)))
   }
 
+  @Test def should_not_return_maps_that_have_contradicting_values_in_pattern_points_endpoint() {
+    // When a pattern has the same pattern node in multiple places in the pattern,
+    // we need to exclude it from the results
+
+    //a-[r1]->b-[r2]->c-[r3]->b
+
+    val nodeA = createNode("A")
+    val nodeB = createNode("B")
+    val nodeC = createNode("C")
+    val nodeD = createNode("D")
+
+    val rel1 = relate(nodeA, nodeB)
+    val rel2 = relate(nodeB, nodeC)
+    val rel3 = relate(nodeC, nodeD)
+
+
+    val kernPath = Seq(nodeA, rel1, nodeB, rel2, nodeC, rel3, nodeD)
+
+    val cPoint = EndPoint("b")
+    val cToB = SingleStepTrail(cPoint, Direction.OUTGOING, "r3", Seq(), "c", None, None, null)
+    val bToC = SingleStepTrail(cToB, Direction.OUTGOING, "r2", Seq(), "b", None, None, null)
+    val aToB = SingleStepTrail(bToC, Direction.OUTGOING, "r1", Seq(), "a", None, None, null)
+
+    val resultMap = aToB.decompose(kernPath).toList
+
+    assert(resultMap === List())
+  }
+
+  @Test def should_not_return_maps_that_have_contradicting_values_in_pattern_points_single() {
+    // When a pattern has the same pattern node in multiple places in the pattern,
+    // we need to exclude it from the results
+
+    //a-[r1]->b-[r2]->c-[r3]->b-[r4]->x
+
+    val nodeA = createNode("A")
+    val nodeB = createNode("B")
+    val nodeC = createNode("C")
+    val nodeD = createNode("D")
+    val nodeX = createNode("X")
+
+    val rel1 = relate(nodeA, nodeB)
+    val rel2 = relate(nodeB, nodeC)
+    val rel3 = relate(nodeC, nodeD)
+    val rel4 = relate(nodeD, nodeX)
+
+    val kernPath = Seq(nodeA, rel1, nodeB, rel2, nodeC, rel3, nodeD, rel4, nodeX)
+
+    val dPoint = EndPoint("x")
+    val bToD = SingleStepTrail(dPoint, Direction.OUTGOING, "r4", Seq(), "b", None, None, null)
+    val cToB = SingleStepTrail(bToD, Direction.OUTGOING, "r3", Seq(), "c", None, None, null)
+    val bToC = SingleStepTrail(cToB, Direction.OUTGOING, "r2", Seq(), "b", None, None, null)
+    val aToB = SingleStepTrail(bToC, Direction.OUTGOING, "r1", Seq(), "a", None, None, null)
+
+    val resultMap = aToB.decompose(kernPath).toList
+
+    assert(resultMap === List())
+  }
+
   @Test def decompose_single_varlength_step() {
     //Given:
     //Pattern: a-[:A*1..2]->b