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/35f936b0 Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/35f936b0 Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/35f936b0 Branch: refs/heads/TINKERPOP-1545-tp32 Commit: 35f936b0e84d31c0d92600dbe129f54babb1900a Parents: be5b64a Author: Daniel Kuppitz <[email protected]> Authored: Mon Jan 9 15:31:54 2017 +0100 Committer: Daniel Kuppitz <[email protected]> Committed: Mon Jan 9 15:47:27 2017 +0100 ---------------------------------------------------------------------- CHANGELOG.asciidoc | 1 + .../IncidentToAdjacentStrategy.java | 61 +++++++++++--------- .../IncidentToAdjacentStrategyTest.java | 23 ++++---- 3 files changed, 47 insertions(+), 38 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/35f936b0/CHANGELOG.asciidoc ---------------------------------------------------------------------- diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 89dcce4..54eef9d 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -569,6 +569,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/35f936b0/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 efe8618..4270ac3 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 @@ -23,11 +23,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.step.util.EmptyStep; import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper; @@ -68,38 +71,13 @@ 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, TreeStep.class, LambdaHolder.class)); private static final String MARKER = Graph.Hidden.hide("gremlin.incidentToAdjacent"); + 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(final Traversal.Admin<?, ?> traversal) { - // using a hidden label marker to denote whether the traversal should not be processed by this strategy - if ((traversal.getParent() instanceof EmptyStep || traversal.getParent() instanceof VertexProgramStep) && - TraversalHelper.hasStepOfAssignableClassRecursively(INVALIDATING_STEP_CLASSES, traversal)) - TraversalHelper.applyTraversalRecursively(t -> t.getStartStep().addLabel(MARKER), traversal); - if (traversal.getStartStep().getLabels().contains(MARKER)) { - traversal.getStartStep().removeLabel(MARKER); - 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. * @@ -114,7 +92,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; } @@ -141,6 +120,32 @@ public final class IncidentToAdjacentStrategy extends AbstractTraversalStrategy< } @Override + public void apply(final Traversal.Admin<?, ?> traversal) { + // using a hidden label marker to denote whether the traversal should not be processed by this strategy + if ((traversal.getParent() instanceof EmptyStep || traversal.getParent() instanceof VertexProgramStep) && + TraversalHelper.hasStepOfAssignableClassRecursively(INVALIDATING_STEP_CLASSES, traversal)) + TraversalHelper.applyTraversalRecursively(t -> t.getStartStep().addLabel(MARKER), traversal); + if (traversal.getStartStep().getLabels().contains(MARKER)) { + traversal.getStartStep().removeLabel(MARKER); + 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/35f936b0/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 6e56ab8..abf1884 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 @@ -45,15 +45,6 @@ public class IncidentToAdjacentStrategyTest { @Parameterized.Parameter(value = 1) public Traversal optimized; - @Test - public void doTest() { - final TraversalStrategies strategies = new DefaultTraversalStrategies(); - strategies.addStrategies(IncidentToAdjacentStrategy.instance()); - this.original.asAdmin().setStrategies(strategies); - this.original.asAdmin().applyStrategies(); - assertEquals(this.optimized, this.original); - } - @Parameterized.Parameters(name = "{0}") public static Iterable<Object[]> generateTestParameters() { @@ -67,12 +58,24 @@ 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().tree().as("a"), __.outE().inV().tree().as("a")}, + {__.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")}}); } + + @Test + public void doTest() { + final TraversalStrategies strategies = new DefaultTraversalStrategies(); + strategies.addStrategies(IncidentToAdjacentStrategy.instance()); + this.original.asAdmin().setStrategies(strategies); + this.original.asAdmin().applyStrategies(); + assertEquals(this.optimized, this.original); + } }
