From commits-return-29269-archive-asf-public=cust-asf.ponee.io@tinkerpop.apache.org Mon May 21 16:00:38 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 6DD6118077B for ; Mon, 21 May 2018 16:00:37 +0200 (CEST) Received: (qmail 7686 invoked by uid 500); 21 May 2018 14:00:36 -0000 Mailing-List: contact commits-help@tinkerpop.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@tinkerpop.apache.org Delivered-To: mailing list commits@tinkerpop.apache.org Received: (qmail 7433 invoked by uid 99); 21 May 2018 14:00:34 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 21 May 2018 14:00:34 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 2EF02E1118; Mon, 21 May 2018 14:00:34 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: spmallette@apache.org To: commits@tinkerpop.apache.org Date: Mon, 21 May 2018 14:00:46 -0000 Message-Id: <4335021c39604bad8134737d2e2a50cc@git.apache.org> In-Reply-To: <207ded4ec8bb45ddac930c0642c5534d@git.apache.org> References: <207ded4ec8bb45ddac930c0642c5534d@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: [14/14] tinkerpop git commit: TINKERPOP-1963 Fixed branch() problems with reducing steps as options TINKERPOP-1963 Fixed branch() problems with reducing steps as options Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/f4a7097b Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/f4a7097b Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/f4a7097b Branch: refs/heads/TINKERPOP-1963 Commit: f4a7097b7d8c0d0227de9a5950a1568ec561e5c8 Parents: 7c69607 Author: Stephen Mallette Authored: Wed May 9 17:52:32 2018 -0400 Committer: Stephen Mallette Committed: Mon May 21 10:00:18 2018 -0400 ---------------------------------------------------------------------- CHANGELOG.asciidoc | 1 + .../traversal/step/branch/BranchStep.java | 42 +++++++++++++++++--- .../step/util/ReducingBarrierStep.java | 1 - .../step/branch/GroovyChooseTest.groovy | 11 +++++ gremlin-test/features/branch/Choose.feature | 27 +++++++++++++ .../traversal/step/branch/ChooseTest.java | 35 ++++++++++++++++ 6 files changed, 110 insertions(+), 7 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/f4a7097b/CHANGELOG.asciidoc ---------------------------------------------------------------------- diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index b286dcb..0008a1a 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -23,6 +23,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima [[release-3-2-10]] === TinkerPop 3.2.10 (Release Date: NOT OFFICIALLY RELEASED YET) +* Fixed bug in `branch()` where reducing steps as options would produce incorrect results. * Removed recursive handling of streaming results from Gremlin-Python driver to avoid max recursion depth errors. * Fixed bug in `GroovyTranslator` that didn't properly handle empty `Map` objects. http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/f4a7097b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/BranchStep.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/BranchStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/BranchStep.java index 778722d..0b1a059 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/BranchStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/BranchStep.java @@ -22,7 +22,9 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; import org.apache.tinkerpop.gremlin.process.traversal.step.Barrier; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalOptionParent; +import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.IdentityStep; import org.apache.tinkerpop.gremlin.process.traversal.step.util.ComputerAwareStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.util.ReducingBarrierStep; import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; import org.apache.tinkerpop.gremlin.process.traversal.util.FastNoSuchElementException; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper; @@ -62,7 +64,13 @@ public class BranchStep extends ComputerAwareStep implements Trav this.traversalOptions.get(pickToken).add(traversalOption); else this.traversalOptions.put(pickToken, new ArrayList<>(Collections.singletonList(traversalOption))); + + // adding an IdentityStep acts as a placeholder when reducing barriers get in the way - see the + // standardAlgorithm() method for more information. + if (TraversalHelper.hasStepOfAssignableClass(ReducingBarrierStep.class, traversalOption)) + traversalOption.addStep(0, new IdentityStep(traversalOption)); traversalOption.addStep(new EndStep(traversalOption)); + if (!this.hasBarrier && !TraversalHelper.getStepsOfAssignableClassRecursively(Barrier.class, traversalOption).isEmpty()) this.hasBarrier = true; this.integrateChild(traversalOption); @@ -89,32 +97,54 @@ public class BranchStep extends ComputerAwareStep implements Trav protected Iterator> standardAlgorithm() { while (true) { if (!this.first) { + // this block is ignored on the first pass through the while(true) giving the opportunity for + // the traversalOptions to be prepared. Iterate all of them and simply return the ones that yield + // results. applyCurrentTraverser() will have only injected the current traverser into the options + // that met the choice requirements. Note that in addGlobalChildOption an IdentityStep was added to + // be a holder for that current traverser. That allows us to check that first step for an injected + // traverser as part of the condition for using that traversal option in the output. This is necessary + // because barriers like fold(), max(), etc. will always return true for hasNext() even if a traverser + // was not seeded in applyCurrentTraverser(). for (final List> options : this.traversalOptions.values()) { for (final Traversal.Admin option : options) { - if (option.hasNext()) + if (option.getStartStep().hasNext() && option.hasNext()) return option.getEndStep(); } } } + this.first = false; - /// + + // pass the current traverser to applyCurrentTraverser() which will make the "choice" of traversal to + // apply with the given traverser. as this is in a while(true) this phase essentially prepares the options + // for execution above if (this.hasBarrier) { if (!this.starts.hasNext()) throw FastNoSuchElementException.instance(); while (this.starts.hasNext()) { - this.handleStart(this.starts.next()); + this.applyCurrentTraverser(this.starts.next()); } } else { - this.handleStart(this.starts.next()); + this.applyCurrentTraverser(this.starts.next()); } } } - private final void handleStart(final Traverser.Admin start) { + /** + * Choose the right traversal option to apply and seed those options with this traverser. + */ + private void applyCurrentTraverser(final Traverser.Admin start) { + // first get the value of the choice based on the current traverser and use that to select the right traversal + // option to which that traverser should be routed final M choice = TraversalUtil.apply(start, this.branchTraversal); - final List> branch = this.traversalOptions.containsKey(choice) ? this.traversalOptions.get(choice) : this.traversalOptions.get(Pick.none); + final List> branch = this.traversalOptions.containsKey(choice) ? + this.traversalOptions.get(choice) : this.traversalOptions.get(Pick.none); + + // if a branch is identified, then split the traverser and add it to the start of the option so that when + // that option is iterated (in the calling method) that value can be applied. if (null != branch) branch.forEach(traversal -> traversal.addStart(start.split())); + if (choice != Pick.any) { final List> anyBranch = this.traversalOptions.get(Pick.any); if (null != anyBranch) http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/f4a7097b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ReducingBarrierStep.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ReducingBarrierStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ReducingBarrierStep.java index 3b2823c..62e12f8 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ReducingBarrierStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ReducingBarrierStep.java @@ -32,7 +32,6 @@ import java.util.function.Supplier; /** * @author Marko A. Rodriguez (http://markorodriguez.com) */ - public abstract class ReducingBarrierStep extends AbstractStep implements Barrier, Generating { protected Supplier seedSupplier; http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/f4a7097b/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/branch/GroovyChooseTest.groovy ---------------------------------------------------------------------- diff --git a/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/branch/GroovyChooseTest.groovy b/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/branch/GroovyChooseTest.groovy index 802c427..b3e9955 100644 --- a/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/branch/GroovyChooseTest.groovy +++ b/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/branch/GroovyChooseTest.groovy @@ -29,6 +29,7 @@ import org.apache.tinkerpop.gremlin.structure.Vertex public abstract class GroovyChooseTest { public static class Traversals extends ChooseTest { + @Override public Traversal get_g_V_chooseXout_countX_optionX2L__nameX_optionX3L__valueMapX() { new ScriptTraversal<>(g, "gremlin-groovy", "g.V.choose(__.out.count).option(2L, __.values('name')).option(3L, __.valueMap())") @@ -63,5 +64,15 @@ public abstract class GroovyChooseTest { public Traversal> get_g_V_hasLabelXpersonX_asXp1X_chooseXoutEXknowsX__outXknowsXX_asXp2X_selectXp1_p2X_byXnameX() { new ScriptTraversal<>(g, "gremlin-groovy", "g.V.hasLabel('person').as('p1').choose(outE('knows'), out('knows')).as('p2').select('p1', 'p2').by('name')"); } + + @Override + public Traversal> get_g_injectX1X_chooseXisX1X__constantX10Xfold__foldX() { + new ScriptTraversal<>(g, "gremlin-groovy", "g.inject(1).choose(__.is(1), __.constant(10).fold(), __.fold())") + } + + @Override + public Traversal> get_g_injectX2X_chooseXisX1X__constantX10Xfold__foldX() { + new ScriptTraversal<>(g, "gremlin-groovy", "g.inject(2).choose(__.is(1), __.constant(10).fold(), __.fold())") + } } } http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/f4a7097b/gremlin-test/features/branch/Choose.feature ---------------------------------------------------------------------- diff --git a/gremlin-test/features/branch/Choose.feature b/gremlin-test/features/branch/Choose.feature index aa5a545..225e40c 100644 --- a/gremlin-test/features/branch/Choose.feature +++ b/gremlin-test/features/branch/Choose.feature @@ -122,3 +122,30 @@ Feature: Step - choose() | m[{"p1":"vadas", "p2":"vadas"}] | | m[{"p1":"josh", "p2":"josh"}] | | m[{"p1":"peter", "p2":"peter"}] | + + Scenario: g_injectX1X_chooseXisX1X__constantX10Xfold__foldX + Given the empty graph + And using the parameter d10 defined as "d[10].i" + And using the parameter d1 defined as "d[1].i" + And the traversal of + """ + g.inject(d1).choose(__.is(d1), __.constant(d10).fold(), __.fold()) + """ + When iterated to list + Then the result should be unordered + | result | + | l[d[10].i] | + + Scenario: g_injectX2X_chooseXisX1X__constantX10Xfold__foldX + Given the empty graph + And using the parameter d10 defined as "d[10].i" + And using the parameter d1 defined as "d[1].i" + And using the parameter d2 defined as "d[2].i" + And the traversal of + """ + g.inject(d2).choose(__.is(d1), __.constant(d10).fold(), __.fold()) + """ + When iterated to list + Then the result should be unordered + | result | + | l[d[2].i] | http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/f4a7097b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/ChooseTest.java ---------------------------------------------------------------------- diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/ChooseTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/ChooseTest.java index e59d659..6da38a7 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/ChooseTest.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/ChooseTest.java @@ -23,6 +23,7 @@ import org.apache.tinkerpop.gremlin.process.AbstractGremlinProcessTest; import org.apache.tinkerpop.gremlin.process.GremlinProcessRunner; import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalOptionParent; import org.apache.tinkerpop.gremlin.process.traversal.step.util.MapHelper; import org.apache.tinkerpop.gremlin.structure.Vertex; @@ -30,7 +31,9 @@ import org.junit.Test; import org.junit.runner.RunWith; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import static org.apache.tinkerpop.gremlin.LoadGraphWith.GraphData.MODERN; @@ -42,6 +45,8 @@ import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.out; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.outE; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.valueMap; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.values; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.core.Is.is; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -64,6 +69,10 @@ public abstract class ChooseTest extends AbstractGremlinProcessTest { public abstract Traversal> get_g_V_hasLabelXpersonX_asXp1X_chooseXoutEXknowsX__outXknowsXX_asXp2X_selectXp1_p2X_byXnameX(); + public abstract Traversal> get_g_injectX1X_chooseXisX1X__constantX10Xfold__foldX(); + + public abstract Traversal> get_g_injectX2X_chooseXisX1X__constantX10Xfold__foldX(); + @Test @LoadGraphWith(MODERN) public void g_V_chooseXout_countX_optionX2L__nameX_optionX3L__valueMapX() { @@ -128,6 +137,22 @@ public abstract class ChooseTest extends AbstractGremlinProcessTest { ), traversal); } + @Test + public void g_injectX1X_chooseXisX1X__constantX10Xfold__foldX() { + final Traversal> traversal = get_g_injectX1X_chooseXisX1X__constantX10Xfold__foldX(); + printTraversalForm(traversal); + assertEquals(Collections.singletonList(10), traversal.next()); + assertThat(traversal.hasNext(), is(false)); + } + + @Test + public void g_injectX2X_chooseXisX1X__constantX10Xfold__foldX() { + final Traversal> traversal = get_g_injectX2X_chooseXisX1X__constantX10Xfold__foldX(); + printTraversalForm(traversal); + assertEquals(Collections.singletonList(2), traversal.next()); + assertThat(traversal.hasNext(), is(false)); + } + public static class Traversals extends ChooseTest { @Override @@ -164,5 +189,15 @@ public abstract class ChooseTest extends AbstractGremlinProcessTest { public Traversal> get_g_V_hasLabelXpersonX_asXp1X_chooseXoutEXknowsX__outXknowsXX_asXp2X_selectXp1_p2X_byXnameX() { return g.V().hasLabel("person").as("p1").choose(outE("knows"), out("knows")).as("p2").select("p1", "p2").by("name"); } + + @Override + public Traversal> get_g_injectX1X_chooseXisX1X__constantX10Xfold__foldX() { + return g.inject(1).choose(__.is(1), __.constant(10).fold(), __.fold()); + } + + @Override + public Traversal> get_g_injectX2X_chooseXisX1X__constantX10Xfold__foldX() { + return g.inject(2).choose(__.is(1), __.constant(10).fold(), __.fold()); + } } } \ No newline at end of file