Added another optimization in `RangeByIsCountStrategy` (covered by unit tests), that removes `count().is()` altogether if it's not needed.
Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/6bb84563 Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/6bb84563 Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/6bb84563 Branch: refs/heads/TINKERPOP-1490 Commit: 6bb84563c912fc4f2ad5b8b1bcf2094bffed8738 Parents: 56e113d Author: Daniel Kuppitz <[email protected]> Authored: Thu Nov 10 19:06:50 2016 +0100 Committer: Daniel Kuppitz <[email protected]> Committed: Thu Nov 10 19:06:50 2016 +0100 ---------------------------------------------------------------------- CHANGELOG.asciidoc | 1 + .../optimization/RangeByIsCountStrategy.java | 55 +++++++++++--------- .../RangeByIsCountStrategyTest.java | 30 ++++++----- 3 files changed, 48 insertions(+), 38 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/6bb84563/CHANGELOG.asciidoc ---------------------------------------------------------------------- diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 3c44f0f..1f4e4cf 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -26,6 +26,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima TinkerPop 3.2.4 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Added another optimization in `RangeByIsCountStrategy`, that removes `count().is()` altogether if it's not needed. * Fixed a OLAP `MatchStep.clone()`-bug that occurs when the `match()` is in a local child. * Fixed a bug in `RangeByIsCountStrategy` where labeled parents shouldn't have the strategy applied to their children. * Fixed a bug in `PathRetractionStrategy` where `MatchEndStep` labels were being dropped when they shouldn't be. http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/6bb84563/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RangeByIsCountStrategy.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RangeByIsCountStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RangeByIsCountStrategy.java index b29ce47..efe7685 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RangeByIsCountStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RangeByIsCountStrategy.java @@ -76,6 +76,10 @@ public final class RangeByIsCountStrategy extends AbstractTraversalStrategy<Trav private RangeByIsCountStrategy() { } + public static RangeByIsCountStrategy instance() { + return INSTANCE; + } + @Override public void apply(final Traversal.Admin<?, ?> traversal) { final TraversalParent parent = traversal.getParent(); @@ -87,7 +91,7 @@ public final class RangeByIsCountStrategy extends AbstractTraversalStrategy<Trav final IsStep isStep = (IsStep) traversal.getSteps().get(i + 1); final P isStepPredicate = isStep.getPredicate(); Long highRange = null; - boolean useNotStep = false; + boolean useNotStep = false, dismissCountIs = false; for (P p : isStepPredicate instanceof ConnectiveP ? ((ConnectiveP<?>) isStepPredicate).getPredicates() : Collections.singletonList(isStepPredicate)) { final Object value = p.getValue(); final BiPredicate predicate = p.getBiPredicate(); @@ -101,10 +105,10 @@ public final class RangeByIsCountStrategy extends AbstractTraversalStrategy<Trav } else { if (parent instanceof RepeatStep) { final RepeatStep repeatStep = (RepeatStep) parent; - useNotStep = Objects.equals(traversal, repeatStep.getUntilTraversal()) + dismissCountIs = useNotStep = Objects.equals(traversal, repeatStep.getUntilTraversal()) || Objects.equals(traversal, repeatStep.getEmitTraversal()); } else { - useNotStep = parent instanceof FilterStep || parent instanceof SideEffectStep; + dismissCountIs = useNotStep = parent instanceof FilterStep || parent instanceof SideEffectStep; } } highRange = highRangeCandidate; @@ -112,6 +116,9 @@ public final class RangeByIsCountStrategy extends AbstractTraversalStrategy<Trav && isStep.getNextStep() instanceof EmptyStep && ((highRange <= 1L && predicate.equals(Compare.lt)) || (highRange == 1L && (predicate.equals(Compare.eq) || predicate.equals(Compare.lte)))); + dismissCountIs &= curr.getLabels().isEmpty() && isStep.getLabels().isEmpty() + && isStep.getNextStep() instanceof EmptyStep + && (highRange == 1L && (predicate.equals(Compare.gt) || predicate.equals(Compare.gte))); } } else { final Long highRangeOffset = RANGE_PREDICATES.get(predicate); @@ -126,29 +133,31 @@ public final class RangeByIsCountStrategy extends AbstractTraversalStrategy<Trav } } if (highRange != null) { - if (useNotStep) { + if (useNotStep || dismissCountIs) { traversal.asAdmin().removeStep(isStep); // IsStep traversal.asAdmin().removeStep(curr); // CountStep size -= 2; - final Traversal.Admin inner; - if (prev != null) { - inner = __.start().asAdmin(); - for (; ; ) { - final Step pp = prev.getPreviousStep(); - inner.addStep(0, prev); - if (pp instanceof EmptyStep || pp instanceof GraphStep || - !(prev instanceof FilterStep || prev instanceof SideEffectStep)) break; - traversal.removeStep(prev); - prev = pp; - size--; + if (!dismissCountIs) { + final Traversal.Admin inner; + if (prev != null) { + inner = __.start().asAdmin(); + for (; ; ) { + final Step pp = prev.getPreviousStep(); + inner.addStep(0, prev); + if (pp instanceof EmptyStep || pp instanceof GraphStep || + !(prev instanceof FilterStep || prev instanceof SideEffectStep)) break; + traversal.removeStep(prev); + prev = pp; + size--; + } + } else { + inner = __.identity().asAdmin(); } - } else { - inner = __.identity().asAdmin(); + if (prev != null) + TraversalHelper.replaceStep(prev, new NotStep<>(traversal, inner), traversal); + else + traversal.asAdmin().addStep(new NotStep<>(traversal, inner)); } - if (prev != null) - TraversalHelper.replaceStep(prev, new NotStep<>(traversal, inner), traversal); - else - traversal.asAdmin().addStep(new NotStep<>(traversal, inner)); } else { TraversalHelper.insertBeforeStep(new RangeGlobalStep<>(traversal, 0L, highRange), curr, traversal); } @@ -170,8 +179,4 @@ public final class RangeByIsCountStrategy extends AbstractTraversalStrategy<Trav !(parent.getNextStep() instanceof MatchStep.MatchEndStep && // if this is in a pattern match, then don't do it. ((MatchStep.MatchEndStep) parent.getNextStep()).getMatchKey().isPresent()); } - - public static RangeByIsCountStrategy instance() { - return INSTANCE; - } } http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/6bb84563/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RangeByIsCountStrategyTest.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RangeByIsCountStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RangeByIsCountStrategyTest.java index 51ad112..55eb840 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RangeByIsCountStrategyTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RangeByIsCountStrategyTest.java @@ -52,19 +52,6 @@ public class RangeByIsCountStrategyTest { @Parameterized.Parameter(value = 1) public Traversal optimized; - void applyRangeByIsCountStrategy(final Traversal traversal) { - final TraversalStrategies strategies = new DefaultTraversalStrategies(); - strategies.addStrategies(RangeByIsCountStrategy.instance()); - traversal.asAdmin().setStrategies(strategies); - traversal.asAdmin().applyStrategies(); - } - - @Test - public void doTest() { - applyRangeByIsCountStrategy(original); - assertEquals(optimized, original); - } - @Parameterized.Parameters(name = "{0}") public static Iterable<Object[]> generateTestParameters() { @@ -97,6 +84,23 @@ public class RangeByIsCountStrategyTest { {__.repeat(__.out()).emit(__.outE().count().is(0)), __.repeat(__.out()).emit(__.not(__.outE()))}, {__.where(__.outE().hasLabel("created").count().is(0)), __.where(__.not(__.outE().hasLabel("created")))}, {__.where(__.out().outE().hasLabel("created").count().is(0)), __.where(__.out().not(__.outE().hasLabel("created")))}, + {__.filter(__.bothE().count().is(gt(0))), __.filter(__.bothE())}, + {__.filter(__.bothE().count().is(gte(1))), __.filter(__.bothE())}, + {__.filter(__.bothE().count().is(gt(1))), __.filter(__.bothE().limit(2).count().is(gt(1)))}, + {__.filter(__.bothE().count().is(gte(2))), __.filter(__.bothE().limit(2).count().is(gte(2)))}, }); } + + void applyRangeByIsCountStrategy(final Traversal traversal) { + final TraversalStrategies strategies = new DefaultTraversalStrategies(); + strategies.addStrategies(RangeByIsCountStrategy.instance()); + traversal.asAdmin().setStrategies(strategies); + traversal.asAdmin().applyStrategies(); + } + + @Test + public void doTest() { + applyRangeByIsCountStrategy(original); + assertEquals(optimized, original); + } }
