Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed shortestPath planning when preceded by unwind #8699

Merged
merged 2 commits into from
Jan 27, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/*
* Copyright (c) 2002-2017 "Neo Technology,"
* Network Engine for Objects in Lund AB [http://neotechnology.com]
*
* This file is part of Neo4j.
*
* Neo4j is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
package org.neo4j.internal.cypher.acceptance

import org.hamcrest.CoreMatchers._
import org.junit.Assert._
import org.neo4j.cypher.internal.RewindableExecutionResult
import org.neo4j.cypher.internal.compatibility.ExecutionResultWrapperFor3_0
import org.neo4j.cypher.{ExecutionEngineFunSuite, NewPlannerTestSupport}
import org.neo4j.graphdb.Node

class ShortestPathComplexQueryAcceptanceTest extends ExecutionEngineFunSuite with NewPlannerTestSupport {

test("allShortestPaths with complex LHS should be planned with exhaustive fallback and include predicate") {
setupModel()
val result = executeUsingCostPlannerOnly(
"""
|//same idea but iterating over two collections of nodes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same idea as what? The below test? I find this in-query comment a bit redundant wrt the test name.

|profile match (charles:Pixie { fname : 'Charles'}),(joey:Pixie { fname : 'Joey'}),(kim:Pixie { fname : 'Kim'})
|with kim as kimDeal, collect(charles) as charlesT, collect(joey) as joeyS
|unwind charlesT AS charlesThompson
|unwind joeyS AS joeySantiago
|match pathx = allShortestPaths((charlesThompson)-[*1..5]-(joeySantiago))
|where none (n IN nodes(pathx) where id(n) = id(kimDeal))
|with nodes(pathx) as nodes
|return nodes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a pretty complex query, I think it'd aid readability to format it more in line with Cypher code style.

""".stripMargin)
val results = result.columnAs("nodes").toList
println(results)
println(result.executionPlanDescription())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove printlns.

val ids = results(0).asInstanceOf[Seq[_]].map(n => n.asInstanceOf[Node].getId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are asserting on the ids only, isn't it easier to adapt the query to return only ids, instead of this mouthful?

E.g:

...
RETURN [n IN nodes | id(n)] AS ids

and then

result.toList should equal List(Map("ids" -> List(0, 4, 3, 2)))

That also makes sure that there isn't a second row returned (which is overlooked by only asserting on results(0)).

ids should be(List(0, 4, 3, 2))
result should use("VarLengthExpand(Into)", "AntiConditionalApply")
}

test("shortestPath with complex LHS should be planned with exhaustive fallback and include predicate") {
System.setProperty("pickBestPlan.VERBOSE","true")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

setupModel()
val result = executeUsingCostPlannerOnly(
"""
|profile match (charles:Pixie { fname : 'Charles'}),(joey:Pixie { fname : 'Joey'}),(kim:Pixie { fname : 'Kim'})
|with kim as kimDeal, collect(charles) as charlesT, collect(joey) as joeyS
|unwind charlesT AS charlesThompson
|unwind joeyS AS joeySantiago
|match pathx = shortestPath((charlesThompson)-[*1..5]-(joeySantiago))
|where none (n IN nodes(pathx) where id(n) = id(kimDeal))
|unwind nodes(pathx) as nodes
|return nodes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I please request Cypher styling of this one too? :)

""".stripMargin)
val results = result.columnAs("nodes").toList
println(results)
println(result.executionPlanDescription())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

assertThat(results.length, equalTo(4))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a pretty vague result assertion; why not assert on the exact result contents?

result should use("VarLengthExpand(Into)", "AntiConditionalApply")
}

def executeUsingCostPlannerOnly(query: String) =
eengine.execute(s"CYPHER planner=IDP $query", Map.empty[String, Any], graph.session()) match {
case e:ExecutionResultWrapperFor3_0 => RewindableExecutionResult(e)
}

private def setupModel(): Unit = {
executeUsingCostPlannerOnly(
"""
|//dataset creation
|merge (p1:Pixie {fname:'Charles'})
|merge (p2:Pixie {fname:'Kim'})
|merge (p3:Pixie {fname:'Joey'})
|merge (p4:Pixie {fname:'David'})
|merge (p5:Pixie {fname:'Paz'})
|merge (p1)-[:KNOWS]->(p2)-[:KNOWS]->(p3)-[:KNOWS]->(p4)-[:KNOWS]->(p5)-[:KNOWS]->(p1)
|return p1,p2,p3,p4,p5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it doesn't matter a lot, but these could be CREATE instead, and RETURN (and the comment) may be omitted.

""".stripMargin)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -214,11 +214,12 @@ case class QueryGraph(patternRelationships: Set[PatternRelationship] = Set.empty
case patternNode if !visited(patternNode) =>
val qg = connectedComponentFor(patternNode, visited)
val coveredIds = qg.coveredIds
val predicates = selections.predicates.filter(_.dependencies.subsetOf(coveredIds ++ argumentIds))
val filteredHints = hints.filter(h => h.variables.forall(variable => coveredIds.contains(IdName(variable.name))))
val shortestPaths = shortestPathPatterns.filter {
p => coveredIds.contains(p.rel.nodes._1) && coveredIds.contains(p.rel.nodes._2)
}
val shortestPathIds = shortestPaths.flatMap(p => p.name)
val predicates = selections.predicates.filter(_.dependencies.subsetOf(coveredIds ++ argumentIds ++ shortestPathIds))
val filteredHints = hints.filter(h => h.variables.forall(variable => coveredIds.contains(IdName(variable.name))))
qg.
withSelections(Selections(predicates)).
withArgumentIds(argumentIds).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,11 @@ case class SingleComponentPlanner(monitor: IDPQueryGraphSolverMonitor,

result
} else {
val solutionPlans = leaves collect {
case plan if planFullyCoversQG(qg, plan) => kit.select(plan, qg)
}
val solutionPlans = if (qg.shortestPathPatterns.isEmpty)
leaves collect {
case plan if planFullyCoversQG(qg, plan) => kit.select(plan, qg)
}
else leaves map (kit.select(_, qg)) filter (planFullyCoversQG(qg, _))
val result = kit.pickBest(solutionPlans).getOrElse(throw new InternalException("Found no leaf plan for connected component. This must not happen. QG: " + qg))
monitor.noIDPIterationFor(qg, result)
result
Expand Down