This is an automated email from the ASF dual-hosted git repository.

dkuppitz pushed a commit to branch TINKERPOP-2095
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git

commit c3b65be205964c41acc7d016b2557e6efa188837
Author: Daniel Kuppitz <daniel_kupp...@hotmail.com>
AuthorDate: Fri Nov 16 10:32:02 2018 -0700

    TINKERPOP-2095 GroupStep looks for irrelevant barrier steps
---
 CHANGELOG.asciidoc                                 |  1 +
 .../process/traversal/step/map/GroupStep.java      | 26 ++++++++++++++++------
 .../process/traversal/util/TraversalHelper.java    |  5 +++++
 gremlin-test/features/sideEffect/Group.feature     | 11 +++++++++
 .../traversal/step/sideEffect/GroupTest.java       | 21 +++++++++++++++++
 5 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index 99313e0..5781bcc 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -25,6 +25,7 @@ 
image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
 
 This release also includes changes from <<release-3-2-11, 3.2.11>>.
 
+* Fixed a bug in `GroupStep` that assigned wrong reducing bi-operators
 * Added `:bytecode` command to help developers debugging `Bytecode`-based 
traversals.
 * Fixed `PersistedOutputRDD` to eager persist RDD by adding `count()` action 
calls.
 * Deserialized `g:Set` to a Python `Set` in GraphSON in `gremlin-python`.
diff --git 
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupStep.java
 
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupStep.java
index 127d562..52b155e 100644
--- 
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupStep.java
+++ 
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupStep.java
@@ -20,6 +20,7 @@
 package org.apache.tinkerpop.gremlin.process.traversal.step.map;
 
 import org.apache.tinkerpop.gremlin.process.traversal.Operator;
+import org.apache.tinkerpop.gremlin.process.traversal.Step;
 import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
 import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
 import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__;
@@ -28,12 +29,12 @@ import 
org.apache.tinkerpop.gremlin.process.traversal.lambda.ElementValueTravers
 import org.apache.tinkerpop.gremlin.process.traversal.lambda.FunctionTraverser;
 import org.apache.tinkerpop.gremlin.process.traversal.lambda.IdentityTraversal;
 import org.apache.tinkerpop.gremlin.process.traversal.lambda.TokenTraversal;
-import org.apache.tinkerpop.gremlin.process.traversal.step.Barrier;
 import org.apache.tinkerpop.gremlin.process.traversal.step.ByModulating;
+import org.apache.tinkerpop.gremlin.process.traversal.step.LocalBarrier;
 import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent;
+import org.apache.tinkerpop.gremlin.process.traversal.step.Barrier;
 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.TraversalHelper;
 import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
 import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
 import org.apache.tinkerpop.gremlin.util.function.HashMapSupplier;
@@ -43,6 +44,7 @@ import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.function.BinaryOperator;
 
@@ -59,11 +61,20 @@ public final class GroupStep<S, K, V> extends 
ReducingBarrierStep<S, Map<K, V>>
     public GroupStep(final Traversal.Admin traversal) {
         super(traversal);
         this.valueTraversal = this.integrateChild(__.fold().asAdmin());
-        this.barrierStep = 
TraversalHelper.getFirstStepOfAssignableClass(Barrier.class, 
this.valueTraversal).orElse(null);
+        this.barrierStep = determineBarrierStep(this.valueTraversal);
         this.setReducingBiOperator(new GroupBiOperator<>(null == 
this.barrierStep ? Operator.assign : 
this.barrierStep.getMemoryComputeKey().getReducer()));
         this.setSeedSupplier(HashMapSupplier.instance());
     }
 
+    private static <S, V> Barrier determineBarrierStep(final 
Traversal.Admin<S, V> traversal) {
+        for (final Step step : traversal.getSteps()) {
+            if (step instanceof Barrier && !(step instanceof LocalBarrier)) {
+                return (Barrier) step;
+            }
+        }
+        return null;
+    }
+
     @Override
     public void modulateBy(final Traversal.Admin<?, ?> kvTraversal) {
         if ('k' == this.state) {
@@ -71,7 +82,7 @@ public final class GroupStep<S, K, V> extends 
ReducingBarrierStep<S, Map<K, V>>
             this.state = 'v';
         } else if ('v' == this.state) {
             this.valueTraversal = 
this.integrateChild(convertValueTraversal(kvTraversal));
-            this.barrierStep = 
TraversalHelper.getFirstStepOfAssignableClass(Barrier.class, 
this.valueTraversal).orElse(null);
+            this.barrierStep = determineBarrierStep(this.valueTraversal);
             this.setReducingBiOperator(new GroupBiOperator<>(null == 
this.barrierStep ? Operator.assign : 
this.barrierStep.getMemoryComputeKey().getReducer()));
             this.state = 'x';
         } else {
@@ -117,7 +128,7 @@ public final class GroupStep<S, K, V> extends 
ReducingBarrierStep<S, Map<K, V>>
         if (null != this.keyTraversal)
             clone.keyTraversal = this.keyTraversal.clone();
         clone.valueTraversal = this.valueTraversal.clone();
-        clone.barrierStep = 
TraversalHelper.getFirstStepOfAssignableClass(Barrier.class, 
clone.valueTraversal).orElse(null);
+        clone.barrierStep = determineBarrierStep(clone.valueTraversal);
         return clone;
     }
 
@@ -185,14 +196,15 @@ public final class GroupStep<S, K, V> extends 
ReducingBarrierStep<S, Map<K, V>>
     }
 
     public static <K, V> Map<K, V> doFinalReduction(final Map<K, Object> map, 
final Traversal.Admin<?, V> valueTraversal) {
-        TraversalHelper.getFirstStepOfAssignableClass(Barrier.class, 
valueTraversal).ifPresent(barrierStep -> {
+        final Barrier barrierStep = determineBarrierStep(valueTraversal);
+        if (barrierStep != null) {
             for (final K key : map.keySet()) {
                 valueTraversal.reset();
                 barrierStep.addBarrier(map.get(key));
                 if (valueTraversal.hasNext())
                     map.put(key, valueTraversal.next());
             }
-        });
+        }
         return (Map<K, V>) map;
     }
 }
\ No newline at end of file
diff --git 
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java
 
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java
index 03ae9bf..21319f5 100644
--- 
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java
+++ 
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java
@@ -40,6 +40,8 @@ import 
org.apache.tinkerpop.gremlin.process.traversal.step.map.LabelStep;
 import org.apache.tinkerpop.gremlin.process.traversal.step.map.MatchStep;
 import org.apache.tinkerpop.gremlin.process.traversal.step.map.PropertiesStep;
 import org.apache.tinkerpop.gremlin.process.traversal.step.map.PropertyMapStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.map.SelectOneStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.map.SelectStep;
 import org.apache.tinkerpop.gremlin.process.traversal.step.map.VertexStep;
 import 
org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.StartStep;
 import org.apache.tinkerpop.gremlin.process.traversal.step.util.BulkSet;
@@ -127,6 +129,9 @@ public final class TraversalHelper {
                         state = 'e';
                 }
                 states.clear();
+                if (step instanceof SelectStep || step instanceof 
SelectOneStep) {
+                    states.add('u');
+                }
                 for (final Traversal.Admin<?, ?> local : ((TraversalParent) 
step).getGlobalChildren()) {
                     final char s = isLocalStarGraph(local, currState);
                     if ('x' == s) return 'x';
diff --git a/gremlin-test/features/sideEffect/Group.feature 
b/gremlin-test/features/sideEffect/Group.feature
index cd2a5ce..f9f54a4 100644
--- a/gremlin-test/features/sideEffect/Group.feature
+++ b/gremlin-test/features/sideEffect/Group.feature
@@ -241,3 +241,14 @@ Feature: Step - group()
     Then the result should be unordered
       | result |
       | m[{"ripple":[], "peter":["created"], "noone":["blah"], "vadas":[], 
"josh":["created", "created"], "lop":[], "marko":[666, "created", "knows", 
"knows"]}] |
+
+  Scenario: 
g_V_hasLabelXpersonX_asXpX_outXcreatedX_group_byXnameX_byXselectXpX_valuesXageX_sumX
+    Given the modern graph
+    And the traversal of
+      """
+      
g.V().hasLabel("person").as("p").out("created").group().by("name").by(__.select("p").values("age").sum())
+      """
+    When iterated to list
+    Then the result should be unordered
+      | result |
+      | m[{"ripple":"d[32].l", "peter":"d[96].l"}] |
diff --git 
a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupTest.java
 
b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupTest.java
index 3e1e53b..38b9c9f 100644
--- 
a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupTest.java
+++ 
b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupTest.java
@@ -99,6 +99,8 @@ public abstract class GroupTest extends 
AbstractGremlinProcessTest {
 
     public abstract Traversal<Vertex, Map<String, List<Object>>> 
get_g_withSideEffectXa__marko_666_noone_blahX_V_groupXaX_byXnameX_byXoutE_label_foldX_capXaX(final
 Map<String, List<Object>> m);
 
+    public abstract Traversal<Vertex, Map<String, Number>> 
get_g_V_hasLabelXpersonX_asXpX_outXcreatedX_group_byXnameX_byXselectXpX_valuesXageX_sumX();
+
     @Test
     @LoadGraphWith(MODERN)
     public void g_V_group_byXnameX() {
@@ -488,6 +490,20 @@ public abstract class GroupTest extends 
AbstractGremlinProcessTest {
         checkSideEffects(traversal.asAdmin().getSideEffects(), "a", 
HashMap.class);
     }
 
+    @Test
+    @LoadGraphWith(MODERN)
+    public void 
g_V_hasLabelXpersonX_asXpX_outXcreatedX_group_byXnameX_byXselectXpX_valuesXageX_sumX()
 {
+        final Traversal<Vertex, Map<String, Number>> traversal = 
get_g_V_hasLabelXpersonX_asXpX_outXcreatedX_group_byXnameX_byXselectXpX_valuesXageX_sumX();
+        printTraversalForm(traversal);
+        final Map<String, Number> map = traversal.next();
+        assertEquals(2, map.size());
+        assertTrue(map.containsKey("ripple"));
+        assertTrue(map.containsKey("lop"));
+        assertEquals(32L, map.get("ripple"));
+        assertEquals(96L, map.get("lop"));
+        assertFalse(traversal.hasNext());
+    }
+
     public static class Traversals extends GroupTest {
 
         @Override
@@ -594,5 +610,10 @@ public abstract class GroupTest extends 
AbstractGremlinProcessTest {
         public Traversal<Vertex, Map<String, List<Object>>> 
get_g_withSideEffectXa__marko_666_noone_blahX_V_groupXaX_byXnameX_byXoutE_label_foldX_capXaX(final
 Map<String, List<Object>> m) {
             return g.withSideEffect("a", 
m).V().group("a").by("name").by(outE().label().fold()).cap("a");
         }
+
+        @Override
+        public Traversal<Vertex, Map<String, Number>> 
get_g_V_hasLabelXpersonX_asXpX_outXcreatedX_group_byXnameX_byXselectXpX_valuesXageX_sumX()
 {
+            return g.V().hasLabel("person").as("p").out("created").<String, 
Number>group().by("name").by(__.select("p").values("age").sum());
+        }
     }
 }

Reply via email to