Fixed bug in `IncidentToAdjacentStrategy`, it was missing some invalidating steps.
Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/52e3b86c Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/52e3b86c Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/52e3b86c Branch: refs/heads/TINKERPOP-1602 Commit: 52e3b86c06938c3427b2452c39a44b75d92e5a6f Parents: 7ba7ecd Author: Daniel Kuppitz <daniel_kupp...@hotmail.com> Authored: Mon Jan 9 15:31:54 2017 +0100 Committer: Daniel Kuppitz <daniel_kupp...@hotmail.com> Committed: Mon Jan 9 15:31:54 2017 +0100 ---------------------------------------------------------------------- CHANGELOG.asciidoc | 1 + .../IncidentToAdjacentStrategy.java | 50 +++++++++-------- .../IncidentToAdjacentStrategyTest.java | 56 ++++++++++---------- 3 files changed, 58 insertions(+), 49 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/52e3b86c/CHANGELOG.asciidoc ---------------------------------------------------------------------- diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index fa67c56..f82fa23 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -26,6 +26,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima TinkerPop 3.1.6 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Fixed bug in `IncidentToAdjacentStrategy`, it was missing some invalidating steps. * Returned a confirmation on session close from Gremlin Server. * Use non-default port for running tests on Gremlin Server. * Fully shutdown metrics services in Gremlin Server on shutdown. http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/52e3b86c/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/IncidentToAdjacentStrategy.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/IncidentToAdjacentStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/IncidentToAdjacentStrategy.java index 712110d4..2edfa17 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/IncidentToAdjacentStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/IncidentToAdjacentStrategy.java @@ -22,10 +22,14 @@ import org.apache.tinkerpop.gremlin.process.traversal.Step; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.step.LambdaHolder; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.CyclicPathStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.SimplePathStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.EdgeOtherVertexStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.EdgeVertexStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.PathStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.map.TreeStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.VertexStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.TreeSideEffectStep; import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper; import org.apache.tinkerpop.gremlin.structure.Direction; @@ -63,31 +67,12 @@ public final class IncidentToAdjacentStrategy extends AbstractTraversalStrategy< implements TraversalStrategy.OptimizationStrategy { private static final IncidentToAdjacentStrategy INSTANCE = new IncidentToAdjacentStrategy(); - private static final Set<Class> INVALIDATING_STEP_CLASSES = new HashSet<>(Arrays.asList(PathStep.class, LambdaHolder.class)); + private static final Set<Class> INVALIDATING_STEP_CLASSES = new HashSet<>(Arrays.asList(CyclicPathStep.class, + PathStep.class, SimplePathStep.class, TreeStep.class, TreeSideEffectStep.class, LambdaHolder.class)); private IncidentToAdjacentStrategy() { } - @Override - public void apply(Traversal.Admin<?, ?> traversal) { - final Traversal.Admin root = TraversalHelper.getRootTraversal(traversal); - if (TraversalHelper.hasStepOfAssignableClassRecursively(INVALIDATING_STEP_CLASSES, root)) - return; - final Collection<Pair<VertexStep, Step>> stepsToReplace = new ArrayList<>(); - Step prev = null; - for (final Step curr : traversal.getSteps()) { - if (isOptimizable(prev, curr)) { - stepsToReplace.add(Pair.with((VertexStep) prev, curr)); - } - prev = curr; - } - if (!stepsToReplace.isEmpty()) { - for (final Pair<VertexStep, Step> pair : stepsToReplace) { - optimizeSteps(traversal, pair.getValue0(), pair.getValue1()); - } - } - } - /** * Checks whether a given step is optimizable or not. * @@ -102,7 +87,8 @@ public final class IncidentToAdjacentStrategy extends AbstractTraversalStrategy< if (step1Dir.equals(Direction.BOTH)) { return step2 instanceof EdgeOtherVertexStep; } - return step2 instanceof EdgeVertexStep && ((EdgeVertexStep) step2).getDirection().equals(step1Dir.opposite()); + return step2 instanceof EdgeOtherVertexStep || (step2 instanceof EdgeVertexStep && + ((EdgeVertexStep) step2).getDirection().equals(step1Dir.opposite())); } return false; } @@ -129,6 +115,26 @@ public final class IncidentToAdjacentStrategy extends AbstractTraversalStrategy< } @Override + public void apply(Traversal.Admin<?, ?> traversal) { + final Traversal.Admin root = TraversalHelper.getRootTraversal(traversal); + if (TraversalHelper.hasStepOfAssignableClassRecursively(INVALIDATING_STEP_CLASSES, root)) + return; + final Collection<Pair<VertexStep, Step>> stepsToReplace = new ArrayList<>(); + Step prev = null; + for (final Step curr : traversal.getSteps()) { + if (isOptimizable(prev, curr)) { + stepsToReplace.add(Pair.with((VertexStep) prev, curr)); + } + prev = curr; + } + if (!stepsToReplace.isEmpty()) { + for (final Pair<VertexStep, Step> pair : stepsToReplace) { + optimizeSteps(traversal, pair.getValue0(), pair.getValue1()); + } + } + } + + @Override public Set<Class<? extends OptimizationStrategy>> applyPrior() { return Collections.singleton(IdentityRemovalStrategy.class); } http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/52e3b86c/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/IncidentToAdjacentStrategyTest.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/IncidentToAdjacentStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/IncidentToAdjacentStrategyTest.java index 45f583e..4fa79b6 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/IncidentToAdjacentStrategyTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/IncidentToAdjacentStrategyTest.java @@ -47,17 +47,16 @@ public class IncidentToAdjacentStrategyTest { @RunWith(Parameterized.class) public static class StandardTest extends AbstractIncidentToAdjacentStrategyTest { - @Parameterized.Parameters(name = "{0}") - public static Iterable<Object[]> data() { - return generateTestParameters(); - } - @Parameterized.Parameter(value = 0) public Traversal original; - @Parameterized.Parameter(value = 1) public Traversal optimized; + @Parameterized.Parameters(name = "{0}") + public static Iterable<Object[]> data() { + return generateTestParameters(); + } + @Before public void setup() { this.traversalEngine = mock(TraversalEngine.class); @@ -73,17 +72,16 @@ public class IncidentToAdjacentStrategyTest { @RunWith(Parameterized.class) public static class ComputerTest extends AbstractIncidentToAdjacentStrategyTest { - @Parameterized.Parameters(name = "{0}") - public static Iterable<Object[]> data() { - return generateTestParameters(); - } - @Parameterized.Parameter(value = 0) public Traversal original; - @Parameterized.Parameter(value = 1) public Traversal optimized; + @Parameterized.Parameters(name = "{0}") + public static Iterable<Object[]> data() { + return generateTestParameters(); + } + @Before public void setup() { this.traversalEngine = mock(TraversalEngine.class); @@ -100,21 +98,6 @@ public class IncidentToAdjacentStrategyTest { protected TraversalEngine traversalEngine; - void applyIncidentToAdjacentStrategy(final Traversal traversal) { - final TraversalStrategies strategies = new DefaultTraversalStrategies(); - strategies.addStrategies(IncidentToAdjacentStrategy.instance()); - - traversal.asAdmin().setStrategies(strategies); - traversal.asAdmin().setEngine(this.traversalEngine); - traversal.asAdmin().applyStrategies(); - - } - - public void doTest(final Traversal traversal, final Traversal optimized) { - applyIncidentToAdjacentStrategy(traversal); - assertEquals(optimized, traversal); - } - static Iterable<Object[]> generateTestParameters() { Function<Traverser<Vertex>, Vertex> lambda = Traverser::get; // to ensure same hashCode @@ -127,11 +110,30 @@ public class IncidentToAdjacentStrategyTest { {__.bothE().bothV(), __.bothE().bothV()}, {__.bothE().inV(), __.bothE().inV()}, {__.bothE().outV(), __.bothE().outV()}, + {__.outE().otherV(), __.out()}, + {__.inE().otherV(), __.in()}, {__.outE().as("a").inV(), __.outE().as("a").inV()}, // todo: this can be optimized, but requires a lot more checks {__.outE().inV().path(), __.outE().inV().path()}, + {__.outE().inV().simplePath(), __.outE().inV().simplePath()}, + {__.outE().inV().tree(), __.outE().inV().tree()}, {__.outE().inV().map(lambda), __.outE().inV().map(lambda)}, {__.union(__.outE().inV(), __.inE().outV()).path(), __.union(__.outE().inV(), __.inE().outV()).path()}, {__.as("a").outE().inV().as("b"), __.as("a").out().as("b")}}); } + + void applyIncidentToAdjacentStrategy(final Traversal traversal) { + final TraversalStrategies strategies = new DefaultTraversalStrategies(); + strategies.addStrategies(IncidentToAdjacentStrategy.instance()); + + traversal.asAdmin().setStrategies(strategies); + traversal.asAdmin().setEngine(this.traversalEngine); + traversal.asAdmin().applyStrategies(); + + } + + public void doTest(final Traversal traversal, final Traversal optimized) { + applyIncidentToAdjacentStrategy(traversal); + assertEquals(optimized, traversal); + } } }
