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 0cfb2ff375244efb936eac7b83de6c6202560359
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 | 4 ++-
gremlin-test/features/sideEffect/Group.feature | 23 +++++++++++++++
.../traversal/step/sideEffect/GroupTest.java | 21 ++++++++++++++
7 files changed, 82 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..6c16b14 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
@@ -75,6 +75,8 @@ const ignoredScenarios = {
'g_V_peerPressure_hasXclusterX': new
IgnoreError(ignoreReason.computerNotSupported),
'g_V_peerPressure_byXclusterX_byXoutEXknowsXX_pageRankX1X_byXrankX_byXoutEXknowsXX_timesX2X_group_byXclusterX_byXrank_sumX_limitX100X':
new IgnoreError(ignoreReason.computerNotSupported),
'g_V_hasXname_rippleX_inXcreatedX_peerPressure_byXoutEX_byXclusterX_repeatXunionXidentity__bothX_timesX2X_dedup_valueMapXname_clusterX':
new IgnoreError(ignoreReason.computerNotSupported),
+
'g_V_hasLabelXpersonX_asXpX_outXcreatedX_group_byXnameX_byXselectXpX_valuesXageX_sumX':
new IgnoreError(IgnoreReason.needsFurtherInvestigation),
+
'g_V_hasLabelXpersonX_asXpX_outXcreatedX_groupXaX_byXnameX_byXselectXpX_valuesXageX_sumX_capXaX':
new IgnoreError(IgnoreReason.needsFurtherInvestigation),
};
defineSupportCode(function(methods) {
@@ -345,4 +347,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..9e136c2 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", "peter":"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())
+ """
+ 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());
+ }
}
}
