From 651367be902a378a6d813ac39efac0bbc20561eb Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Tue, 29 Aug 2017 15:46:32 -0600 Subject: [PATCH 1/2] fixed a bug in match() where mid-clause where() variables were not being considered (only start where()s were). --- CHANGELOG.asciidoc | 1 + .../process/traversal/step/map/MatchStep.java | 11 ++++++-- .../traversal/step/map/GroovyMatchTest.groovy | 12 +++++++++ .../process/traversal/step/map/MatchTest.java | 26 +++++++++++++++++++ 4 files changed, 48 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index d62df5cbaf1..1623d1803e2 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -26,6 +26,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima TinkerPop 3.2.7 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Fixed a bug in `MatchStep` where mid-traversal `where()` variables were not being considered in start-scope. * Ensured that plugins were applied in the order they were configured. * Fixed a bug in `Neo4jGremlinPlugin` that prevented it from loading properly in the `GremlinPythonScriptEngine`. * Fixed a bug that prevented Gremlin from ordering lists and streams made of mixed number types. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MatchStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MatchStep.java index 8a8237a26df..ed6014ac1ea 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MatchStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MatchStep.java @@ -513,8 +513,15 @@ public Set getScopeKeys() { this.scopeKeys = new HashSet<>(); if (null != this.selectKey) this.scopeKeys.add(this.selectKey); - if (this.getNextStep() instanceof WhereTraversalStep || this.getNextStep() instanceof WherePredicateStep) - this.scopeKeys.addAll(((Scoping) this.getNextStep()).getScopeKeys()); + final Set endLabels = ((MatchStep) this.getTraversal().getParent()).getMatchEndLabels(); + Stream.concat( + TraversalHelper.getStepsOfAssignableClassRecursively(WherePredicateStep.class, this.getTraversal()).stream(), + TraversalHelper.getStepsOfAssignableClassRecursively(WhereTraversalStep.class, this.getTraversal()).stream()). + flatMap(s -> s.getScopeKeys().stream()).filter(endLabels::contains).forEach(this.scopeKeys::add); + // this is the old way but it only checked for where() as the next step, not arbitrarily throughout traversal + // just going to keep this in case something pops up in the future and this is needed as an original reference. + /* if (this.getNextStep() instanceof WhereTraversalStep || this.getNextStep() instanceof WherePredicateStep) + this.scopeKeys.addAll(((Scoping) this.getNextStep()).getScopeKeys());*/ this.scopeKeys = Collections.unmodifiableSet(this.scopeKeys); } return this.scopeKeys; diff --git a/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroovyMatchTest.groovy b/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroovyMatchTest.groovy index 8c36d262faf..45408ac805b 100644 --- a/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroovyMatchTest.groovy +++ b/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroovyMatchTest.groovy @@ -377,5 +377,17 @@ public abstract class GroovyMatchTest { __.as("a").in("followedBy").count().is(gt(10)).as("b")).count; """) } + + @Override + public Traversal get_g_V_matchXa_hasXsong_name_sunshineX__a_mapX0followedBy_weight_meanX_b__a_0followedBy_c__c_filterXweight_whereXgteXbXXX_outV_dX_selectXdX_byXnameX() { + new ScriptTraversal<>(g, "gremlin-groovy", """ + g.V().match( + __.as("a").has("song", "name", "HERE COMES SUNSHINE"), + __.as("a").map(inE("followedBy").weight.mean()).as("b"), + __.as("a").inE("followedBy").as("c"), + __.as("c").filter(values("weight").where(gte("b"))).outV.as("d")). + select("d").by("name"); + """) + } } } \ No newline at end of file diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MatchTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MatchTest.java index b8bf1b61886..f4bee284edd 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MatchTest.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MatchTest.java @@ -46,11 +46,13 @@ import static org.apache.tinkerpop.gremlin.process.traversal.P.neq; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.and; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.as; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.inE; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.match; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.not; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.or; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.out; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.repeat; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.values; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.where; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -159,6 +161,10 @@ public abstract class MatchTest extends AbstractGremlinProcessTest { // test inline counts public abstract Traversal get_g_V_matchXa_followedBy_count_isXgtX10XX_b__a_0followedBy_count_isXgtX10XX_bX_count(); + // test mid-clause variables + public abstract Traversal get_g_V_matchXa_hasXsong_name_sunshineX__a_mapX0followedBy_weight_meanX_b__a_0followedBy_c__c_filterXweight_whereXgteXbXXX_outV_dX_selectXdX_byXnameX(); + + @Test @LoadGraphWith(MODERN) public void g_V_valueMap_matchXa_selectXnameX_bX() { @@ -566,6 +572,16 @@ public void g_V_matchXa_followedBy_count_isXgtX10XX_b__a_0followedBy_count_isXgt checkResults(Collections.singletonList(6L), traversal); } + @Test + @LoadGraphWith(GRATEFUL) + public void g_V_matchXa_hasXsong_name_sunshineX__a_mapX0followedBy_weight_meanX_b__a_0followedBy_c__c_filterXweight_whereXgteXbXXX_outV_dX_selectXdX_byXnameX() { + final Traversal traversal = get_g_V_matchXa_hasXsong_name_sunshineX__a_mapX0followedBy_weight_meanX_b__a_0followedBy_c__c_filterXweight_whereXgteXbXXX_outV_dX_selectXdX_byXnameX(); + printTraversalForm(traversal); + checkResults(Arrays.asList("THE MUSIC NEVER STOPPED", "PROMISED LAND", "PLAYING IN THE BAND", + "CASEY JONES", "BIG RIVER", "EL PASO", "LIBERTY", "LOOKS LIKE RAIN"), traversal); + } + + public static class GreedyMatchTraversals extends Traversals { @Before public void setupTest() { @@ -850,5 +866,15 @@ public Traversal get_g_V_matchXa_followedBy_count_isXgtX10XX_b__a_ as("a").out("followedBy").count().is(P.gt(10)).as("b"), as("a").in("followedBy").count().is(P.gt(10)).as("b")).count(); } + + @Override + public Traversal get_g_V_matchXa_hasXsong_name_sunshineX__a_mapX0followedBy_weight_meanX_b__a_0followedBy_c__c_filterXweight_whereXgteXbXXX_outV_dX_selectXdX_byXnameX() { + return g.V().match( + as("a").has("song", "name", "HERE COMES SUNSHINE"), + as("a").map(inE("followedBy").values("weight").mean()).as("b"), + as("a").inE("followedBy").as("c"), + as("c").filter(values("weight").where(P.gte("b"))).outV().as("d")). + select("d").by("name"); + } } } From 189b1bc81a04c62032247f6ba984cf5e58b173b7 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Wed, 30 Aug 2017 08:59:30 -0600 Subject: [PATCH 2/2] added @dkuppitz tweak to remove stream() usage. --- .../process/traversal/step/map/MatchStep.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MatchStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MatchStep.java index ed6014ac1ea..dfe52b026f6 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MatchStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MatchStep.java @@ -514,10 +514,14 @@ public Set getScopeKeys() { if (null != this.selectKey) this.scopeKeys.add(this.selectKey); final Set endLabels = ((MatchStep) this.getTraversal().getParent()).getMatchEndLabels(); - Stream.concat( - TraversalHelper.getStepsOfAssignableClassRecursively(WherePredicateStep.class, this.getTraversal()).stream(), - TraversalHelper.getStepsOfAssignableClassRecursively(WhereTraversalStep.class, this.getTraversal()).stream()). - flatMap(s -> s.getScopeKeys().stream()).filter(endLabels::contains).forEach(this.scopeKeys::add); + TraversalHelper.anyStepRecursively(step -> { + if (step instanceof WherePredicateStep || step instanceof WhereTraversalStep) { + for (final String key : ((Scoping) step).getScopeKeys()) { + if (endLabels.contains(key)) this.scopeKeys.add(key); + } + } + return false; + }, this.getTraversal()); // this is the old way but it only checked for where() as the next step, not arbitrarily throughout traversal // just going to keep this in case something pops up in the future and this is needed as an original reference. /* if (this.getNextStep() instanceof WhereTraversalStep || this.getNextStep() instanceof WherePredicateStep)