Repository: tinkerpop
Updated Branches:
  refs/heads/TINKERPOP-1456 d2ae1aaa8 -> 5fc2163df


SubgraphStrategy no longer filter()-wraps criteria if the criteria is a chain 
of filters or connectives. This ensures that has() folds to VertexStep and 
VertexEdgeSteps -- by most providers.


Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo
Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/5fc2163d
Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/5fc2163d
Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/5fc2163d

Branch: refs/heads/TINKERPOP-1456
Commit: 5fc2163df107ddae830bd68f7a3926d4fcea8211
Parents: d2ae1aa
Author: Marko A. Rodriguez <okramma...@gmail.com>
Authored: Tue Sep 20 15:05:12 2016 -0600
Committer: Marko A. Rodriguez <okramma...@gmail.com>
Committed: Tue Sep 20 15:05:12 2016 -0600

----------------------------------------------------------------------
 CHANGELOG.asciidoc                              |   1 +
 .../strategy/decoration/SubgraphStrategy.java   | 110 +++++++++++++++----
 .../decoration/SubgraphStrategyProcessTest.java |   8 +-
 3 files changed, 96 insertions(+), 23 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/5fc2163d/CHANGELOG.asciidoc
----------------------------------------------------------------------
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index 94391b9..4c94aad 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -29,6 +29,7 @@ TinkerPop 3.2.3 (Release Date: NOT OFFICIALLY RELEASED YET)
 * Fixed a bug in `TraversalVertexProgram` (OLAP) around ordering and 
connectives (i.e. `and()` and `or()`).
 * Added `AbstractGremlinProcessTest.checkOrderedResults()` to make testing 
ordered results easier.
 * `AbstractLambdaTraversal` now supports a `bypassTraversal` and thus, it is 
possible for strategies to redefine such lambda traversals.
+* `SubgraphStrategy` no longer `filter()`-wraps if the criteria is a chain of 
filters or connectives.
 * `SubgraphStrategy` now supports vertex property filtering.
 * Fixed a bug in Gremlin-Python `P` where predicates reversed the order of the 
predicates.
 * Fixed a naming bug in Gremlin-Python where `P._and` and `P._or` should be 
`P.and_` and `P.or_`. (*breaking*)

http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/5fc2163d/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategy.java
----------------------------------------------------------------------
diff --git 
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategy.java
 
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategy.java
index 475a3d7..de7649d 100644
--- 
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategy.java
+++ 
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategy.java
@@ -24,6 +24,8 @@ import 
org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy;
 import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__;
 import 
org.apache.tinkerpop.gremlin.process.traversal.lambda.ElementValueTraversal;
 import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent;
+import 
org.apache.tinkerpop.gremlin.process.traversal.step.filter.ConnectiveStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.filter.FilterStep;
 import 
org.apache.tinkerpop.gremlin.process.traversal.step.filter.LambdaFilterStep;
 import org.apache.tinkerpop.gremlin.process.traversal.step.filter.OrStep;
 import 
org.apache.tinkerpop.gremlin.process.traversal.step.filter.TraversalFilterStep;
@@ -48,7 +50,9 @@ import org.apache.tinkerpop.gremlin.structure.Vertex;
 import org.apache.tinkerpop.gremlin.structure.VertexProperty;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
+import java.util.Set;
 import java.util.UUID;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -68,24 +72,47 @@ public final class SubgraphStrategy extends 
AbstractTraversalStrategy<TraversalS
     private final Traversal.Admin<Vertex, ?> vertexCriterion;
     private final Traversal.Admin<Edge, ?> edgeCriterion;
     private final Traversal.Admin<VertexProperty, ?> vertexPropertyCriterion;
+
+    private final boolean vertexCriterionIsAllFilter;
+    private final boolean edgeCriterionIsAllFilter;
+    private final boolean vertexPropertyCriterionIsAllFilter;
+
+    private static final Set<Class<? extends DecorationStrategy>> POSTS = 
Collections.singleton(ConnectiveStrategy.class);
+
     private final String MARKER = 
Graph.Hidden.hide(UUID.randomUUID().toString());
 
     private SubgraphStrategy(final Traversal<Vertex, ?> vertexCriterion, final 
Traversal<Edge, ?> edgeCriterion, final Traversal<VertexProperty, ?> 
vertexPropertyCriterion) {
+        this.vertexCriterionIsAllFilter = isAllFilters(vertexCriterion);
+        this.edgeCriterionIsAllFilter = isAllFilters(edgeCriterion);
+        this.vertexPropertyCriterionIsAllFilter = 
isAllFilters(vertexPropertyCriterion);
+
+
         this.vertexCriterion = null == vertexCriterion ? null : 
vertexCriterion.asAdmin();
 
         // if there is no vertex predicate there is no need to test either 
side of the edge
         if (null == this.vertexCriterion) {
             this.edgeCriterion = null == edgeCriterion ? null : 
edgeCriterion.asAdmin();
         } else {
-            final Traversal.Admin<Edge, ?> vertexPredicate = __.<Edge>and(
-                    __.inV().filter(this.vertexCriterion.clone()),
-                    __.outV().filter(this.vertexCriterion.clone())).asAdmin();
+            final Traversal.Admin<Edge, ?> vertexPredicate;
+            if (this.vertexCriterionIsAllFilter) {
+                final Traversal.Admin<Edge, Vertex> left = 
__.<Edge>inV().asAdmin();
+                final Traversal.Admin<Edge, Vertex> right = 
__.<Edge>outV().asAdmin();
+                TraversalHelper.insertTraversal(0, 
this.vertexCriterion.clone(), left);
+                TraversalHelper.insertTraversal(0, 
this.vertexCriterion.clone(), right);
+                vertexPredicate = __.<Edge>and(left, right).asAdmin();
+            } else
+                vertexPredicate = __.<Edge>and(
+                        __.inV().filter(this.vertexCriterion.clone()),
+                        
__.outV().filter(this.vertexCriterion.clone())).asAdmin();
 
             // if there is a vertex predicate then there is an implied edge 
filter on vertices even if there is no
             // edge predicate provided by the user.
             if (null == edgeCriterion)
                 this.edgeCriterion = vertexPredicate;
-            else
+            else if (this.edgeCriterionIsAllFilter) {
+                this.edgeCriterion = edgeCriterion.asAdmin();
+                
TraversalHelper.insertTraversal(this.edgeCriterion.getSteps().size() - 1, 
vertexPredicate, this.edgeCriterion);
+            } else
                 this.edgeCriterion = edgeCriterion.asAdmin().addStep(new 
TraversalFilterStep<>(edgeCriterion.asAdmin(), vertexPredicate));
         }
 
@@ -99,6 +126,16 @@ public final class SubgraphStrategy extends 
AbstractTraversalStrategy<TraversalS
             this.metadataLabelStartStep(this.vertexPropertyCriterion);
     }
 
+    private static final boolean isAllFilters(final Traversal<?, ?> traversal) 
{
+        if (null == traversal)
+            return false;
+        for (final Step<?, ?> step : traversal.asAdmin().getSteps()) {
+            if (!(step instanceof FilterStep || step instanceof 
ConnectiveStep))
+                return false;
+        }
+        return true;
+    }
+
     private final void metadataLabelStartStep(final Traversal.Admin<?, ?> 
traversal) {
         traversal.getStartStep().addLabel(MARKER);
         for (final Step<?, ?> step : traversal.getSteps()) {
@@ -126,7 +163,7 @@ public final class SubgraphStrategy extends 
AbstractTraversalStrategy<TraversalS
             
vertexStepsToInsertFilterAfter.addAll(TraversalHelper.getStepsOfAssignableClass(AddVertexStep.class,
 traversal));
             
vertexStepsToInsertFilterAfter.addAll(TraversalHelper.getStepsOfAssignableClass(AddVertexStartStep.class,
 traversal));
             
vertexStepsToInsertFilterAfter.addAll(graphSteps.stream().filter(GraphStep::returnsVertex).collect(Collectors.toList()));
-            applyCriterion(vertexStepsToInsertFilterAfter, traversal, 
this.vertexCriterion);
+            applyCriterion(vertexStepsToInsertFilterAfter, traversal, 
this.vertexCriterion, this.vertexCriterionIsAllFilter);
         }
 
         if (null != this.edgeCriterion) {
@@ -134,7 +171,7 @@ public final class SubgraphStrategy extends 
AbstractTraversalStrategy<TraversalS
             
edgeStepsToInsertFilterAfter.addAll(TraversalHelper.getStepsOfAssignableClass(AddEdgeStep.class,
 traversal));
             
edgeStepsToInsertFilterAfter.addAll(graphSteps.stream().filter(GraphStep::returnsEdge).collect(Collectors.toList()));
             
edgeStepsToInsertFilterAfter.addAll(vertexSteps.stream().filter(VertexStep::returnsEdge).collect(Collectors.toList()));
-            applyCriterion(edgeStepsToInsertFilterAfter, traversal, 
this.edgeCriterion);
+            applyCriterion(edgeStepsToInsertFilterAfter, traversal, 
this.edgeCriterion, this.edgeCriterionIsAllFilter);
         }
 
         // turn g.V().out() to g.V().outE().inV() only if there is an edge 
predicate otherwise
@@ -142,7 +179,10 @@ public final class SubgraphStrategy extends 
AbstractTraversalStrategy<TraversalS
             if (step.returnsEdge())
                 continue;
             if (null != this.vertexCriterion && null == edgeCriterion) {
-                TraversalHelper.insertAfterStep(new 
TraversalFilterStep<>(traversal, (Traversal) this.vertexCriterion.clone()), 
step, traversal);
+                if (this.vertexCriterionIsAllFilter)
+                    TraversalHelper.insertTraversal((Step) step, 
this.vertexCriterion.clone(), traversal);
+                else
+                    TraversalHelper.insertAfterStep(new 
TraversalFilterStep<>(traversal, (Traversal) this.vertexCriterion.clone()), 
step, traversal);
             } else {
                 final VertexStep<Edge> someEStep = new VertexStep<>(traversal, 
Edge.class, step.getDirection(), step.getEdgeLabels());
                 final Step<Edge, Vertex> someVStep = step.getDirection() == 
Direction.BOTH ?
@@ -158,18 +198,28 @@ public final class SubgraphStrategy extends 
AbstractTraversalStrategy<TraversalS
                 }
 
                 if (null != this.edgeCriterion)
-                    TraversalHelper.insertAfterStep(new 
TraversalFilterStep<>(traversal, this.edgeCriterion.clone()), someEStep, 
traversal);
+                    if (this.edgeCriterionIsAllFilter)
+                        TraversalHelper.insertTraversal(someEStep, 
this.edgeCriterion.clone(), traversal);
+                    else
+                        TraversalHelper.insertAfterStep(new 
TraversalFilterStep<>(traversal, this.edgeCriterion.clone()), someEStep, 
traversal);
                 if (null != this.vertexCriterion)
-                    TraversalHelper.insertAfterStep(new 
TraversalFilterStep<>(traversal, this.vertexCriterion.clone()), someVStep, 
traversal);
+                    if (this.edgeCriterionIsAllFilter)
+                        TraversalHelper.insertTraversal(someVStep, 
this.vertexCriterion.clone(), traversal);
+                    else
+                        TraversalHelper.insertAfterStep(new 
TraversalFilterStep<>(traversal, this.vertexCriterion.clone()), someVStep, 
traversal);
+
+
             }
         }
 
         // turn g.V().properties() to g.V().properties().xxx
         // turn g.V().values() to g.V().properties().xxx.value()\
         if (null != this.vertexPropertyCriterion) {
-            final OrStep<Object> wrappedCriterion = new OrStep<>(traversal,
+            final OrStep<Object> wrappedCriterion = new OrStep(traversal,
                     new DefaultTraversal<>().addStep(new 
LambdaFilterStep<>(traversal, t -> !(t.get() instanceof VertexProperty))),
-                    new DefaultTraversal<>().addStep(new 
TraversalFilterStep<>(traversal, this.vertexPropertyCriterion.clone())));
+                    this.vertexPropertyCriterionIsAllFilter ?
+                            this.vertexPropertyCriterion.clone() :
+                            new DefaultTraversal<>().addStep(new 
TraversalFilterStep<>(traversal, this.vertexPropertyCriterion.clone())));
             // turn all ElementValueTraversals into filters
             for (final Step<?, ?> step : traversal.getSteps()) {
                 // gremlin> 
g.V().local(properties('name','stateOfMind').group().by(key()).by(value().fold()))
@@ -178,9 +228,12 @@ public final class SubgraphStrategy extends 
AbstractTraversalStrategy<TraversalS
                             .filter(t -> t instanceof ElementValueTraversal)
                             .forEach(t -> {
                                 final Traversal.Admin<?, ?> temp = new 
DefaultTraversal<>();
-                                temp.addStep(new PropertiesStep<>(temp, 
PropertyType.VALUE, ((ElementValueTraversal) t).getPropertyKey()));
-                                temp.setParent((TraversalParent)step);
+                                temp.addStep(new PropertiesStep<>(temp, 
PropertyType.PROPERTY, ((ElementValueTraversal) t).getPropertyKey()));
+                                temp.addStep(wrappedCriterion.clone());
+                                temp.addStep(new PropertyValueStep<>(temp));
+                                temp.setParent((TraversalParent) step);
                                 ((ElementValueTraversal) 
t).setBypassTraversal(temp);
+                                ((TraversalParent) step).integrateChild(temp);
                             });
                 }
             }
@@ -208,8 +261,16 @@ public final class SubgraphStrategy extends 
AbstractTraversalStrategy<TraversalS
                 }
             }
         }
+        // when there is no filter()-wrap, the marked steps exist at the same 
traversal level
+        for (final Step step : traversal.getSteps()) {
+            step.removeLabel(MARKER);
+        }
     }
 
+    @Override
+    public Set<Class<? extends DecorationStrategy>> applyPost() {
+        return POSTS;
+    }
 
     public Traversal<Vertex, ?> getVertexCriterion() {
         return this.vertexCriterion;
@@ -229,15 +290,24 @@ public final class SubgraphStrategy extends 
AbstractTraversalStrategy<TraversalS
     }
 
     private void applyCriterion(final List<Step> stepsToApplyCriterionAfter, 
final Traversal.Admin traversal,
-                                final Traversal.Admin<? extends Element, ?> 
criterion) {
+                                final Traversal.Admin<? extends Element, ?> 
criterion, final boolean isAllFilter) {
         for (final Step<?, ?> step : stepsToApplyCriterionAfter) {
-            // re-assign the step label to the criterion because the label 
should apply seamlessly after the filter
-            final Step filter = new TraversalFilterStep<>(traversal, 
criterion.clone());
-            for (final String label : step.getLabels()) {
-                step.removeLabel(label);
-                filter.addLabel(label);
+            if (isAllFilter) {
+                final Traversal.Admin<? extends Element, ?> criterionClone = 
criterion.clone();
+                for (final String label : step.getLabels()) {
+                    step.removeLabel(label);
+                    criterionClone.getEndStep().addLabel(label);
+                }
+                TraversalHelper.insertTraversal((Step) step, criterionClone, 
traversal);
+            } else {
+                // re-assign the step label to the criterion because the label 
should apply seamlessly after the filter
+                final Step filter = new TraversalFilterStep<>(traversal, 
criterion.clone());
+                for (final String label : step.getLabels()) {
+                    step.removeLabel(label);
+                    filter.addLabel(label);
+                }
+                TraversalHelper.insertAfterStep(filter, step, traversal);
             }
-            TraversalHelper.insertAfterStep(filter, step, traversal);
         }
     }
 

http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/5fc2163d/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyProcessTest.java
----------------------------------------------------------------------
diff --git 
a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyProcessTest.java
 
b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyProcessTest.java
index e303281..c5b2d5a 100644
--- 
a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyProcessTest.java
+++ 
b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyProcessTest.java
@@ -26,9 +26,10 @@ import org.apache.tinkerpop.gremlin.process.traversal.P;
 import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
 import 
org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
 import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__;
+import 
org.apache.tinkerpop.gremlin.process.traversal.step.filter.TraversalFilterStep;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper;
 import org.apache.tinkerpop.gremlin.structure.Edge;
 import org.apache.tinkerpop.gremlin.structure.Vertex;
-import 
org.apache.tinkerpop.gremlin.structure.util.detached.DetachedVertexProperty;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
@@ -47,6 +48,7 @@ import static 
org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.outE;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.core.IsCollectionContaining.hasItems;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
@@ -374,11 +376,12 @@ public class SubgraphStrategyProcessTest extends 
AbstractGremlinProcessTest {
         GraphTraversalSource sg = 
create(SubgraphStrategy.build().vertexProperties(has("startTime", 
P.gt(2005))).create());
         checkResults(Arrays.asList("purcellville", "baltimore", "oakland", 
"seattle", "aachen"), sg.V().properties("location").value());
         checkResults(Arrays.asList("purcellville", "baltimore", "oakland", 
"seattle", "aachen"), sg.V().values("location"));
+        
assertFalse(TraversalHelper.hasStepOfAssignableClassRecursively(TraversalFilterStep.class,
 sg.V().properties("location").value().iterate().asAdmin()));
         // check to make sure edge properties are not analyzed
         sg = create(SubgraphStrategy.build().vertexProperties(has("startTime", 
P.gt(2005))).create());
         checkResults(Arrays.asList("purcellville", "baltimore", "oakland", 
"seattle", "aachen"), 
sg.V().as("a").properties("location").as("b").select("a").outE().properties().select("b").value().dedup());
         checkResults(Arrays.asList("purcellville", "baltimore", "oakland", 
"seattle", "aachen"), 
sg.V().as("a").values("location").as("b").select("a").outE().properties().select("b").dedup());
-        //
+        
assertFalse(TraversalHelper.hasStepOfAssignableClassRecursively(TraversalFilterStep.class,
 
sg.V().as("a").values("location").as("b").select("a").outE().properties().select("b").dedup().iterate().asAdmin()));
         //
         sg = create(SubgraphStrategy.build().vertices(has("name", 
P.neq("stephen"))).vertexProperties(has("startTime", P.gt(2005))).create());
         checkResults(Arrays.asList("baltimore", "oakland", "seattle", 
"aachen"), sg.V().properties("location").value());
@@ -389,7 +392,6 @@ public class SubgraphStrategyProcessTest extends 
AbstractGremlinProcessTest {
         checkResults(Arrays.asList("baltimore", "oakland", "seattle"), 
sg.V().values("location"));
         //
         sg = create(SubgraphStrategy.build().vertices(has("name", 
P.eq("matthias"))).vertexProperties(has("startTime", P.gte(2014))).create());
-        System.out.println(sg.V().groupCount().by("location").explain());
         checkResults(makeMapList(1, "seattle", 1L), 
sg.V().groupCount().by("location"));
         //
         sg = 
create(SubgraphStrategy.build().vertices(has("location")).vertexProperties(hasNot("endTime")).create());

Reply via email to