You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by sp...@apache.org on 2021/08/31 19:37:49 UTC
[tinkerpop] 15/18: TINKERPOP-2585 Strategies were being applied
when unecessary
This is an automated email from the ASF dual-hosted git repository.
spmallette pushed a commit to branch 3.5-dev-rebuild
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit eb3225feadfefbd606aada095743a944a5c3b229
Author: Stephen Mallette <st...@amazon.com>
AuthorDate: Tue Aug 3 12:26:02 2021 -0400
TINKERPOP-2585 Strategies were being applied when unecessary
Not sure if this was related to the original issue but decided to tack this in here, but basically strategies were being repeatedly applied under the 3.5.0 strategy model when they shouldn't have and were therefore yielding incorrect results at times. Hard to say exactly what might have been affected but the issues surfaced in the new test that was added here.
---
.../MessagePassingReductionStrategy.java | 4 +--
.../optimization/InlineFilterStrategy.java | 4 ---
.../optimization/RepeatUnrollStrategy.java | 3 ++-
.../process/traversal/util/TraversalHelper.java | 8 +++++-
.../Gremlin.Net.IntegrationTest/Gherkin/Gremlin.cs | 1 +
.../gremlin-javascript/test/cucumber/gremlin.js | 1 +
gremlin-python/src/main/python/radish/gremlin.py | 1 +
gremlin-test/features/branch/Repeat.feature | 12 +++++++++
.../process/traversal/step/branch/RepeatTest.java | 30 +++++++++++++++++++++-
9 files changed, 54 insertions(+), 10 deletions(-)
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/strategy/optimization/MessagePassingReductionStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/strategy/optimization/MessagePassingReductionStrategy.java
index f32d376..5c85d3e 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/strategy/optimization/MessagePassingReductionStrategy.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/strategy/optimization/MessagePassingReductionStrategy.java
@@ -92,9 +92,7 @@ public final class MessagePassingReductionStrategy extends AbstractTraversalStra
final Traversal.Admin<?, ?> computerTraversal = step.computerTraversal.get().clone();
// apply the strategies up to this point
final List<TraversalStrategy<?>> strategies = step.getTraversal().getStrategies().toList();
- final int indexOfStrategy = strategies.indexOf(MessagePassingReductionStrategy.instance());
- if (indexOfStrategy > 0)
- TraversalHelper.applySingleLevelStrategies(step.getTraversal(), computerTraversal, strategies.get(indexOfStrategy - 1).getClass());
+
if (computerTraversal.getSteps().size() > 1 &&
!(computerTraversal.getStartStep().getNextStep() instanceof Barrier) &&
TraversalHelper.hasStepOfAssignableClassRecursively(Arrays.asList(VertexStep.class, EdgeVertexStep.class), computerTraversal) &&
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategy.java
index 41d6f01..6d71c84 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategy.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategy.java
@@ -178,7 +178,6 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver
RangeGlobalStep.class,
DedupGlobalStep.class,
LambdaHolder.class)) {
- TraversalHelper.applySingleLevelStrategies(traversal, childTraversal, InlineFilterStrategy.class);
final Step<?, ?> finalStep = childTraversal.getEndStep();
TraversalHelper.insertTraversal((Step) step, childTraversal, traversal);
TraversalHelper.copyLabels(step, finalStep, false);
@@ -251,7 +250,6 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver
Step<?, ?> finalStep = null;
for (int i = childTraversals.size() - 1; i >= 0; i--) {
final Traversal.Admin<?, ?> childTraversal = childTraversals.get(i);
- TraversalHelper.applySingleLevelStrategies(traversal, childTraversal, InlineFilterStrategy.class);
if (null == finalStep)
finalStep = childTraversal.getEndStep();
TraversalHelper.insertTraversal((Step) step, childTraversals.get(i), traversal);
@@ -281,8 +279,6 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver
final String endLabel = ((MatchStep.MatchEndStep) matchTraversal.getEndStep()).getMatchKey().orElse(null); // why would this exist? but just in case
matchTraversal.removeStep(0); // remove MatchStartStep
matchTraversal.removeStep(matchTraversal.getSteps().size() - 1); // remove MatchEndStep
- TraversalHelper.applySingleLevelStrategies(traversal, matchTraversal, InlineFilterStrategy.class);
-
matchTraversal.getEndStep().addLabel(startLabel);
if (null != endLabel) matchTraversal.getEndStep().addLabel(endLabel);
TraversalHelper.insertTraversal((Step) step.getPreviousStep(), matchTraversal, traversal);
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RepeatUnrollStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RepeatUnrollStrategy.java
index be82d0c..18481cd 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RepeatUnrollStrategy.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RepeatUnrollStrategy.java
@@ -77,9 +77,10 @@ public final class RepeatUnrollStrategy extends AbstractTraversalStrategy<Traver
repeatStep.getUntilTraversal() instanceof LoopTraversal && ((LoopTraversal) repeatStep.getUntilTraversal()).getMaxLoops() > 0 &&
!TraversalHelper.hasStepOfAssignableClassRecursively(Scope.global, DedupGlobalStep.class, repeatStep.getRepeatTraversal()) &&
!TraversalHelper.hasStepOfAssignableClassRecursively(INVALIDATING_STEPS, repeatStep.getRepeatTraversal())) {
+
final Traversal.Admin<?, ?> repeatTraversal = repeatStep.getGlobalChildren().get(0);
repeatTraversal.removeStep(repeatTraversal.getSteps().size() - 1); // removes the RepeatEndStep
- TraversalHelper.applySingleLevelStrategies(traversal, repeatTraversal, RepeatUnrollStrategy.class);
+
final int repeatLength = repeatTraversal.getSteps().size();
int insertIndex = i;
final int loops = (int) ((LoopTraversal) repeatStep.getUntilTraversal()).getMaxLoops();
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 4e7eb92..c20961c 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
@@ -683,7 +683,13 @@ public final class TraversalHelper {
return false;
}
- public static void applySingleLevelStrategies(final Traversal.Admin<?, ?> parentTraversal, final Traversal.Admin<?, ?> childTraversal, final Class<? extends TraversalStrategy> stopAfterStrategy) {
+ /**
+ * @deprecated As of release 3.5.2, not replaced as strategies are not applied in this fashion after 3.5.0
+ */
+ @Deprecated
+ public static void applySingleLevelStrategies(final Traversal.Admin<?, ?> parentTraversal,
+ final Traversal.Admin<?, ?> childTraversal,
+ final Class<? extends TraversalStrategy> stopAfterStrategy) {
childTraversal.setStrategies(parentTraversal.getStrategies());
childTraversal.setSideEffects(parentTraversal.getSideEffects());
parentTraversal.getGraph().ifPresent(childTraversal::setGraph);
diff --git a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/Gremlin.cs b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/Gremlin.cs
index 339cb73..407c9b8 100644
--- a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/Gremlin.cs
+++ b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/Gremlin.cs
@@ -90,6 +90,7 @@ namespace Gremlin.Net.IntegrationTest.Gherkin
{"g_VX1X_optionalXaddVXdogXX_label", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.AddV("person").Property(T.Id,1).Property("name","marko").Property("age",29).As("marko").AddV("person").Property(T.Id,2).Property("name","vadas").Property("age",27).As("vadas").AddV("software").Property(T.Id,3).Property("name","lop").Property("lang","java").As("lop").AddV("person").Property(T.Id,4).Property("name","josh").Property("age",32).As("josh" [...]
{"g_V_repeatXoutX_timesX2X_emit_path", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V().Repeat(__.Out()).Times(2).Emit().Path()}},
{"g_V_repeatXoutX_timesX2X_repeatXinX_timesX2X_name", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V().Repeat(__.Out()).Times(2).Repeat(__.In()).Times(2).Values<object>("name")}},
+ {"g_V_repeatXoutE_inVX_timesX2X_path_by_name_by_label", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V().Repeat(__.OutE().InV()).Times(2).Path().By("name").By(T.Label)}},
{"g_V_repeatXoutX_timesX2X", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V().Repeat(__.Out()).Times(2)}},
{"g_V_repeatXoutX_timesX2X_emit", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V().Repeat(__.Out()).Times(2).Emit()}},
{"g_VX1X_timesX2X_repeatXoutX_name", new List<Func<GraphTraversalSource, IDictionary<string, object>, ITraversal>> {(g,p) =>g.V(p["vid1"]).Times(2).Repeat(__.Out()).Values<object>("name")}},
diff --git a/gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/gremlin.js b/gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/gremlin.js
index 1c29ea1..be4f51f 100644
--- a/gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/gremlin.js
+++ b/gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/gremlin.js
@@ -79,6 +79,7 @@ const gremlins = {
g_VX1X_optionalXaddVXdogXX_label: [function({g, vid1}) { return g.addV("person").property(T.id,1).property("name","marko").property("age",29).as("marko").addV("person").property(T.id,2).property("name","vadas").property("age",27).as("vadas").addV("software").property(T.id,3).property("name","lop").property("lang","java").as("lop").addV("person").property(T.id,4).property("name","josh").property("age",32).as("josh").addV("software").property(T.id,5).property("name","ripple").property( [...]
g_V_repeatXoutX_timesX2X_emit_path: [function({g}) { return g.V().repeat(__.out()).times(2).emit().path() }],
g_V_repeatXoutX_timesX2X_repeatXinX_timesX2X_name: [function({g}) { return g.V().repeat(__.out()).times(2).repeat(__.in_()).times(2).values("name") }],
+ g_V_repeatXoutE_inVX_timesX2X_path_by_name_by_label: [function({g}) { return g.V().repeat(__.outE().inV()).times(2).path().by("name").by(T.label) }],
g_V_repeatXoutX_timesX2X: [function({g}) { return g.V().repeat(__.out()).times(2) }],
g_V_repeatXoutX_timesX2X_emit: [function({g}) { return g.V().repeat(__.out()).times(2).emit() }],
g_VX1X_timesX2X_repeatXoutX_name: [function({g, vid1}) { return g.V(vid1).times(2).repeat(__.out()).values("name") }],
diff --git a/gremlin-python/src/main/python/radish/gremlin.py b/gremlin-python/src/main/python/radish/gremlin.py
index f74b037..4d58428 100644
--- a/gremlin-python/src/main/python/radish/gremlin.py
+++ b/gremlin-python/src/main/python/radish/gremlin.py
@@ -64,6 +64,7 @@ world.gremlins = {
'g_VX1X_optionalXaddVXdogXX_label': [(lambda g, vid1=None:g.addV('person').property(T.id,1).property('name','marko').property('age',29).as_('marko').addV('person').property(T.id,2).property('name','vadas').property('age',27).as_('vadas').addV('software').property(T.id,3).property('name','lop').property('lang','java').as_('lop').addV('person').property(T.id,4).property('name','josh').property('age',32).as_('josh').addV('software').property(T.id,5).property('name','ripple').property('l [...]
'g_V_repeatXoutX_timesX2X_emit_path': [(lambda g:g.V().repeat(__.out()).times(2).emit().path())],
'g_V_repeatXoutX_timesX2X_repeatXinX_timesX2X_name': [(lambda g:g.V().repeat(__.out()).times(2).repeat(__.in_()).times(2).name)],
+ 'g_V_repeatXoutE_inVX_timesX2X_path_by_name_by_label': [(lambda g:g.V().repeat(__.outE().inV()).times(2).path().by('name').by(T.label))],
'g_V_repeatXoutX_timesX2X': [(lambda g:g.V().repeat(__.out()).times(2))],
'g_V_repeatXoutX_timesX2X_emit': [(lambda g:g.V().repeat(__.out()).times(2).emit())],
'g_VX1X_timesX2X_repeatXoutX_name': [(lambda g, vid1=None:g.V(vid1).times(2).repeat(__.out()).name)],
diff --git a/gremlin-test/features/branch/Repeat.feature b/gremlin-test/features/branch/Repeat.feature
index 223b0ef..55d24da 100644
--- a/gremlin-test/features/branch/Repeat.feature
+++ b/gremlin-test/features/branch/Repeat.feature
@@ -47,6 +47,18 @@ Feature: Step - repeat()
| marko |
| marko |
+ Scenario: g_V_repeatXoutE_inVX_timesX2X_path_by_name_by_label
+ Given the modern graph
+ And the traversal of
+ """
+ g.V().repeat(__.outE().inV()).times(2).path().by("name").by(T.label);
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | p[marko,knows,josh,created,lop] |
+ | p[marko,knows,josh,created,ripple] |
+
Scenario: g_V_repeatXoutX_timesX2X
Given the modern graph
And the traversal of
diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatTest.java
index 3681aef..2475e41 100644
--- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatTest.java
+++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatTest.java
@@ -48,6 +48,7 @@ import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.loops;
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.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.collection.IsIn.oneOf;
import static org.hamcrest.core.Is.is;
import static org.hamcrest.collection.IsIterableContainingInOrder.contains;
import static org.hamcrest.core.IsEqual.equalTo;
@@ -72,6 +73,8 @@ public abstract class RepeatTest extends AbstractGremlinProcessTest {
public abstract Traversal<Vertex, Vertex> get_g_V_repeatXoutX_timesX2X();
+ public abstract Traversal<Vertex, Path> get_g_V_repeatXoutE_inVX_timesX2X_path_by_name_by_label();
+
public abstract Traversal<Vertex, Vertex> get_g_V_repeatXoutX_timesX2X_emit();
public abstract Traversal<Vertex, Path> get_g_V_hasXloop_name_loopX_repeatXinX_timesX5X_path_by_name();
@@ -160,7 +163,7 @@ public abstract class RepeatTest extends AbstractGremlinProcessTest {
int counter = 0;
while (traversal.hasNext()) {
counter++;
- Vertex vertex = traversal.next();
+ final Vertex vertex = traversal.next();
assertTrue(vertex.value("name").equals("lop") || vertex.value("name").equals("ripple"));
}
assertEquals(2, counter);
@@ -169,6 +172,26 @@ public abstract class RepeatTest extends AbstractGremlinProcessTest {
@Test
@LoadGraphWith(MODERN)
+ public void g_V_repeatXoutE_inVX_timesX2X_path_by_name_by_label() {
+ final Traversal<Vertex, Path> traversal = get_g_V_repeatXoutE_inVX_timesX2X_path_by_name_by_label();
+
+ printTraversalForm(traversal);
+ int counter = 0;
+ while (traversal.hasNext()) {
+ counter++;
+ final Path p = traversal.next();
+ assertEquals("marko", p.get(0));
+ assertEquals("knows", p.get(1));
+ assertEquals("josh", p.get(2));
+ assertEquals("created", p.get(3));
+ assertThat(p.get(4), oneOf("ripple", "lop"));
+ }
+ assertEquals(2, counter);
+ assertFalse(traversal.hasNext());
+ }
+
+ @Test
+ @LoadGraphWith(MODERN)
public void g_V_repeatXoutX_timesX2X_emit() {
final Traversal<Vertex, Vertex> traversal = get_g_V_repeatXoutX_timesX2X_emit();
printTraversalForm(traversal);
@@ -477,6 +500,11 @@ public abstract class RepeatTest extends AbstractGremlinProcessTest {
}
@Override
+ public Traversal<Vertex, Path> get_g_V_repeatXoutE_inVX_timesX2X_path_by_name_by_label() {
+ return g.V().repeat(outE().inV()).times(2).path().by("name").by(T.label);
+ }
+
+ @Override
public Traversal<Vertex, Vertex> get_g_V_repeatXoutX_timesX2X_emit() {
return g.V().repeat(out()).times(2).emit();
}