found a fixed a bug in RepeatUnrollStragegy. Global stateful steps like DedupGlobalStep can not be unrolled else you have split the global state amongst times(x) local steps. Added a test case to DedupGlobalStep g.V().repeat(dedup()).times(2).
Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/3fd74fc2 Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/3fd74fc2 Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/3fd74fc2 Branch: refs/heads/tp32 Commit: 3fd74fc23cb2f1e25d555be01abd4d979967f239 Parents: dc38b43 Author: Marko A. Rodriguez <[email protected]> Authored: Fri Jan 6 13:02:31 2017 -0700 Committer: Marko A. Rodriguez <[email protected]> Committed: Fri Jan 6 13:02:31 2017 -0700 ---------------------------------------------------------------------- CHANGELOG.asciidoc | 1 + .../traversal/step/branch/RepeatStep.java | 17 ++++++++++++++++- .../traversal/step/filter/DedupGlobalStep.java | 3 +-- .../optimization/RepeatUnrollStrategy.java | 6 +++++- .../step/filter/GroovyDedupTest.groovy | 5 +++++ .../traversal/step/filter/DedupTest.java | 20 +++++++++++++++++++- 6 files changed, 47 insertions(+), 5 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/3fd74fc2/CHANGELOG.asciidoc ---------------------------------------------------------------------- diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index f8c9743..d7f4256 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) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Fixed a bug in `RepeatUnrollStrategy` where stateful `DedupGlobalStep` was cloned and thus, maintained two deduplication sets. * Added documentation around "terminal steps" in Gremlin: `hasNext()`, `next()`, `toList()`, etc. * Fixed minor bug in `gremlin-driver` where closing a session-based `Client` without initializing it could generate an error. * Relieved synchronization pressure in various areas of `TinkerGraphComputer`. http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/3fd74fc2/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatStep.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatStep.java index ebdb657..0fc2cdd 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatStep.java @@ -90,12 +90,16 @@ public final class RepeatStep<S> extends ComputerAwareStep<S, S> implements Trav return this.emitTraversal; } + public Traversal.Admin<S, S> getRepeatTraversal() { + return this.repeatTraversal; + } + public List<Traversal.Admin<S, S>> getGlobalChildren() { return null == this.repeatTraversal ? Collections.emptyList() : Collections.singletonList(this.repeatTraversal); } public List<Traversal.Admin<S, ?>> getLocalChildren() { - final List<Traversal.Admin<S, ?>> list = new ArrayList<>(); + final List<Traversal.Admin<S, ?>> list = new ArrayList<>(2); if (null != this.untilTraversal) list.add(this.untilTraversal); if (null != this.emitTraversal) @@ -123,6 +127,17 @@ public final class RepeatStep<S> extends ComputerAwareStep<S, S> implements Trav return StringFactory.stepString(this, this.repeatTraversal, untilString(), emitString()); } + @Override + public void reset() { + super.reset(); + if (null != this.emitTraversal) + this.emitTraversal.reset(); + if (null != this.untilTraversal) + this.untilTraversal.reset(); + if (null != this.repeatTraversal) + this.repeatTraversal.reset(); + } + private final String untilString() { return null == this.untilTraversal ? "until(false)" : "until(" + this.untilTraversal + ')'; } http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/3fd74fc2/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupGlobalStep.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupGlobalStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupGlobalStep.java index d024456..96bd0be 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupGlobalStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupGlobalStep.java @@ -77,7 +77,6 @@ public final class DedupGlobalStep<S> extends FilterStep<S> implements Traversal this.dedupLabels.forEach(label -> objects.add(TraversalUtil.applyNullable((S) this.getScopeValue(Pop.last, label, traverser), this.dedupTraversal))); return this.duplicateSet.add(objects); } - } @Override @@ -128,7 +127,7 @@ public final class DedupGlobalStep<S> extends FilterStep<S> implements Traversal @Override public void setTraversal(final Traversal.Admin<?, ?> parentTraversal) { super.setTraversal(parentTraversal); - integrateChild(this.dedupTraversal); + this.integrateChild(this.dedupTraversal); } @Override http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/3fd74fc2/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RepeatUnrollStrategy.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RepeatUnrollStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RepeatUnrollStrategy.java index 31eb0d2..0a7cd4e 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RepeatUnrollStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RepeatUnrollStrategy.java @@ -19,11 +19,13 @@ package org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization; +import org.apache.tinkerpop.gremlin.process.traversal.Scope; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.lambda.LoopTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.Barrier; import org.apache.tinkerpop.gremlin.process.traversal.step.branch.RepeatStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.DedupGlobalStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.NoOpBarrierStep; import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper; @@ -48,7 +50,9 @@ public final class RepeatUnrollStrategy extends AbstractTraversalStrategy<Traver for (int i = 0; i < traversal.getSteps().size(); i++) { if (traversal.getSteps().get(i) instanceof RepeatStep) { final RepeatStep<?> repeatStep = (RepeatStep) traversal.getSteps().get(i); - if (null == repeatStep.getEmitTraversal() && repeatStep.getUntilTraversal() instanceof LoopTraversal && ((LoopTraversal) repeatStep.getUntilTraversal()).getMaxLoops() > 0) { + if (null == repeatStep.getEmitTraversal() && + repeatStep.getUntilTraversal() instanceof LoopTraversal && ((LoopTraversal) repeatStep.getUntilTraversal()).getMaxLoops() > 0 && + !TraversalHelper.hasStepOfAssignableClassRecursively(Scope.global, DedupGlobalStep.class, repeatStep.getRepeatTraversal())) { final Traversal.Admin<?, ?> repeatTraversal = repeatStep.getGlobalChildren().get(0); repeatTraversal.removeStep(repeatTraversal.getSteps().size() - 1); // removes the RepeatEndStep TraversalHelper.applySingleLevelStrategies(traversal, repeatTraversal, RepeatUnrollStrategy.class); http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/3fd74fc2/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/filter/GroovyDedupTest.groovy ---------------------------------------------------------------------- diff --git a/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/filter/GroovyDedupTest.groovy b/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/filter/GroovyDedupTest.groovy index 83428c2..8f5e928 100644 --- a/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/filter/GroovyDedupTest.groovy +++ b/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/filter/GroovyDedupTest.groovy @@ -104,5 +104,10 @@ public abstract class GroovyDedupTest { public Traversal<Vertex, Collection<Vertex>> get_g_V_asXaX_repeatXbothX_timesX3X_emit_asXbX_group_byXselectXaXX_byXselectXbX_dedup_order_byXidX_foldX_selectXvaluesX_unfold_dedup() { new ScriptTraversal<>(g, "gremlin-groovy", "g.V.as('a').repeat(both()).times(3).emit.as('b').group.by(select('a')).by(select('b').dedup.order.by(id).fold).select(values).unfold.dedup") } + + @Override + public Traversal<Vertex, Long> get_g_V_repeatXdedupX_timesX2X_count() { + new ScriptTraversal<>(g, "gremlin-groovy", "g.V.repeat(dedup()).times(2).count") + } } } http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/3fd74fc2/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupTest.java ---------------------------------------------------------------------- diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupTest.java index 19e685a..183a3a9 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupTest.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupTest.java @@ -42,6 +42,7 @@ import java.util.Set; import static org.apache.tinkerpop.gremlin.LoadGraphWith.GraphData.MODERN; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.both; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.bothE; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.dedup; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.select; import static org.apache.tinkerpop.gremlin.structure.Column.values; import static org.junit.Assert.assertEquals; @@ -87,6 +88,8 @@ public abstract class DedupTest extends AbstractGremlinProcessTest { public abstract Traversal<Vertex, Collection<Vertex>> get_g_V_asXaX_repeatXbothX_timesX3X_emit_asXbX_group_byXselectXaXX_byXselectXbX_dedup_order_byXidX_foldX_selectXvaluesX_unfold_dedup(); + public abstract Traversal<Vertex, Long> get_g_V_repeatXdedupX_timesX2X_count(); + @Test @LoadGraphWith(MODERN) public void g_V_out_in_valuesXnameX_fold_dedupXlocalX_unfold() { @@ -284,7 +287,7 @@ public abstract class DedupTest extends AbstractGremlinProcessTest { } @Test - @LoadGraphWith + @LoadGraphWith(MODERN) public void g_V_asXaX_repeatXbothX_timesX3X_emit_asXbX_group_byXselectXaXX_byXselectXbX_dedup_order_byXidX_foldX_selectXvaluesX_unfold_dedup() { final Traversal<Vertex, Collection<Vertex>> traversal = get_g_V_asXaX_repeatXbothX_timesX3X_emit_asXbX_group_byXselectXaXX_byXselectXbX_dedup_order_byXidX_foldX_selectXvaluesX_unfold_dedup(); printTraversalForm(traversal); @@ -299,6 +302,16 @@ public abstract class DedupTest extends AbstractGremlinProcessTest { assertTrue(vertices.contains(convertToVertex(graph, "ripple"))); } + @Test + @LoadGraphWith(MODERN) + public void g_V_repeatXdedupX_timesX2X_count() { + final Traversal<Vertex, Long> traversal = get_g_V_repeatXdedupX_timesX2X_count(); + printTraversalForm(traversal); + assertEquals(0L, traversal.next().longValue()); + assertFalse(traversal.hasNext()); + } + + public static class Traversals extends DedupTest { @Override public Traversal<Vertex, String> get_g_V_out_in_valuesXnameX_fold_dedupXlocalX_unfold() { @@ -374,5 +387,10 @@ public abstract class DedupTest extends AbstractGremlinProcessTest { public Traversal<Vertex, Collection<Vertex>> get_g_V_asXaX_repeatXbothX_timesX3X_emit_asXbX_group_byXselectXaXX_byXselectXbX_dedup_order_byXidX_foldX_selectXvaluesX_unfold_dedup() { return g.V().as("a").repeat(both()).times(3).emit().as("b").group().by(select("a")).by(select("b").dedup().order().by(T.id).fold()).select(values).<Collection<Vertex>>unfold().dedup(); } + + @Override + public Traversal<Vertex, Long> get_g_V_repeatXdedupX_timesX2X_count() { + return g.V().repeat(dedup()).times(2).count(); + } } }
