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 c62a152645de420e2e3b8f3129b5f451c84612df 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 | 33 +++++++++++++++++----- .../step/sideEffect/GroupSideEffectStep.java | 6 ++-- .../process/traversal/util/TraversalHelper.java | 5 ++++ .../test/cucumber/feature-steps.js | 2 +- gremlin-test/features/sideEffect/Group.feature | 23 +++++++++++++++ .../traversal/step/sideEffect/GroupTest.java | 21 ++++++++++++++ 7 files changed, 80 insertions(+), 11 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..b8c2f13 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; @@ -59,11 +60,28 @@ 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()); } + /** + * Determines the first (non-local) barrier step in the provided traversal. This method is used by {@link GroupStep} + * and {@link org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.GroupSideEffectStep} to ultimately + * determine the reducing reducing bi-operator. + * + * @param traversal The traversal to inspect. + * @return The first non-local barrier step or {@code null} if no such step was found. + */ + public 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 +89,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 +135,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 +203,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/step/sideEffect/GroupSideEffectStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupSideEffectStep.java index 9847a53..4a38cb7 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupSideEffectStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupSideEffectStep.java @@ -55,7 +55,7 @@ public final class GroupSideEffectStep<S, K, V> extends SideEffectStep<S> implem super(traversal); this.sideEffectKey = sideEffectKey; this.valueTraversal = this.integrateChild(__.fold().asAdmin()); - this.barrierStep = TraversalHelper.getFirstStepOfAssignableClass(Barrier.class, this.valueTraversal).orElse(null); + this.barrierStep = GroupStep.determineBarrierStep(this.valueTraversal); this.getTraversal().getSideEffects().registerIfAbsent(this.sideEffectKey, HashMapSupplier.instance(), new GroupStep.GroupBiOperator<>(null == this.barrierStep ? Operator.assign : @@ -69,7 +69,7 @@ public final class GroupSideEffectStep<S, K, V> extends SideEffectStep<S> implem this.state = 'v'; } else if ('v' == this.state) { this.valueTraversal = this.integrateChild(GroupStep.convertValueTraversal(kvTraversal)); - this.barrierStep = TraversalHelper.getFirstStepOfAssignableClass(Barrier.class, this.valueTraversal).orElse(null); + this.barrierStep = GroupStep.determineBarrierStep(this.valueTraversal); this.getTraversal().getSideEffects().register(this.sideEffectKey, null, new GroupStep.GroupBiOperator<>(null == this.barrierStep ? Operator.assign : @@ -124,7 +124,7 @@ public final class GroupSideEffectStep<S, K, V> extends SideEffectStep<S> implem 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 = GroupStep.determineBarrierStep(clone.valueTraversal); return clone; } 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-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js b/gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js index 6374d8b..a3577da 100644 --- a/gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js +++ b/gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js @@ -345,4 +345,4 @@ function IgnoreError(reason) { Error.captureStackTrace(this, IgnoreError); } -util.inherits(IgnoreError, Error); \ No newline at end of file +util.inherits(IgnoreError, Error); diff --git a/gremlin-test/features/sideEffect/Group.feature b/gremlin-test/features/sideEffect/Group.feature index cd2a5ce..d87d42f 100644 --- a/gremlin-test/features/sideEffect/Group.feature +++ b/gremlin-test/features/sideEffect/Group.feature @@ -241,3 +241,26 @@ 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", "lop":"d[96].l"}] | + + Scenario: g_V_hasLabelXpersonX_asXpX_outXcreatedX_groupXaX_byXnameX_byXselectXpX_valuesXageX_sumX_capXaX + Given the modern graph + And the traversal of + """ + g.V().hasLabel("person").as("p").out("created").group("a").by("name").by(__.select("p").values("age").sum()).cap("a") + """ + When iterated to list + Then the result should be unordered + | result | + | m[{"ripple":"d[32].l", "lop":"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()); + } } }