Fixes #654: Some paths are returned the wrong way around

systay · Jun 29, 2012 · 894023e · 894023e
1 parent b632e23
commit 894023e
Show file tree

Hide file tree

Showing 6 changed files with 99 additions and 28 deletions.
diff --git a/cypher/CHANGES.txt b/cypher/CHANGES.txt
@@ -2,6 +2,7 @@
 --------------------
 o Fixes problem when graph elements are deleted multiple times in the same query
 o Fixes #625: Values passed through WITH have the wrong type
+o Fixes #654: Some paths are returned the wrong way
 
 1.8.M05 (2012-06-21)
 --------------------

diff --git a/cypher/src/main/scala/org/neo4j/cypher/internal/parser/v1_8/AbstractPattern.scala b/cypher/src/main/scala/org/neo4j/cypher/internal/parser/v1_8/AbstractPattern.scala
@@ -20,11 +20,13 @@
 package org.neo4j.cypher.internal.parser.v1_8
 
 import org.neo4j.graphdb.Direction
-import org.neo4j.cypher.internal.commands.{Predicate, Expression}
+import org.neo4j.cypher.internal.commands.{Entity, Predicate, Expression}
 import collection.Map
 import org.neo4j.cypher.SyntaxException
 
-abstract sealed class AbstractPattern
+abstract sealed class AbstractPattern {
+  def makeOutgoing:AbstractPattern
+}
 
 object PatternWithEnds {
   def unapply(p: AbstractPattern): Option[(ParsedEntity, ParsedEntity, Seq[String], Direction, Boolean, Option[Int], Option[String], Predicate)] = p match {
@@ -42,7 +44,9 @@ abstract class PatternWithPathName(val pathName: String) extends AbstractPattern
 
 case class ParsedEntity(expression: Expression,
                         props: Map[String, Expression],
-                        predicate: Predicate) extends AbstractPattern
+                        predicate: Predicate) extends AbstractPattern{
+  def makeOutgoing = this
+}
 
 case class ParsedRelation(name: String,
                           props: Map[String, Expression],
@@ -51,10 +55,37 @@ case class ParsedRelation(name: String,
                           typ: Seq[String],
                           dir: Direction,
                           optional: Boolean,
-                          predicate: Predicate) extends PatternWithPathName(name) {
+                          predicate: Predicate) extends PatternWithPathName(name) with Turnable {
   def rename(newName: String): PatternWithPathName = copy(name = newName)
+
+  def turn(start: ParsedEntity, end: ParsedEntity, dir: Direction): AbstractPattern =
+    copy(start = start, end = end, dir = dir)
 }
 
+trait Turnable {
+  def turn(start: ParsedEntity, end: ParsedEntity, dir: Direction): AbstractPattern
+
+  // It's easier on everything if all relationships are either outgoing or both, but never incoming.
+  // So we turn all patterns around, facing the same way
+  def dir: Direction
+  def start:ParsedEntity
+  def end:ParsedEntity
+
+  def makeOutgoing = {
+    dir match {
+      case Direction.INCOMING => turn(start = end, end = start, dir = Direction.OUTGOING)
+      case Direction.OUTGOING => this
+      case Direction.BOTH     => (start.expression, end.expression) match {
+        case (Entity(a), Entity(b)) if a < b  => this
+        case (Entity(a), Entity(b)) if a >= b => turn(start = end, end = start, dir = dir)
+        case _                                => this
+      }
+    }
+  }
+
+}
+
+
 case class ParsedVarLengthRelation(name: String,
                                    props: Map[String, Expression],
                                    start: ParsedEntity,
@@ -65,8 +96,11 @@ case class ParsedVarLengthRelation(name: String,
                                    predicate: Predicate,
                                    minHops: Option[Int],
                                    maxHops: Option[Int],
-                                   relIterator: Option[String]) extends PatternWithPathName(name) {
+                                   relIterator: Option[String]) extends PatternWithPathName(name) with Turnable {
   def rename(newName: String): PatternWithPathName = copy(name = newName)
+
+  def turn(start: ParsedEntity, end: ParsedEntity, dir: Direction): AbstractPattern =
+    copy(start = start, end = end, dir = dir)
 }
 
 case class ParsedShortestPath(name: String,
@@ -81,8 +115,12 @@ case class ParsedShortestPath(name: String,
                               single: Boolean,
                               relIterator: Option[String]) extends PatternWithPathName(name) {
 def rename(newName: String): PatternWithPathName = copy(name = newName)
+
+  def makeOutgoing = this
 }
 
 case class ParsedNamedPath(name: String, pieces: Seq[AbstractPattern]) extends PatternWithPathName(name) {
   def rename(newName: String): PatternWithPathName = copy(name = newName)
+
+  def makeOutgoing = this
 }
diff --git a/cypher/src/main/scala/org/neo4j/cypher/internal/parser/v1_8/ParserPattern.scala b/cypher/src/main/scala/org/neo4j/cypher/internal/parser/v1_8/ParserPattern.scala
@@ -66,10 +66,10 @@ trait ParserPattern extends Base {
 
   private def patternBit: Parser[Seq[AbstractPattern]] =
     pathAssignment |
-      path |
+      pathFacingOut |
       singleNode
 
-  private def singleNode: ParserPattern.this.type#Parser[Seq[ParsedEntity]] = {
+  private def singleNode: Parser[Seq[ParsedEntity]] = {
     node ^^ (n => Seq(n))
   }
 
@@ -107,20 +107,21 @@ trait ParserPattern extends Base {
   }
 
   private def path: Parser[List[AbstractPattern]] = relationship | shortestPath
+  private def pathFacingOut: Parser[List[AbstractPattern]] = relationshipFacingOut | shortestPath
+
+  private def relationshipFacingOut = relationship ^^ (x => x.map(_.makeOutgoing))
 
   private def relationship: Parser[List[AbstractPattern]] = {
     node ~ rep1(tail) ^^ {
       case head ~ tails =>
         var start = head
         val links = tails.map {
           case Tail(dir, relName, relProps, end, None, types, optional) =>
-            val (l, r, d) = turnStartAndEndCorrect(dir, end, start)
-            val t = ParsedRelation(namer.name(relName), relProps, l, r, types, d, optional, True())
+            val t = ParsedRelation(namer.name(relName), relProps, start, end, types, dir, optional, True())
             start = end
             t
           case Tail(dir, relName, relProps, end, Some((min, max)), types, optional) =>
-            val (l, r, d) = turnStartAndEndCorrect(dir, end, start)
-            val t = ParsedVarLengthRelation(namer.name(None), relProps, l, r, types, d, optional, True(), min, max, relName)
+            val t = ParsedVarLengthRelation(namer.name(None), relProps, start, end, types, dir, optional, True(), min, max, relName)
             start = end
             t
         }
@@ -153,19 +154,6 @@ trait ParserPattern extends Base {
         relIterator = relIterator))
   }
 
-  // It's easier on everything if all relationships are either outgoing or both, but never incoming.
-  // So we turn all patterns around, facing the same way
-  private def turnStartAndEndCorrect(dir: Direction, end: ParsedEntity, start: ParsedEntity): (ParsedEntity, ParsedEntity, Direction) = {
-    dir match {
-      case Direction.INCOMING => (end, start, Direction.OUTGOING)
-      case Direction.OUTGOING => (start, end, Direction.OUTGOING)
-      case Direction.BOTH => (start.expression, end.expression) match {
-        case (Entity(a), Entity(b)) if a < b => (start, end, Direction.BOTH)
-        case (Entity(a), Entity(b)) if a >= b => (end, start, Direction.BOTH)
-        case _ => (start, end, Direction.BOTH)
-      }
-    }
-  }
 
   private def tailWithRelData: Parser[Tail] = opt("<") ~ "-" ~ "[" ~ opt(identity) ~ opt("?") ~ opt(":" ~> rep1sep(identity, "|")) ~ variable_length ~ props ~ "]" ~ "-" ~ opt(">") ~ node ^^ {
     case l ~ "-" ~ "[" ~ rel ~ optional ~ typez ~ varLength ~ properties ~ "]" ~ "-" ~ r ~ end => Tail(direction(l, r), rel, properties, end, varLength, typez.toSeq.flatten.distinct, optional.isDefined)

diff --git a/cypher/src/main/scala/org/neo4j/cypher/internal/parser/v1_8/StartClause.scala b/cypher/src/main/scala/org/neo4j/cypher/internal/parser/v1_8/StartClause.scala
@@ -42,11 +42,11 @@ trait StartClause extends Base with Expressions {
 
   private def translate(abstractPattern: AbstractPattern): Maybe[Any] = abstractPattern match {
     case ParsedNamedPath(name, patterns) =>
-      val namedPathPatterns = patterns.map(matchTranslator).reduce(_ ++ _)
-      val startItems = patterns.map(translate).reduce(_ ++ _)
+      val namedPathPatterns: Maybe[Any] = patterns.map(matchTranslator).reduce(_ ++ _)
+      val startItems = patterns.map(p => translate(p.makeOutgoing)).reduce(_ ++ _)
 
       startItems match {
-        case No(msg) => No(msg)
+        case No(msg)    => No(msg)
         case Yes(stuff) => namedPathPatterns.seqMap(p => {
           val namedPath: NamedPath = NamedPath(name, p.map(_.asInstanceOf[Pattern]): _*)
           Seq(NamedPathWStartItems(namedPath, stuff.map(_.asInstanceOf[StartItem])))

diff --git a/cypher/src/test/scala/org/neo4j/cypher/ExecutionEngineTest.scala b/cypher/src/test/scala/org/neo4j/cypher/ExecutionEngineTest.scala
@@ -2169,4 +2169,28 @@ RETURN x0.name?
 
     println(result.dumpToString())
   }
+
+  @Test
+  def pathDirectionRespected() {
+    val a = createNode()
+    val b = createNode()
+    relate(a, b)
+    val result = parseAndExecute("START a=node(1) match p=b<--a return p").toList.head("p").asInstanceOf[Path]
+
+    assert(result.startNode() === b)
+    assert(result.endNode() === a)
+  }
+
+  @Test
+  def shortestPathDirectionRespected() {
+    val a = createNode()
+    val b = createNode()
+    relate(a, b)
+    val result = parseAndExecute("START a=node(1), b=node(2) match p=shortestPath(b<-[*]-a) return p").toList.head("p").asInstanceOf[Path]
+
+    assert(result.startNode() === b)
+    assert(result.endNode() === a)
+  }
+
+
 }
diff --git a/cypher/src/test/scala/org/neo4j/cypher/MutatingIntegrationTests.scala b/cypher/src/test/scala/org/neo4j/cypher/MutatingIntegrationTests.scala
@@ -23,7 +23,7 @@ import org.junit.Test
 import org.junit.Assert._
 import collection.JavaConverters._
 import org.scalatest.Assertions
-import org.neo4j.graphdb.{NotFoundException, Relationship, Node}
+import org.neo4j.graphdb.{Path, NotFoundException, Relationship, Node}
 import java.util.HashMap
 
 class MutatingIntegrationTests extends ExecutionEngineHelper with Assertions with StatisticsChecker {
@@ -448,6 +448,26 @@ return distinct center""")
 
     assertStats(result, deletedNodes = 1)
   }
+
+  @Test
+  def created_paths_honor_directions() {
+    val a = createNode()
+    val b = createNode()
+    val result = parseAndExecute("start a=node(1), b=node(2) create p = a<-[:X]-b return p").toList.head("p").asInstanceOf[Path]
+
+    assert(result.startNode() === a)
+    assert(result.endNode() === b)
+  }
+
+  @Test
+  def related_paths_honor_directions() {
+    val a = createNode()
+    val b = createNode()
+    val result = parseAndExecute("start a=node(1), b=node(2) relate p = a<-[:X]-b return p").toList.head("p").asInstanceOf[Path]
+
+    assert(result.startNode() === a)
+    assert(result.endNode() === b)
+  }
 }
 
 trait StatisticsChecker extends Assertions {