tinkerpop-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From spmalle...@apache.org
Subject [14/14] tinkerpop git commit: TINKERPOP-1963 Fixed branch() problems with reducing steps as options
Date Mon, 21 May 2018 14:00:46 GMT
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 <spmva@genoprime.com>
Authored: Wed May 9 17:52:32 2018 -0400
Committer: Stephen Mallette <spmva@genoprime.com>
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<S, E, M> extends ComputerAwareStep<S,
E> 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<S, E, M> extends ComputerAwareStep<S,
E> implements Trav
     protected Iterator<Traverser.Admin<E>> 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<Traversal.Admin<S, E>> options : this.traversalOptions.values())
{
                     for (final Traversal.Admin<S, E> 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<S> start) {
+    /**
+     * Choose the right traversal option to apply and seed those options with this traverser.
+     */
+    private void applyCurrentTraverser(final Traverser.Admin<S> 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<Traversal.Admin<S, E>> branch = this.traversalOptions.containsKey(choice)
? this.traversalOptions.get(choice) : this.traversalOptions.get(Pick.none);
+        final List<Traversal.Admin<S, E>> 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<Traversal.Admin<S, E>> 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<S, E> extends AbstractStep<S, E> implements
Barrier<E>, Generating<E, E> {
 
     protected Supplier<E> 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<Vertex, Object> 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<Vertex, Map<String, String>> 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<Integer, List<Integer>> get_g_injectX1X_chooseXisX1X__constantX10Xfold__foldX()
{
+            new ScriptTraversal<>(g, "gremlin-groovy", "g.inject(1).choose(__.is(1),
__.constant(10).fold(), __.fold())")
+        }
+
+        @Override
+        public Traversal<Integer, List<Integer>> 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<Vertex, Map<String, String>> get_g_V_hasLabelXpersonX_asXp1X_chooseXoutEXknowsX__outXknowsXX_asXp2X_selectXp1_p2X_byXnameX();
 
+    public abstract Traversal<Integer, List<Integer>> get_g_injectX1X_chooseXisX1X__constantX10Xfold__foldX();
+
+    public abstract Traversal<Integer, List<Integer>> 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<Integer, List<Integer>> 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<Integer, List<Integer>> 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<Vertex, Map<String, String>> 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").<String>select("p1",
"p2").by("name");
         }
+
+        @Override
+        public Traversal<Integer, List<Integer>> get_g_injectX1X_chooseXisX1X__constantX10Xfold__foldX()
{
+            return g.inject(1).choose(__.is(1), __.constant(10).fold(), __.fold());
+        }
+
+        @Override
+        public Traversal<Integer, List<Integer>> get_g_injectX2X_chooseXisX1X__constantX10Xfold__foldX()
{
+            return g.inject(2).choose(__.is(1), __.constant(10).fold(), __.fold());
+        }
     }
 }
\ No newline at end of file


Mime
View raw message