From 4f723eed82294b95f8409db5cc890f9f41ed84ae Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Mon, 19 Sep 2016 13:52:33 -0600 Subject: [PATCH 01/22] Added support for SubgraphStrategy.vertexProperties(). Added some test cases to verify proper functioning. Also, cleaned up Stream-stuff in SubgraphStrategy. Going to do some more cleanup there to make things clean and efficient. --- .../strategy/decoration/SubgraphStrategy.java | 107 ++++++++++++++---- .../SubgraphStrategyProcessTest.java | 25 +++- 2 files changed, 105 insertions(+), 27 deletions(-) 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 967a19b230e..f1a42cd3b76 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 @@ -23,6 +23,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.step.TraversalParent; +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; import org.apache.tinkerpop.gremlin.process.traversal.step.map.AddEdgeStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.AddVertexStartStep; @@ -30,14 +32,19 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.map.EdgeOtherVertexStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.EdgeVertexStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.GraphStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.map.PropertiesStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.map.PropertyValueStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.VertexStep; import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversal; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper; import org.apache.tinkerpop.gremlin.structure.Direction; import org.apache.tinkerpop.gremlin.structure.Edge; import org.apache.tinkerpop.gremlin.structure.Element; import org.apache.tinkerpop.gremlin.structure.Graph; +import org.apache.tinkerpop.gremlin.structure.PropertyType; import org.apache.tinkerpop.gremlin.structure.Vertex; +import org.apache.tinkerpop.gremlin.structure.VertexProperty; import java.util.ArrayList; import java.util.List; @@ -58,9 +65,10 @@ public final class SubgraphStrategy extends AbstractTraversalStrategy vertexCriterion; private final Traversal.Admin edgeCriterion; + private final Traversal.Admin vertexPropertyCriterion; private final String MARKER = Graph.Hidden.hide(UUID.randomUUID().toString()); - private SubgraphStrategy(final Traversal vertexCriterion, final Traversal edgeCriterion) { + private SubgraphStrategy(final Traversal vertexCriterion, final Traversal edgeCriterion, final Traversal 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 @@ -79,10 +87,14 @@ private SubgraphStrategy(final Traversal vertexCriterion, final Trave this.edgeCriterion = edgeCriterion.asAdmin().addStep(new TraversalFilterStep<>(edgeCriterion.asAdmin(), vertexPredicate)); } + this.vertexPropertyCriterion = null == vertexPropertyCriterion ? null : vertexPropertyCriterion.asAdmin(); + if (null != this.vertexCriterion) this.metadataLabelStartStep(this.vertexCriterion); if (null != this.edgeCriterion) this.metadataLabelStartStep(this.edgeCriterion); + if (null != this.vertexPropertyCriterion) + this.metadataLabelStartStep(this.vertexPropertyCriterion); } private final void metadataLabelStartStep(final Traversal.Admin traversal) { @@ -123,61 +135,101 @@ public void apply(final Traversal.Admin traversal) { applyCriterion(edgeStepsToInsertFilterAfter, traversal, this.edgeCriterion); } - // explode g.V().out() to g.V().outE().inV() only if there is an edge predicate otherwise - vertexSteps.stream().filter(VertexStep::returnsVertex).forEach(step -> { + // turn g.V().out() to g.V().outE().inV() only if there is an edge predicate otherwise + for (final VertexStep step : vertexSteps) { + if (step.returnsEdge()) + continue; if (null != this.vertexCriterion && null == edgeCriterion) { - TraversalHelper.insertAfterStep(new TraversalFilterStep<>(traversal, this.vertexCriterion.clone()), step, traversal); + TraversalHelper.insertAfterStep(new TraversalFilterStep<>(traversal, (Traversal) this.vertexCriterion.clone()), step, traversal); } else { - final VertexStep someEStep = new VertexStep<>(traversal, Edge.class, step.getDirection(), step.getEdgeLabels()); - final Step someVStep = step.getDirection() == Direction.BOTH ? + final VertexStep someEStep = new VertexStep<>(traversal, Edge.class, step.getDirection(), step.getEdgeLabels()); + final Step someVStep = step.getDirection() == Direction.BOTH ? new EdgeOtherVertexStep(traversal) : new EdgeVertexStep(traversal, step.getDirection().opposite()); - // if step was labeled then propagate those labels to the new step that will return the vertex - transferLabels(step, someVStep); - - TraversalHelper.replaceStep(step, someEStep, traversal); + TraversalHelper.replaceStep((Step) step, someEStep, traversal); TraversalHelper.insertAfterStep(someVStep, someEStep, traversal); + // if step was labeled then propagate those labels to the new step that will return the vertex + for (final String label : step.getLabels()) { + step.removeLabel(label); + someVStep.addLabel(label); + } if (null != this.edgeCriterion) TraversalHelper.insertAfterStep(new TraversalFilterStep<>(traversal, this.edgeCriterion.clone()), someEStep, traversal); if (null != this.vertexCriterion) 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 wrappedCriterion = new OrStep<>(traversal, + new DefaultTraversal<>().addStep(new LambdaFilterStep<>(traversal, t -> !(t.get() instanceof VertexProperty))), + new DefaultTraversal<>().addStep(new TraversalFilterStep<>(traversal, this.vertexPropertyCriterion.clone()))); + for (final PropertiesStep step : TraversalHelper.getStepsOfAssignableClass(PropertiesStep.class, traversal)) { + if (PropertyType.PROPERTY.equals(step.getReturnType())) { + // if the property step returns a property, then simply append the criterion + final OrStep clonedWrappedCriterion = (OrStep) wrappedCriterion.clone(); + TraversalHelper.insertAfterStep(clonedWrappedCriterion, (Step) step, traversal); + for (final String label : step.getLabels()) { + step.removeLabel(label); + clonedWrappedCriterion.addLabel(label); + } + } else { + // if the property step returns value, then replace it with a property step, append criterion, then append a value() step + final Step propertiesStep = new PropertiesStep(traversal, PropertyType.PROPERTY, step.getPropertyKeys()); + TraversalHelper.replaceStep(step, propertiesStep, traversal); + final Step filterStep = wrappedCriterion.clone(); + TraversalHelper.insertAfterStep(filterStep, propertiesStep, traversal); + final Step propertyValueStep = new PropertyValueStep(traversal); + TraversalHelper.insertAfterStep(propertyValueStep, filterStep, traversal); + // add labels to the value step after the filter has been applied + for (final String label : step.getLabels()) { + propertyValueStep.addLabel(label); + } + } + } + } } + public Traversal getVertexCriterion() { - return vertexCriterion; + return this.vertexCriterion; } public Traversal getEdgeCriterion() { - return edgeCriterion; + return this.edgeCriterion; } + public Traversal getVertexPropertyCriterion() { + return this.vertexPropertyCriterion; + } + + public static Builder build() { return new Builder(); } private void applyCriterion(final List stepsToApplyCriterionAfter, final Traversal.Admin traversal, final Traversal.Admin criterion) { - stepsToApplyCriterionAfter.forEach(s -> { + 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()); - transferLabels(s, filter); - TraversalHelper.insertAfterStep(filter, s, traversal); - }); - } - - private static void transferLabels(final Step from, final Step to) { - from.getLabels().forEach(label -> to.addLabel((String) label)); - to.getLabels().forEach(label -> from.removeLabel((String) label)); + for (final String label : step.getLabels()) { + step.removeLabel(label); + filter.addLabel(label); + } + TraversalHelper.insertAfterStep(filter, step, traversal); + } } public final static class Builder { private Traversal vertexPredicate = null; private Traversal edgePredicate = null; + private Traversal vertexPropertyPredicate = null; private Builder() { } @@ -192,6 +244,11 @@ public Builder edges(final Traversal edgePredicate) { return this; } + public Builder vertexProperties(final Traversal vertexPropertyPredicate) { + this.vertexPropertyPredicate = vertexPropertyPredicate; + return this; + } + @Deprecated /** * @deprecated Since 3.2.2, use {@code Builder#vertices} instead. @@ -209,9 +266,9 @@ public Builder edgeCriterion(final Traversal predicate) { } public SubgraphStrategy create() { - if (null == this.edgePredicate && null == this.vertexPredicate) - throw new IllegalStateException("A subgraph must be filtered by an edge or vertex criterion"); - return new SubgraphStrategy(this.vertexPredicate, this.edgePredicate); + if (null == this.vertexPredicate && null == this.edgePredicate && null == this.vertexPropertyPredicate) + throw new IllegalStateException("A subgraph must be filtered by a vertex, edge, or vertex property criterion"); + return new SubgraphStrategy(this.vertexPredicate, this.edgePredicate, this.vertexPropertyPredicate); } } } \ No newline at end of file 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 41a730a0181..1d49d928a90 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 @@ -21,10 +21,8 @@ import org.apache.tinkerpop.gremlin.LoadGraphWith; import org.apache.tinkerpop.gremlin.process.AbstractGremlinProcessTest; import org.apache.tinkerpop.gremlin.process.GremlinProcessRunner; -import org.apache.tinkerpop.gremlin.process.IgnoreEngine; import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; -import org.apache.tinkerpop.gremlin.process.traversal.TraversalEngine; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.apache.tinkerpop.gremlin.structure.Edge; @@ -36,6 +34,7 @@ import java.util.List; import java.util.NoSuchElementException; +import static org.apache.tinkerpop.gremlin.LoadGraphWith.GraphData.CREW; import static org.apache.tinkerpop.gremlin.LoadGraphWith.GraphData.MODERN; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.both; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.bothE; @@ -366,6 +365,28 @@ public void shouldGetExcludedEdge() throws Exception { sg.E(sg.E(convertToEdgeId("marko", "knows", "vadas")).next()).next(); } + @Test + @LoadGraphWith(CREW) + public void shouldFilterVertexProperties() throws Exception { + 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")); + // 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()); + // + // + 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()); + checkResults(Arrays.asList("baltimore", "oakland", "seattle", "aachen"), sg.V().values("location")); + // + sg = create(SubgraphStrategy.build().vertices(has("name", P.not(P.within("stephen", "daniel")))).vertexProperties(has("startTime", P.gt(2005))).create()); + checkResults(Arrays.asList("baltimore", "oakland", "seattle"), sg.V().properties("location").value()); + checkResults(Arrays.asList("baltimore", "oakland", "seattle"), sg.V().values("location")); + } + + private GraphTraversalSource create(final SubgraphStrategy strategy) { return graphProvider.traversal(graph, strategy); } From d2ae1aaa8bce78d260e53dc4ecc047c78f905b16 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Tue, 20 Sep 2016 10:04:56 -0600 Subject: [PATCH 02/22] AbstractLambdaTraversals now support a bypassTraversal which allows strategies to easily change the semantics of the lambda traversal. Found a bug in TraversalVertexProgram around order() and the use of ConnectiveSteps. Added more tests to SubgraphStrategyProcessTest to test vertex properties and ordering. Added checkOrderedResult() to AbstractGremlinProcessTest which makes it easy to check ordered streams. Updated OrderTest to use this model -- much simpler. --- CHANGELOG.asciidoc | 4 + .../computer/traversal/MasterExecutor.java | 7 +- .../lambda/AbstractLambdaTraversal.java | 68 ++++++++++---- .../lambda/ElementValueTraversal.java | 12 ++- .../strategy/decoration/SubgraphStrategy.java | 16 ++++ .../process/AbstractGremlinProcessTest.java | 13 +++ .../process/traversal/step/map/OrderTest.java | 94 ++++--------------- .../SubgraphStrategyProcessTest.java | 10 ++ 8 files changed, 122 insertions(+), 102 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 7c4178d7e7e..94391b997cb 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -26,6 +26,10 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima 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` 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*) * `where()` predicate-based steps now support `by()`-modulation. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/MasterExecutor.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/MasterExecutor.java index b83b6d680d4..f81ca140a94 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/MasterExecutor.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/MasterExecutor.java @@ -20,15 +20,16 @@ package org.apache.tinkerpop.gremlin.process.computer.traversal; import org.apache.tinkerpop.gremlin.process.computer.Memory; -import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.HaltedTraverserStrategy; import org.apache.tinkerpop.gremlin.process.traversal.Path; import org.apache.tinkerpop.gremlin.process.traversal.Step; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; import org.apache.tinkerpop.gremlin.process.traversal.step.Barrier; import org.apache.tinkerpop.gremlin.process.traversal.step.LocalBarrier; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.ConnectiveStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.HasStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.RangeGlobalStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.TailGlobalStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.WherePredicateStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.IdStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.LabelStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.PropertiesStep; @@ -38,6 +39,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.map.SackStep; import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyStep; import org.apache.tinkerpop.gremlin.process.traversal.step.util.ReducingBarrierStep; +import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.HaltedTraverserStrategy; import org.apache.tinkerpop.gremlin.process.traversal.traverser.util.TraverserSet; import org.apache.tinkerpop.gremlin.process.traversal.util.PureTraversal; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalMatrix; @@ -144,6 +146,7 @@ private static boolean isLocalElement(final Step step) { return step instanceof PropertiesStep || step instanceof PropertyMapStep || step instanceof IdStep || step instanceof LabelStep || step instanceof SackStep || step instanceof PropertyKeyStep || step instanceof PropertyValueStep || - step instanceof TailGlobalStep || step instanceof RangeGlobalStep || step instanceof HasStep; + step instanceof TailGlobalStep || step instanceof RangeGlobalStep || step instanceof HasStep || + step instanceof ConnectiveStep; } } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java index 77b66756de0..b2335b9818e 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java @@ -45,73 +45,90 @@ public abstract class AbstractLambdaTraversal implements Traversal.Admin REQUIREMENTS = Collections.singleton(TraverserRequirement.OBJECT); + protected Traversal.Admin bypassTraversal = null; + + public void setBypassTraversal(final Traversal.Admin bypassTraversal) { + this.bypassTraversal = bypassTraversal; + } + + @Override public List getSteps() { - return Collections.emptyList(); + return null == this.bypassTraversal ? Collections.emptyList() : this.bypassTraversal.getSteps(); } + @Override public Bytecode getBytecode() { - return new Bytecode(); + return null == this.bypassTraversal ? new Bytecode() : this.bypassTraversal.getBytecode(); } + @Override public void reset() { - + if (null != this.bypassTraversal) + this.bypassTraversal.reset(); } @Override public Traversal.Admin addStep(final int index, final Step step) throws IllegalStateException { - return (Traversal.Admin) this; + return null == this.bypassTraversal ? (Traversal.Admin) this : this.bypassTraversal.addStep(index, step); } @Override public Traversal.Admin removeStep(final int index) throws IllegalStateException { - return (Traversal.Admin) this; + return null == this.bypassTraversal ? (Traversal.Admin) this : this.bypassTraversal.removeStep(index); } @Override public void applyStrategies() throws IllegalStateException { - + if (null != this.bypassTraversal) + this.bypassTraversal.applyStrategies(); } @Override public TraverserGenerator getTraverserGenerator() { - return B_O_TraverserGenerator.instance(); + return null == this.bypassTraversal ? B_O_TraverserGenerator.instance() : this.bypassTraversal.getTraverserGenerator(); } @Override public void setSideEffects(final TraversalSideEffects sideEffects) { - + if (null != this.bypassTraversal) + this.bypassTraversal.setSideEffects(sideEffects); } @Override public TraversalSideEffects getSideEffects() { - return EmptyTraversalSideEffects.instance(); + return null == this.bypassTraversal ? EmptyTraversalSideEffects.instance() : this.bypassTraversal.getSideEffects(); } @Override public void setStrategies(final TraversalStrategies strategies) { - + if (null != this.bypassTraversal) + this.bypassTraversal.setStrategies(strategies); } @Override public TraversalStrategies getStrategies() { - return EmptyTraversalStrategies.instance(); + return null == this.bypassTraversal ? EmptyTraversalStrategies.instance() : this.bypassTraversal.getStrategies(); } @Override public void setParent(final TraversalParent step) { - + if (null != this.bypassTraversal) + this.bypassTraversal.setParent(step); } @Override public TraversalParent getParent() { - return EmptyStep.instance(); + return null == this.bypassTraversal ? EmptyStep.instance() : this.bypassTraversal.getParent(); } @Override public Traversal.Admin clone() { try { - return (AbstractLambdaTraversal) super.clone(); + final AbstractLambdaTraversal clone = (AbstractLambdaTraversal) super.clone(); + if (null != this.bypassTraversal) + clone.bypassTraversal = this.bypassTraversal.clone(); + return clone; } catch (final CloneNotSupportedException e) { throw new IllegalStateException(e.getMessage(), e); } @@ -119,41 +136,54 @@ public Traversal.Admin clone() { @Override public E next() { + if (null != this.bypassTraversal) + return this.bypassTraversal.next(); + throw new UnsupportedOperationException("The " + this.getClass().getSimpleName() + " can only be used as a predicate traversal"); + } + + @Override + public Traverser.Admin nextTraverser() { + if (null != this.bypassTraversal) + return this.bypassTraversal.nextTraverser(); throw new UnsupportedOperationException("The " + this.getClass().getSimpleName() + " can only be used as a predicate traversal"); } @Override public boolean hasNext() { - return true; + return null == this.bypassTraversal || this.bypassTraversal.hasNext(); } @Override public void addStart(final Traverser.Admin start) { + if (null != this.bypassTraversal) + this.bypassTraversal.addStart(start); } @Override public boolean isLocked() { - return true; + return null == this.bypassTraversal || this.bypassTraversal.isLocked(); } @Override public Optional getGraph() { - return Optional.empty(); + return null == this.bypassTraversal ? Optional.empty() : this.bypassTraversal.getGraph(); } @Override public void setGraph(final Graph graph) { + if (null != this.bypassTraversal) + this.bypassTraversal.setGraph(graph); } @Override public Set getTraverserRequirements() { - return REQUIREMENTS; + return null == this.bypassTraversal ? REQUIREMENTS : this.bypassTraversal.getTraverserRequirements(); } @Override public int hashCode() { - return this.getClass().hashCode(); + return null == this.bypassTraversal ? this.getClass().hashCode() : this.bypassTraversal.hashCode(); } @Override diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ElementValueTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ElementValueTraversal.java index a38e8e2ce04..2e9b26c72f7 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ElementValueTraversal.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ElementValueTraversal.java @@ -19,6 +19,7 @@ package org.apache.tinkerpop.gremlin.process.traversal.lambda; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil; import org.apache.tinkerpop.gremlin.structure.Element; /** @@ -38,9 +39,14 @@ public V next() { return this.value; } + @Override + public boolean hasNext() { + return true; + } + @Override public void addStart(final Traverser.Admin start) { - this.value = start.get().value(this.propertyKey); + this.value = null == this.bypassTraversal ? start.get().value(this.propertyKey) : TraversalUtil.apply(start, this.bypassTraversal); } public String getPropertyKey() { @@ -49,11 +55,11 @@ public String getPropertyKey() { @Override public String toString() { - return "value(" + this.propertyKey + ')'; + return "value(" + (null == this.bypassTraversal ? this.propertyKey : this.bypassTraversal) + ')'; } @Override public int hashCode() { - return this.getClass().hashCode() ^ this.propertyKey.hashCode(); + return super.hashCode() ^ this.propertyKey.hashCode(); } } 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 f1a42cd3b76..475a3d7c60a 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 @@ -22,6 +22,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traversal; 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.LambdaFilterStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.OrStep; @@ -50,6 +51,7 @@ import java.util.List; import java.util.UUID; import java.util.stream.Collectors; +import java.util.stream.Stream; /** * This {@link TraversalStrategy} provides a way to limit the view of a {@link Traversal}. By providing @@ -168,6 +170,20 @@ public void apply(final Traversal.Admin traversal) { final OrStep wrappedCriterion = new OrStep<>(traversal, new DefaultTraversal<>().addStep(new LambdaFilterStep<>(traversal, t -> !(t.get() instanceof VertexProperty))), 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())) + if (step instanceof TraversalParent) { + Stream.concat(((TraversalParent) step).getGlobalChildren().stream(), ((TraversalParent) step).getLocalChildren().stream()) + .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); + ((ElementValueTraversal) t).setBypassTraversal(temp); + }); + } + } for (final PropertiesStep step : TraversalHelper.getStepsOfAssignableClass(PropertiesStep.class, traversal)) { if (PropertyType.PROPERTY.equals(step.getReturnType())) { // if the property step returns a property, then simply append the criterion diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/AbstractGremlinProcessTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/AbstractGremlinProcessTest.java index 201822cf308..7f685d23b5e 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/AbstractGremlinProcessTest.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/AbstractGremlinProcessTest.java @@ -102,6 +102,19 @@ public static void checkSideEffects(final TraversalSideEffects sideEffects, fina assertEquals(StringFactory.traversalSideEffectsString(sideEffects), sideEffects.toString()); } + public static void checkOrderedResults(final List expectedResults, final Traversal traversal) { + final List results = traversal.toList(); + assertFalse(traversal.hasNext()); + if (expectedResults.size() != results.size()) { + logger.error("Expected results: " + expectedResults); + logger.error("Actual results: " + results); + assertEquals("Checking result size", expectedResults.size(), results.size()); + } + for (int i = 0; i < expectedResults.size(); i++) { + assertEquals(expectedResults.get(i), results.get(i)); + } + } + public static void checkResults(final List expectedResults, final Traversal traversal) { final List results = traversal.toList(); assertFalse(traversal.hasNext()); diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/OrderTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/OrderTest.java index 453a66376e9..7935252533b 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/OrderTest.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/OrderTest.java @@ -101,7 +101,7 @@ public abstract class OrderTest extends AbstractGremlinProcessTest { public void g_V_name_order() { final Traversal traversal = get_g_V_name_order(); printTraversalForm(traversal); - assertCommon(traversal); + checkOrderedResults(Arrays.asList("josh", "lop", "marko", "peter", "ripple", "vadas"), traversal); } @Test @@ -109,14 +109,7 @@ public void g_V_name_order() { public void g_V_name_order_byXa1_b1X_byXb2_a2X() { final Traversal traversal = get_g_V_name_order_byXa1_b1X_byXb2_a2X(); printTraversalForm(traversal); - final List names = traversal.toList(); - assertEquals(names.size(), 6); - assertEquals("marko", names.get(0)); - assertEquals("vadas", names.get(1)); - assertEquals("peter", names.get(2)); - assertEquals("ripple", names.get(3)); - assertEquals("josh", names.get(4)); - assertEquals("lop", names.get(5)); + checkOrderedResults(Arrays.asList("marko", "vadas", "peter", "ripple", "josh", "lop"), traversal); } @Test @@ -124,7 +117,7 @@ public void g_V_name_order_byXa1_b1X_byXb2_a2X() { public void g_V_order_byXname_incrX_name() { final Traversal traversal = get_g_V_order_byXname_incrX_name(); printTraversalForm(traversal); - assertCommon(traversal); + checkOrderedResults(Arrays.asList("josh", "lop", "marko", "peter", "ripple", "vadas"), traversal); } @Test @@ -132,18 +125,7 @@ public void g_V_order_byXname_incrX_name() { public void g_V_order_byXnameX_name() { final Traversal traversal = get_g_V_order_byXnameX_name(); printTraversalForm(traversal); - assertCommon(traversal); - } - - private static void assertCommon(Traversal traversal) { - final List names = traversal.toList(); - assertEquals(names.size(), 6); - assertEquals("josh", names.get(0)); - assertEquals("lop", names.get(1)); - assertEquals("marko", names.get(2)); - assertEquals("peter", names.get(3)); - assertEquals("ripple", names.get(4)); - assertEquals("vadas", names.get(5)); + checkOrderedResults(Arrays.asList("josh", "lop", "marko", "peter", "ripple", "vadas"), traversal); } @Test @@ -151,15 +133,7 @@ private static void assertCommon(Traversal traversal) { public void g_V_outE_order_byXweight_decrX_weight() { final Traversal traversal = get_g_V_outE_order_byXweight_decrX_weight(); printTraversalForm(traversal); - final List weights = traversal.toList(); - assertEquals(6, weights.size()); - assertEquals(Double.valueOf(1.0d), weights.get(0)); - assertEquals(Double.valueOf(1.0d), weights.get(1)); - assertEquals(Double.valueOf(0.5d), weights.get(2)); - assertEquals(Double.valueOf(0.4d), weights.get(3)); - assertEquals(Double.valueOf(0.4d), weights.get(4)); - assertEquals(Double.valueOf(0.2d), weights.get(5)); - + checkOrderedResults(Arrays.asList(1.0d, 1.0d, 0.5d, 0.4d, 0.4d, 0.2d), traversal); } @Test @@ -167,14 +141,7 @@ public void g_V_outE_order_byXweight_decrX_weight() { public void g_V_order_byXname_a1_b1X_byXname_b2_a2X_name() { final Traversal traversal = get_g_V_order_byXname_a1_b1X_byXname_b2_a2X_name(); printTraversalForm(traversal); - final List names = traversal.toList(); - assertEquals(names.size(), 6); - assertEquals("marko", names.get(0)); - assertEquals("vadas", names.get(1)); - assertEquals("peter", names.get(2)); - assertEquals("ripple", names.get(3)); - assertEquals("josh", names.get(4)); - assertEquals("lop", names.get(5)); + checkOrderedResults(Arrays.asList("marko", "vadas", "peter", "ripple", "josh", "lop"), traversal); } @Test @@ -337,11 +304,7 @@ public void g_V_hasLabelXpersonX_fold_orderXlocalX_byXageX() { public void g_V_hasLabelXpersonX_order_byXvalueXageX__decrX_name() { final Traversal traversal = get_g_V_hasLabelXpersonX_order_byXvalueXageX__decrX_name(); printTraversalForm(traversal); - assertEquals("peter", traversal.next()); - assertEquals("josh", traversal.next()); - assertEquals("marko", traversal.next()); - assertEquals("vadas", traversal.next()); - assertFalse(traversal.hasNext()); + checkOrderedResults(Arrays.asList("peter", "josh", "marko", "vadas"), traversal); } @Test @@ -349,19 +312,10 @@ public void g_V_hasLabelXpersonX_order_byXvalueXageX__decrX_name() { public void g_V_properties_order_byXkey_decrX_key() { final Traversal traversal = get_g_V_properties_order_byXkey_decrX_key(); printTraversalForm(traversal); - assertEquals("name", traversal.next()); - assertEquals("name", traversal.next()); - assertEquals("name", traversal.next()); - assertEquals("name", traversal.next()); - assertEquals("name", traversal.next()); - assertEquals("name", traversal.next()); - assertEquals("lang", traversal.next()); - assertEquals("lang", traversal.next()); - assertEquals("age", traversal.next()); - assertEquals("age", traversal.next()); - assertEquals("age", traversal.next()); - assertEquals("age", traversal.next()); - assertFalse(traversal.hasNext()); + checkOrderedResults(Arrays.asList( + "name", "name", "name", "name", "name", "name", + "lang", "lang", + "age", "age", "age", "age"), traversal); } @Test @@ -391,14 +345,7 @@ public void g_V_hasXsong_name_OHBOYX_outXfollowedByX_outXfollowedByX_order_byXpe public void g_V_both_hasLabelXpersonX_order_byXage_decrX_limitX5X_name() { final Traversal traversal = get_g_V_both_hasLabelXpersonX_order_byXage_decrX_limitX5X_name(); printTraversalForm(traversal); - final List results = traversal.toList(); - assertEquals(5, results.size()); - assertFalse(traversal.hasNext()); - assertEquals("peter", results.get(0)); - assertEquals("josh", results.get(1)); - assertEquals("josh", results.get(2)); - assertEquals("josh", results.get(3)); - assertEquals("marko", results.get(4)); + checkOrderedResults(Arrays.asList("peter", "josh", "josh", "josh", "marko"), traversal); } @Test @@ -421,19 +368,10 @@ public void g_V_both_hasLabelXpersonX_order_byXage_decrX_name() { public void g_V_hasLabelXsongX_order_byXperfomances_decrX_byXnameX_rangeX110_120X_name() { final Traversal traversal = get_g_V_hasLabelXsongX_order_byXperfomances_decrX_byXnameX_rangeX110_120X_name(); printTraversalForm(traversal); - final List results = traversal.toList(); - assertEquals(10, results.size()); - assertEquals("WANG DANG DOODLE", results.get(0)); - assertEquals("THE ELEVEN", results.get(1)); - assertEquals("WAY TO GO HOME", results.get(2)); - assertEquals("FOOLISH HEART", results.get(3)); - assertEquals("GIMME SOME LOVING", results.get(4)); - assertEquals("DUPREES DIAMOND BLUES", results.get(5)); - assertEquals("CORRINA", results.get(6)); - assertEquals("PICASSO MOON", results.get(7)); - assertEquals("KNOCKING ON HEAVENS DOOR", results.get(8)); - assertEquals("MEMPHIS BLUES", results.get(9)); - assertFalse(traversal.hasNext()); + checkOrderedResults(Arrays.asList( + "WANG DANG DOODLE", "THE ELEVEN", "WAY TO GO HOME", "FOOLISH HEART", + "GIMME SOME LOVING", "DUPREES DIAMOND BLUES", "CORRINA", "PICASSO MOON", + "KNOCKING ON HEAVENS DOOR", "MEMPHIS BLUES"), traversal); } public static class Traversals extends OrderTest { 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 1d49d928a90..e3032819fc2 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 @@ -21,12 +21,14 @@ import org.apache.tinkerpop.gremlin.LoadGraphWith; import org.apache.tinkerpop.gremlin.process.AbstractGremlinProcessTest; import org.apache.tinkerpop.gremlin.process.GremlinProcessRunner; +import org.apache.tinkerpop.gremlin.process.traversal.Order; 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.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; @@ -39,6 +41,7 @@ import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.both; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.bothE; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.has; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.hasNot; 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; @@ -384,6 +387,13 @@ public void shouldFilterVertexProperties() throws Exception { sg = create(SubgraphStrategy.build().vertices(has("name", P.not(P.within("stephen", "daniel")))).vertexProperties(has("startTime", P.gt(2005))).create()); checkResults(Arrays.asList("baltimore", "oakland", "seattle"), sg.V().properties("location").value()); 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()); + checkOrderedResults(Arrays.asList("aachen", "purcellville", "santa fe", "seattle"), sg.V().order().by("location", Order.incr).values("location")); } From 5fc2163df107ddae830bd68f7a3926d4fcea8211 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Tue, 20 Sep 2016 15:05:12 -0600 Subject: [PATCH 03/22] 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. --- CHANGELOG.asciidoc | 1 + .../strategy/decoration/SubgraphStrategy.java | 110 ++++++++++++++---- .../SubgraphStrategyProcessTest.java | 8 +- 3 files changed, 96 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 94391b997cb..4c94aade3db 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*) 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 475a3d7c60a..de7649dbd4e 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.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.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 vertexCriterion; private final Traversal.Admin edgeCriterion; private final Traversal.Admin vertexPropertyCriterion; + + private final boolean vertexCriterionIsAllFilter; + private final boolean edgeCriterionIsAllFilter; + private final boolean vertexPropertyCriterionIsAllFilter; + + private static final Set> POSTS = Collections.singleton(ConnectiveStrategy.class); + private final String MARKER = Graph.Hidden.hide(UUID.randomUUID().toString()); private SubgraphStrategy(final Traversal vertexCriterion, final Traversal edgeCriterion, final Traversal 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 vertexPredicate = __.and( - __.inV().filter(this.vertexCriterion.clone()), - __.outV().filter(this.vertexCriterion.clone())).asAdmin(); + final Traversal.Admin vertexPredicate; + if (this.vertexCriterionIsAllFilter) { + final Traversal.Admin left = __.inV().asAdmin(); + final Traversal.Admin right = __.outV().asAdmin(); + TraversalHelper.insertTraversal(0, this.vertexCriterion.clone(), left); + TraversalHelper.insertTraversal(0, this.vertexCriterion.clone(), right); + vertexPredicate = __.and(left, right).asAdmin(); + } else + vertexPredicate = __.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 @@ private SubgraphStrategy(final Traversal vertexCriterion, final Trave 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 void apply(final Traversal.Admin traversal) { 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 void apply(final Traversal.Admin traversal) { 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 void apply(final Traversal.Admin traversal) { 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 someEStep = new VertexStep<>(traversal, Edge.class, step.getDirection(), step.getEdgeLabels()); final Step someVStep = step.getDirection() == Direction.BOTH ? @@ -158,18 +198,28 @@ public void apply(final Traversal.Admin traversal) { } 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 wrappedCriterion = new OrStep<>(traversal, + final OrStep 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 void apply(final Traversal.Admin traversal) { .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 void apply(final Traversal.Admin traversal) { } } } + // 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> applyPost() { + return POSTS; + } public Traversal getVertexCriterion() { return this.vertexCriterion; @@ -229,15 +290,24 @@ public static Builder build() { } private void applyCriterion(final List stepsToApplyCriterionAfter, final Traversal.Admin traversal, - final Traversal.Admin criterion) { + final Traversal.Admin 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 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); } } 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 e3032819fc2..c5b2d5aeb5b 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.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.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 void shouldFilterVertexProperties() throws Exception { 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 void shouldFilterVertexProperties() throws Exception { 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()); From beaf5208ddafd90277e9c5b0f28074cf0b8cb6d4 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Wed, 21 Sep 2016 11:48:33 -0600 Subject: [PATCH 04/22] added ClassFilterStep which checks the class type of a traverser object. this is an internal utility class not exposed at the GraphTraversal level. --- CHANGELOG.asciidoc | 1 + .../step/filter/ClassFilterStep.java | 54 +++++++++++++++++++ .../strategy/decoration/SubgraphStrategy.java | 5 +- 3 files changed, 57 insertions(+), 3 deletions(-) create mode 100644 gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/ClassFilterStep.java diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 4c94aade3db..edc479f9dec 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. +* Added an internal utility `ClassFilterStep` which determines if the traverser object's class is an instance of the provided class. * `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. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/ClassFilterStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/ClassFilterStep.java new file mode 100644 index 00000000000..16520057bf2 --- /dev/null +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/ClassFilterStep.java @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.tinkerpop.gremlin.process.traversal.step.filter; + +import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.structure.util.StringFactory; + +/** + * @author Marko A. Rodriguez (http://markorodriguez.com) + */ +public final class ClassFilterStep extends FilterStep { + + private final Class classFilter; + private final boolean isInstanceCheck; + + public ClassFilterStep(final Traversal.Admin traversal, final Class classFilter, final boolean isInstanceCheck) { + super(traversal); + this.classFilter = classFilter; + this.isInstanceCheck = isInstanceCheck; + } + + public boolean filter(final Traverser.Admin traverser) { + return !(this.isInstanceCheck ? + this.classFilter.isInstance(traverser.get()) : + this.classFilter.equals(traverser.get().getClass())); + + } + + public int hashCode() { + return super.hashCode() ^ this.classFilter.hashCode() + Boolean.valueOf(this.isInstanceCheck).hashCode(); + } + + public String toString() { + return StringFactory.stepString(this, this.classFilter); + } +} 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 de7649dbd4e..5e92bc19822 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,9 +24,9 @@ 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.ClassFilterStep; 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; import org.apache.tinkerpop.gremlin.process.traversal.step.map.AddEdgeStep; @@ -216,13 +216,12 @@ public void apply(final Traversal.Admin traversal) { // turn g.V().values() to g.V().properties().xxx.value()\ if (null != this.vertexPropertyCriterion) { final OrStep wrappedCriterion = new OrStep(traversal, - new DefaultTraversal<>().addStep(new LambdaFilterStep<>(traversal, t -> !(t.get() instanceof VertexProperty))), + new DefaultTraversal<>().addStep(new ClassFilterStep<>(traversal, VertexProperty.class, true)), 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())) if (step instanceof TraversalParent) { Stream.concat(((TraversalParent) step).getGlobalChildren().stream(), ((TraversalParent) step).getLocalChildren().stream()) .filter(t -> t instanceof ElementValueTraversal) From b5ec675062ac3f1a7ea5f7af5ff12e51ffc94ead Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Wed, 21 Sep 2016 14:11:47 -0600 Subject: [PATCH 05/22] PropertyMapStep now supports a propertyTraversal for accessing properties from the element. ConnectiveStrategy will concatenate two AND traversals if they are both filter based. SubgraphStrategy now handles valueMap() and propertyMap() correctly. Added TraversalHelper.isAllFilters() to check if a traversal is completely filter-based. --- CHANGELOG.asciidoc | 4 +- .../lambda/AbstractLambdaTraversal.java | 4 +- .../step/filter/ClassFilterStep.java | 15 ++- .../traversal/step/map/PropertyMapStep.java | 97 ++++++++++++++----- .../decoration/ConnectiveStrategy.java | 13 ++- .../strategy/decoration/SubgraphStrategy.java | 31 +++--- .../traversal/util/TraversalHelper.java | 11 ++- .../SubgraphStrategyProcessTest.java | 9 ++ 8 files changed, 130 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index edc479f9dec..6233f73963b 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -28,8 +28,10 @@ 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. +* `AbstractLambdaTraversal` now supports a `bypassTraversal` where it is possible for strategies to redefine such lambda traversals. * Added an internal utility `ClassFilterStep` which determines if the traverser object's class is an instance of the provided class. +* `ConnectiveStrategy` will concatenate two traversals instead of and'ing them if they are both filter-based. +* `PropertyMapStep` supports a provided traversal for accessing the properties of the element. * `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. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java index b2335b9818e..8f910a02177 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/AbstractLambdaTraversal.java @@ -113,8 +113,10 @@ public TraversalStrategies getStrategies() { @Override public void setParent(final TraversalParent step) { - if (null != this.bypassTraversal) + if (null != this.bypassTraversal) { this.bypassTraversal.setParent(step); + step.integrateChild(this.bypassTraversal); + } } @Override diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/ClassFilterStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/ClassFilterStep.java index 16520057bf2..13f2aacf806 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/ClassFilterStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/ClassFilterStep.java @@ -29,26 +29,23 @@ public final class ClassFilterStep extends FilterStep { private final Class classFilter; - private final boolean isInstanceCheck; + private final boolean allowClasses; - public ClassFilterStep(final Traversal.Admin traversal, final Class classFilter, final boolean isInstanceCheck) { + public ClassFilterStep(final Traversal.Admin traversal, final Class classFilter, final boolean allowClasses) { super(traversal); this.classFilter = classFilter; - this.isInstanceCheck = isInstanceCheck; + this.allowClasses = allowClasses; } public boolean filter(final Traverser.Admin traverser) { - return !(this.isInstanceCheck ? - this.classFilter.isInstance(traverser.get()) : - this.classFilter.equals(traverser.get().getClass())); - + return this.allowClasses == this.classFilter.isInstance(traverser.get()); } public int hashCode() { - return super.hashCode() ^ this.classFilter.hashCode() + Boolean.valueOf(this.isInstanceCheck).hashCode(); + return super.hashCode() ^ this.classFilter.hashCode() ^ Boolean.hashCode(this.allowClasses); } public String toString() { - return StringFactory.stepString(this, this.classFilter); + return StringFactory.stepString(this, this.classFilter.getSimpleName()); } } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertyMapStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertyMapStep.java index 8e4e11064f9..d10b12b14bd 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertyMapStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertyMapStep.java @@ -20,56 +20,84 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; -import org.apache.tinkerpop.gremlin.structure.*; -import org.apache.tinkerpop.gremlin.structure.util.ElementHelper; +import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil; +import org.apache.tinkerpop.gremlin.structure.Element; +import org.apache.tinkerpop.gremlin.structure.Property; +import org.apache.tinkerpop.gremlin.structure.PropertyType; +import org.apache.tinkerpop.gremlin.structure.T; +import org.apache.tinkerpop.gremlin.structure.Vertex; +import org.apache.tinkerpop.gremlin.structure.VertexProperty; import org.apache.tinkerpop.gremlin.structure.util.StringFactory; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; +import java.util.Iterator; +import java.util.List; import java.util.Map; import java.util.Set; /** * @author Marko A. Rodriguez (http://markorodriguez.com) */ -public class PropertyMapStep extends MapStep> { +public class PropertyMapStep extends MapStep> implements TraversalParent { protected final String[] propertyKeys; protected final PropertyType returnType; protected final boolean includeTokens; + protected Traversal.Admin propertyTraversal; public PropertyMapStep(final Traversal.Admin traversal, final boolean includeTokens, final PropertyType propertyType, final String... propertyKeys) { super(traversal); this.includeTokens = includeTokens; this.propertyKeys = propertyKeys; this.returnType = propertyType; + this.propertyTraversal = null; } @Override protected Map map(final Traverser.Admin traverser) { - if (this.returnType.equals(PropertyType.VALUE)) { - final Element element = traverser.get(); - final Map map = traverser.get() instanceof Vertex ? - (Map) ElementHelper.vertexPropertyValueMap((Vertex) element, propertyKeys) : - (Map) ElementHelper.propertyValueMap(element, propertyKeys); - if (includeTokens) { - if (element instanceof VertexProperty) { - map.put(T.id, element.id()); - map.put(T.key, ((VertexProperty) element).key()); - map.put(T.value, ((VertexProperty) element).value()); - } else { - map.put(T.id, element.id()); - map.put(T.label, element.label()); + final Map map = new HashMap<>(); + final Element element = traverser.get(); + final boolean isVertex = traverser.get() instanceof Vertex; + final Iterator properties = null == this.propertyTraversal ? + (Iterator) element.properties(this.propertyKeys) : + TraversalUtil.applyAll(traverser, this.propertyTraversal); + while (properties.hasNext()) { + final Property property = properties.next(); + if (isVertex) { + List values = (List) map.get(property.key()); + if (null == values) { + values = new ArrayList(); + map.put(property.key(), values); } + values.add(this.returnType == PropertyType.VALUE ? property.value() : property); + } else + map.put(property.key(), this.returnType == PropertyType.VALUE ? property.value() : property); + } + if (this.returnType == PropertyType.VALUE && this.includeTokens) { + if (element instanceof VertexProperty) { + map.put(T.id, element.id()); + map.put(T.key, ((VertexProperty) element).key()); + map.put(T.value, ((VertexProperty) element).value()); + } else { + map.put(T.id, element.id()); + map.put(T.label, element.label()); } - return map; - - } else { - return traverser.get() instanceof Vertex ? - (Map) ElementHelper.vertexPropertyMap((Vertex) traverser.get(), propertyKeys) : - (Map) ElementHelper.propertyMap(traverser.get(), propertyKeys); } + return (Map) map; + } + + @Override + public List> getLocalChildren() { + return null == this.propertyTraversal ? Collections.emptyList() : Collections.singletonList(this.propertyTraversal); + } + + public void setPropertyTraversal(final Traversal.Admin propertyTraversal) { + this.propertyTraversal = this.integrateChild(propertyTraversal); } public PropertyType getReturnType() { @@ -88,17 +116,36 @@ public String toString() { return StringFactory.stepString(this, Arrays.asList(this.propertyKeys), this.returnType.name().toLowerCase()); } + @Override + public PropertyMapStep clone() { + final PropertyMapStep clone = (PropertyMapStep) super.clone(); + if (null != this.propertyTraversal) + clone.propertyTraversal = this.propertyTraversal.clone(); + return clone; + } + @Override public int hashCode() { int result = super.hashCode() ^ this.returnType.hashCode() ^ Boolean.hashCode(this.includeTokens); - for (final String propertyKey : this.propertyKeys) { - result ^= propertyKey.hashCode(); + if (null == this.propertyTraversal) { + for (final String propertyKey : this.propertyKeys) { + result ^= propertyKey.hashCode(); + } + } else { + result ^= this.propertyTraversal.hashCode(); } return result; } + @Override + public void setTraversal(final Traversal.Admin parentTraversal) { + super.setTraversal(parentTraversal); + if (null != this.propertyTraversal) + this.integrateChild(this.propertyTraversal); + } + @Override public Set getRequirements() { - return Collections.singleton(TraverserRequirement.OBJECT); + return this.getSelfAndChildRequirements(TraverserRequirement.OBJECT); } } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/ConnectiveStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/ConnectiveStrategy.java index 7ec17426ef3..8627a70a6d4 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/ConnectiveStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/ConnectiveStrategy.java @@ -104,10 +104,15 @@ private static void processConjunctionMarker(final Class traversal) { // turn g.V().values() to g.V().properties().xxx.value()\ if (null != this.vertexPropertyCriterion) { final OrStep wrappedCriterion = new OrStep(traversal, - new DefaultTraversal<>().addStep(new ClassFilterStep<>(traversal, VertexProperty.class, true)), + new DefaultTraversal<>().addStep(new ClassFilterStep<>(traversal, VertexProperty.class, false)), 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()) { if (step instanceof TraversalParent) { - Stream.concat(((TraversalParent) step).getGlobalChildren().stream(), ((TraversalParent) step).getLocalChildren().stream()) - .filter(t -> t instanceof ElementValueTraversal) - .forEach(t -> { - final Traversal.Admin temp = new DefaultTraversal<>(); - 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); - }); + if (step instanceof PropertyMapStep) { + final Traversal.Admin temp = new DefaultTraversal<>(); + temp.addStep(new PropertiesStep<>(temp, PropertyType.PROPERTY, ((PropertyMapStep) step).getPropertyKeys())); + temp.addStep(wrappedCriterion.clone()); + ((PropertyMapStep) step).setPropertyTraversal(temp); + } else { + Stream.concat(((TraversalParent) step).getGlobalChildren().stream(), ((TraversalParent) step).getLocalChildren().stream()) + .filter(t -> t instanceof ElementValueTraversal) + .forEach(t -> { + final Traversal.Admin temp = new DefaultTraversal<>(); + 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); + }); + } } } for (final PropertiesStep step : TraversalHelper.getStepsOfAssignableClass(PropertiesStep.class, traversal)) { 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 14c0b5f128e..2000a928079 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 @@ -22,8 +22,6 @@ import org.apache.tinkerpop.gremlin.process.traversal.Scope; import org.apache.tinkerpop.gremlin.process.traversal.Step; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; -import org.apache.tinkerpop.gremlin.process.traversal.TraversalSource; -import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.lambda.ElementValueTraversal; import org.apache.tinkerpop.gremlin.process.traversal.lambda.TokenTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.ByModulating; @@ -32,6 +30,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.step.branch.RepeatStep; 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.NotStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.WherePredicateStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.WhereTraversalStep; @@ -571,4 +570,12 @@ public static void removeAllSteps(final Traversal.Admin traversal) { traversal.removeStep(0); } } + + public static boolean filterOnlyTraversal(final Traversal.Admin traversal) { + for (final Step step : traversal.getSteps()) { + if (!(step instanceof FilterStep) && !(step instanceof ConnectiveStep)) + return false; + } + return true; + } } 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 c5b2d5aeb5b..fe50d209eeb 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 @@ -28,6 +28,7 @@ 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.Column; import org.apache.tinkerpop.gremlin.structure.Edge; import org.apache.tinkerpop.gremlin.structure.Vertex; import org.junit.Test; @@ -396,6 +397,14 @@ public void shouldFilterVertexProperties() throws Exception { // sg = create(SubgraphStrategy.build().vertices(has("location")).vertexProperties(hasNot("endTime")).create()); checkOrderedResults(Arrays.asList("aachen", "purcellville", "santa fe", "seattle"), sg.V().order().by("location", Order.incr).values("location")); + // + sg = create(SubgraphStrategy.build().vertices(has("location")).vertexProperties(hasNot("endTime")).create()); + checkResults(Arrays.asList("aachen", "purcellville", "santa fe", "seattle"), sg.V().valueMap("location").select(Column.values).unfold().unfold()); + checkResults(Arrays.asList("aachen", "purcellville", "santa fe", "seattle"), sg.V().propertyMap("location").select(Column.values).unfold().unfold().value()); + // + sg = create(SubgraphStrategy.build().edges(__.hasLabel("uses").has("skill", 5)).create()); + checkResults(Arrays.asList(5, 5, 5), sg.V().outE().valueMap().select(Column.values).unfold()); + checkResults(Arrays.asList(5, 5, 5), sg.V().outE().propertyMap().select(Column.values).unfold().value()); } From 226d7a90a01ef32c90aa29aaa4c54c2c846f8835 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Wed, 21 Sep 2016 15:06:12 -0600 Subject: [PATCH 06/22] SubgraphStrategy is smart about trying to determine whether the property is a VertexProperty or a stanard Property. If the former, then no OrStep wrap is needed and the filter can be inlined and thus, likely that the graph database optimizers will use vertex-centric indices. --- .../strategy/decoration/SubgraphStrategy.java | 122 +++++++++++++----- 1 file changed, 87 insertions(+), 35 deletions(-) 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 d34f142e769..a49d98aceda 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 @@ -39,6 +39,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.map.PropertyMapStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.PropertyValueStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.VertexStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyStep; import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversal; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper; @@ -147,6 +148,24 @@ private final void metadataLabelStartStep(final Traversal.Admin traversal) } } + private static final char processesPropertyType(Step step) { + while (!(step instanceof EmptyStep)) { + if (step instanceof FilterStep || step instanceof ConnectiveStep) + step = step.getPreviousStep(); + else if (step instanceof GraphStep && ((GraphStep) step).returnsVertex()) + return 'v'; + else if (step instanceof EdgeVertexStep) + return 'v'; + else if (step instanceof VertexStep) + return ((VertexStep) step).returnsVertex() ? 'v' : 'p'; + else if (step instanceof PropertyMapStep || step instanceof PropertiesStep) + return 'p'; + else + return 'x'; + } + return 'x'; + } + @Override public void apply(final Traversal.Admin traversal) { // do not apply subgraph strategy to already created subgraph filter branches (or else you get infinite recursion) @@ -214,62 +233,95 @@ public void apply(final Traversal.Admin traversal) { } // turn g.V().properties() to g.V().properties().xxx - // turn g.V().values() to g.V().properties().xxx.value()\ + // turn g.V().values() to g.V().properties().xxx.value() if (null != this.vertexPropertyCriterion) { - final OrStep wrappedCriterion = new OrStep(traversal, + final OrStep checkPropertyCriterion = new OrStep(traversal, new DefaultTraversal<>().addStep(new ClassFilterStep<>(traversal, VertexProperty.class, false)), this.vertexPropertyCriterionIsAllFilter ? this.vertexPropertyCriterion.clone() : new DefaultTraversal<>().addStep(new TraversalFilterStep<>(traversal, this.vertexPropertyCriterion.clone()))); + final Traversal.Admin nonCheckPropertyCriterion = 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()) { if (step instanceof TraversalParent) { if (step instanceof PropertyMapStep) { - final Traversal.Admin temp = new DefaultTraversal<>(); - temp.addStep(new PropertiesStep<>(temp, PropertyType.PROPERTY, ((PropertyMapStep) step).getPropertyKeys())); - temp.addStep(wrappedCriterion.clone()); - ((PropertyMapStep) step).setPropertyTraversal(temp); + final char propertyType = processesPropertyType(step.getPreviousStep()); + if ('p' != propertyType) { + final Traversal.Admin temp = new DefaultTraversal<>(); + temp.addStep(new PropertiesStep<>(temp, PropertyType.PROPERTY, ((PropertyMapStep) step).getPropertyKeys())); + if ('v' == propertyType) + TraversalHelper.insertTraversal(0, nonCheckPropertyCriterion.clone(), temp); + else + temp.addStep(checkPropertyCriterion.clone()); + ((PropertyMapStep) step).setPropertyTraversal(temp); + } } else { Stream.concat(((TraversalParent) step).getGlobalChildren().stream(), ((TraversalParent) step).getLocalChildren().stream()) .filter(t -> t instanceof ElementValueTraversal) .forEach(t -> { - final Traversal.Admin temp = new DefaultTraversal<>(); - 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); + final char propertyType = processesPropertyType(step.getPreviousStep()); + if ('p' != propertyType) { + final Traversal.Admin temp = new DefaultTraversal<>(); + temp.addStep(new PropertiesStep<>(temp, PropertyType.PROPERTY, ((ElementValueTraversal) t).getPropertyKey())); + if ('v' == propertyType) + TraversalHelper.insertTraversal(0, nonCheckPropertyCriterion.clone(), temp); + else + temp.addStep(checkPropertyCriterion.clone()); + temp.addStep(new PropertyValueStep<>(temp)); + temp.setParent((TraversalParent) step); + ((ElementValueTraversal) t).setBypassTraversal(temp); + } }); } } } for (final PropertiesStep step : TraversalHelper.getStepsOfAssignableClass(PropertiesStep.class, traversal)) { - if (PropertyType.PROPERTY.equals(step.getReturnType())) { - // if the property step returns a property, then simply append the criterion - final OrStep clonedWrappedCriterion = (OrStep) wrappedCriterion.clone(); - TraversalHelper.insertAfterStep(clonedWrappedCriterion, (Step) step, traversal); - for (final String label : step.getLabels()) { - step.removeLabel(label); - clonedWrappedCriterion.addLabel(label); - } - } else { - // if the property step returns value, then replace it with a property step, append criterion, then append a value() step - final Step propertiesStep = new PropertiesStep(traversal, PropertyType.PROPERTY, step.getPropertyKeys()); - TraversalHelper.replaceStep(step, propertiesStep, traversal); - final Step filterStep = wrappedCriterion.clone(); - TraversalHelper.insertAfterStep(filterStep, propertiesStep, traversal); - final Step propertyValueStep = new PropertyValueStep(traversal); - TraversalHelper.insertAfterStep(propertyValueStep, filterStep, traversal); - // add labels to the value step after the filter has been applied - for (final String label : step.getLabels()) { - propertyValueStep.addLabel(label); + final char propertyType = processesPropertyType(step.getPreviousStep()); + if ('p' != propertyType) { + if (PropertyType.PROPERTY == ((PropertiesStep) step).getReturnType()) { + // if the property step returns a property, then simply append the criterion + if ('v' == propertyType) { + final Traversal.Admin temp = nonCheckPropertyCriterion.clone(); + TraversalHelper.insertTraversal((Step) step, temp, traversal); + for (final String label : step.getLabels()) { + step.removeLabel(label); + temp.getEndStep().addLabel(label); + } + } else { + final Step temp = checkPropertyCriterion.clone(); + TraversalHelper.insertAfterStep(temp, (Step) step, traversal); + for (final String label : step.getLabels()) { + step.removeLabel(label); + temp.addLabel(label); + } + } + } else { + // if the property step returns value, then replace it with a property step, append criterion, then append a value() step + final Step propertiesStep = new PropertiesStep(traversal, PropertyType.PROPERTY, ((PropertiesStep) step).getPropertyKeys()); + TraversalHelper.replaceStep(step, propertiesStep, traversal); + final Step propertyValueStep = new PropertyValueStep(traversal); + for (final String label : step.getLabels()) { + propertyValueStep.addLabel(label); + } + if ('v' == propertyType) { + final Traversal.Admin temp = nonCheckPropertyCriterion.clone(); + TraversalHelper.insertAfterStep(propertyValueStep, propertiesStep, traversal); + TraversalHelper.insertTraversal(propertiesStep, temp, traversal); + } else { + final Step filterStep = checkPropertyCriterion.clone(); + TraversalHelper.insertAfterStep(filterStep, propertiesStep, traversal); + TraversalHelper.insertAfterStep(propertyValueStep, filterStep, traversal); + } } } } - } - // when there is no filter()-wrap, the marked steps exist at the same traversal level - for (final Step step : traversal.getSteps()) { - step.removeLabel(MARKER); + // when there is no filter()-wrap, the marked steps exist at the same traversal level + for (final Step step : traversal.getSteps()) { + step.removeLabel(MARKER); + } } } From 0bbf6a3efb74dd4ea2e3e4384bd7ab3e22e9b9b3 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Wed, 21 Sep 2016 15:08:39 -0600 Subject: [PATCH 07/22] a loop got fuggled. --- .../traversal/strategy/decoration/SubgraphStrategy.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 a49d98aceda..7551d1413aa 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 @@ -318,10 +318,10 @@ public void apply(final Traversal.Admin traversal) { } } } - // when there is no filter()-wrap, the marked steps exist at the same traversal level - for (final Step step : traversal.getSteps()) { - step.removeLabel(MARKER); - } + } + // when there is no filter()-wrap, the marked steps exist at the same traversal level + for (final Step step : traversal.getSteps()) { + step.removeLabel(MARKER); } } From 1f70b434b459c088c4be20d1bf9b126e31eda72c Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Wed, 21 Sep 2016 15:18:48 -0600 Subject: [PATCH 08/22] added more test cases to SubgraphStrategyProcessTest and cleanedup SubgraphStrategy a bit. --- .../traversal/strategy/decoration/SubgraphStrategy.java | 8 +++----- .../strategy/decoration/SubgraphStrategyProcessTest.java | 6 ++++++ 2 files changed, 9 insertions(+), 5 deletions(-) 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 7551d1413aa..d9612096692 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 @@ -307,13 +307,11 @@ public void apply(final Traversal.Admin traversal) { propertyValueStep.addLabel(label); } if ('v' == propertyType) { - final Traversal.Admin temp = nonCheckPropertyCriterion.clone(); TraversalHelper.insertAfterStep(propertyValueStep, propertiesStep, traversal); - TraversalHelper.insertTraversal(propertiesStep, temp, traversal); + TraversalHelper.insertTraversal(propertiesStep, nonCheckPropertyCriterion.clone(), traversal); } else { - final Step filterStep = checkPropertyCriterion.clone(); - TraversalHelper.insertAfterStep(filterStep, propertiesStep, traversal); - TraversalHelper.insertAfterStep(propertyValueStep, filterStep, traversal); + TraversalHelper.insertAfterStep(propertyValueStep, propertiesStep, traversal); + TraversalHelper.insertAfterStep(checkPropertyCriterion.clone(), propertiesStep, traversal); } } } 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 fe50d209eeb..f61aedbd2d3 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 @@ -30,6 +30,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper; import org.apache.tinkerpop.gremlin.structure.Column; import org.apache.tinkerpop.gremlin.structure.Edge; +import org.apache.tinkerpop.gremlin.structure.T; import org.apache.tinkerpop.gremlin.structure.Vertex; import org.junit.Test; import org.junit.runner.RunWith; @@ -405,6 +406,11 @@ public void shouldFilterVertexProperties() throws Exception { sg = create(SubgraphStrategy.build().edges(__.hasLabel("uses").has("skill", 5)).create()); checkResults(Arrays.asList(5, 5, 5), sg.V().outE().valueMap().select(Column.values).unfold()); checkResults(Arrays.asList(5, 5, 5), sg.V().outE().propertyMap().select(Column.values).unfold().value()); + // + sg = create(SubgraphStrategy.build().vertexProperties(__.hasNot("skill")).create()); + checkResults(Arrays.asList(3, 3, 3, 4, 4, 5, 5, 5), sg.V().outE("uses").values("skill")); + checkResults(Arrays.asList(3, 3, 3, 4, 4, 5, 5, 5), sg.V().as("a").properties().select("a").dedup().outE().values("skill")); + checkResults(Arrays.asList(3, 3, 3, 4, 4, 5, 5, 5), sg.V().as("a").properties().select("a").outE().properties("skill").as("b").dedup().select("b").by(__.value())); } From 23a4e86b2992f507fdb1536060b9344c9a09d188 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Thu, 22 Sep 2016 13:45:33 -0600 Subject: [PATCH 09/22] there is a bug in StarGraph property attachment/detachment that caused a contrived test case in SubgraphStrategyProcessTest to fail. I got the same effect with another traversal and have logged the StarGraph bug in JIRA. --- .../strategy/decoration/SubgraphStrategyProcessTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 f61aedbd2d3..29910a7a3c5 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 @@ -410,7 +410,7 @@ public void shouldFilterVertexProperties() throws Exception { sg = create(SubgraphStrategy.build().vertexProperties(__.hasNot("skill")).create()); checkResults(Arrays.asList(3, 3, 3, 4, 4, 5, 5, 5), sg.V().outE("uses").values("skill")); checkResults(Arrays.asList(3, 3, 3, 4, 4, 5, 5, 5), sg.V().as("a").properties().select("a").dedup().outE().values("skill")); - checkResults(Arrays.asList(3, 3, 3, 4, 4, 5, 5, 5), sg.V().as("a").properties().select("a").outE().properties("skill").as("b").dedup().select("b").by(__.value())); + checkResults(Arrays.asList(3, 3, 3, 4, 4, 5, 5, 5), sg.V().as("a").properties().select("a").dedup().outE().properties("skill").as("b").identity().select("b").by(__.value())); } From 37f0606c29533555b699ea34c0a8aed402e2e0cb Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Fri, 23 Sep 2016 12:11:46 -0600 Subject: [PATCH 10/22] added a better example to SugraphStategy in the reference docs. Also added an explain() so people can see whats going on. --- docs/src/reference/the-traversal.asciidoc | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/docs/src/reference/the-traversal.asciidoc b/docs/src/reference/the-traversal.asciidoc index e8d40e502bc..2fafbd96bbc 100644 --- a/docs/src/reference/the-traversal.asciidoc +++ b/docs/src/reference/the-traversal.asciidoc @@ -2599,17 +2599,25 @@ ReadOnlyStrategy SubgraphStrategy ~~~~~~~~~~~~~~~~ -`SubgraphStrategy` is quite similar to `PartitionStrategy` in that it restrains a `Traversal` to certain vertices -and edges as determined by a `Traversal` criterion defined individually for each. +`SubgraphStrategy` is similar to `PartitionStrategy` in that it constrains a `Traversal` to certain vertices, edges, + and vertex properties as determined by a `Traversal`-based criterion defined individually for each. [gremlin-groovy] ---- -graph = TinkerFactory.createModern() -strategy = SubgraphStrategy.build().edgeCriterion(hasId(8,9,10)).create() -g = graph.traversal().withStrategies(strategy) -g.V() // shows all vertices as no filter for vertices was specified -g.E() // shows only the edges defined in the edgeCriterion +graph = TinkerFactory.createTheCrew() +g = graph.traversal() +g.V().as('a').values('location').as('b'). <1> + select('a','b').by('name').by() +g = g.withStrategies(SubgraphStrategy.build().vertexProperties(hasNot('endTime')).create()) <2> +g.V().as('a').values('location').as('b'). <3> + select('a','b').by('name').by() +g.V().as('a').values('location').as('b'). + select('a','b').by('name').by().explain() ---- This strategy is implemented such that the vertices attached to an `Edge` must both satisfy the `vertexCriterion` (if present) in order for the `Edge` to be considered a part of the subgraph. + +<1> Get all vertices and their vertex property locations. +<2> Create a `SubgraphStrategy` where vertex properties must not have an `endTime`-property (thus, the current location). +<3> Get all vertices and their current vertex property locations. \ No newline at end of file From dabeb02deb00b85d23ca8b70bfcf7f6ea8ec2ed1 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Fri, 23 Sep 2016 16:03:37 -0600 Subject: [PATCH 11/22] added lots of good stuff that all revolves around SubgraphStategy. InlineFilterStrategy tries to inline filters. Epic. And/OrStep are now FilterSteps - epic. Lots of cleanup and simplification of SubgraphStrategy cause of it. --- CHANGELOG.asciidoc | 8 +- docs/src/reference/the-traversal.asciidoc | 21 +++- .../traversal/TraversalStrategies.java | 3 +- .../traversal/step/filter/AndStep.java | 19 +--- .../traversal/step/filter/ConnectiveStep.java | 2 +- .../traversal/step/filter/NotStep.java | 2 +- .../process/traversal/step/filter/OrStep.java | 14 +-- .../decoration/ConnectiveStrategy.java | 11 +- .../strategy/decoration/SubgraphStrategy.java | 102 ++++-------------- .../optimization/InlineFilterStrategy.java | 100 +++++++++++++++++ .../StandardVerificationStrategy.java | 18 +++- .../traversal/util/TraversalHelper.java | 16 ++- .../decoration/SubgraphStrategyTest.java | 67 +++++++++++- .../InlineFilterStrategyTest.java | 78 ++++++++++++++ .../SubgraphStrategyProcessTest.java | 7 +- .../TinkerGraphGroovyTranslatorProvider.java | 8 ++ 16 files changed, 346 insertions(+), 130 deletions(-) create mode 100644 gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategy.java create mode 100644 gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategyTest.java diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 6233f73963b..0c459e86a1d 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -26,13 +26,17 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima TinkerPop 3.2.3 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Added `TraversalHelper.applySingleLevelStrategies()` which will apply a subset of strategies but not walk the child tree. +* Added the concept that hidden labels using during traversal compilation are removed at the end during `StandardVerificationStrategy`. (*breaking*) +* Added `InlineFilterStrategy` which will determine if a `TraversalFilterStep` or `AndStep` children are filters and if so, inline them. +* Removed `IdentityRemovalStrategy` from the default listing as its not worth the clock cycles. +* Removed the "!" symbol in `NotStep.toString()` as it is confusing and the `NotStep`-name is sufficient. * 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` where it is possible for strategies to redefine such lambda traversals. * Added an internal utility `ClassFilterStep` which determines if the traverser object's class is an instance of the provided class. -* `ConnectiveStrategy` will concatenate two traversals instead of and'ing them if they are both filter-based. +* `ConnectiveStep` extends `FilterStep` and thus, is more appropriately categorized in the step hierarchy. * `PropertyMapStep` supports a provided traversal for accessing the properties of the element. -* `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*) diff --git a/docs/src/reference/the-traversal.asciidoc b/docs/src/reference/the-traversal.asciidoc index 2fafbd96bbc..8fd6e480cc0 100644 --- a/docs/src/reference/the-traversal.asciidoc +++ b/docs/src/reference/the-traversal.asciidoc @@ -2620,4 +2620,23 @@ This strategy is implemented such that the vertices attached to an `Edge` must b <1> Get all vertices and their vertex property locations. <2> Create a `SubgraphStrategy` where vertex properties must not have an `endTime`-property (thus, the current location). -<3> Get all vertices and their current vertex property locations. \ No newline at end of file +<3> Get all vertices and their current vertex property locations. + +The example below uses all three filters: vertex, edge, and vertex property. People vertices must have lived in more than three places, +edges must be labeled "develops," and vertex properties must be the persons current location or a non-location property. + +[gremlin-groovy] +---- +graph = TinkerFactory.createTheCrew() +g = graph.traversal().withStrategies(SubgraphStrategy.build(). + vertices(or(hasNot('location'),properties('location').count().is(gt(3)))). + edges(hasLabel('develops')). + vertexProperties(or(hasLabel(neq('location')),hasNot('endTime'))).create()) +g.V().valueMap(true) +g.E().valueMap(true) +g.V().outE().inV(). + path(). + by('name'). + by(). + by('name') +---- \ No newline at end of file diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalStrategies.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalStrategies.java index 4b898330123..a01eef6d074 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalStrategies.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/TraversalStrategies.java @@ -26,6 +26,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.FilterRankingStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.IdentityRemovalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.IncidentToAdjacentStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.InlineFilterStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.MatchPredicateStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.OrderLimitStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.PathProcessorStrategy; @@ -204,8 +205,8 @@ public static final class GlobalCache { ConnectiveStrategy.instance(), IncidentToAdjacentStrategy.instance(), AdjacentToIncidentStrategy.instance(), + InlineFilterStrategy.instance(), FilterRankingStrategy.instance(), - IdentityRemovalStrategy.instance(), MatchPredicateStrategy.instance(), RepeatUnrollStrategy.instance(), RangeByIsCountStrategy.instance(), diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AndStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AndStep.java index 9bc6f4dae7e..5d9d1249575 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AndStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AndStep.java @@ -22,8 +22,6 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traverser; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil; -import java.util.NoSuchElementException; - /** * @author Marko A. Rodriguez (http://markorodriguez.com) */ @@ -34,18 +32,11 @@ public AndStep(final Traversal.Admin traversal, final Traversal... travers } @Override - protected Traverser.Admin processNextStart() throws NoSuchElementException { - while (true) { - final Traverser.Admin start = this.starts.next(); - boolean alive = true; - for (final Traversal.Admin traversal : this.traversals) { - if (!TraversalUtil.test(start, traversal)) { - alive = false; - break; - } - } - if (alive) - return start; + protected boolean filter(final Traverser.Admin traverser) { + for (final Traversal.Admin traversal : this.traversals) { + if (!TraversalUtil.test(traverser, traversal)) + return false; } + return true; } } \ No newline at end of file diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/ConnectiveStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/ConnectiveStep.java index 6eac30be8f5..08d3b2f0a5d 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/ConnectiveStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/ConnectiveStep.java @@ -33,7 +33,7 @@ /** * @author Marko A. Rodriguez (http://markorodriguez.com) */ -public abstract class ConnectiveStep extends AbstractStep implements TraversalParent { +public abstract class ConnectiveStep extends FilterStep implements TraversalParent { public enum Connective {AND, OR} diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/NotStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/NotStep.java index 83059ce4dca..bdf9eec23fa 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/NotStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/NotStep.java @@ -64,7 +64,7 @@ public void setTraversal(final Traversal.Admin parentTraversal) { @Override public String toString() { - return StringFactory.stepString(this, "!" + this.notTraversal); + return StringFactory.stepString(this, this.notTraversal); } @Override diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/OrStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/OrStep.java index 70dc430e4e9..72866a17b89 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/OrStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/OrStep.java @@ -22,8 +22,6 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traverser; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil; -import java.util.NoSuchElementException; - /** * @author Marko A. Rodriguez (http://markorodriguez.com) */ @@ -34,13 +32,11 @@ public OrStep(final Traversal.Admin traversal, final Traversal... traversa } @Override - protected Traverser.Admin processNextStart() throws NoSuchElementException { - while (true) { - final Traverser.Admin start = this.starts.next(); - for (final Traversal.Admin traversal : this.traversals) { - if (TraversalUtil.test(start, traversal)) - return start; - } + protected boolean filter(final Traverser.Admin traverser) { + for (final Traversal.Admin traversal : this.traversals) { + if (TraversalUtil.test(traverser, traversal)) + return true; } + return false; } } \ No newline at end of file diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/ConnectiveStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/ConnectiveStrategy.java index 8627a70a6d4..754fb03a85c 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/ConnectiveStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/ConnectiveStrategy.java @@ -104,14 +104,9 @@ private static void processConjunctionMarker(final Class edgeCriterion; private final Traversal.Admin vertexPropertyCriterion; - private final boolean vertexCriterionIsAllFilter; - private final boolean edgeCriterionIsAllFilter; - private final boolean vertexPropertyCriterionIsAllFilter; - private static final Set> POSTS = Collections.singleton(ConnectiveStrategy.class); private final String MARKER = Graph.Hidden.hide(UUID.randomUUID().toString()); private SubgraphStrategy(final Traversal vertexCriterion, final Traversal edgeCriterion, final Traversal vertexPropertyCriterion) { - this.vertexCriterionIsAllFilter = isAllFilters(vertexCriterion); - this.edgeCriterionIsAllFilter = isAllFilters(edgeCriterion); - this.vertexPropertyCriterionIsAllFilter = isAllFilters(vertexPropertyCriterion); - this.vertexCriterion = null == vertexCriterion ? null : vertexCriterion.asAdmin(); @@ -96,26 +89,15 @@ private SubgraphStrategy(final Traversal vertexCriterion, final Trave this.edgeCriterion = null == edgeCriterion ? null : edgeCriterion.asAdmin(); } else { final Traversal.Admin vertexPredicate; - if (this.vertexCriterionIsAllFilter) { - final Traversal.Admin left = __.inV().asAdmin(); - final Traversal.Admin right = __.outV().asAdmin(); - TraversalHelper.insertTraversal(0, this.vertexCriterion.clone(), left); - TraversalHelper.insertTraversal(0, this.vertexCriterion.clone(), right); - vertexPredicate = __.and(left, right).asAdmin(); - } else - vertexPredicate = __.and( - __.inV().filter(this.vertexCriterion.clone()), - __.outV().filter(this.vertexCriterion.clone())).asAdmin(); + vertexPredicate = __.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 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)); + this.edgeCriterion = null == edgeCriterion ? + vertexPredicate : + edgeCriterion.asAdmin().addStep(new TraversalFilterStep<>(edgeCriterion.asAdmin(), vertexPredicate)); } this.vertexPropertyCriterion = null == vertexPropertyCriterion ? null : vertexPropertyCriterion.asAdmin(); @@ -128,16 +110,6 @@ else if (this.edgeCriterionIsAllFilter) { 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()) { @@ -150,7 +122,7 @@ private final void metadataLabelStartStep(final Traversal.Admin traversal) private static final char processesPropertyType(Step step) { while (!(step instanceof EmptyStep)) { - if (step instanceof FilterStep || step instanceof ConnectiveStep) + if (step instanceof FilterStep || step instanceof SideEffectStep) step = step.getPreviousStep(); else if (step instanceof GraphStep && ((GraphStep) step).returnsVertex()) return 'v'; @@ -169,10 +141,8 @@ else if (step instanceof PropertyMapStep || step instanceof PropertiesStep) @Override public void apply(final Traversal.Admin traversal) { // do not apply subgraph strategy to already created subgraph filter branches (or else you get infinite recursion) - if (traversal.getStartStep().getLabels().contains(MARKER)) { - traversal.getStartStep().removeLabel(MARKER); + if (traversal.getStartStep().getLabels().contains(MARKER)) return; - } // final List graphSteps = TraversalHelper.getStepsOfAssignableClass(GraphStep.class, traversal); final List vertexSteps = TraversalHelper.getStepsOfAssignableClass(VertexStep.class, traversal); @@ -183,7 +153,7 @@ public void apply(final Traversal.Admin traversal) { 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, this.vertexCriterionIsAllFilter); + applyCriterion(vertexStepsToInsertFilterAfter, traversal, this.vertexCriterion); } if (null != this.edgeCriterion) { @@ -191,7 +161,7 @@ public void apply(final Traversal.Admin traversal) { 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, this.edgeCriterionIsAllFilter); + applyCriterion(edgeStepsToInsertFilterAfter, traversal, this.edgeCriterion); } // turn g.V().out() to g.V().outE().inV() only if there is an edge predicate otherwise @@ -199,10 +169,7 @@ public void apply(final Traversal.Admin traversal) { if (step.returnsEdge()) continue; if (null != this.vertexCriterion && null == edgeCriterion) { - if (this.vertexCriterionIsAllFilter) - TraversalHelper.insertTraversal((Step) step, this.vertexCriterion.clone(), traversal); - else - TraversalHelper.insertAfterStep(new TraversalFilterStep<>(traversal, (Traversal) this.vertexCriterion.clone()), step, traversal); + TraversalHelper.insertAfterStep(new TraversalFilterStep<>(traversal, (Traversal) this.vertexCriterion.clone()), step, traversal); } else { final VertexStep someEStep = new VertexStep<>(traversal, Edge.class, step.getDirection(), step.getEdgeLabels()); final Step someVStep = step.getDirection() == Direction.BOTH ? @@ -218,15 +185,9 @@ public void apply(final Traversal.Admin traversal) { } if (null != this.edgeCriterion) - if (this.edgeCriterionIsAllFilter) - TraversalHelper.insertTraversal(someEStep, this.edgeCriterion.clone(), traversal); - else - TraversalHelper.insertAfterStep(new TraversalFilterStep<>(traversal, this.edgeCriterion.clone()), someEStep, traversal); + TraversalHelper.insertAfterStep(new TraversalFilterStep<>(traversal, this.edgeCriterion.clone()), someEStep, traversal); if (null != this.vertexCriterion) - if (this.edgeCriterionIsAllFilter) - TraversalHelper.insertTraversal(someVStep, this.vertexCriterion.clone(), traversal); - else - TraversalHelper.insertAfterStep(new TraversalFilterStep<>(traversal, this.vertexCriterion.clone()), someVStep, traversal); + TraversalHelper.insertAfterStep(new TraversalFilterStep<>(traversal, this.vertexCriterion.clone()), someVStep, traversal); } @@ -237,12 +198,8 @@ public void apply(final Traversal.Admin traversal) { if (null != this.vertexPropertyCriterion) { final OrStep checkPropertyCriterion = new OrStep(traversal, new DefaultTraversal<>().addStep(new ClassFilterStep<>(traversal, VertexProperty.class, false)), - this.vertexPropertyCriterionIsAllFilter ? - this.vertexPropertyCriterion.clone() : - new DefaultTraversal<>().addStep(new TraversalFilterStep<>(traversal, this.vertexPropertyCriterion.clone()))); - final Traversal.Admin nonCheckPropertyCriterion = this.vertexPropertyCriterionIsAllFilter ? - this.vertexPropertyCriterion.clone() : - new DefaultTraversal<>().addStep(new TraversalFilterStep<>(traversal, this.vertexPropertyCriterion.clone())); + new DefaultTraversal<>().addStep(new TraversalFilterStep<>(traversal, this.vertexPropertyCriterion.clone()))); + final Traversal.Admin nonCheckPropertyCriterion = new DefaultTraversal<>().addStep(new TraversalFilterStep<>(traversal, this.vertexPropertyCriterion.clone())); // turn all ElementValueTraversals into filters for (final Step step : traversal.getSteps()) { @@ -317,10 +274,6 @@ public void apply(final Traversal.Admin traversal) { } } } - // 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 @@ -346,24 +299,15 @@ public static Builder build() { } private void applyCriterion(final List stepsToApplyCriterionAfter, final Traversal.Admin traversal, - final Traversal.Admin criterion, final boolean isAllFilter) { + final Traversal.Admin criterion) { for (final Step step : stepsToApplyCriterionAfter) { - if (isAllFilter) { - final Traversal.Admin 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); + // 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); } } 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 new file mode 100644 index 00000000000..beeca95972b --- /dev/null +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategy.java @@ -0,0 +1,100 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization; + +import org.apache.tinkerpop.gremlin.process.computer.traversal.strategy.optimization.GraphFilterStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.Step; +import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.AndStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.FilterStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.TraversalFilterStep; +import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper; + +import java.util.Arrays; +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +/** + * @author Marko A. Rodriguez (http://markorodriguez.com) + */ +public final class InlineFilterStrategy extends AbstractTraversalStrategy implements TraversalStrategy.OptimizationStrategy { + + private static final InlineFilterStrategy INSTANCE = new InlineFilterStrategy(); + private static final Set> POSTS = new HashSet<>(Arrays.asList( + FilterRankingStrategy.class, + GraphFilterStrategy.class)); + + private InlineFilterStrategy() { + } + + @Override + public void apply(final Traversal.Admin traversal) { + boolean changed = true; // recursively walk child traversals trying to inline them into the current traversal line. + while (changed) { + changed = false; + for (final TraversalFilterStep step : TraversalHelper.getStepsOfAssignableClass(TraversalFilterStep.class, traversal)) { + final Traversal.Admin childTraversal = step.getLocalChildren().get(0); + if (TraversalHelper.allStepsInstanceOf(childTraversal, FilterStep.class, true)) { + changed = true; + TraversalHelper.applySingleLevelStrategies(childTraversal, traversal.getStrategies(), InlineFilterStrategy.class); + final Step finalStep = childTraversal.getEndStep(); + TraversalHelper.insertTraversal((Step) step, childTraversal, traversal); + traversal.removeStep(step); + for (final String label : step.getLabels()) { + finalStep.addLabel(label); + } + } + } + for (final AndStep step : TraversalHelper.getStepsOfAssignableClass(AndStep.class, traversal)) { + if (!step.getLocalChildren().stream().filter(t -> !TraversalHelper.allStepsInstanceOf(t, FilterStep.class, true)).findAny().isPresent()) { + changed = true; + final List> childTraversals = (List) step.getLocalChildren(); + Step finalStep = null; + for (int i = childTraversals.size() - 1; i >= 0; i--) { + final Traversal.Admin childTraversal = childTraversals.get(i); + TraversalHelper.applySingleLevelStrategies(childTraversal, traversal.getStrategies(), InlineFilterStrategy.class); + if (null == finalStep) + finalStep = childTraversal.getEndStep(); + TraversalHelper.insertTraversal((Step) step, childTraversals.get(i), traversal); + + } + traversal.removeStep(step); + if (null != finalStep) { + for (final String label : step.getLabels()) { + finalStep.addLabel(label); + } + } + } + } + } + } + + @Override + public Set> applyPost() { + return POSTS; + } + + public static InlineFilterStrategy instance() { + return INSTANCE; + } +} \ No newline at end of file diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/StandardVerificationStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/StandardVerificationStrategy.java index 95aa2e7c48d..4f272cc338b 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/StandardVerificationStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/StandardVerificationStrategy.java @@ -29,8 +29,10 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.util.RequirementsStep; import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper; +import org.apache.tinkerpop.gremlin.structure.Graph; import java.util.Collections; +import java.util.HashSet; import java.util.Set; /** @@ -50,20 +52,28 @@ public void apply(final Traversal.Admin traversal) { throw new VerificationException("VertexComputing steps must be executed with a GraphComputer: " + TraversalHelper.getStepsOfAssignableClass(VertexComputing.class, traversal), traversal); } - traversal.getSteps().forEach(step -> { + for (final Step step : traversal.getSteps()) { + final Set hiddenLabels = new HashSet<>(); + for (String label : step.getLabels()) { + if (Graph.Hidden.isHidden(label)) + hiddenLabels.add(label); + } + for(final String label : hiddenLabels) { + step.removeLabel(label); + } if (step instanceof ReducingBarrierStep && step.getTraversal().getParent() instanceof RepeatStep && step.getTraversal().getParent().getGlobalChildren().get(0).getSteps().contains(step)) throw new VerificationException("The parent of a reducing barrier can not be repeat()-step: " + step, traversal); - }); + } // The ProfileSideEffectStep must be the last step, 2nd last step when accompanied by the cap step, // or 3rd to last when the traversal ends with a RequirementsStep. - final Step endStep = traversal.asAdmin().getEndStep(); + final Step endStep = traversal.asAdmin().getEndStep(); if (TraversalHelper.hasStepOfClass(ProfileSideEffectStep.class, traversal) && !(endStep instanceof ProfileSideEffectStep || (endStep instanceof SideEffectCapStep && endStep.getPreviousStep() instanceof ProfileSideEffectStep) || (endStep instanceof RequirementsStep && ( endStep.getPreviousStep() instanceof SideEffectCapStep || - endStep.getPreviousStep() instanceof ProfileSideEffectStep)))) { + endStep.getPreviousStep() instanceof ProfileSideEffectStep)))) { throw new VerificationException("When specified, the profile()-Step must be the last step or followed only by the cap()-step and/or requirements step.", traversal); } 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 2000a928079..697dc2a627d 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 @@ -22,6 +22,8 @@ import org.apache.tinkerpop.gremlin.process.traversal.Scope; import org.apache.tinkerpop.gremlin.process.traversal.Step; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies; +import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.lambda.ElementValueTraversal; import org.apache.tinkerpop.gremlin.process.traversal.lambda.TokenTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.ByModulating; @@ -30,7 +32,6 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.step.branch.RepeatStep; 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.NotStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.WherePredicateStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.WhereTraversalStep; @@ -571,11 +572,20 @@ public static void removeAllSteps(final Traversal.Admin traversal) { } } - public static boolean filterOnlyTraversal(final Traversal.Admin traversal) { + public static boolean allStepsInstanceOf(final Traversal.Admin traversal, final Class classToCheck, boolean checkIsInstanceOf) { for (final Step step : traversal.getSteps()) { - if (!(step instanceof FilterStep) && !(step instanceof ConnectiveStep)) + boolean isInstance = classToCheck.isInstance(step); + if ((isInstance && !checkIsInstanceOf) || (!isInstance && checkIsInstanceOf)) return false; } return true; } + + public static void applySingleLevelStrategies(final Traversal.Admin traversal, final TraversalStrategies traversalStrategies, final Class stopAfterStrategy) { + for (final TraversalStrategy strategy : traversalStrategies.toList()) { + strategy.apply(traversal); + if (null != stopAfterStrategy && stopAfterStrategy.isInstance(strategy)) + break; + } + } } diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyTest.java index 86fdb67de9f..2e89f6b748f 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyTest.java @@ -18,8 +18,10 @@ */ package org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration; +import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Step; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.DefaultGraphTraversal; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.AndStep; @@ -27,11 +29,26 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.map.EdgeVertexStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.VertexStep; import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.IdentityStep; +import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.InlineFilterStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.strategy.verification.StandardVerificationStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversalStrategies; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper; import org.apache.tinkerpop.gremlin.structure.Vertex; +import org.apache.tinkerpop.gremlin.structure.VertexProperty; import org.hamcrest.CoreMatchers; import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import java.util.Arrays; + +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.has; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.hasLabel; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.inV; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.is; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.out; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.outV; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.values; import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; @@ -39,11 +56,51 @@ /** * @author Stephen Mallette (http://stephen.genoprime.com) */ +@RunWith(Parameterized.class) public class SubgraphStrategyTest { + @Parameterized.Parameter(value = 0) + public Traversal original; + + @Parameterized.Parameter(value = 1) + public Traversal optimized; + + + void applySubgraphStrategyTest(final Traversal traversal) { + final TraversalStrategies strategies = new DefaultTraversalStrategies(); + strategies.addStrategies(SubgraphStrategy.build(). + vertices(__.and(has("name", "marko"), has("age", 29))). + edges(hasLabel("knows")). + vertexProperties(__.values().count().and(is(P.lt(10)), is(0))).create()); + strategies.addStrategies(InlineFilterStrategy.instance()); + strategies.addStrategies(StandardVerificationStrategy.instance()); + traversal.asAdmin().setStrategies(strategies); + traversal.asAdmin().applyStrategies(); + } + + @Test + public void doTest() { + applySubgraphStrategyTest(original); + assertEquals(optimized, original); + } + + @Parameterized.Parameters(name = "{0}") + public static Iterable generateTestParameters() { + + return Arrays.asList(new Traversal[][]{ + {__.outE(), __.outE().hasLabel("knows").and( + inV().has("name", "marko").has("age", 29), + outV().has("name", "marko").has("age", 29))}, + {__.V(), __.V().has("name", "marko").has("age", 29)}, + {__.V().has("location", "santa fe"), __.V().has("name", "marko").has("age", 29).has("location", "santa fe")}, + {__.V().where(has("location", "santa fe")), __.V().has("name", "marko").has("age", 29).has("location", "santa fe")}, + {__.V().where(has("location", "santa fe")).values("location"), __.V().has("name", "marko").has("age", 29).has("location", "santa fe").properties("location").filter(values().count().is(P.lt(10)).is(0)).value()} + }); + } + @Test public void shouldAddFilterAfterVertex() { - final SubgraphStrategy strategy = SubgraphStrategy.build().vertexCriterion(__.identity()).create(); + final SubgraphStrategy strategy = SubgraphStrategy.build().vertices(__.identity()).create(); final Traversal t = __.inV(); strategy.apply(t.asAdmin()); final EdgeVertexStep edgeVertexStep = (EdgeVertexStep) t.asAdmin().getStartStep(); @@ -55,7 +112,7 @@ public void shouldAddFilterAfterVertex() { @Test public void shouldAddFilterAfterEdge() { - final SubgraphStrategy strategy = SubgraphStrategy.build().edgeCriterion(__.identity()).create(); + final SubgraphStrategy strategy = SubgraphStrategy.build().edges(__.identity()).create(); final Traversal t = __.inE(); strategy.apply(t.asAdmin()); final VertexStep vertexStep = (VertexStep) t.asAdmin().getStartStep(); @@ -67,7 +124,7 @@ public void shouldAddFilterAfterEdge() { @Test public void shouldAddBothFiltersAfterVertex() { - final SubgraphStrategy strategy = SubgraphStrategy.build().edgeCriterion(__.identity()).vertexCriterion(__.identity()).create(); + final SubgraphStrategy strategy = SubgraphStrategy.build().edges(__.identity()).vertices(__.identity()).create(); final Traversal t = __.inE(); strategy.apply(t.asAdmin()); final VertexStep vertexStep = (VertexStep) t.asAdmin().getStartStep(); @@ -80,8 +137,8 @@ public void shouldAddBothFiltersAfterVertex() { @Test public void shouldNotRetainMetadataLabelMarkers() { final SubgraphStrategy strategy = SubgraphStrategy.build().vertices(__.out().hasLabel("person")).create(); - final Traversal.Admin t = __.out().inE().asAdmin(); - t.setStrategies(t.getStrategies().clone().addStrategies(strategy)); + final Traversal.Admin t = out().inE().asAdmin(); + t.setStrategies(t.getStrategies().clone().addStrategies(strategy, StandardVerificationStrategy.instance())); t.applyStrategies(); assertEquals(t.getSteps().get(0).getClass(), VertexStep.class); assertEquals(t.getSteps().get(1).getClass(), TraversalFilterStep.class); diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategyTest.java new file mode 100644 index 00000000000..b77ecc213a5 --- /dev/null +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategyTest.java @@ -0,0 +1,78 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization; + +import org.apache.tinkerpop.gremlin.process.traversal.P; +import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies; +import org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversalStrategies; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +import java.util.Arrays; + +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.and; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.filter; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.has; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.out; +import static org.junit.Assert.assertEquals; + +/** + * @author Marko A. Rodriguez (http://markorodriguez.com) + */ +@RunWith(Parameterized.class) +public class InlineFilterStrategyTest { + + @Parameterized.Parameter(value = 0) + public Traversal original; + + @Parameterized.Parameter(value = 1) + public Traversal optimized; + + + void applyInlineFilterStrategy(final Traversal traversal) { + final TraversalStrategies strategies = new DefaultTraversalStrategies(); + strategies.addStrategies(InlineFilterStrategy.instance()); + traversal.asAdmin().setStrategies(strategies); + traversal.asAdmin().applyStrategies(); + } + + @Test + public void doTest() { + applyInlineFilterStrategy(original); + assertEquals(optimized, original); + } + + @Parameterized.Parameters(name = "{0}") + public static Iterable generateTestParameters() { + + return Arrays.asList(new Traversal[][]{ + {filter(out("knows")), filter(out("knows"))}, + {filter(has("age", P.gt(10))).as("a"), has("age", P.gt(10)).as("a")}, + {filter(has("age", P.gt(10)).as("b")).as("a"), has("age", P.gt(10)).as("b","a")}, + {filter(has("age", P.gt(10))), has("age", P.gt(10))}, + {filter(and(has("age", P.gt(10)), has("name", "marko"))), has("age", P.gt(10)).has("name", "marko")}, + {and(has("age", P.gt(10)), filter(has("age", 22))), has("age", P.gt(10)).has("age", 22)}, + {and(has("age", P.gt(10)).as("a"), and(filter(has("age", 22).as("b")).as("c"), has("name", "marko").as("d"))), has("age", P.gt(10)).as("a").has("age", 22).as("b","c").has("name", "marko").as("d")}, + {and(has("age", P.gt(10)), and(out("knows"), has("name", "marko"))), has("age", P.gt(10)).and(out("knows"), has("name", "marko"))} + }); + } +} 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 29910a7a3c5..1a854bd79e1 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 @@ -27,6 +27,7 @@ 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.strategy.optimization.InlineFilterStrategy; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper; import org.apache.tinkerpop.gremlin.structure.Column; import org.apache.tinkerpop.gremlin.structure.Edge; @@ -378,12 +379,14 @@ public void shouldFilterVertexProperties() throws Exception { 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())); + if (sg.getStrategies().getStrategy(InlineFilterStrategy.class).isPresent()) + 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())); + if (sg.getStrategies().getStrategy(InlineFilterStrategy.class).isPresent()) + 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()); diff --git a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/groovy/jsr223/TinkerGraphGroovyTranslatorProvider.java b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/groovy/jsr223/TinkerGraphGroovyTranslatorProvider.java index f7a93573b2e..a014efa8264 100644 --- a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/groovy/jsr223/TinkerGraphGroovyTranslatorProvider.java +++ b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/groovy/jsr223/TinkerGraphGroovyTranslatorProvider.java @@ -24,6 +24,10 @@ import org.apache.tinkerpop.gremlin.process.traversal.TraversalInterruptionComputerTest; import org.apache.tinkerpop.gremlin.process.traversal.TraversalInterruptionTest; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.apache.tinkerpop.gremlin.process.traversal.step.map.OrderTest; +import org.apache.tinkerpop.gremlin.process.traversal.step.map.PageRankTest; +import org.apache.tinkerpop.gremlin.process.traversal.step.map.PeerPressureTest; +import org.apache.tinkerpop.gremlin.process.traversal.step.map.ProfileTest; import org.apache.tinkerpop.gremlin.process.traversal.step.map.ProgramTest; import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.ElementIdStrategyProcessTest; import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.TranslationStrategy; @@ -47,9 +51,13 @@ public class TinkerGraphGroovyTranslatorProvider extends TinkerGraphProvider { "g_VX1X_out_injectXv2X_name", "shouldNeverPropagateANoBulkTraverser", "shouldNeverPropagateANullValuedTraverser", + OrderTest.Traversals.class.getCanonicalName(), + ProfileTest.Traversals.class.getCanonicalName(), ProgramTest.Traversals.class.getCanonicalName(), TraversalInterruptionTest.class.getCanonicalName(), TraversalInterruptionComputerTest.class.getCanonicalName(), + PageRankTest.Traversals.class.getCanonicalName(), + PeerPressureTest.Traversals.class.getCanonicalName(), ElementIdStrategyProcessTest.class.getCanonicalName())); From 8ffd3ca84857319a1c6fa17450bc35b09c819cb0 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Fri, 23 Sep 2016 16:56:24 -0600 Subject: [PATCH 12/22] a little smarter about applySingleLevelTraversal(). And added that method to RepeatUnrollStrategy as it inlines a child traversal. --- .../strategy/optimization/InlineFilterStrategy.java | 4 ++-- .../strategy/optimization/RepeatUnrollStrategy.java | 1 + .../process/traversal/util/TraversalHelper.java | 10 ++++++---- .../jsr223/TinkerGraphGroovyTranslatorProvider.java | 10 ++-------- 4 files changed, 11 insertions(+), 14 deletions(-) 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 beeca95972b..0944361b526 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 @@ -56,7 +56,7 @@ public void apply(final Traversal.Admin traversal) { final Traversal.Admin childTraversal = step.getLocalChildren().get(0); if (TraversalHelper.allStepsInstanceOf(childTraversal, FilterStep.class, true)) { changed = true; - TraversalHelper.applySingleLevelStrategies(childTraversal, traversal.getStrategies(), InlineFilterStrategy.class); + TraversalHelper.applySingleLevelStrategies(traversal, childTraversal, InlineFilterStrategy.class); final Step finalStep = childTraversal.getEndStep(); TraversalHelper.insertTraversal((Step) step, childTraversal, traversal); traversal.removeStep(step); @@ -72,7 +72,7 @@ public void apply(final Traversal.Admin traversal) { Step finalStep = null; for (int i = childTraversals.size() - 1; i >= 0; i--) { final Traversal.Admin childTraversal = childTraversals.get(i); - TraversalHelper.applySingleLevelStrategies(childTraversal, traversal.getStrategies(), InlineFilterStrategy.class); + TraversalHelper.applySingleLevelStrategies(traversal, childTraversal, InlineFilterStrategy.class); if (null == finalStep) finalStep = childTraversal.getEndStep(); TraversalHelper.insertTraversal((Step) step, childTraversals.get(i), 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 fdc7e850647..f996ec5970f 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 @@ -51,6 +51,7 @@ public void apply(final Traversal.Admin traversal) { final Traversal.Admin repeatTraversal = repeatStep.getGlobalChildren().get(0); final int repeatLength = repeatTraversal.getSteps().size() - 1; repeatTraversal.removeStep(repeatLength); // removes the RepeatEndStep + TraversalHelper.applySingleLevelStrategies(traversal, repeatTraversal, RepeatUnrollStrategy.class); int insertIndex = i; final int loops = (int) ((LoopTraversal) repeatStep.getUntilTraversal()).getMaxLoops(); for (int j = 0; j < loops; j++) { 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 697dc2a627d..054a1dfcd87 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 @@ -22,7 +22,6 @@ import org.apache.tinkerpop.gremlin.process.traversal.Scope; import org.apache.tinkerpop.gremlin.process.traversal.Step; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; -import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.lambda.ElementValueTraversal; import org.apache.tinkerpop.gremlin.process.traversal.lambda.TokenTraversal; @@ -581,9 +580,12 @@ public static boolean allStepsInstanceOf(final Traversal.Admin traversal, return true; } - public static void applySingleLevelStrategies(final Traversal.Admin traversal, final TraversalStrategies traversalStrategies, final Class stopAfterStrategy) { - for (final TraversalStrategy strategy : traversalStrategies.toList()) { - strategy.apply(traversal); + public static void applySingleLevelStrategies(final Traversal.Admin parentTraversal, final Traversal.Admin childTraversal, final Class stopAfterStrategy) { + childTraversal.setStrategies(parentTraversal.getStrategies()); + childTraversal.setSideEffects(parentTraversal.getSideEffects()); + parentTraversal.getGraph().ifPresent(childTraversal::setGraph); + for (final TraversalStrategy strategy : parentTraversal.getStrategies().toList()) { + strategy.apply(childTraversal); if (null != stopAfterStrategy && stopAfterStrategy.isInstance(strategy)) break; } diff --git a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/groovy/jsr223/TinkerGraphGroovyTranslatorProvider.java b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/groovy/jsr223/TinkerGraphGroovyTranslatorProvider.java index a014efa8264..7591bf0f581 100644 --- a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/groovy/jsr223/TinkerGraphGroovyTranslatorProvider.java +++ b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/groovy/jsr223/TinkerGraphGroovyTranslatorProvider.java @@ -21,13 +21,10 @@ import org.apache.tinkerpop.gremlin.LoadGraphWith; import org.apache.tinkerpop.gremlin.groovy.jsr223.GroovyTranslator; +import org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest; import org.apache.tinkerpop.gremlin.process.traversal.TraversalInterruptionComputerTest; import org.apache.tinkerpop.gremlin.process.traversal.TraversalInterruptionTest; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; -import org.apache.tinkerpop.gremlin.process.traversal.step.map.OrderTest; -import org.apache.tinkerpop.gremlin.process.traversal.step.map.PageRankTest; -import org.apache.tinkerpop.gremlin.process.traversal.step.map.PeerPressureTest; -import org.apache.tinkerpop.gremlin.process.traversal.step.map.ProfileTest; import org.apache.tinkerpop.gremlin.process.traversal.step.map.ProgramTest; import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.ElementIdStrategyProcessTest; import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.TranslationStrategy; @@ -51,13 +48,10 @@ public class TinkerGraphGroovyTranslatorProvider extends TinkerGraphProvider { "g_VX1X_out_injectXv2X_name", "shouldNeverPropagateANoBulkTraverser", "shouldNeverPropagateANullValuedTraverser", - OrderTest.Traversals.class.getCanonicalName(), - ProfileTest.Traversals.class.getCanonicalName(), + GraphComputerTest.class.getCanonicalName(), ProgramTest.Traversals.class.getCanonicalName(), TraversalInterruptionTest.class.getCanonicalName(), TraversalInterruptionComputerTest.class.getCanonicalName(), - PageRankTest.Traversals.class.getCanonicalName(), - PeerPressureTest.Traversals.class.getCanonicalName(), ElementIdStrategyProcessTest.class.getCanonicalName())); From 5b2c723f4a05ea98fae178cdb139ff4a42457a5f Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Mon, 26 Sep 2016 08:41:17 -0600 Subject: [PATCH 13/22] added propertyTraversal to toString() of PropertyMapStep (if it exists). Added TraversalHelper.copyLabels() which makes label copying between steps much easier and less error prone. TinkerGraphStepStrategy is smart about hoping over FilterStepp in search of more HasContainer steps. --- CHANGELOG.asciidoc | 2 + .../traversal/step/map/PropertyMapStep.java | 4 +- .../strategy/decoration/SubgraphStrategy.java | 32 +++--------- .../traversal/util/TraversalHelper.java | 8 +++ .../optimization/TinkerGraphStepStrategy.java | 20 +++++--- .../TinkerGraphStepStrategyTest.java | 49 ++++++++++++++++++- 6 files changed, 82 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 0c459e86a1d..1adb1132fcf 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -26,6 +26,8 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima TinkerPop 3.2.3 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* `TinkerGraphStepStrategy` is now smart about non-`HasContainer` filter steps. +* Added `TraversalHelper.copyLabels()` for copying (or moving) labels form one step to another. * Added `TraversalHelper.applySingleLevelStrategies()` which will apply a subset of strategies but not walk the child tree. * Added the concept that hidden labels using during traversal compilation are removed at the end during `StandardVerificationStrategy`. (*breaking*) * Added `InlineFilterStrategy` which will determine if a `TraversalFilterStep` or `AndStep` children are filters and if so, inline them. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertyMapStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertyMapStep.java index d10b12b14bd..5ab3a175f37 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertyMapStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertyMapStep.java @@ -113,7 +113,9 @@ public boolean isIncludeTokens() { } public String toString() { - return StringFactory.stepString(this, Arrays.asList(this.propertyKeys), this.returnType.name().toLowerCase()); + return null != this.propertyTraversal ? + StringFactory.stepString(this, this.propertyTraversal, this.returnType.name().toLowerCase()) : + StringFactory.stepString(this, Arrays.asList(this.propertyKeys), this.returnType.name().toLowerCase()); } @Override 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 6ff69b24b82..0a9cb159ddc 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 @@ -25,7 +25,6 @@ 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.ClassFilterStep; -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.OrStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.TraversalFilterStep; @@ -141,8 +140,10 @@ else if (step instanceof PropertyMapStep || step instanceof PropertiesStep) @Override public void apply(final Traversal.Admin traversal) { // do not apply subgraph strategy to already created subgraph filter branches (or else you get infinite recursion) - if (traversal.getStartStep().getLabels().contains(MARKER)) + if (traversal.getStartStep().getLabels().contains(MARKER)) { + traversal.getStartStep().removeLabel(MARKER); return; + } // final List graphSteps = TraversalHelper.getStepsOfAssignableClass(GraphStep.class, traversal); final List vertexSteps = TraversalHelper.getStepsOfAssignableClass(VertexStep.class, traversal); @@ -178,18 +179,12 @@ public void apply(final Traversal.Admin traversal) { TraversalHelper.replaceStep((Step) step, someEStep, traversal); TraversalHelper.insertAfterStep(someVStep, someEStep, traversal); - // if step was labeled then propagate those labels to the new step that will return the vertex - for (final String label : step.getLabels()) { - step.removeLabel(label); - someVStep.addLabel(label); - } + TraversalHelper.copyLabels(step, someVStep, true); if (null != this.edgeCriterion) TraversalHelper.insertAfterStep(new TraversalFilterStep<>(traversal, this.edgeCriterion.clone()), someEStep, traversal); if (null != this.vertexCriterion) TraversalHelper.insertAfterStep(new TraversalFilterStep<>(traversal, this.vertexCriterion.clone()), someVStep, traversal); - - } } @@ -243,26 +238,18 @@ public void apply(final Traversal.Admin traversal) { if ('v' == propertyType) { final Traversal.Admin temp = nonCheckPropertyCriterion.clone(); TraversalHelper.insertTraversal((Step) step, temp, traversal); - for (final String label : step.getLabels()) { - step.removeLabel(label); - temp.getEndStep().addLabel(label); - } + TraversalHelper.copyLabels(step, temp.getEndStep(), true); } else { final Step temp = checkPropertyCriterion.clone(); TraversalHelper.insertAfterStep(temp, (Step) step, traversal); - for (final String label : step.getLabels()) { - step.removeLabel(label); - temp.addLabel(label); - } + TraversalHelper.copyLabels(step, temp, true); } } else { // if the property step returns value, then replace it with a property step, append criterion, then append a value() step final Step propertiesStep = new PropertiesStep(traversal, PropertyType.PROPERTY, ((PropertiesStep) step).getPropertyKeys()); TraversalHelper.replaceStep(step, propertiesStep, traversal); final Step propertyValueStep = new PropertyValueStep(traversal); - for (final String label : step.getLabels()) { - propertyValueStep.addLabel(label); - } + TraversalHelper.copyLabels(step, propertyValueStep, false); if ('v' == propertyType) { TraversalHelper.insertAfterStep(propertyValueStep, propertiesStep, traversal); TraversalHelper.insertTraversal(propertiesStep, nonCheckPropertyCriterion.clone(), traversal); @@ -303,11 +290,8 @@ private void applyCriterion(final List stepsToApplyCriterionAfter, final T 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); - } TraversalHelper.insertAfterStep(filter, step, traversal); + TraversalHelper.copyLabels(step, filter, true); } } 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 054a1dfcd87..75647414031 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 @@ -571,6 +571,14 @@ public static void removeAllSteps(final Traversal.Admin traversal) { } } + public static void copyLabels(final Step fromStep, final Step toStep, final boolean moveLabels) { + for (final String label : fromStep.getLabels()) { + toStep.addLabel(label); + if (moveLabels) + fromStep.removeLabel(label); + } + } + public static boolean allStepsInstanceOf(final Traversal.Admin traversal, final Class classToCheck, boolean checkIsInstanceOf) { for (final Step step : traversal.getSteps()) { boolean isInstance = classToCheck.isInstance(step); diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategy.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategy.java index a5f258aca42..97d30e95560 100644 --- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategy.java +++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategy.java @@ -22,7 +22,9 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.step.HasContainerHolder; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.FilterStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.GraphStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer; import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper; import org.apache.tinkerpop.gremlin.tinkergraph.process.traversal.step.sideEffect.TinkerGraphStep; @@ -45,14 +47,18 @@ public void apply(final Traversal.Admin traversal) { TraversalHelper.getStepsOfClass(GraphStep.class, traversal).forEach(originalGraphStep -> { final TinkerGraphStep tinkerGraphStep = new TinkerGraphStep<>(originalGraphStep); TraversalHelper.replaceStep(originalGraphStep, (Step) tinkerGraphStep, traversal); + Step lastNonHolderStep = tinkerGraphStep; Step currentStep = tinkerGraphStep.getNextStep(); - while (currentStep instanceof HasContainerHolder) { - ((HasContainerHolder) currentStep).getHasContainers().forEach(hasContainer -> { - if (!GraphStep.processHasContainerIds(tinkerGraphStep, hasContainer)) - tinkerGraphStep.addHasContainer(hasContainer); - }); - currentStep.getLabels().forEach(tinkerGraphStep::addLabel); - traversal.removeStep(currentStep); + while (currentStep instanceof HasContainerHolder || currentStep instanceof FilterStep) { + if (currentStep instanceof HasContainerHolder) { + for (final HasContainer hasContainer : ((HasContainerHolder) currentStep).getHasContainers()) { + if (!GraphStep.processHasContainerIds(tinkerGraphStep, hasContainer)) + tinkerGraphStep.addHasContainer(hasContainer); + } + TraversalHelper.copyLabels(currentStep, lastNonHolderStep, false); + traversal.removeStep(currentStep); + } else + lastNonHolderStep = currentStep; currentStep = currentStep.getNextStep(); } }); diff --git a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategyTest.java b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategyTest.java index 6e4850d12b7..6e7a383d1bc 100644 --- a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategyTest.java +++ b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategyTest.java @@ -24,10 +24,18 @@ import org.apache.tinkerpop.gremlin.process.traversal.TraversalEngine; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.HasStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.OrStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.TraversalFilterStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.map.VertexStep; import org.apache.tinkerpop.gremlin.tinkergraph.process.traversal.step.sideEffect.TinkerGraphStep; import org.junit.Test; +import java.util.Arrays; + +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.has; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.out; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; /** * @author Marko A. Rodriguez (http://markorodriguez.com) @@ -36,6 +44,45 @@ public class TinkerGraphStepStrategyTest extends AbstractGremlinProcessTest { + @Test + @IgnoreEngine(TraversalEngine.Type.COMPUTER) + public void shouldSkipFilterSteps() { + GraphTraversal.Admin traversal = g.V().as("a").has("name", "marko").as("b").has("nothing").has("age", 32).as("c").asAdmin(); + traversal.applyStrategies(); + assertEquals(2, traversal.getSteps().size()); + assertEquals(TinkerGraphStep.class, traversal.getStartStep().getClass()); + assertEquals(TraversalFilterStep.class, traversal.getEndStep().getClass()); + assertTrue(traversal.getStartStep().getLabels().containsAll(Arrays.asList("a", "b"))); + assertTrue(traversal.getEndStep().getLabels().containsAll(Arrays.asList("c"))); + assertEquals("name", ((TinkerGraphStep) traversal.getStartStep()).getHasContainers().get(0).getKey()); + assertEquals("marko", ((TinkerGraphStep) traversal.getStartStep()).getHasContainers().get(0).getValue()); + assertEquals("age", ((TinkerGraphStep) traversal.getStartStep()).getHasContainers().get(1).getKey()); + assertEquals(32, ((TinkerGraphStep) traversal.getStartStep()).getHasContainers().get(1).getValue()); + // + traversal = g.V().as("a").has("name", "marko").as("b").or(has("nothing"), out("something")).has("age", 32).as("c").asAdmin(); + traversal.applyStrategies(); + assertEquals(2, traversal.getSteps().size()); + assertEquals(TinkerGraphStep.class, traversal.getStartStep().getClass()); + assertEquals(OrStep.class, traversal.getEndStep().getClass()); + assertTrue(traversal.getStartStep().getLabels().containsAll(Arrays.asList("a", "b"))); + assertTrue(traversal.getEndStep().getLabels().containsAll(Arrays.asList("c"))); + assertEquals("name", ((TinkerGraphStep) traversal.getStartStep()).getHasContainers().get(0).getKey()); + assertEquals("marko", ((TinkerGraphStep) traversal.getStartStep()).getHasContainers().get(0).getValue()); + assertEquals("age", ((TinkerGraphStep) traversal.getStartStep()).getHasContainers().get(1).getKey()); + assertEquals(32, ((TinkerGraphStep) traversal.getStartStep()).getHasContainers().get(1).getValue()); + // + traversal = g.V().as("a").has("name", "marko").as("b").out("something").has("age", 32).as("c").asAdmin(); + traversal.applyStrategies(); + assertEquals(3, traversal.getSteps().size()); + assertEquals(TinkerGraphStep.class, traversal.getStartStep().getClass()); + assertEquals(VertexStep.class, traversal.getSteps().get(1).getClass()); + assertEquals(HasStep.class, traversal.getEndStep().getClass()); + assertTrue(traversal.getStartStep().getLabels().containsAll(Arrays.asList("a", "b"))); + assertTrue(traversal.getEndStep().getLabels().containsAll(Arrays.asList("c"))); + assertEquals("name", ((TinkerGraphStep) traversal.getStartStep()).getHasContainers().get(0).getKey()); + assertEquals("marko", ((TinkerGraphStep) traversal.getStartStep()).getHasContainers().get(0).getValue()); + } + @Test @IgnoreEngine(TraversalEngine.Type.COMPUTER) public void shouldFoldInHasContainers() { @@ -75,7 +122,7 @@ public void shouldFoldInHasContainers() { assertEquals(TinkerGraphStep.class, traversal.getSteps().get(2).getClass()); assertEquals(1, ((TinkerGraphStep) traversal.getSteps().get(2)).getHasContainers().size()); assertEquals("name", ((TinkerGraphStep) traversal.getSteps().get(2)).getHasContainers().get(0).getKey()); - assertEquals("daniel", ((TinkerGraphStep) traversal.getSteps().get(2)).getHasContainers().get(0).getValue()); + assertEquals("daniel", ((TinkerGraphStep) traversal.getSteps().get(2)).getHasContainers().get(0).getValue()); assertEquals(TinkerGraphStep.class, traversal.getEndStep().getClass()); } From be990d42160131ef78168bc3c3b124d41943c3e0 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Mon, 26 Sep 2016 08:46:27 -0600 Subject: [PATCH 14/22] used the new TraversalHelper.copyLabels() in RepeatUnrollStrategy and got rid of a stream(). --- .../strategy/optimization/RepeatUnrollStrategy.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) 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 f996ec5970f..9e1a98dee48 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 @@ -19,7 +19,6 @@ package org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization; -import org.apache.tinkerpop.gremlin.process.traversal.Step; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.lambda.LoopTraversal; @@ -61,11 +60,8 @@ public void apply(final Traversal.Admin traversal) { traversal.addStep(++insertIndex, new NoOpBarrierStep<>(traversal, 5000)); } // label last step if repeat() was labeled - if (!repeatStep.getLabels().isEmpty()) { - final Step lastStep = traversal.getSteps().get(insertIndex); - repeatStep.getLabels().forEach(lastStep::addLabel); - } - + if (!repeatStep.getLabels().isEmpty()) + TraversalHelper.copyLabels(repeatStep, traversal.getSteps().get(insertIndex), false); traversal.removeStep(i); // remove the RepeatStep } } From a49213fbd9181b6006ea46e465ed2a0376861682 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Mon, 26 Sep 2016 09:43:04 -0600 Subject: [PATCH 15/22] per @spmallettes request -- added JavaDoc to InlineFilter'Strategy. Also, used the new TraversalHelper.copyLabels() method in InlineFilterStrategy. --- .../optimization/InlineFilterStrategy.java | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) 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 0944361b526..ba2f8326b59 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 @@ -35,7 +35,17 @@ import java.util.Set; /** + * InlineFilterStrategy analyzes filter-steps with child traversals that themselves are pure filters. + * If the child traversals are pure filters then the wrapping parent filter is not needed and thus, the + * children can be "inlined." + *

+ * * @author Marko A. Rodriguez (http://markorodriguez.com) + * @example 

+ * __.filter(has("name","marko"))                                // is replaced by __.has("name","marko")
+ * __.and(has("name"),has("age"))                                // is replaced by __.has("name").has("age")
+ * __.and(filter(has("name","marko").has("age")),hasNot("blah")) // is replaced by __.has("name","marko").has("age").hasNot("blah")
+ *
*/ public final class InlineFilterStrategy extends AbstractTraversalStrategy implements TraversalStrategy.OptimizationStrategy { @@ -59,10 +69,8 @@ public void apply(final Traversal.Admin traversal) { TraversalHelper.applySingleLevelStrategies(traversal, childTraversal, InlineFilterStrategy.class); final Step finalStep = childTraversal.getEndStep(); TraversalHelper.insertTraversal((Step) step, childTraversal, traversal); + TraversalHelper.copyLabels(step, finalStep, false); traversal.removeStep(step); - for (final String label : step.getLabels()) { - finalStep.addLabel(label); - } } } for (final AndStep step : TraversalHelper.getStepsOfAssignableClass(AndStep.class, traversal)) { @@ -78,12 +86,8 @@ public void apply(final Traversal.Admin traversal) { TraversalHelper.insertTraversal((Step) step, childTraversals.get(i), traversal); } + if (null != finalStep) TraversalHelper.copyLabels(step, finalStep, false); traversal.removeStep(step); - if (null != finalStep) { - for (final String label : step.getLabels()) { - finalStep.addLabel(label); - } - } } } } From f87c976459241a2a84eb10621f20b93c05ab2c1e Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Mon, 26 Sep 2016 12:22:09 -0600 Subject: [PATCH 16/22] fixed a bug in RepeatUnrollStrategy associated with semi-compilation of repeat-traversal prior to inlining. Added test cases to RepeatUnrollStrategyTest that verify behavior. The added test cases are neat in that the 3 parameter is a list of strategies to co-test with RepeatUnrollStrategy. A good pattern moving forward for seeing how different strategies play off each other. --- .../optimization/RepeatUnrollStrategy.java | 4 +- .../RepeatUnrollStrategyTest.java | 65 +++++++++++-------- 2 files changed, 39 insertions(+), 30 deletions(-) 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 9e1a98dee48..36f20cae906 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 @@ -48,9 +48,9 @@ public void apply(final Traversal.Admin traversal) { final RepeatStep repeatStep = (RepeatStep) traversal.getSteps().get(i); if (null == repeatStep.getEmitTraversal() && repeatStep.getUntilTraversal() instanceof LoopTraversal && ((LoopTraversal) repeatStep.getUntilTraversal()).getMaxLoops() > 0) { final Traversal.Admin repeatTraversal = repeatStep.getGlobalChildren().get(0); - final int repeatLength = repeatTraversal.getSteps().size() - 1; - repeatTraversal.removeStep(repeatLength); // removes the RepeatEndStep + 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(); for (int j = 0; j < loops; j++) { diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RepeatUnrollStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RepeatUnrollStrategyTest.java index 4a02218211b..3ebc7d8e4fa 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RepeatUnrollStrategyTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RepeatUnrollStrategyTest.java @@ -34,10 +34,14 @@ import org.junit.runners.Parameterized; import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; import java.util.Iterator; import java.util.function.Predicate; import java.util.stream.IntStream; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.out; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.path; import static org.junit.Assert.assertEquals; /** @@ -56,47 +60,52 @@ public static class ParameterizedTests { @Parameterized.Parameter(value = 1) public Traversal optimized; - - private void applyRepeatUnrollStrategy(final Traversal traversal) { - final TraversalStrategies strategies = new DefaultTraversalStrategies(); - strategies.addStrategies(RepeatUnrollStrategy.instance()); - traversal.asAdmin().setStrategies(strategies); - traversal.asAdmin().applyStrategies(); - - } + @Parameterized.Parameter(value = 2) + public Collection otherStrategies; @Test public void doTest() { - applyRepeatUnrollStrategy(original); - assertEquals(optimized, original); + final TraversalStrategies strategies = new DefaultTraversalStrategies(); + strategies.addStrategies(RepeatUnrollStrategy.instance()); + for (final TraversalStrategy strategy : this.otherStrategies) { + strategies.addStrategies(strategy); + } + this.original.asAdmin().setStrategies(strategies); + this.original.asAdmin().applyStrategies(); + assertEquals(this.optimized, this.original); } @Parameterized.Parameters(name = "{0}") public static Iterable generateTestParameters() { final int maxBarrierSize = 5000; final Predicate> predicate = t -> t.loops() > 5; - return Arrays.asList(new Traversal[][]{ - {__.repeat(__.out()).times(0), __.repeat(__.out()).times(0)}, - {__.times(0).repeat(__.out()), __.times(0).repeat(__.out())}, - {__.identity(), __.identity()}, - {__.out().as("a").in().repeat(__.outE("created").bothV()).times(2).in(), __.out().as("a").in().outE("created").bothV().barrier(maxBarrierSize).outE("created").bothV().barrier(maxBarrierSize).in()}, - {__.out().repeat(__.outE("created").bothV()).times(1).in(), __.out().outE("created").bothV().barrier(maxBarrierSize).in()}, - {__.repeat(__.outE("created").bothV()).times(1).in(), __.outE("created").bothV().barrier(maxBarrierSize).in()}, - {__.repeat(__.out()).times(2).as("x").repeat(__.in().as("b")).times(3), __.out().barrier(maxBarrierSize).out().barrier(maxBarrierSize).as("x").in().as("b").barrier(maxBarrierSize).in().as("b").barrier(maxBarrierSize).in().as("b").barrier(maxBarrierSize)}, - {__.repeat(__.outE("created").inV()).times(2), __.outE("created").inV().barrier(maxBarrierSize).outE("created").inV().barrier(maxBarrierSize)}, - {__.repeat(__.out()).times(3), __.out().barrier(maxBarrierSize).out().barrier(maxBarrierSize).out().barrier(maxBarrierSize)}, - {__.repeat(__.local(__.select("a").out("knows"))).times(2), __.local(__.select("a").out("knows")).barrier(maxBarrierSize).local(__.select("a").out("knows")).barrier(maxBarrierSize)}, - {__.times(2).repeat(__.out()), __.out().barrier(maxBarrierSize).out().barrier(maxBarrierSize)}, - {__.out().times(2).repeat(__.out().as("a")).as("x"), __.out().out().as("a").barrier(maxBarrierSize).out().as("a").barrier(maxBarrierSize).as("x")}, - {__.repeat(__.out()).emit().times(2), __.repeat(__.out()).emit().times(2)}, - {__.repeat(__.out()).until(predicate), __.repeat(__.out()).until(predicate)}, - {__.repeat(__.out()).until(predicate).repeat(__.out()).times(2), __.repeat(__.out()).until(predicate).out().barrier(maxBarrierSize).out().barrier(maxBarrierSize)}, - {__.repeat(__.union(__.both(), __.identity())).times(2).out(), __.union(__.both(), __.identity()).barrier(maxBarrierSize).union(__.both(), __.identity()).barrier(maxBarrierSize).out()}, - {__.in().repeat(__.out("knows")).times(3).as("a").count().is(0), __.in().out("knows").barrier(maxBarrierSize).out("knows").barrier(maxBarrierSize).out("knows").as("a").count().is(0)}, + return Arrays.asList(new Object[][]{ + {__.repeat(out()).times(0), __.repeat(out()).times(0), Collections.emptyList()}, + {__.times(0).repeat(out()), __.times(0).repeat(out()), Collections.emptyList()}, + {__.identity(), __.identity(), Collections.emptyList()}, + {out().as("a").in().repeat(__.outE("created").bothV()).times(2).in(), out().as("a").in().outE("created").bothV().barrier(maxBarrierSize).outE("created").bothV().barrier(maxBarrierSize).in(), Collections.emptyList()}, + {out().repeat(__.outE("created").bothV()).times(1).in(), out().outE("created").bothV().barrier(maxBarrierSize).in(), Collections.emptyList()}, + {__.repeat(__.outE("created").bothV()).times(1).in(), __.outE("created").bothV().barrier(maxBarrierSize).in(), Collections.emptyList()}, + {__.repeat(out()).times(2).as("x").repeat(__.in().as("b")).times(3), out().barrier(maxBarrierSize).out().barrier(maxBarrierSize).as("x").in().as("b").barrier(maxBarrierSize).in().as("b").barrier(maxBarrierSize).in().as("b").barrier(maxBarrierSize), Collections.emptyList()}, + {__.repeat(__.outE("created").inV()).times(2), __.outE("created").inV().barrier(maxBarrierSize).outE("created").inV().barrier(maxBarrierSize), Collections.emptyList()}, + {__.repeat(out()).times(3), out().barrier(maxBarrierSize).out().barrier(maxBarrierSize).out().barrier(maxBarrierSize), Collections.emptyList()}, + {__.repeat(__.local(__.select("a").out("knows"))).times(2), __.local(__.select("a").out("knows")).barrier(maxBarrierSize).local(__.select("a").out("knows")).barrier(maxBarrierSize), Collections.emptyList()}, + {__.times(2).repeat(out()), out().barrier(maxBarrierSize).out().barrier(maxBarrierSize), Collections.emptyList()}, + {__.out().times(2).repeat(out().as("a")).as("x"), out().out().as("a").barrier(maxBarrierSize).out().as("a").barrier(maxBarrierSize).as("x"), Collections.emptyList()}, + {__.repeat(out()).emit().times(2), __.repeat(out()).emit().times(2), Collections.emptyList()}, + {__.repeat(out()).until(predicate), __.repeat(out()).until(predicate), Collections.emptyList()}, + {__.repeat(out()).until(predicate).repeat(out()).times(2), __.repeat(out()).until(predicate).out().barrier(maxBarrierSize).out().barrier(maxBarrierSize), Collections.emptyList()}, + {__.repeat(__.union(__.both(), __.identity())).times(2).out(), __.union(__.both(), __.identity()).barrier(maxBarrierSize).union(__.both(), __.identity()).barrier(maxBarrierSize).out(), Collections.emptyList()}, + {__.in().repeat(out("knows")).times(3).as("a").count().is(0), __.in().out("knows").barrier(maxBarrierSize).out("knows").barrier(maxBarrierSize).out("knows").as("a").count().is(0), Collections.emptyList()}, + // + {__.repeat(__.outE().inV()).times(2), __.outE().inV().barrier(maxBarrierSize).outE().inV().barrier(maxBarrierSize), Collections.emptyList()}, + {__.repeat(__.outE().filter(path()).inV()).times(2), __.outE().filter(path()).inV().barrier(maxBarrierSize).outE().filter(path()).inV().barrier(maxBarrierSize), Collections.singletonList(IncidentToAdjacentStrategy.instance())}, + {__.repeat(__.outE().inV()).times(2), __.out().barrier(maxBarrierSize).out().barrier(maxBarrierSize), Collections.singletonList(IncidentToAdjacentStrategy.instance())}, }); } } + public static class PerformanceTest extends TraversalStrategyPerformanceTest { @Override From 2cd8b80d0408caa21b7f56f5260ffda4c27124f7 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Mon, 26 Sep 2016 12:33:13 -0600 Subject: [PATCH 17/22] fixed up SubgraphStrategyTest so that non-parameterized tests are not run multiple times for each parameterization -- stole the model from RepeatUnrollStrategyTest. --- .../strategy/decoration/SubgraphStrategy.java | 33 ++-- .../decoration/SubgraphStrategyTest.java | 172 +++++++++--------- 2 files changed, 107 insertions(+), 98 deletions(-) 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 0a9cb159ddc..01879699820 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 @@ -102,23 +102,33 @@ private SubgraphStrategy(final Traversal vertexCriterion, final Trave this.vertexPropertyCriterion = null == vertexPropertyCriterion ? null : vertexPropertyCriterion.asAdmin(); if (null != this.vertexCriterion) - this.metadataLabelStartStep(this.vertexCriterion); + this.addLabelMarker(this.vertexCriterion); if (null != this.edgeCriterion) - this.metadataLabelStartStep(this.edgeCriterion); + this.addLabelMarker(this.edgeCriterion); if (null != this.vertexPropertyCriterion) - this.metadataLabelStartStep(this.vertexPropertyCriterion); + this.addLabelMarker(this.vertexPropertyCriterion); } - private final void metadataLabelStartStep(final Traversal.Admin traversal) { + private final void addLabelMarker(final Traversal.Admin traversal) { traversal.getStartStep().addLabel(MARKER); for (final Step step : traversal.getSteps()) { if (step instanceof TraversalParent) { - ((TraversalParent) step).getLocalChildren().forEach(this::metadataLabelStartStep); - ((TraversalParent) step).getGlobalChildren().forEach(this::metadataLabelStartStep); + ((TraversalParent) step).getLocalChildren().forEach(this::addLabelMarker); + ((TraversalParent) step).getGlobalChildren().forEach(this::addLabelMarker); } } } + private static void applyCriterion(final List stepsToApplyCriterionAfter, final Traversal.Admin traversal, + final Traversal.Admin criterion) { + 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()); + TraversalHelper.insertAfterStep(filter, step, traversal); + TraversalHelper.copyLabels(step, filter, true); + } + } + private static final char processesPropertyType(Step step) { while (!(step instanceof EmptyStep)) { if (step instanceof FilterStep || step instanceof SideEffectStep) @@ -280,21 +290,10 @@ public Set> applyPost() { return this.vertexPropertyCriterion; } - public static Builder build() { return new Builder(); } - private void applyCriterion(final List stepsToApplyCriterionAfter, final Traversal.Admin traversal, - final Traversal.Admin criterion) { - 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()); - TraversalHelper.insertAfterStep(filter, step, traversal); - TraversalHelper.copyLabels(step, filter, true); - } - } - public final static class Builder { private Traversal vertexPredicate = null; diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyTest.java index 2e89f6b748f..708283860c3 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyTest.java @@ -37,6 +37,7 @@ import org.apache.tinkerpop.gremlin.structure.VertexProperty; import org.hamcrest.CoreMatchers; import org.junit.Test; +import org.junit.experimental.runners.Enclosed; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -56,98 +57,107 @@ /** * @author Stephen Mallette (http://stephen.genoprime.com) */ -@RunWith(Parameterized.class) +@RunWith(Enclosed.class) public class SubgraphStrategyTest { - @Parameterized.Parameter(value = 0) - public Traversal original; + @RunWith(Parameterized.class) + public static class ParameterizedTests { - @Parameterized.Parameter(value = 1) - public Traversal optimized; + @Parameterized.Parameter(value = 0) + public Traversal original; + @Parameterized.Parameter(value = 1) + public Traversal optimized; - void applySubgraphStrategyTest(final Traversal traversal) { - final TraversalStrategies strategies = new DefaultTraversalStrategies(); - strategies.addStrategies(SubgraphStrategy.build(). - vertices(__.and(has("name", "marko"), has("age", 29))). - edges(hasLabel("knows")). - vertexProperties(__.values().count().and(is(P.lt(10)), is(0))).create()); - strategies.addStrategies(InlineFilterStrategy.instance()); - strategies.addStrategies(StandardVerificationStrategy.instance()); - traversal.asAdmin().setStrategies(strategies); - traversal.asAdmin().applyStrategies(); - } - @Test - public void doTest() { - applySubgraphStrategyTest(original); - assertEquals(optimized, original); - } + void applySubgraphStrategyTest(final Traversal traversal) { + final TraversalStrategies strategies = new DefaultTraversalStrategies(); + strategies.addStrategies(SubgraphStrategy.build(). + vertices(__.and(has("name", "marko"), has("age", 29))). + edges(hasLabel("knows")). + vertexProperties(__.values().count().and(is(P.lt(10)), is(0))).create()); + strategies.addStrategies(InlineFilterStrategy.instance()); + strategies.addStrategies(StandardVerificationStrategy.instance()); + traversal.asAdmin().setStrategies(strategies); + traversal.asAdmin().applyStrategies(); + } - @Parameterized.Parameters(name = "{0}") - public static Iterable generateTestParameters() { - - return Arrays.asList(new Traversal[][]{ - {__.outE(), __.outE().hasLabel("knows").and( - inV().has("name", "marko").has("age", 29), - outV().has("name", "marko").has("age", 29))}, - {__.V(), __.V().has("name", "marko").has("age", 29)}, - {__.V().has("location", "santa fe"), __.V().has("name", "marko").has("age", 29).has("location", "santa fe")}, - {__.V().where(has("location", "santa fe")), __.V().has("name", "marko").has("age", 29).has("location", "santa fe")}, - {__.V().where(has("location", "santa fe")).values("location"), __.V().has("name", "marko").has("age", 29).has("location", "santa fe").properties("location").filter(values().count().is(P.lt(10)).is(0)).value()} - }); - } + @Test + public void doTest() { + applySubgraphStrategyTest(original); + assertEquals(optimized, original); + } - @Test - public void shouldAddFilterAfterVertex() { - final SubgraphStrategy strategy = SubgraphStrategy.build().vertices(__.identity()).create(); - final Traversal t = __.inV(); - strategy.apply(t.asAdmin()); - final EdgeVertexStep edgeVertexStep = (EdgeVertexStep) t.asAdmin().getStartStep(); - assertEquals(TraversalFilterStep.class, edgeVertexStep.getNextStep().getClass()); - final TraversalFilterStep h = (TraversalFilterStep) t.asAdmin().getEndStep(); - assertEquals(1, h.getLocalChildren().size()); - assertThat(((DefaultGraphTraversal) h.getLocalChildren().get(0)).getEndStep(), CoreMatchers.instanceOf(IdentityStep.class)); - } + @Parameterized.Parameters(name = "{0}") + public static Iterable generateTestParameters() { - @Test - public void shouldAddFilterAfterEdge() { - final SubgraphStrategy strategy = SubgraphStrategy.build().edges(__.identity()).create(); - final Traversal t = __.inE(); - strategy.apply(t.asAdmin()); - final VertexStep vertexStep = (VertexStep) t.asAdmin().getStartStep(); - assertEquals(TraversalFilterStep.class, vertexStep.getNextStep().getClass()); - final TraversalFilterStep h = (TraversalFilterStep) t.asAdmin().getEndStep(); - assertEquals(1, h.getLocalChildren().size()); - assertThat(((DefaultGraphTraversal) h.getLocalChildren().get(0)).getEndStep(), CoreMatchers.instanceOf(IdentityStep.class)); + return Arrays.asList(new Traversal[][]{ + {__.outE(), __.outE().hasLabel("knows").and( + inV().has("name", "marko").has("age", 29), + outV().has("name", "marko").has("age", 29))}, + {__.V(), __.V().has("name", "marko").has("age", 29)}, + {__.V().has("location", "santa fe"), __.V().has("name", "marko").has("age", 29).has("location", "santa fe")}, + {__.V().where(has("location", "santa fe")), __.V().has("name", "marko").has("age", 29).has("location", "santa fe")}, + {__.V().where(has("location", "santa fe")).values("location"), __.V().has("name", "marko").has("age", 29).has("location", "santa fe").properties("location").filter(values().count().is(P.lt(10)).is(0)).value()} + }); + } } - @Test - public void shouldAddBothFiltersAfterVertex() { - final SubgraphStrategy strategy = SubgraphStrategy.build().edges(__.identity()).vertices(__.identity()).create(); - final Traversal t = __.inE(); - strategy.apply(t.asAdmin()); - final VertexStep vertexStep = (VertexStep) t.asAdmin().getStartStep(); - assertEquals(TraversalFilterStep.class, vertexStep.getNextStep().getClass()); - final TraversalFilterStep h = (TraversalFilterStep) t.asAdmin().getEndStep(); - assertEquals(1, h.getLocalChildren().size()); - assertThat(((DefaultGraphTraversal) h.getLocalChildren().get(0)).getEndStep(), CoreMatchers.instanceOf(TraversalFilterStep.class)); - } + public static class RewriteTest { - @Test - public void shouldNotRetainMetadataLabelMarkers() { - final SubgraphStrategy strategy = SubgraphStrategy.build().vertices(__.out().hasLabel("person")).create(); - final Traversal.Admin t = out().inE().asAdmin(); - t.setStrategies(t.getStrategies().clone().addStrategies(strategy, StandardVerificationStrategy.instance())); - t.applyStrategies(); - assertEquals(t.getSteps().get(0).getClass(), VertexStep.class); - assertEquals(t.getSteps().get(1).getClass(), TraversalFilterStep.class); - assertEquals(AndStep.class, ((TraversalFilterStep) t.getSteps().get(1)).getLocalChildren().get(0).getStartStep().getClass()); - assertEquals(0, ((TraversalFilterStep) t.getSteps().get(1)).getLocalChildren().get(0).getStartStep().getLabels().size()); - assertEquals(t.getSteps().get(2).getClass(), EdgeVertexStep.class); - assertEquals(t.getSteps().get(3).getClass(), TraversalFilterStep.class); - assertEquals(VertexStep.class, ((TraversalFilterStep) t.getSteps().get(3)).getLocalChildren().get(0).getStartStep().getClass()); - assertEquals(0, ((TraversalFilterStep) t.getSteps().get(3)).getLocalChildren().get(0).getStartStep().getLabels().size()); - TraversalHelper.getStepsOfAssignableClassRecursively(Step.class, t).forEach(step -> assertTrue(step.getLabels().isEmpty())); + @Test + public void shouldAddFilterAfterVertex() { + final SubgraphStrategy strategy = SubgraphStrategy.build().vertices(__.identity()).create(); + final Traversal t = __.inV(); + strategy.apply(t.asAdmin()); + final EdgeVertexStep edgeVertexStep = (EdgeVertexStep) t.asAdmin().getStartStep(); + assertEquals(TraversalFilterStep.class, edgeVertexStep.getNextStep().getClass()); + final TraversalFilterStep h = (TraversalFilterStep) t.asAdmin().getEndStep(); + assertEquals(1, h.getLocalChildren().size()); + assertThat(((DefaultGraphTraversal) h.getLocalChildren().get(0)).getEndStep(), CoreMatchers.instanceOf(IdentityStep.class)); + } + + @Test + public void shouldAddFilterAfterEdge() { + final SubgraphStrategy strategy = SubgraphStrategy.build().edges(__.identity()).create(); + final Traversal t = __.inE(); + strategy.apply(t.asAdmin()); + final VertexStep vertexStep = (VertexStep) t.asAdmin().getStartStep(); + assertEquals(TraversalFilterStep.class, vertexStep.getNextStep().getClass()); + final TraversalFilterStep h = (TraversalFilterStep) t.asAdmin().getEndStep(); + assertEquals(1, h.getLocalChildren().size()); + assertThat(((DefaultGraphTraversal) h.getLocalChildren().get(0)).getEndStep(), CoreMatchers.instanceOf(IdentityStep.class)); + } + + @Test + public void shouldAddBothFiltersAfterVertex() { + final SubgraphStrategy strategy = SubgraphStrategy.build().edges(__.identity()).vertices(__.identity()).create(); + final Traversal t = __.inE(); + strategy.apply(t.asAdmin()); + final VertexStep vertexStep = (VertexStep) t.asAdmin().getStartStep(); + assertEquals(TraversalFilterStep.class, vertexStep.getNextStep().getClass()); + final TraversalFilterStep h = (TraversalFilterStep) t.asAdmin().getEndStep(); + assertEquals(1, h.getLocalChildren().size()); + assertThat(((DefaultGraphTraversal) h.getLocalChildren().get(0)).getEndStep(), CoreMatchers.instanceOf(TraversalFilterStep.class)); + } + + @Test + public void shouldNotRetainMarkers() { + final SubgraphStrategy strategy = SubgraphStrategy.build().vertices(__.out().hasLabel("person")).create(); + final Traversal.Admin t = out().inE().asAdmin(); + t.setStrategies(t.getStrategies().clone().addStrategies(strategy, StandardVerificationStrategy.instance())); + t.applyStrategies(); + assertEquals(t.getSteps().get(0).getClass(), VertexStep.class); + assertEquals(t.getSteps().get(1).getClass(), TraversalFilterStep.class); + assertEquals(AndStep.class, ((TraversalFilterStep) t.getSteps().get(1)).getLocalChildren().get(0).getStartStep().getClass()); + assertEquals(0, ((TraversalFilterStep) t.getSteps().get(1)).getLocalChildren().get(0).getStartStep().getLabels().size()); + assertEquals(t.getSteps().get(2).getClass(), EdgeVertexStep.class); + assertEquals(t.getSteps().get(3).getClass(), TraversalFilterStep.class); + assertEquals(VertexStep.class, ((TraversalFilterStep) t.getSteps().get(3)).getLocalChildren().get(0).getStartStep().getClass()); + assertEquals(0, ((TraversalFilterStep) t.getSteps().get(3)).getLocalChildren().get(0).getStartStep().getLabels().size()); + TraversalHelper.getStepsOfAssignableClassRecursively(Step.class, t).forEach(step -> assertTrue(step.getLabels().isEmpty())); + } } + + } From 1e330cdc1dd26a4dc82f57b4e03ebc19eb8d7338 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Mon, 26 Sep 2016 12:46:14 -0600 Subject: [PATCH 18/22] simplified the hidden label removal algorithm in StandardVerificationStrategy. Updated CHANGELOG with note about RepeatUnrollStrategy bug fix. --- CHANGELOG.asciidoc | 1 + .../verification/StandardVerificationStrategy.java | 7 +------ 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 1adb1132fcf..b484e7a11cb 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.3 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Fixed a bug in `RepeatUnrollStrategy` where inlined traversal did not have strategies applied to it. * `TinkerGraphStepStrategy` is now smart about non-`HasContainer` filter steps. * Added `TraversalHelper.copyLabels()` for copying (or moving) labels form one step to another. * Added `TraversalHelper.applySingleLevelStrategies()` which will apply a subset of strategies but not walk the child tree. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/StandardVerificationStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/StandardVerificationStrategy.java index 4f272cc338b..917fab9adee 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/StandardVerificationStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/StandardVerificationStrategy.java @@ -32,7 +32,6 @@ import org.apache.tinkerpop.gremlin.structure.Graph; import java.util.Collections; -import java.util.HashSet; import java.util.Set; /** @@ -53,13 +52,9 @@ public void apply(final Traversal.Admin traversal) { } for (final Step step : traversal.getSteps()) { - final Set hiddenLabels = new HashSet<>(); for (String label : step.getLabels()) { if (Graph.Hidden.isHidden(label)) - hiddenLabels.add(label); - } - for(final String label : hiddenLabels) { - step.removeLabel(label); + step.removeLabel(label); } if (step instanceof ReducingBarrierStep && step.getTraversal().getParent() instanceof RepeatStep && step.getTraversal().getParent().getGlobalChildren().get(0).getSteps().contains(step)) throw new VerificationException("The parent of a reducing barrier can not be repeat()-step: " + step, traversal); From 7f65a723d3e7e9b6954c320a6adf6fd30390fcd6 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Tue, 27 Sep 2016 08:27:02 -0600 Subject: [PATCH 19/22] Added lots more tests to various strategies. The inline aspect of MatchPredicateStrategy was moved to InlineFilterStrategy. In migrating that chunk, fixed a strategy application bug that was present in MatchPredicateStrategy. Tweaked up various XXXStrategyTest in the new model where extra strategies can be provded in the parameters. --- CHANGELOG.asciidoc | 4 +- .../optimization/InlineFilterStrategy.java | 58 ++++++++- .../optimization/MatchPredicateStrategy.java | 49 +------- .../traversal/util/TraversalHelper.java | 12 +- .../FilterRankingStrategyTest.java | 44 ++++--- .../InlineFilterStrategyTest.java | 32 +++-- .../MatchPredicateStrategyTest.java | 119 ++++++------------ 7 files changed, 147 insertions(+), 171 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index b484e7a11cb..bada500bdfb 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -26,12 +26,14 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima TinkerPop 3.2.3 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Fixed a end-step label bug in `MatchPredicateStrategy`. +* Fixed a bug in `MatchPredicateStrategy` where inlined traversals did not have strategies applied to it. * Fixed a bug in `RepeatUnrollStrategy` where inlined traversal did not have strategies applied to it. * `TinkerGraphStepStrategy` is now smart about non-`HasContainer` filter steps. * Added `TraversalHelper.copyLabels()` for copying (or moving) labels form one step to another. * Added `TraversalHelper.applySingleLevelStrategies()` which will apply a subset of strategies but not walk the child tree. * Added the concept that hidden labels using during traversal compilation are removed at the end during `StandardVerificationStrategy`. (*breaking*) -* Added `InlineFilterStrategy` which will determine if a `TraversalFilterStep` or `AndStep` children are filters and if so, inline them. +* Added `InlineFilterStrategy` which will determine if a `TraversalFilterStep`, `AndStep`, `MatchStep` children are filters and if so, inline them. * Removed `IdentityRemovalStrategy` from the default listing as its not worth the clock cycles. * Removed the "!" symbol in `NotStep.toString()` as it is confusing and the `NotStep`-name is sufficient. * Fixed a bug in `TraversalVertexProgram` (OLAP) around ordering and connectives (i.e. `and()` and `or()`). 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 ba2f8326b59..cbc7748de65 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 @@ -25,10 +25,14 @@ import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.AndStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.FilterStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.HasStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.TraversalFilterStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.map.MatchStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyStep; import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper; +import java.util.ArrayList; import java.util.Arrays; import java.util.HashSet; import java.util.List; @@ -45,12 +49,14 @@ * __.filter(has("name","marko")) // is replaced by __.has("name","marko") * __.and(has("name"),has("age")) // is replaced by __.has("name").has("age") * __.and(filter(has("name","marko").has("age")),hasNot("blah")) // is replaced by __.has("name","marko").has("age").hasNot("blah") + * __.match(as('a').has(key,value),...) // is replaced by __.as('a').has(key,value).match(...) * */ public final class InlineFilterStrategy extends AbstractTraversalStrategy implements TraversalStrategy.OptimizationStrategy { private static final InlineFilterStrategy INSTANCE = new InlineFilterStrategy(); private static final Set> POSTS = new HashSet<>(Arrays.asList( + MatchPredicateStrategy.class, FilterRankingStrategy.class, GraphFilterStrategy.class)); @@ -62,9 +68,10 @@ public void apply(final Traversal.Admin traversal) { boolean changed = true; // recursively walk child traversals trying to inline them into the current traversal line. while (changed) { changed = false; - for (final TraversalFilterStep step : TraversalHelper.getStepsOfAssignableClass(TraversalFilterStep.class, traversal)) { + // filter(x.y) --> x.y + for (final TraversalFilterStep step : TraversalHelper.getStepsOfClass(TraversalFilterStep.class, traversal)) { final Traversal.Admin childTraversal = step.getLocalChildren().get(0); - if (TraversalHelper.allStepsInstanceOf(childTraversal, FilterStep.class, true)) { + if (TraversalHelper.allStepsInstanceOf(childTraversal, FilterStep.class)) { changed = true; TraversalHelper.applySingleLevelStrategies(traversal, childTraversal, InlineFilterStrategy.class); final Step finalStep = childTraversal.getEndStep(); @@ -73,8 +80,9 @@ public void apply(final Traversal.Admin traversal) { traversal.removeStep(step); } } - for (final AndStep step : TraversalHelper.getStepsOfAssignableClass(AndStep.class, traversal)) { - if (!step.getLocalChildren().stream().filter(t -> !TraversalHelper.allStepsInstanceOf(t, FilterStep.class, true)).findAny().isPresent()) { + // and(x,y) --> x.y + for (final AndStep step : TraversalHelper.getStepsOfClass(AndStep.class, traversal)) { + if (!step.getLocalChildren().stream().filter(t -> !TraversalHelper.allStepsInstanceOf(t, FilterStep.class)).findAny().isPresent()) { changed = true; final List> childTraversals = (List) step.getLocalChildren(); Step finalStep = null; @@ -90,7 +98,49 @@ public void apply(final Traversal.Admin traversal) { traversal.removeStep(step); } } + // match(as('a').has(key,value),...) --> as('a').has(key,value).match(...) + if (traversal.getParent() instanceof EmptyStep) { + for (final MatchStep step : TraversalHelper.getStepsOfClass(MatchStep.class, traversal)) { + final String startLabel = determineStartLabelForHasPullOut(step); + if (null != startLabel) { + for (final Traversal.Admin matchTraversal : new ArrayList<>(step.getGlobalChildren())) { + if (!(step.getPreviousStep() instanceof EmptyStep) && + TraversalHelper.allStepsInstanceOf(matchTraversal, + HasStep.class, + MatchStep.MatchStartStep.class, + MatchStep.MatchEndStep.class) && + matchTraversal.getStartStep() instanceof MatchStep.MatchStartStep && + startLabel.equals(((MatchStep.MatchStartStep) matchTraversal.getStartStep()).getSelectKey().orElse(null))) { + changed = true; + 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); + step.removeGlobalChild(matchTraversal); + step.getPreviousStep().addLabel(startLabel); + if (null != endLabel) matchTraversal.getEndStep().addLabel(endLabel); + TraversalHelper.insertTraversal((Step) step.getPreviousStep(), matchTraversal, traversal); + } + } + if (step.getGlobalChildren().isEmpty()) + traversal.removeStep(step); + } + } + } + } + } + + private static final String determineStartLabelForHasPullOut(final MatchStep matchStep) { + final String startLabel = MatchStep.Helper.computeStartLabel(matchStep.getGlobalChildren()); + Step previousStep = matchStep.getPreviousStep(); + if (previousStep.getLabels().contains(startLabel)) + return startLabel; + while (!(previousStep instanceof EmptyStep)) { + if (!previousStep.getLabels().isEmpty()) + return null; + previousStep = previousStep.getPreviousStep(); } + return startLabel; } @Override diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/MatchPredicateStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/MatchPredicateStrategy.java index f025749205d..a93dcd98a93 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/MatchPredicateStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/MatchPredicateStrategy.java @@ -22,20 +22,18 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.DedupGlobalStep; -import org.apache.tinkerpop.gremlin.process.traversal.step.filter.HasStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.WherePredicateStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.WhereTraversalStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.MatchStep; 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.util.EmptyStep; import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversal; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper; +import java.util.Arrays; import java.util.HashSet; import java.util.Set; -import java.util.stream.Collectors; /** * MatchWhereStrategy will fold any post-{@code where()} step that maintains a traversal constraint into @@ -53,11 +51,7 @@ public final class MatchPredicateStrategy extends AbstractTraversalStrategy implements TraversalStrategy.OptimizationStrategy { private static final MatchPredicateStrategy INSTANCE = new MatchPredicateStrategy(); - private static final Set> PRIORS = new HashSet<>(); - - static { - PRIORS.add(IdentityRemovalStrategy.class); - } + private static final Set> PRIORS = new HashSet<>(Arrays.asList(IdentityRemovalStrategy.class, InlineFilterStrategy.class)); private MatchPredicateStrategy() { } @@ -89,48 +83,9 @@ public void apply(final Traversal.Admin traversal) { } else break; } - // match(as('a').has(key,value),...) --> as('a').has(key,value).match(...) - final String startLabel = this.determineStartLabelForHasPullOut(matchStep); - if (null != startLabel) { - ((MatchStep) matchStep).getGlobalChildren().stream().collect(Collectors.toList()).forEach(matchTraversal -> { - if (matchTraversal.getStartStep() instanceof MatchStep.MatchStartStep && - ((MatchStep.MatchStartStep) matchTraversal.getStartStep()).getSelectKey().isPresent() && - ((MatchStep.MatchStartStep) matchTraversal.getStartStep()).getSelectKey().get().equals(startLabel) && - !(matchStep.getPreviousStep() instanceof EmptyStep) && - !matchTraversal.getSteps().stream() - .filter(step -> !(step instanceof MatchStep.MatchStartStep) && - !(step instanceof MatchStep.MatchEndStep) && - !(step instanceof HasStep)) - .findAny() - .isPresent()) { - matchStep.removeGlobalChild(matchTraversal); - matchTraversal.removeStep(0); // remove MatchStartStep - matchTraversal.removeStep(matchTraversal.getSteps().size() - 1); // remove MatchEndStep - matchStep.getPreviousStep().addLabel(startLabel); - TraversalHelper.insertTraversal(matchStep.getPreviousStep(), matchTraversal, traversal); - } - }); - } }); } - private String determineStartLabelForHasPullOut(final MatchStep matchStep) { - if (!(matchStep.getTraversal().getParent() instanceof EmptyStep)) - return null; - else { - final String startLabel = MatchStep.Helper.computeStartLabel(matchStep.getGlobalChildren()); - Step previousStep = matchStep.getPreviousStep(); - if (previousStep.getLabels().contains(startLabel)) - return startLabel; - while (!(previousStep instanceof EmptyStep)) { - if (!previousStep.getLabels().isEmpty()) - return null; - previousStep = previousStep.getPreviousStep(); - } - return startLabel; - } - } - public static MatchPredicateStrategy instance() { return INSTANCE; } 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 75647414031..d38ac6ef35c 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 @@ -579,10 +579,16 @@ public static void copyLabels(final Step fromStep, final Step toStep } } - public static boolean allStepsInstanceOf(final Traversal.Admin traversal, final Class classToCheck, boolean checkIsInstanceOf) { + public static boolean allStepsInstanceOf(final Traversal.Admin traversal, final Class... classesToCheck) { for (final Step step : traversal.getSteps()) { - boolean isInstance = classToCheck.isInstance(step); - if ((isInstance && !checkIsInstanceOf) || (!isInstance && checkIsInstanceOf)) + boolean foundInstance = false; + for (final Class classToCheck : classesToCheck) { + if (classToCheck.isInstance(step)) { + foundInstance = true; + break; + } + } + if (!foundInstance) return false; } return true; diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/FilterRankingStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/FilterRankingStrategyTest.java index 2a57185036e..71f5b4b7d05 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/FilterRankingStrategyTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/FilterRankingStrategyTest.java @@ -20,6 +20,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies; +import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversalStrategies; import org.junit.Test; @@ -27,6 +28,8 @@ import org.junit.runners.Parameterized; import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; import static org.junit.Assert.assertEquals; @@ -47,33 +50,36 @@ public static Iterable data() { @Parameterized.Parameter(value = 1) public Traversal optimized; - - void applyFilterRankingStrategy(final Traversal traversal) { - final TraversalStrategies strategies = new DefaultTraversalStrategies(); - strategies.addStrategies(FilterRankingStrategy.instance(), IdentityRemovalStrategy.instance()); - traversal.asAdmin().setStrategies(strategies); - traversal.asAdmin().applyStrategies(); - } + @Parameterized.Parameter(value = 2) + public Collection otherStrategies; @Test public void doTest() { - applyFilterRankingStrategy(original); - assertEquals(optimized, original); + final TraversalStrategies strategies = new DefaultTraversalStrategies(); + strategies.addStrategies(FilterRankingStrategy.instance()); + for (final TraversalStrategy strategy : this.otherStrategies) { + strategies.addStrategies(strategy); + } + this.original.asAdmin().setStrategies(strategies); + this.original.asAdmin().applyStrategies(); + assertEquals(this.optimized, this.original); } @Parameterized.Parameters(name = "{0}") public static Iterable generateTestParameters() { - return Arrays.asList(new Traversal[][]{ - {__.dedup().order(), __.dedup().order()}, - {__.order().dedup(), __.dedup().order()}, - {__.identity().order().dedup(), __.dedup().order()}, - {__.order().identity().dedup(), __.dedup().order()}, - {__.order().out().dedup(), __.order().out().dedup()}, - {__.has("value", 0).filter(__.out()).dedup(), __.has("value", 0).filter(__.out()).dedup()}, - {__.dedup().filter(__.out()).has("value", 0), __.has("value", 0).filter(__.out()).dedup()}, - {__.filter(__.out()).dedup().has("value", 0), __.has("value", 0).filter(__.out()).dedup()}, - {__.has("value", 0).filter(__.out()).dedup(), __.has("value", 0).filter(__.out()).dedup()}, + return Arrays.asList(new Object[][]{ + {__.dedup().order(), __.dedup().order(), Collections.emptyList()}, + {__.order().dedup(), __.dedup().order(), Collections.emptyList()}, + {__.order().as("a").dedup(), __.order().as("a").dedup(), Collections.emptyList()}, + {__.identity().order().dedup(), __.dedup().order(), Collections.singletonList(IdentityRemovalStrategy.instance())}, + {__.order().identity().dedup(), __.dedup().order(), Collections.singletonList(IdentityRemovalStrategy.instance())}, + {__.order().out().dedup(), __.order().out().dedup(), Collections.emptyList()}, + {__.has("value", 0).filter(__.out()).dedup(), __.has("value", 0).filter(__.out()).dedup(), Collections.emptyList()}, + {__.dedup().filter(__.out()).has("value", 0), __.has("value", 0).filter(__.out()).dedup(), Collections.emptyList()}, + {__.filter(__.out()).dedup().has("value", 0), __.has("value", 0).filter(__.out()).dedup(), Collections.emptyList()}, + {__.has("value", 0).filter(__.out()).dedup(), __.has("value", 0).filter(__.out()).dedup(), Collections.emptyList()}, + {__.has("value", 0).and(__.has("age"), __.has("name", "marko")).is(10), __.is(10).has("value", 0).has("name", "marko").has("age"), Collections.singletonList(InlineFilterStrategy.instance())}, }); } } diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategyTest.java index b77ecc213a5..e9e605a5e71 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategyTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategyTest.java @@ -29,9 +29,13 @@ import java.util.Arrays; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.V; 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.__.filter; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.has; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.map; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.match; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.out; import static org.junit.Assert.assertEquals; @@ -47,18 +51,13 @@ public class InlineFilterStrategyTest { @Parameterized.Parameter(value = 1) public Traversal optimized; - - void applyInlineFilterStrategy(final Traversal traversal) { - final TraversalStrategies strategies = new DefaultTraversalStrategies(); - strategies.addStrategies(InlineFilterStrategy.instance()); - traversal.asAdmin().setStrategies(strategies); - traversal.asAdmin().applyStrategies(); - } - @Test public void doTest() { - applyInlineFilterStrategy(original); - assertEquals(optimized, original); + final TraversalStrategies strategies = new DefaultTraversalStrategies(); + strategies.addStrategies(InlineFilterStrategy.instance()); + this.original.asAdmin().setStrategies(strategies); + this.original.asAdmin().applyStrategies(); + assertEquals(this.optimized, this.original); } @Parameterized.Parameters(name = "{0}") @@ -67,12 +66,19 @@ public static Iterable generateTestParameters() { return Arrays.asList(new Traversal[][]{ {filter(out("knows")), filter(out("knows"))}, {filter(has("age", P.gt(10))).as("a"), has("age", P.gt(10)).as("a")}, - {filter(has("age", P.gt(10)).as("b")).as("a"), has("age", P.gt(10)).as("b","a")}, + {filter(has("age", P.gt(10)).as("b")).as("a"), has("age", P.gt(10)).as("b", "a")}, {filter(has("age", P.gt(10))), has("age", P.gt(10))}, {filter(and(has("age", P.gt(10)), has("name", "marko"))), has("age", P.gt(10)).has("name", "marko")}, + // {and(has("age", P.gt(10)), filter(has("age", 22))), has("age", P.gt(10)).has("age", 22)}, - {and(has("age", P.gt(10)).as("a"), and(filter(has("age", 22).as("b")).as("c"), has("name", "marko").as("d"))), has("age", P.gt(10)).as("a").has("age", 22).as("b","c").has("name", "marko").as("d")}, - {and(has("age", P.gt(10)), and(out("knows"), has("name", "marko"))), has("age", P.gt(10)).and(out("knows"), has("name", "marko"))} + {and(has("age", P.gt(10)).as("a"), and(filter(has("age", 22).as("b")).as("c"), has("name", "marko").as("d"))), has("age", P.gt(10)).as("a").has("age", 22).as("b", "c").has("name", "marko").as("d")}, + {and(has("age", P.gt(10)), and(out("knows"), has("name", "marko"))), has("age", P.gt(10)).and(out("knows"), has("name", "marko"))}, + // + {V().match(as("a").has("age", 10), as("a").filter(has("name")).as("b")), V().as("a").has("age", 10).match(as("a").has("name").as("b"))}, + {match(as("a").has("age", 10), as("a").filter(has("name")).as("b")), match(as("a").has("age", 10), as("a").has("name").as("b"))}, + {map(match(as("a").has("age", 10), as("a").filter(has("name")).as("b"))), map(match(as("a").has("age", 10), as("a").has("name").as("b")))}, + {V().match(as("a").has("age", 10)), V().as("a").has("age", 10)}, + {V().match(as("a").has("age", 10).as("b"), as("a").filter(has("name")).as("b")), V().as("a").has("age", 10).as("b").match(as("a").has("name").as("b"))}, }); } } diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/MatchPredicateStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/MatchPredicateStrategyTest.java index 41d3525ec27..509004ce9e4 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/MatchPredicateStrategyTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/MatchPredicateStrategyTest.java @@ -20,113 +20,64 @@ import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; -import org.apache.tinkerpop.gremlin.process.traversal.TraversalEngine; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies; +import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversalStrategies; -import org.junit.Before; import org.junit.Test; -import org.junit.experimental.runners.Enclosed; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.as; import static org.junit.Assert.assertEquals; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; /** * @author Marko A. Rodriguez (http://markorodriguez.com) */ -@RunWith(Enclosed.class) +@RunWith(Parameterized.class) public class MatchPredicateStrategyTest { - @RunWith(Parameterized.class) - public static class StandardTest extends AbstractMatchPredicateStrategyTest { + @Parameterized.Parameter(value = 0) + public Traversal original; - @Parameterized.Parameters(name = "{0}") - public static Iterable data() { - return generateTestParameters(); - } - - @Parameterized.Parameter(value = 0) - public Traversal original; - - @Parameterized.Parameter(value = 1) - public Traversal optimized; + @Parameterized.Parameter(value = 1) + public Traversal optimized; - @Before - public void setup() { - this.traversalEngine = mock(TraversalEngine.class); - when(this.traversalEngine.getType()).thenReturn(TraversalEngine.Type.STANDARD); - } + @Parameterized.Parameter(value = 2) + public Collection otherStrategies; - @Test - public void shouldApplyStrategy() { - doTest(original, optimized); + @Test + public void doTest() { + final TraversalStrategies strategies = new DefaultTraversalStrategies(); + strategies.addStrategies(MatchPredicateStrategy.instance()); + for (final TraversalStrategy strategy : this.otherStrategies) { + strategies.addStrategies(strategy); } + this.original.asAdmin().setStrategies(strategies); + this.original.asAdmin().applyStrategies(); + assertEquals(this.optimized, this.original); } - @RunWith(Parameterized.class) - public static class ComputerTest extends AbstractMatchPredicateStrategyTest { - - @Parameterized.Parameters(name = "{0}") - public static Iterable data() { - return generateTestParameters(); - } - - @Parameterized.Parameter(value = 0) - public Traversal original; - - @Parameterized.Parameter(value = 1) - public Traversal optimized; - - @Before - public void setup() { - this.traversalEngine = mock(TraversalEngine.class); - when(this.traversalEngine.getType()).thenReturn(TraversalEngine.Type.COMPUTER); - } - - @Test - public void shouldApplyStrategy() { - doTest(original, optimized); - } + @Parameterized.Parameters(name = "{0}") + public static Iterable generateTestParameters() { + + return Arrays.asList(new Object[][]{ + {__.out().match(as("a").has("name", "marko"), as("a").out().as("b")), __.out().as("a").has("name", "marko").match(as("a").out().as("b")), Collections.singletonList(InlineFilterStrategy.instance())}, // has() pull out + {__.out().as("a").match(as("a").has("name", "marko"), as("a").out().as("b")), __.out().as("a").has("name", "marko").match(as("a").out().as("b")), Collections.singletonList(InlineFilterStrategy.instance())}, // has() pull out + {__.out().as("a").out().match(as("a").has("name", "marko"), as("a").out().as("b")), __.out().as("a").out().match(as("a").has("name", "marko"), as("a").out().as("b")), Collections.emptyList()}, // no has() pull out + {__.map(__.match(as("a").has("name", "marko"), as("a").out().as("b"))), __.map(__.match(as("a").has("name", "marko"), as("a").out().as("b"))), Collections.emptyList()}, // no has() pull out + {__.out().as("c").match(as("a").has("name", "marko"), as("a").out().as("b")), __.out().as("c").match(as("a").has("name", "marko"), as("a").out().as("b")), Collections.emptyList()}, // no has() pull out + ///////// + {__.match(as("a").out().as("b"), __.where(as("b").out("knows").as("c"))), __.match(as("a").out().as("b"), as("b").where(__.out("knows").as("c"))), Collections.emptyList()}, // make as().where() + {__.match(as("a").out().as("b"), __.where("a", P.gt("b"))), __.match(as("a").out().as("b"), __.as("a").where(P.gt("b"))), Collections.emptyList()}, // make as().where() + ///////// + // {__.match(as("a").out().as("b")).where(as("b").out("knows").as("c")), __.match(as("a").out().as("b"), as("b").where(__.out("knows").as("c"))), Collections.emptyList()}, // make as().where() + // {__.match(as("a").out().as("b")).where("a", P.gt("b")), __.match(as("a").out().as("b"), __.as("a").where(P.gt("b"))), Collections.emptyList()}, // make as().where() + }); } - private static abstract class AbstractMatchPredicateStrategyTest { - - protected TraversalEngine traversalEngine; - - void applyMatchWhereStrategy(final Traversal traversal) { - final TraversalStrategies strategies = new DefaultTraversalStrategies(); - strategies.addStrategies(MatchPredicateStrategy.instance(), IdentityRemovalStrategy.instance()); - traversal.asAdmin().setStrategies(strategies); - //traversal.asAdmin().setEngine(this.traversalEngine); - traversal.asAdmin().applyStrategies(); - } - - public void doTest(final Traversal traversal, final Traversal optimized) { - applyMatchWhereStrategy(traversal); - assertEquals(optimized, traversal); - } - - static Iterable generateTestParameters() { - - return Arrays.asList(new Traversal[][]{ - {__.out().match(as("a").has("name", "marko"), as("a").out().as("b")), __.out().as("a").has("name", "marko").match(as("a").out().as("b"))}, // has() pull out - {__.out().as("a").match(as("a").has("name", "marko"), as("a").out().as("b")), __.out().as("a").has("name", "marko").match(as("a").out().as("b"))}, // has() pull out - {__.out().as("a").out().match(as("a").has("name", "marko"), as("a").out().as("b")), __.out().as("a").out().match(as("a").has("name", "marko"), as("a").out().as("b"))}, // no has() pull out - {__.map(__.match(as("a").has("name", "marko"), as("a").out().as("b"))), __.map(__.match(as("a").has("name", "marko"), as("a").out().as("b")))}, // no has() pull out - {__.out().as("c").match(as("a").has("name", "marko"), as("a").out().as("b")), __.out().as("c").match(as("a").has("name", "marko"), as("a").out().as("b"))}, // no has() pull out - ///////// - {__.match(as("a").out().as("b"), __.where(as("b").out("knows").as("c"))), __.match(as("a").out().as("b"), as("b").where(__.out("knows").as("c")))}, // make as().where() - {__.match(as("a").out().as("b"), __.where("a", P.gt("b"))), __.match(as("a").out().as("b"), __.as("a").where(P.gt("b")))}, // make as().where() - ///////// - //{__.match(as("a").out().as("b")).where(as("b").out("knows").as("c")), __.match(as("a").out().as("b"), as("b").where(__.out("knows").as("c")))}, // make as().where() - //{__.match(as("a").out().as("b")).where("a", P.gt("b")), __.match(as("a").out().as("b"), __.as("a").where(P.gt("b")))}, // make as().where() - }); - } - } } From bf14ef708ca0c7d992e56db57352e5159c97afda Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Tue, 27 Sep 2016 08:57:04 -0600 Subject: [PATCH 20/22] more tests to MatchPredicateStrategyTest -- as()-pullouts on where()-inserts works. --- .../strategy/optimization/MatchPredicateStrategy.java | 6 +++++- .../strategy/optimization/MatchPredicateStrategyTest.java | 6 ++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/MatchPredicateStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/MatchPredicateStrategy.java index a93dcd98a93..28e5677378b 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/MatchPredicateStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/MatchPredicateStrategy.java @@ -21,6 +21,8 @@ import org.apache.tinkerpop.gremlin.process.traversal.Step; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.DefaultGraphTraversal; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.DedupGlobalStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.WherePredicateStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.WhereTraversalStep; @@ -72,7 +74,9 @@ public void apply(final Traversal.Admin traversal) { (nextStep instanceof SelectOneStep && ((SelectOneStep) nextStep).getLocalChildren().isEmpty())) { if (nextStep instanceof WherePredicateStep || nextStep instanceof WhereTraversalStep) { traversal.removeStep(nextStep); - matchStep.addGlobalChild(new DefaultTraversal<>().addStep(nextStep)); + matchStep.addGlobalChild(traversal instanceof GraphTraversal ? + new DefaultGraphTraversal<>().addStep(nextStep) : + new DefaultTraversal<>().addStep(nextStep)); nextStep = matchStep.getNextStep(); } else if (nextStep instanceof DedupGlobalStep && !((DedupGlobalStep) nextStep).getScopeKeys().isEmpty() && ((DedupGlobalStep) nextStep).getLocalChildren().isEmpty() && !TraversalHelper.onGraphComputer(traversal)) { traversal.removeStep(nextStep); diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/MatchPredicateStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/MatchPredicateStrategyTest.java index 509004ce9e4..8037bd64f59 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/MatchPredicateStrategyTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/MatchPredicateStrategyTest.java @@ -24,6 +24,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversalStrategies; +import org.apache.tinkerpop.gremlin.structure.Vertex; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -33,6 +34,7 @@ import java.util.Collections; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.as; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.has; import static org.junit.Assert.assertEquals; /** @@ -75,8 +77,8 @@ public static Iterable generateTestParameters() { {__.match(as("a").out().as("b"), __.where(as("b").out("knows").as("c"))), __.match(as("a").out().as("b"), as("b").where(__.out("knows").as("c"))), Collections.emptyList()}, // make as().where() {__.match(as("a").out().as("b"), __.where("a", P.gt("b"))), __.match(as("a").out().as("b"), __.as("a").where(P.gt("b"))), Collections.emptyList()}, // make as().where() ///////// - // {__.match(as("a").out().as("b")).where(as("b").out("knows").as("c")), __.match(as("a").out().as("b"), as("b").where(__.out("knows").as("c"))), Collections.emptyList()}, // make as().where() - // {__.match(as("a").out().as("b")).where("a", P.gt("b")), __.match(as("a").out().as("b"), __.as("a").where(P.gt("b"))), Collections.emptyList()}, // make as().where() + {__.match(as("a").out().as("b")).where(as("b").filter(has("name")).out("knows").as("c")), __.match(as("a").out().as("b"), as("b").where(has("name").out("knows").as("c"))), Collections.singletonList(InlineFilterStrategy.instance())}, // make as().where() + {__.match(as("a").out().as("b")).where("a", P.gt("b")), __.match(as("a").out().as("b"), as("a").where(P.gt("b"))), Collections.emptyList()}, // make as().where() }); } From bb5e71be7017f9fa2da5ce32b8c9f767e8d90acc Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Tue, 27 Sep 2016 10:40:03 -0600 Subject: [PATCH 21/22] more test cases for InlineFilterStrategy and added a constraint to make it less likely to happen for and() and filter(). --- .../optimization/InlineFilterStrategy.java | 31 +++++++++++++++++-- .../traversal/util/TraversalHelper.java | 12 ++++++- .../InlineFilterStrategyTest.java | 13 ++++++-- 3 files changed, 50 insertions(+), 6 deletions(-) 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 cbc7748de65..c3f913ec38e 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 @@ -23,9 +23,14 @@ import org.apache.tinkerpop.gremlin.process.traversal.Step; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.step.LambdaHolder; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.AndStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.DedupGlobalStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.DropStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.FilterStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.HasStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.RangeGlobalStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.filter.TailGlobalStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.TraversalFilterStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.MatchStep; import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyStep; @@ -71,7 +76,13 @@ public void apply(final Traversal.Admin traversal) { // filter(x.y) --> x.y for (final TraversalFilterStep step : TraversalHelper.getStepsOfClass(TraversalFilterStep.class, traversal)) { final Traversal.Admin childTraversal = step.getLocalChildren().get(0); - if (TraversalHelper.allStepsInstanceOf(childTraversal, FilterStep.class)) { + if (TraversalHelper.hasAllStepsOfClass(childTraversal, FilterStep.class) && + !TraversalHelper.hasStepOfClass(childTraversal, + DropStep.class, + RangeGlobalStep.class, + TailGlobalStep.class, + DedupGlobalStep.class, + LambdaHolder.class)) { changed = true; TraversalHelper.applySingleLevelStrategies(traversal, childTraversal, InlineFilterStrategy.class); final Step finalStep = childTraversal.getEndStep(); @@ -82,7 +93,20 @@ public void apply(final Traversal.Admin traversal) { } // and(x,y) --> x.y for (final AndStep step : TraversalHelper.getStepsOfClass(AndStep.class, traversal)) { - if (!step.getLocalChildren().stream().filter(t -> !TraversalHelper.allStepsInstanceOf(t, FilterStep.class)).findAny().isPresent()) { + boolean process = true; + for (final Traversal.Admin childTraversal : step.getLocalChildren()) { + if (!TraversalHelper.hasAllStepsOfClass(childTraversal, FilterStep.class) || + TraversalHelper.hasStepOfClass(childTraversal, + DropStep.class, + RangeGlobalStep.class, + TailGlobalStep.class, + DedupGlobalStep.class, + LambdaHolder.class)) { + process = false; + break; + } + } + if (process) { changed = true; final List> childTraversals = (List) step.getLocalChildren(); Step finalStep = null; @@ -105,7 +129,7 @@ public void apply(final Traversal.Admin traversal) { if (null != startLabel) { for (final Traversal.Admin matchTraversal : new ArrayList<>(step.getGlobalChildren())) { if (!(step.getPreviousStep() instanceof EmptyStep) && - TraversalHelper.allStepsInstanceOf(matchTraversal, + TraversalHelper.hasAllStepsOfClass(matchTraversal, HasStep.class, MatchStep.MatchStartStep.class, MatchStep.MatchEndStep.class) && @@ -128,6 +152,7 @@ public void apply(final Traversal.Admin traversal) { } } } + } private static final String determineStartLabelForHasPullOut(final MatchStep matchStep) { 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 d38ac6ef35c..2192666d159 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 @@ -579,7 +579,7 @@ public static void copyLabels(final Step fromStep, final Step toStep } } - public static boolean allStepsInstanceOf(final Traversal.Admin traversal, final Class... classesToCheck) { + public static boolean hasAllStepsOfClass(final Traversal.Admin traversal, final Class... classesToCheck) { for (final Step step : traversal.getSteps()) { boolean foundInstance = false; for (final Class classToCheck : classesToCheck) { @@ -594,6 +594,16 @@ public static boolean allStepsInstanceOf(final Traversal.Admin traversal, return true; } + public static boolean hasStepOfClass(final Traversal.Admin traversal, final Class... classesToCheck) { + for (final Step step : traversal.getSteps()) { + for (final Class classToCheck : classesToCheck) { + if (classToCheck.isInstance(step)) + return true; + } + } + return false; + } + public static void applySingleLevelStrategies(final Traversal.Admin parentTraversal, final Traversal.Admin childTraversal, final Class stopAfterStrategy) { childTraversal.setStrategies(parentTraversal.getStrategies()); childTraversal.setSideEffects(parentTraversal.getSideEffects()); diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategyTest.java index e9e605a5e71..aeae593ac39 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategyTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategyTest.java @@ -32,11 +32,15 @@ import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.V; 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.__.dedup; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.drop; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.filter; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.has; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.limit; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.map; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.match; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.out; +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.tail; import static org.junit.Assert.assertEquals; /** @@ -67,8 +71,8 @@ public static Iterable generateTestParameters() { {filter(out("knows")), filter(out("knows"))}, {filter(has("age", P.gt(10))).as("a"), has("age", P.gt(10)).as("a")}, {filter(has("age", P.gt(10)).as("b")).as("a"), has("age", P.gt(10)).as("b", "a")}, - {filter(has("age", P.gt(10))), has("age", P.gt(10))}, - {filter(and(has("age", P.gt(10)), has("name", "marko"))), has("age", P.gt(10)).has("name", "marko")}, + {filter(has("age", P.gt(10)).as("a")), has("age", P.gt(10)).as("a")}, + {filter(and(has("age", P.gt(10)).as("a"), has("name", "marko"))), has("age", P.gt(10)).as("a").has("name", "marko")}, // {and(has("age", P.gt(10)), filter(has("age", 22))), has("age", P.gt(10)).has("age", 22)}, {and(has("age", P.gt(10)).as("a"), and(filter(has("age", 22).as("b")).as("c"), has("name", "marko").as("d"))), has("age", P.gt(10)).as("a").has("age", 22).as("b", "c").has("name", "marko").as("d")}, @@ -79,6 +83,11 @@ public static Iterable generateTestParameters() { {map(match(as("a").has("age", 10), as("a").filter(has("name")).as("b"))), map(match(as("a").has("age", 10), as("a").has("name").as("b")))}, {V().match(as("a").has("age", 10)), V().as("a").has("age", 10)}, {V().match(as("a").has("age", 10).as("b"), as("a").filter(has("name")).as("b")), V().as("a").has("age", 10).as("b").match(as("a").has("name").as("b"))}, + // + {filter(dedup()), filter(dedup())}, + {filter(filter(drop())), filter(drop())}, + {and(has("name"), limit(10).has("age")), and(has("name"), limit(10).has("age"))}, + {filter(tail(10).as("a")), filter(tail(10).as("a"))}, }); } } From 8feb995868291a05b615d7b5f49c2f1f241817c3 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Tue, 27 Sep 2016 10:59:04 -0600 Subject: [PATCH 22/22] TreeStep should trigger a failure in IncidentToAdjacentStrategy. Fixed and added test case. TINKERPOP-1423 --- CHANGELOG.asciidoc | 1 + .../optimization/IncidentToAdjacentStrategy.java | 4 +++- .../IncidentToAdjacentStrategyTest.java | 16 ++++++---------- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index bada500bdfb..06164341b2e 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.3 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Fixed a bug in `IncidentToAdjacentStrategy` where `TreeStep` traversals were allowed. * Fixed a end-step label bug in `MatchPredicateStrategy`. * Fixed a bug in `MatchPredicateStrategy` where inlined traversals did not have strategies applied to it. * Fixed a bug in `RepeatUnrollStrategy` where inlined traversal did not have strategies applied to it. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/IncidentToAdjacentStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/IncidentToAdjacentStrategy.java index 18e1c506ee9..153e43a9849 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/IncidentToAdjacentStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/IncidentToAdjacentStrategy.java @@ -25,6 +25,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.map.EdgeOtherVertexStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.EdgeVertexStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.PathStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.map.TreeStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.VertexStep; import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper; @@ -57,13 +58,14 @@ * __.bothE().otherV() // is replaced by __.both() * __.bothE().bothV() // will not be modified * __.outE().inV().path() // will not be modified + * __.outE().inV().tree() // will not be modified * */ public final class IncidentToAdjacentStrategy extends AbstractTraversalStrategy implements TraversalStrategy.OptimizationStrategy { private static final IncidentToAdjacentStrategy INSTANCE = new IncidentToAdjacentStrategy(); - private static final Set INVALIDATING_STEP_CLASSES = new HashSet<>(Arrays.asList(PathStep.class, LambdaHolder.class)); + private static final Set INVALIDATING_STEP_CLASSES = new HashSet<>(Arrays.asList(PathStep.class, TreeStep.class, LambdaHolder.class)); private IncidentToAdjacentStrategy() { } diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/IncidentToAdjacentStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/IncidentToAdjacentStrategyTest.java index 7139f7b45ac..6e56ab80e94 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/IncidentToAdjacentStrategyTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/IncidentToAdjacentStrategyTest.java @@ -39,24 +39,19 @@ @RunWith(Parameterized.class) public class IncidentToAdjacentStrategyTest { - @Parameterized.Parameter(value = 0) public Traversal original; @Parameterized.Parameter(value = 1) public Traversal optimized; - void applyIncidentToAdjacentStrategy(final Traversal traversal) { - final TraversalStrategies strategies = new DefaultTraversalStrategies(); - strategies.addStrategies(IncidentToAdjacentStrategy.instance()); - traversal.asAdmin().setStrategies(strategies); - traversal.asAdmin().applyStrategies(); - } - @Test public void doTest() { - applyIncidentToAdjacentStrategy(original); - assertEquals(optimized, original); + final TraversalStrategies strategies = new DefaultTraversalStrategies(); + strategies.addStrategies(IncidentToAdjacentStrategy.instance()); + this.original.asAdmin().setStrategies(strategies); + this.original.asAdmin().applyStrategies(); + assertEquals(this.optimized, this.original); } @Parameterized.Parameters(name = "{0}") @@ -74,6 +69,7 @@ public static Iterable generateTestParameters() { {__.bothE().outV(), __.bothE().outV()}, {__.outE().as("a").inV(), __.outE().as("a").inV()}, // todo: this can be optimized, but requires a lot more checks {__.outE().inV().path(), __.outE().inV().path()}, + {__.outE().inV().tree().as("a"), __.outE().inV().tree().as("a")}, {__.outE().inV().map(lambda), __.outE().inV().map(lambda)}, {__.union(__.outE().inV(), __.inE().outV()).path(), __.union(__.outE().inV(), __.inE().outV()).path()}, {__.as("a").outE().inV().as("b"), __.as("a").out().as("b")}});