Moved the responsibility of creating a pattern graph to the MatchBuil…

…der in preparation for splitting up the matching later

cypher/src/main/scala/org/neo4j/cypher/internal/commands/PathExpression.scala

@@ -24,10 +24,11 @@ import org.neo4j.cypher.internal.pipes.matching.MatchingContext
 import collection.Map
 import org.neo4j.helpers.ThisShouldNotHappenError
 import org.neo4j.graphdb.Path
+import org.neo4j.cypher.internal.executionplan.builders.PatternGraphBuilder
 
-case class PathExpression(pathPattern: Seq[Pattern]) extends Expression with PathExtractor {
+case class PathExpression(pathPattern: Seq[Pattern]) extends Expression with PathExtractor with PatternGraphBuilder {
   val symbols = new SymbolTable(declareDependencies(AnyType()).distinct: _*)
-  val matchingContext = new MatchingContext(pathPattern, symbols, Seq())
+  val matchingContext = new MatchingContext(pathPattern, symbols, Seq(), buildPatternGraph(symbols, pathPattern))
   val interestingPoints = pathPattern.flatMap(_.possibleStartPoints.map(_.name)).distinct
 
   def compute(m: Map[String, Any]): Any = {

cypher/src/main/scala/org/neo4j/cypher/internal/executionplan/builders/MatchBuilder.scala

@@ -20,20 +20,27 @@
 package org.neo4j.cypher.internal.executionplan.builders
 
 import org.neo4j.cypher.internal.pipes.{MatchPipe, Pipe}
-import org.neo4j.cypher.internal.commands.{ShortestPath, StartItem, Pattern}
+import org.neo4j.cypher.internal.commands._
 import org.neo4j.cypher.internal.executionplan.{ExecutionPlanInProgress, PlanBuilder}
+import org.neo4j.cypher.internal.symbols.{NodeType, SymbolTable}
+import org.neo4j.cypher.internal.pipes.matching.{PatternRelationship, PatternNode, PatternGraph}
+import org.neo4j.cypher.SyntaxException
+import org.neo4j.cypher.internal.executionplan.builders.Unsolved
+import scala.Some
+import org.neo4j.cypher.internal.executionplan.ExecutionPlanInProgress
+import org.neo4j.cypher.internal.commands.ShortestPath
 
-class MatchBuilder extends PlanBuilder {
+class MatchBuilder extends PlanBuilder with PatternGraphBuilder {
   def apply(plan: ExecutionPlanInProgress) = {
     val q = plan.query
     val p = plan.pipe
 
-
     val items = q.patterns.filter(yesOrNo(_, p, q.start))
     val patterns = items.map(_.token)
     val predicates = q.where.filter(!_.solved).map(_.token)
+    val graph = buildPatternGraph(p.symbols, patterns)
 
-    val newPipe = new MatchPipe(p, patterns, predicates)
+    val newPipe = new MatchPipe(p, patterns, predicates, graph)
 
     plan.copy(
       query = q.copy(patterns = q.patterns.filterNot(items.contains) ++ items.map(_.solve)),
@@ -63,4 +70,37 @@ class MatchBuilder extends PlanBuilder {
   }
 
   def priority = PlanBuilder.Match
+}
+
+trait PatternGraphBuilder {
+  def buildPatternGraph(boundIdentifiers: SymbolTable, patterns: Seq[Pattern]): PatternGraph = {
+    val patternNodeMap: scala.collection.mutable.Map[String, PatternNode] = scala.collection.mutable.Map()
+    val patternRelMap: scala.collection.mutable.Map[String, PatternRelationship] = scala.collection.mutable.Map()
+
+    boundIdentifiers.identifiers.
+      filter(_.typ == NodeType()). //Find all bound nodes...
+      foreach(id => patternNodeMap(id.name) = new PatternNode(id.name)) //...and create patternNodes for them
+
+    patterns.foreach(_ match {
+      case RelatedTo(left, right, rel, relType, dir, optional, predicate) => {
+        val leftNode: PatternNode = patternNodeMap.getOrElseUpdate(left, new PatternNode(left))
+        val rightNode: PatternNode = patternNodeMap.getOrElseUpdate(right, new PatternNode(right))
+
+        if (patternRelMap.contains(rel)) {
+          throw new SyntaxException("Can't re-use pattern relationship '%s' with different start/end nodes.".format(rel))
+        }
+
+        patternRelMap(rel) = leftNode.relateTo(rel, rightNode, relType, dir, optional, predicate)
+      }
+      case VarLengthRelatedTo(pathName, start, end, minHops, maxHops, relType, dir, iterableRel, optional, predicate) => {
+        val startNode: PatternNode = patternNodeMap.getOrElseUpdate(start, new PatternNode(start))
+        val endNode: PatternNode = patternNodeMap.getOrElseUpdate(end, new PatternNode(end))
+        patternRelMap(pathName) = startNode.relateViaVariableLengthPathTo(pathName, endNode, minHops, maxHops, relType, dir, iterableRel, optional, predicate)
+      }
+      case _ =>
+    })
+
+    new PatternGraph(patternNodeMap.toMap, patternRelMap.toMap, boundIdentifiers)
+  }
+
 }

cypher/src/main/scala/org/neo4j/cypher/internal/pipes/MatchPipe.scala

@@ -19,12 +19,12 @@
  */
 package org.neo4j.cypher.internal.pipes
 
-import matching.MatchingContext
+import matching.{PatternGraph, MatchingContext}
 import java.lang.String
 import org.neo4j.cypher.internal.commands.{Predicate, Pattern}
 
-class MatchPipe(source: Pipe, patterns: Seq[Pattern], predicates: Seq[Predicate]) extends Pipe {
-  val matchingContext = new MatchingContext(patterns, source.symbols, predicates)
+class MatchPipe(source: Pipe, patterns: Seq[Pattern], predicates: Seq[Predicate], patternGraph:PatternGraph) extends Pipe {
+  val matchingContext = new MatchingContext(patterns, source.symbols, predicates, patternGraph)
   val symbols = matchingContext.symbols
 
   def createResults(state: QueryState) =

cypher/src/main/scala/org/neo4j/cypher/internal/pipes/matching/MatchingContext.scala

@@ -31,8 +31,10 @@ import org.neo4j.cypher.internal.symbols._
  * The deciding factor is whether or not the pattern has loops in it. If it does, we have to use the much more
  * expensive pattern matching. If it doesn't, we get away with much simpler methods
  */
-class MatchingContext(patterns: Seq[Pattern], boundIdentifiers: SymbolTable, predicates: Seq[Predicate] = Seq()) {
-  val patternGraph = buildPatternGraph()
+class MatchingContext(patterns: Seq[Pattern],
+                      boundIdentifiers: SymbolTable,
+                      predicates: Seq[Predicate] = Seq(),
+                      patternGraph: PatternGraph) {
   val containsHardPatterns = patterns.find(!_.isInstanceOf[RelatedTo]).nonEmpty
   val builder: MatcherBuilder = decideWhichMatcherToUse()
 
@@ -53,48 +55,15 @@ class MatchingContext(patterns: Seq[Pattern], boundIdentifiers: SymbolTable, pre
   }
 
   private def decideWhichMatcherToUse(): MatcherBuilder = {
-    /*if (JoinerBuilder.canHandlePatter(patternGraph)) {
-      new JoinerBuilder(patternGraph, predicates)
-    } else */
     if(SimplePatternMatcherBuilder.canHandle(patternGraph)) {
       new SimplePatternMatcherBuilder(patternGraph, predicates, symbols)
     } else {
-
       new PatterMatchingBuilder(patternGraph, predicates)
     }
   }
-
-  private def buildPatternGraph(): PatternGraph = {
-    val patternNodeMap: scala.collection.mutable.Map[String, PatternNode] = scala.collection.mutable.Map()
-    val patternRelMap: scala.collection.mutable.Map[String, PatternRelationship] = scala.collection.mutable.Map()
-
-    boundIdentifiers.identifiers.
-      filter(_.typ == NodeType()). //Find all bound nodes...
-      foreach(id => patternNodeMap(id.name) = new PatternNode(id.name)) //...and create patternNodes for them
-
-    patterns.foreach(_ match {
-      case RelatedTo(left, right, rel, relType, dir, optional, predicate) => {
-        val leftNode: PatternNode = patternNodeMap.getOrElseUpdate(left, new PatternNode(left))
-        val rightNode: PatternNode = patternNodeMap.getOrElseUpdate(right, new PatternNode(right))
-
-        if (patternRelMap.contains(rel)) {
-          throw new SyntaxException("Can't re-use pattern relationship '%s' with different start/end nodes.".format(rel))
-        }
-
-        patternRelMap(rel) = leftNode.relateTo(rel, rightNode, relType, dir, optional, predicate)
-      }
-      case VarLengthRelatedTo(pathName, start, end, minHops, maxHops, relType, dir, iterableRel, optional, predicate) => {
-        val startNode: PatternNode = patternNodeMap.getOrElseUpdate(start, new PatternNode(start))
-        val endNode: PatternNode = patternNodeMap.getOrElseUpdate(end, new PatternNode(end))
-        patternRelMap(pathName) = startNode.relateViaVariableLengthPathTo(pathName, endNode, minHops, maxHops, relType, dir, iterableRel, optional, predicate)
-      }
-      case _ =>
-    })
-
-    new PatternGraph(patternNodeMap.toMap, patternRelMap.toMap, boundIdentifiers)
-  }
 }
 
 trait MatcherBuilder {
   def getMatches(sourceRow: Map[String, Any]): Traversable[Map[String, Any]]
-}
+}
+

cypher/src/main/scala/org/neo4j/cypher/internal/pipes/matching/PatternGraph.scala

@@ -58,14 +58,13 @@ class PatternGraph(val patternNodes: Map[String, PatternNode],
 
     val hasLoops = checkIfWeHaveLoops(boundElements, allElements)
     val optionalSet = getOptionalElements(boundElements, allElements)
-    val doubleOptionals = getDoubleOptionals(boundElements)
+    val doubleOptionals = getDoubleOptionals(boundElements, optionalSet)
 
     (elementsMap, optionalSet, hasLoops, doubleOptionals)
   }
 
-  private def reduceDoubleOptionals(dops: Seq[DoubleOptionalPath]) = dops.distinct
 
-  private def getDoubleOptionals(boundPatternElements: Seq[PatternElement]): Seq[DoubleOptionalPath] = {
+  private def getDoubleOptionals(boundPatternElements: Seq[PatternElement], optionalElements:Set[String]): Seq[DoubleOptionalPath] = {
     var doubleOptionals = Seq[DoubleOptionalPath]()
 
 
@@ -117,7 +116,12 @@ class PatternGraph(val patternNodes: Map[String, PatternNode],
       path = Seq()
     ))
 
-    reduceDoubleOptionals(doubleOptionals)
+    //Before we return, let's remove all double optional paths that have a at least one node between the optional
+    //relationships that is not optional.
+    doubleOptionals.distinct.filterNot(dop => {
+      val optionalPartOfDop: Seq[PatternElement] = dop.path.tail.reverse.tail
+      optionalPartOfDop.exists(x => !optionalElements.contains(x.key))
+    })
   }
 
   private def getOptionalElements(boundPatternElements: Seq[PatternElement], allPatternElements: Seq[PatternElement]): Set[String] = {

cypher/src/test/scala/org/neo4j/cypher/internal/executionplan/builders/MatchBuilderTest.scala

@@ -105,4 +105,15 @@ class MatchBuilderTest extends BuilderTest {
 
     assertFalse(builder.canWorkWith(plan(inP, inQ)))
   }
+
+  @Test
+  def should_accept_non_optional_parts_of_the_query_first() {
+    val inQ = PartiallySolvedQuery().
+      copy(start = Seq(Solved(NodeById("a", 0)), Solved(NodeById("b", 0))),
+      patterns = Seq(Unsolved(ShortestPath("p", "a", "b", Seq(), Direction.OUTGOING, None, false, true, None))))
+
+    val inP = createPipe(nodes = Seq("l"))
+
+    assertFalse(builder.canWorkWith(plan(inP, inQ)))
+  }
 }