From da02d96594139d3187c93ada35fd670d688ba81d Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Thu, 2 Mar 2017 10:44:58 -0500 Subject: [PATCH 01/17] TINKERPOP-1642 Improved performance of addV() and chained mutating statements Added more tests for Parameters. Performance improved by about 2.5x given the benchmarks. Also long chained mutating traversals are now inserting roughly on par with single insert traversals (prior to this change they were about 25% slower). --- .../traversal/dsl/graph/GraphTraversal.java | 9 +- .../traversal/step/util/Parameters.java | 33 ++++- .../traversal/step/util/ParametersTest.java | 119 +++++++++++++++++- 3 files changed, 146 insertions(+), 15 deletions(-) diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java index bde1dea35a2..afa23d7ace3 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java @@ -1010,7 +1010,6 @@ public default GraphTraversal addV(final Object... propertyKeyValues) for (int i = 0; i < propertyKeyValues.length; i = i + 2) { this.property(propertyKeyValues[i], propertyKeyValues[i + 1]); } - //((AddVertexStep) this.asAdmin().getEndStep()).addPropertyMutations(propertyKeyValues); return (GraphTraversal) this; } @@ -2055,11 +2054,13 @@ public default GraphTraversal property(final VertexProperty.Cardinality ca this.asAdmin().getBytecode().addStep(Symbols.property, key, value, keyValues); else this.asAdmin().getBytecode().addStep(Symbols.property, cardinality, key, value, keyValues); + // if it can be detected that this call to property() is related to an addV/E() then we can attempt to fold // the properties into that step to gain an optimization for those graphs that support such capabilities. - if ((this.asAdmin().getEndStep() instanceof AddVertexStep || this.asAdmin().getEndStep() instanceof AddEdgeStep - || this.asAdmin().getEndStep() instanceof AddVertexStartStep) && keyValues.length == 0 && null == cardinality) { - ((Mutating) this.asAdmin().getEndStep()).addPropertyMutations(key, value); + final Step endStep = this.asAdmin().getEndStep(); + if ((endStep instanceof AddVertexStep || endStep instanceof AddEdgeStep || endStep instanceof AddVertexStartStep) && + keyValues.length == 0 && null == cardinality) { + ((Mutating) endStep).addPropertyMutations(key, value); } else { this.asAdmin().addStep(new AddPropertyStep(this.asAdmin(), cardinality, key, value)); ((AddPropertyStep) this.asAdmin().getEndStep()).addPropertyMutations(keyValues); diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java index 3584c87e7ad..67cb2f9d98e 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java @@ -48,6 +48,11 @@ public final class Parameters implements Cloneable, Serializable { private Map> parameters = new HashMap<>(); + /** + * Determines if there are traversals present in the parameters {@code Map}. + */ + private boolean traversalsPresent = false; + /** * Checks for existence of key in parameter set. * @@ -138,6 +143,11 @@ public void set(final Object... keyValues) { Parameters.legalPropertyKeyValueArray(keyValues); for (int i = 0; i < keyValues.length; i = i + 2) { if (keyValues[i + 1] != null) { + // track whether or not traversals are present so that elsewhere in Parameters there is no need + // to iterate all values to not find any. + if (!traversalsPresent && keyValues[i + 1] instanceof Traversal.Admin) + traversalsPresent = true; + List values = this.parameters.get(keyValues[i]); if (null == values) { values = new ArrayList<>(); @@ -150,18 +160,31 @@ public void set(final Object... keyValues) { } } + /** + * Calls {@link TraversalParent#integrateChild(Traversal.Admin)} on any traversal objects in the parameter list. + * This method requires that {@link #set(Object...)} is called prior to this method as the it will switch the + * {@code traversalsPresent} flag field if a {@link Traversal.Admin} object is present and allow this method to + * do its work. + */ public void integrateTraversals(final TraversalParent step) { - for (final List values : this.parameters.values()) { - for (final Object object : values) { - if (object instanceof Traversal.Admin) { - step.integrateChild((Traversal.Admin) object); + if (traversalsPresent) { + for (final List values : this.parameters.values()) { + for (final Object object : values) { + if (object instanceof Traversal.Admin) { + step.integrateChild((Traversal.Admin) object); + } } } } } + /** + * Gets all the {@link Traversal.Admin} objects in the map of parameters. + */ public List> getTraversals() { - List> result = new ArrayList<>(); + if (!traversalsPresent) return Collections.emptyList(); + + final List> result = new ArrayList<>(); for (final List list : this.parameters.values()) { for (final Object object : list) { if (object instanceof Traversal.Admin) { diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ParametersTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ParametersTest.java index 7490dea668f..27b92930bae 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ParametersTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ParametersTest.java @@ -18,8 +18,12 @@ */ package org.apache.tinkerpop.gremlin.process.traversal.step.util; +import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; +import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.junit.Test; +import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.Map; @@ -28,11 +32,65 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.contains; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; /** * @author Stephen Mallette (http://stephen.genoprime.com) */ public class ParametersTest { + @Test + public void shouldGetKeyValuesEmpty() { + final Parameters parameters = new Parameters(); + assertThat(Arrays.equals(parameters.getKeyValues(mock(Traverser.Admin.class)), new Object[0]), is(true)); + } + + @Test + public void shouldGetKeyValues() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "b", "bat", "c", "cat"); + + final Object[] params = parameters.getKeyValues(mock(Traverser.Admin.class)); + assertEquals(6, params.length); + assertThat(Arrays.equals(new Object[] {"a", "axe", "b", "bat", "c", "cat"}, params), is(true)); + } + + @Test + public void shouldGetKeyValuesIgnoringSomeKeys() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "b", "bat", "c", "cat"); + + final Object[] params = parameters.getKeyValues(mock(Traverser.Admin.class), "b"); + assertEquals(4, params.length); + assertThat(Arrays.equals(new Object[] {"a", "axe", "c", "cat"}, params), is(true)); + } + + @Test + public void shouldGetUsingTraverserOverload() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "b", "bat", "c", "cat"); + + assertEquals(Collections.singletonList("axe"), parameters.get(mock(Traverser.Admin.class), "a", () -> "x")); + assertEquals(Collections.singletonList("bat"), parameters.get(mock(Traverser.Admin.class), "b", () -> "x")); + assertEquals(Collections.singletonList("cat"), parameters.get(mock(Traverser.Admin.class), "c", () -> "x")); + assertEquals(Collections.singletonList("zebra"), parameters.get(mock(Traverser.Admin.class), "z", () -> "zebra")); + } + + @Test + public void shouldGetMultipleUsingTraverserOverload() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat"); + + final Map> params = parameters.getRaw(); + assertEquals(3, params.size()); + assertEquals(Arrays.asList("axe", "ant"), parameters.get(mock(Traverser.Admin.class), "a", () -> "x")); + assertEquals(Arrays.asList("bat", "ball"), parameters.get(mock(Traverser.Admin.class), "b", () -> "x")); + assertEquals(Collections.singletonList("cat"), parameters.get(mock(Traverser.Admin.class), "c", () -> "x")); + assertEquals(Collections.singletonList("zebra"), parameters.get(mock(Traverser.Admin.class), "z", () -> "zebra")); + } + @Test public void shouldGetRaw() { final Parameters parameters = new Parameters(); @@ -143,11 +201,60 @@ public void shouldGetDefault() { final Parameters parameters = new Parameters(); parameters.set("a", "axe", "b", "bat", "c", "cat"); - final Map> params = parameters.getRaw(); - assertEquals(3, params.size()); - assertEquals("axe", params.get("a").get(0)); - assertEquals("bat", params.get("b").get(0)); - assertEquals("cat", params.get("c").get(0)); - assertEquals("zebra", parameters.get("z", () -> "zebra").get(0)); + assertEquals(Collections.singletonList("axe"), parameters.get("a", () -> "x")); + assertEquals(Collections.singletonList("bat"), parameters.get("b", () -> "x")); + assertEquals(Collections.singletonList("cat"), parameters.get("c", () -> "x")); + assertEquals(Collections.singletonList("zebra"), parameters.get("z", () -> "zebra")); + } + + @Test + public void shouldClone() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat"); + + final Parameters parametersClone = parameters.clone(); + + assertEquals(parameters.getRaw(), parametersClone.getRaw()); + } + + @Test + public void shouldBeDifferent() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat"); + + final Parameters parametersDifferent = new Parameters(); + parametersDifferent.set("a", "ant", "a", "axe", "b", "bat", "b", "ball", "c", "cat"); + + assertNotEquals(parameters.getRaw(), parametersDifferent.getRaw()); + } + + @Test + public void shouldGetNoTraversals() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat"); + assertEquals(Collections.emptyList(), parameters.getTraversals()); + } + + @Test + public void shouldGetTraversals() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat", "t", __.outE("knows")); + assertEquals(Collections.singletonList(__.outE("knows")), parameters.getTraversals()); + } + + @Test + public void shouldIntegrateTraversals() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat", "t", __.outE("knows")); + + final TraversalParent mock = mock(TraversalParent.class); + + // the mock can return nothing of consequence as the return isn't used in this case. we just need to + // validate that the method gets called as a result of calls to set/integrateTraversals() + when(mock.integrateChild(__.outE("knows").asAdmin())).thenReturn(null); + + parameters.integrateTraversals(mock); + + verify(mock).integrateChild(__.outE("knows").asAdmin()); } } From 3dea5bdd9b8ce2bf6d28882e38e07d93162bcb11 Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Fri, 3 Mar 2017 06:47:56 -0500 Subject: [PATCH 02/17] TINKERPOP-1642 Cached the Traversal list in Parameters This leads to a pretty good boost in performance from the prior commit especially for long chained mutating traversals that add vertices and edges as there is a 3X improvement in those cases. --- .../traversal/step/util/Parameters.java | 59 ++++++++++++------- .../traversal/step/util/ParametersTest.java | 50 ++++++++++++++++ 2 files changed, 87 insertions(+), 22 deletions(-) diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java index 67cb2f9d98e..3f0cb7c694b 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java @@ -26,6 +26,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil; import org.apache.tinkerpop.gremlin.structure.Element; import org.apache.tinkerpop.gremlin.structure.T; +import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils; import java.io.Serializable; import java.util.ArrayList; @@ -49,9 +50,11 @@ public final class Parameters implements Cloneable, Serializable { private Map> parameters = new HashMap<>(); /** - * Determines if there are traversals present in the parameters {@code Map}. + * A cached list of traversals that server as parameter values. The list is cached on calls to + * {@link #set(Object...)} because when the parameter map is large the cost of iterating it repeatedly on the + * high number of calls to {@link #getTraversals()} and {@link #integrateTraversals(TraversalParent)} is great. */ - private boolean traversalsPresent = false; + private List traversals = new ArrayList<>(); /** * Checks for existence of key in parameter set. @@ -73,6 +76,10 @@ public void rename(final Object oldKey, final Object newKey) { this.parameters.put(newKey, this.parameters.remove(oldKey)); } + /** + * Gets the list of values for a key, while resolving the values of any parameters that are {@link Traversal} + * objects. + */ public List get(final Traverser.Admin traverser, final Object key, final Supplier defaultValue) { final List values = (List) this.parameters.get(key); if (null == values) return Collections.singletonList(defaultValue.get()); @@ -102,9 +109,28 @@ public List get(final Object key, final Supplier defaultValue) { * @return the value of the removed key */ public Object remove(final Object key) { - return parameters.remove(key); + final List o = parameters.remove(key); + + // once a key is removed, it's possible that the traversal cache will need to be regenerated + if (IteratorUtils.anyMatch(o.iterator(), p -> p instanceof Traversal.Admin)) { + traversals.clear(); + traversals = new ArrayList<>(); + for (final List list : this.parameters.values()) { + for (final Object object : list) { + if (object instanceof Traversal.Admin) { + traversals.add((Traversal.Admin) object); + } + } + } + } + + return o; } + /** + * Gets the array of keys/values of the parameters while resolving parameter values that contain + * {@link Traversal} instances + */ public Object[] getKeyValues(final Traverser.Admin traverser, final Object... exceptKeys) { if (this.parameters.size() == 0) return EMPTY_ARRAY; final List keyValues = new ArrayList<>(); @@ -143,10 +169,10 @@ public void set(final Object... keyValues) { Parameters.legalPropertyKeyValueArray(keyValues); for (int i = 0; i < keyValues.length; i = i + 2) { if (keyValues[i + 1] != null) { - // track whether or not traversals are present so that elsewhere in Parameters there is no need + // track the list of traversals that are present so that elsewhere in Parameters there is no need // to iterate all values to not find any. - if (!traversalsPresent && keyValues[i + 1] instanceof Traversal.Admin) - traversalsPresent = true; + if (keyValues[i + 1] instanceof Traversal.Admin) + traversals.add((Traversal.Admin) keyValues[i + 1]); List values = this.parameters.get(keyValues[i]); if (null == values) { @@ -167,14 +193,8 @@ public void set(final Object... keyValues) { * do its work. */ public void integrateTraversals(final TraversalParent step) { - if (traversalsPresent) { - for (final List values : this.parameters.values()) { - for (final Object object : values) { - if (object instanceof Traversal.Admin) { - step.integrateChild((Traversal.Admin) object); - } - } - } + for (Traversal.Admin t : traversals) { + step.integrateChild(t); } } @@ -182,15 +202,10 @@ public void integrateTraversals(final TraversalParent step) { * Gets all the {@link Traversal.Admin} objects in the map of parameters. */ public List> getTraversals() { - if (!traversalsPresent) return Collections.emptyList(); - + // stupid generics - just need to return "traversals" final List> result = new ArrayList<>(); - for (final List list : this.parameters.values()) { - for (final Object object : list) { - if (object instanceof Traversal.Admin) { - result.add((Traversal.Admin) object); - } - } + for (Traversal.Admin t : traversals) { + result.add(t); } return result; } diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ParametersTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ParametersTest.java index 27b92930bae..9c96c1cd6c4 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ParametersTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ParametersTest.java @@ -18,6 +18,7 @@ */ package org.apache.tinkerpop.gremlin.process.traversal.step.util; +import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; @@ -31,6 +32,7 @@ import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.contains; +import static org.hamcrest.core.IsInstanceOf.instanceOf; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; import static org.mockito.Mockito.mock; @@ -152,6 +154,39 @@ public void shouldRemove() { assertEquals("cat", after.get("c").get(0)); } + @Test + public void shouldRemoveRefreshTraversalCache() { + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "b", "bat", "c", "cat", "c", mock(Traversal.Admin.class), "t", mock(Traversal.Admin.class)); + + final Map> before = parameters.getRaw(); + assertEquals(4, before.size()); + assertEquals(2, parameters.getTraversals().size()); + assertEquals("axe", before.get("a").get(0)); + assertEquals("bat", before.get("b").get(0)); + assertEquals("cat", before.get("c").get(0)); + assertThat(before.get("c").get(1), instanceOf(Traversal.Admin.class)); + assertThat(before.get("t").get(0), instanceOf(Traversal.Admin.class)); + + parameters.remove("t"); + + final Map> afterRemoveT = parameters.getRaw(); + assertEquals(3, afterRemoveT.size()); + assertEquals(1, parameters.getTraversals().size()); + assertEquals("axe", afterRemoveT.get("a").get(0)); + assertEquals("bat", afterRemoveT.get("b").get(0)); + assertEquals("cat", afterRemoveT.get("c").get(0)); + assertThat(afterRemoveT.get("c").get(1), instanceOf(Traversal.Admin.class)); + + parameters.remove("c"); + + final Map> afterRemoveC = parameters.getRaw(); + assertEquals(2, afterRemoveC.size()); + assertEquals(0, parameters.getTraversals().size()); + assertEquals("axe", afterRemoveC.get("a").get(0)); + assertEquals("bat", afterRemoveC.get("b").get(0)); + } + @Test public void shouldRename() { final Parameters parameters = new Parameters(); @@ -215,6 +250,21 @@ public void shouldClone() { final Parameters parametersClone = parameters.clone(); assertEquals(parameters.getRaw(), parametersClone.getRaw()); + assertEquals(parameters.getTraversals(), parametersClone.getTraversals()); + } + + @Test + public void shouldCloneWithTraversals() { + final Traversal.Admin t = mock(Traversal.Admin.class); + when(t.clone()).thenReturn(t); + + final Parameters parameters = new Parameters(); + parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat", "t", t); + + final Parameters parametersClone = parameters.clone(); + + assertEquals(parameters.getRaw(), parametersClone.getRaw()); + assertEquals(parameters.getTraversals().size(), parametersClone.getTraversals().size()); } @Test From e8011e4fea81dd7d4bf187ffa011ab1912335cf3 Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Fri, 3 Mar 2017 08:45:24 -0500 Subject: [PATCH 03/17] TINKERPOP-1642 Minor fixes --- .../gremlin/process/traversal/step/util/Parameters.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java index 3f0cb7c694b..6640e8714a0 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java @@ -50,7 +50,7 @@ public final class Parameters implements Cloneable, Serializable { private Map> parameters = new HashMap<>(); /** - * A cached list of traversals that server as parameter values. The list is cached on calls to + * A cached list of traversals that serve as parameter values. The list is cached on calls to * {@link #set(Object...)} because when the parameter map is large the cost of iterating it repeatedly on the * high number of calls to {@link #getTraversals()} and {@link #integrateTraversals(TraversalParent)} is great. */ @@ -129,10 +129,10 @@ public Object remove(final Object key) { /** * Gets the array of keys/values of the parameters while resolving parameter values that contain - * {@link Traversal} instances + * {@link Traversal} instances. */ public Object[] getKeyValues(final Traverser.Admin traverser, final Object... exceptKeys) { - if (this.parameters.size() == 0) return EMPTY_ARRAY; + if (this.parameters.isEmpty()) return EMPTY_ARRAY; final List keyValues = new ArrayList<>(); for (final Map.Entry> entry : this.parameters.entrySet()) { if (!ArrayUtils.contains(exceptKeys, entry.getKey())) { From b49c5beec0c7ae891b8267130d97366bee0cd20a Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Fri, 3 Mar 2017 09:27:58 -0700 Subject: [PATCH 04/17] Added ScopingStrategy which does a single TraversalHelper.getLabels() call and recurssively tells where() and select() steps the path labels accessed by the Traversal. --- CHANGELOG.asciidoc | 1 + .../traversal/TraversalStrategies.java | 2 + .../process/traversal/step/Scoping.java | 10 ++++ .../step/filter/WherePredicateStep.java | 11 +++- .../step/filter/WhereTraversalStep.java | 16 +++++- .../traversal/step/map/SelectOneStep.java | 21 +++++-- .../traversal/step/map/SelectStep.java | 15 ++++- .../finalization/ScopingStrategy.java | 57 +++++++++++++++++++ 8 files changed, 121 insertions(+), 12 deletions(-) create mode 100644 gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/ScopingStrategy.java diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 7d4e74b8ed9..82cdd8d2f77 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -44,6 +44,7 @@ TinkerPop 3.2.5 (Release Date: NOT OFFICIALLY RELEASED YET) * Added `FromToModulating` interface for use with `to()`- and `from()`-based step modulators. * Added `Path.subPath()` which supports isolating a sub-path from `Path` via to/from-labels. * Fixed an `NullPointerException` in `GraphMLReader` that occurred when an `` didn't have an ID field and the base graph supported ID assignment. +* Added `ScopingStrategy` which will computer and provide all `Scoping` steps with the path labels of the global `Traversal`. * Split `ComputerVerificationStrategy` into two strategies: `ComputerVerificationStrategy` and `ComputerFinalizationStrategy`. * Removed `HasTest.g_V_hasId_compilationEquality` from process test suite as it makes too many assumptions about provider compilation. * Deprecated `CustomizerProvider` infrastructure. 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 63ae23f2711..0ddae91d6f8 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 @@ -24,6 +24,7 @@ import org.apache.tinkerpop.gremlin.process.computer.traversal.strategy.optimization.MessagePassingReductionStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.ConnectiveStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.finalization.ProfileStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.strategy.finalization.ScopingStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.AdjacentToIncidentStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.FilterRankingStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.IncidentToAdjacentStrategy; @@ -214,6 +215,7 @@ public static final class GlobalCache { RangeByIsCountStrategy.instance(), PathRetractionStrategy.instance(), LazyBarrierStrategy.instance(), + ScopingStrategy.instance(), ProfileStrategy.instance(), StandardVerificationStrategy.instance()); GRAPH_CACHE.put(Graph.class, graphStrategies); diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java index 8c27405e276..68655e41287 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java @@ -70,4 +70,14 @@ public default S getNullableScopeValue(final Pop pop, final String key, fina } public Set getScopeKeys(); + + /** + * If a Scoping step can do intelligent optimizations by knowing the step labels being accessed globally, then it should implement this label. + * The default implementation does nothing. + * + * @param labels the step labels of the global traversal + */ + public default void setPathLabels(final Set labels) { + + } } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java index 05637f6541e..b213314f9c7 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java @@ -49,6 +49,7 @@ public final class WherePredicateStep extends FilterStep implements Scopin protected String startKey; protected List selectKeys; + private Boolean pathSelectKey = null; protected P predicate; protected final Set scopeKeys = new HashSet<>(); protected Set keepLabels; @@ -137,13 +138,21 @@ public int hashCode() { @Override public Set getRequirements() { - final Set requirements = + final Set requirements = null == this.pathSelectKey ? TraversalHelper.getLabels(TraversalHelper.getRootTraversal(this.traversal)).stream().filter(this.scopeKeys::contains).findAny().isPresent() ? + TYPICAL_GLOBAL_REQUIREMENTS : + TYPICAL_LOCAL_REQUIREMENTS : + this.pathSelectKey ? TYPICAL_GLOBAL_REQUIREMENTS : TYPICAL_LOCAL_REQUIREMENTS; return this.getSelfAndChildRequirements(requirements.toArray(new TraverserRequirement[requirements.size()])); } + @Override + public void setPathLabels(final Set labels) { + this.pathSelectKey = labels.stream().filter(this.scopeKeys::contains).findAny().isPresent(); + } + @Override public List> getLocalChildren() { return (List) this.traversalRing.getTraversals(); diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WhereTraversalStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WhereTraversalStep.java index 1dd92e2bc0e..c004d30f285 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WhereTraversalStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WhereTraversalStep.java @@ -46,6 +46,7 @@ public final class WhereTraversalStep extends FilterStep implements Traver protected Traversal.Admin whereTraversal; protected final Set scopeKeys = new HashSet<>(); + private Boolean pathSelectKey = null; protected Set keepLabels; public WhereTraversalStep(final Traversal.Admin traversal, final Traversal whereTraversal) { @@ -134,9 +135,18 @@ public int hashCode() { @Override public Set getRequirements() { - return TraversalHelper.getLabels(TraversalHelper.getRootTraversal(this.getTraversal())).stream().filter(this.scopeKeys::contains).findAny().isPresent() ? - TYPICAL_GLOBAL_REQUIREMENTS : - TYPICAL_LOCAL_REQUIREMENTS; + return null == this.pathSelectKey ? + TraversalHelper.getLabels(TraversalHelper.getRootTraversal(this.getTraversal())).stream().filter(this.scopeKeys::contains).findAny().isPresent() ? + TYPICAL_GLOBAL_REQUIREMENTS : + TYPICAL_LOCAL_REQUIREMENTS : + this.pathSelectKey ? + TYPICAL_GLOBAL_REQUIREMENTS : + TYPICAL_LOCAL_REQUIREMENTS; + } + + @Override + public void setPathLabels(final Set labels) { + this.pathSelectKey = labels.stream().filter(this.scopeKeys::contains).findAny().isPresent(); } @Override diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectOneStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectOneStep.java index a51a8d25e87..baedbcfdbe2 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectOneStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectOneStep.java @@ -41,6 +41,7 @@ public final class SelectOneStep extends MapStep implements Traversa private final Pop pop; private final String selectKey; + private Boolean pathSelectKey = null; private Traversal.Admin selectTraversal = null; private Set keepLabels; @@ -103,9 +104,17 @@ public void modulateBy(final Traversal.Admin selectTraversal) { @Override public Set getRequirements() { - return this.getSelfAndChildRequirements(TraversalHelper.getLabels(TraversalHelper.getRootTraversal(this.traversal)).contains(this.selectKey) ? - TYPICAL_GLOBAL_REQUIREMENTS_ARRAY : - TYPICAL_LOCAL_REQUIREMENTS_ARRAY); + if (null == this.pathSelectKey) + return this.getSelfAndChildRequirements(TraversalHelper.getLabels(TraversalHelper.getRootTraversal(this.traversal)).contains(this.selectKey) ? + TYPICAL_GLOBAL_REQUIREMENTS_ARRAY : + TYPICAL_LOCAL_REQUIREMENTS_ARRAY); + else + return this.getSelfAndChildRequirements(this.pathSelectKey ? TYPICAL_GLOBAL_REQUIREMENTS_ARRAY : TYPICAL_LOCAL_REQUIREMENTS_ARRAY); + } + + @Override + public void setPathLabels(final Set labels) { + this.pathSelectKey = labels.contains(this.selectKey); } @Override @@ -123,12 +132,14 @@ public void setKeepLabels(final Set labels) { } @Override - public Set getKeepLabels() { return this.keepLabels; } + public Set getKeepLabels() { + return this.keepLabels; + } @Override protected Traverser.Admin processNextStart() { final Traverser.Admin traverser = super.processNextStart(); - if(!(this.getTraversal().getParent() instanceof MatchStep)) { + if (!(this.getTraversal().getParent() instanceof MatchStep)) { PathProcessor.processTraverserPathLabels(traverser, this.keepLabels); } return traverser; diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectStep.java index 77db60f3a82..3a963803960 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectStep.java @@ -48,6 +48,7 @@ public final class SelectStep extends MapStep> implement private TraversalRing traversalRing = new TraversalRing<>(); private final Pop pop; private final List selectKeys; + private Boolean pathSelectKey = null; private final Set selectKeysSet; private Set keepLabels; @@ -120,9 +121,17 @@ public void modulateBy(final Traversal.Admin selectTraversal) { @Override public Set getRequirements() { - return this.getSelfAndChildRequirements(TraversalHelper.getLabels(TraversalHelper.getRootTraversal(this.traversal)).stream().filter(this.selectKeys::contains).findAny().isPresent() ? - TYPICAL_GLOBAL_REQUIREMENTS_ARRAY : - TYPICAL_LOCAL_REQUIREMENTS_ARRAY); + if (null == this.pathSelectKey) + return this.getSelfAndChildRequirements(TraversalHelper.getLabels(TraversalHelper.getRootTraversal(this.traversal)).stream().filter(this.selectKeys::contains).findAny().isPresent() ? + TYPICAL_GLOBAL_REQUIREMENTS_ARRAY : + TYPICAL_LOCAL_REQUIREMENTS_ARRAY); + else + return this.getSelfAndChildRequirements(this.pathSelectKey ? TYPICAL_GLOBAL_REQUIREMENTS_ARRAY : TYPICAL_LOCAL_REQUIREMENTS_ARRAY); + } + + @Override + public void setPathLabels(final Set labels) { + this.pathSelectKey = labels.stream().filter(this.selectKeysSet::contains).findAny().isPresent(); } @Override diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/ScopingStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/ScopingStrategy.java new file mode 100644 index 00000000000..ce66da4e65c --- /dev/null +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/ScopingStrategy.java @@ -0,0 +1,57 @@ +/* + * 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.finalization; + +import org.apache.tinkerpop.gremlin.process.traversal.Traversal; +import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.step.Scoping; +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.Set; + +/** + * @author Marko A. Rodriguez (http://markorodriguez.com) + */ +public final class ScopingStrategy extends AbstractTraversalStrategy implements TraversalStrategy.FinalizationStrategy { + + private static final ScopingStrategy INSTANCE = new ScopingStrategy(); + + + private ScopingStrategy() { + } + + @Override + public void apply(final Traversal.Admin traversal) { + // only operate on the root traversal and apply global step labels recursively + if (!(traversal.getParent() instanceof EmptyStep)) + return; + + final Set labels = TraversalHelper.getLabels(traversal); + for (final Scoping scoping : TraversalHelper.getStepsOfAssignableClassRecursively(Scoping.class, traversal)) { + scoping.setPathLabels(labels); + } + } + + public static final ScopingStrategy instance() { + return INSTANCE; + } +} From 5711ee26c89f02896d404b47eebe7ca75ea71e94 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Mon, 6 Mar 2017 08:07:39 -0700 Subject: [PATCH 05/17] Add a slight optimization to ScopingStrategy that will not compute step labels if no Scoping steps are defined. Also added JavaDoc and comments accordingly. --- .../gremlin/process/traversal/step/Scoping.java | 5 +++++ .../strategy/finalization/ScopingStrategy.java | 12 ++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java index 68655e41287..fae52d7072b 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java @@ -69,6 +69,11 @@ public default S getNullableScopeValue(final Pop pop, final String key, fina return null; } + /** + * Get the labels that this scoping step will access during the traversal + * + * @return the accessed labels of the scoping step + */ public Set getScopeKeys(); /** diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/ScopingStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/ScopingStrategy.java index ce66da4e65c..073f45e9165 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/ScopingStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/ScopingStrategy.java @@ -19,6 +19,7 @@ package org.apache.tinkerpop.gremlin.process.traversal.strategy.finalization; +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.Scoping; @@ -29,6 +30,10 @@ import java.util.Set; /** + * ScopingStrategy will analyze the traversal for step labels (e.g. as()) and provide {@link Scoping} steps that information. + * This enables Scoping steps to avoid having to generate step label data at {@link Step#getRequirements()} and thus, + * may significantly reduce compilation times. + * * @author Marko A. Rodriguez (http://markorodriguez.com) */ public final class ScopingStrategy extends AbstractTraversalStrategy implements TraversalStrategy.FinalizationStrategy { @@ -41,11 +46,14 @@ private ScopingStrategy() { @Override public void apply(final Traversal.Admin traversal) { - // only operate on the root traversal and apply global step labels recursively - if (!(traversal.getParent() instanceof EmptyStep)) + // only operate on the root traversal and only if it contains scoping steps + if (!(traversal.getParent() instanceof EmptyStep) || + !TraversalHelper.hasStepOfAssignableClassRecursively(Scoping.class, traversal)) return; + // get the labels associated with the traveral final Set labels = TraversalHelper.getLabels(traversal); + // tell all scoping steps what those labels are for (final Scoping scoping : TraversalHelper.getStepsOfAssignableClassRecursively(Scoping.class, traversal)) { scoping.setPathLabels(labels); } From 6294c077ff1e7a26b14749220b722d5bd124acd0 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Wed, 8 Mar 2017 07:37:07 -0700 Subject: [PATCH 06/17] Wow. Can't believe we didn't do this from the start. LABELED_PATH is set if a Step is labeled. Dar. So much simpler than all the recurssion in TraversalHelper.getLabels() and having a ScopingStrategy.... @spmallette -- can you verify that your Mutating traversal profiling is faster now. --- CHANGELOG.asciidoc | 1 + .../traversal/TraversalStrategies.java | 2 - .../process/traversal/step/Scoping.java | 15 ----- .../step/filter/WherePredicateStep.java | 15 +---- .../step/filter/WhereTraversalStep.java | 14 +--- .../traversal/step/map/SelectOneStep.java | 13 +--- .../traversal/step/map/SelectStep.java | 13 +--- .../finalization/ScopingStrategy.java | 65 ------------------- .../traversal/util/DefaultTraversal.java | 2 + .../traversal/util/TraversalHelper.java | 18 +++++ .../traversal/step/filter/WhereStepTest.java | 4 +- .../process/computer/GraphComputerTest.java | 2 + 12 files changed, 29 insertions(+), 135 deletions(-) delete mode 100644 gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/ScopingStrategy.java diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 82cdd8d2f77..6e88565136f 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -30,6 +30,7 @@ TinkerPop 3.2.5 (Release Date: NOT OFFICIALLY RELEASED YET) * De-registered metrics on Gremlin Server shutdown. * Added "help" command option on `:remote config` for plugins that support that feature in the Gremlin Console. * Allowed for multiple scripts and related arguments to be passed to `gremlin.sh` via `-i` and `-e`. +* `LABELED_PATH` requirement is now set if any step in the traversal is labeled. * Updated `PathRetractionStrategy` to not run if the provided traversal contains a `VertexProgramStep` that has a `LABELED_PATH` requirement. * Added various metrics to the `GremlinGroovyScriptEngine` around script compilation and exposed them in Gremlin Server. * Moved the `caffeine` dependency down to `gremlin-groovy` and out of `gremlin-server`. 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 0ddae91d6f8..63ae23f2711 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 @@ -24,7 +24,6 @@ import org.apache.tinkerpop.gremlin.process.computer.traversal.strategy.optimization.MessagePassingReductionStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.ConnectiveStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.finalization.ProfileStrategy; -import org.apache.tinkerpop.gremlin.process.traversal.strategy.finalization.ScopingStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.AdjacentToIncidentStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.FilterRankingStrategy; import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.IncidentToAdjacentStrategy; @@ -215,7 +214,6 @@ public static final class GlobalCache { RangeByIsCountStrategy.instance(), PathRetractionStrategy.instance(), LazyBarrierStrategy.instance(), - ScopingStrategy.instance(), ProfileStrategy.instance(), StandardVerificationStrategy.instance()); GRAPH_CACHE.put(Graph.class, graphStrategies); diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java index fae52d7072b..22109bf41b8 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java @@ -34,11 +34,6 @@ public interface Scoping { public static enum Variable {START, END} - public static final Set TYPICAL_LOCAL_REQUIREMENTS = EnumSet.of(TraverserRequirement.OBJECT, TraverserRequirement.SIDE_EFFECTS); - public static final Set TYPICAL_GLOBAL_REQUIREMENTS = EnumSet.of(TraverserRequirement.OBJECT, TraverserRequirement.LABELED_PATH, TraverserRequirement.SIDE_EFFECTS); - public static final TraverserRequirement[] TYPICAL_LOCAL_REQUIREMENTS_ARRAY = new TraverserRequirement[]{TraverserRequirement.OBJECT, TraverserRequirement.SIDE_EFFECTS}; - public static final TraverserRequirement[] TYPICAL_GLOBAL_REQUIREMENTS_ARRAY = new TraverserRequirement[]{TraverserRequirement.OBJECT, TraverserRequirement.LABELED_PATH, TraverserRequirement.SIDE_EFFECTS}; - public default S getScopeValue(final Pop pop, final String key, final Traverser.Admin traverser) throws IllegalArgumentException { if (traverser.getSideEffects().exists(key)) return traverser.getSideEffects().get(key); @@ -75,14 +70,4 @@ public default S getNullableScopeValue(final Pop pop, final String key, fina * @return the accessed labels of the scoping step */ public Set getScopeKeys(); - - /** - * If a Scoping step can do intelligent optimizations by knowing the step labels being accessed globally, then it should implement this label. - * The default implementation does nothing. - * - * @param labels the step labels of the global traversal - */ - public default void setPathLabels(final Set labels) { - - } } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java index b213314f9c7..1b248af6952 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java @@ -49,7 +49,6 @@ public final class WherePredicateStep extends FilterStep implements Scopin protected String startKey; protected List selectKeys; - private Boolean pathSelectKey = null; protected P predicate; protected final Set scopeKeys = new HashSet<>(); protected Set keepLabels; @@ -138,19 +137,7 @@ public int hashCode() { @Override public Set getRequirements() { - final Set requirements = null == this.pathSelectKey ? - TraversalHelper.getLabels(TraversalHelper.getRootTraversal(this.traversal)).stream().filter(this.scopeKeys::contains).findAny().isPresent() ? - TYPICAL_GLOBAL_REQUIREMENTS : - TYPICAL_LOCAL_REQUIREMENTS : - this.pathSelectKey ? - TYPICAL_GLOBAL_REQUIREMENTS : - TYPICAL_LOCAL_REQUIREMENTS; - return this.getSelfAndChildRequirements(requirements.toArray(new TraverserRequirement[requirements.size()])); - } - - @Override - public void setPathLabels(final Set labels) { - this.pathSelectKey = labels.stream().filter(this.scopeKeys::contains).findAny().isPresent(); + return this.getSelfAndChildRequirements(TraverserRequirement.OBJECT, TraverserRequirement.SIDE_EFFECTS); } @Override diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WhereTraversalStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WhereTraversalStep.java index c004d30f285..476ce114a27 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WhereTraversalStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WhereTraversalStep.java @@ -46,7 +46,6 @@ public final class WhereTraversalStep extends FilterStep implements Traver protected Traversal.Admin whereTraversal; protected final Set scopeKeys = new HashSet<>(); - private Boolean pathSelectKey = null; protected Set keepLabels; public WhereTraversalStep(final Traversal.Admin traversal, final Traversal whereTraversal) { @@ -135,18 +134,7 @@ public int hashCode() { @Override public Set getRequirements() { - return null == this.pathSelectKey ? - TraversalHelper.getLabels(TraversalHelper.getRootTraversal(this.getTraversal())).stream().filter(this.scopeKeys::contains).findAny().isPresent() ? - TYPICAL_GLOBAL_REQUIREMENTS : - TYPICAL_LOCAL_REQUIREMENTS : - this.pathSelectKey ? - TYPICAL_GLOBAL_REQUIREMENTS : - TYPICAL_LOCAL_REQUIREMENTS; - } - - @Override - public void setPathLabels(final Set labels) { - this.pathSelectKey = labels.stream().filter(this.scopeKeys::contains).findAny().isPresent(); + return this.getSelfAndChildRequirements(TraverserRequirement.OBJECT, TraverserRequirement.SIDE_EFFECTS); } @Override diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectOneStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectOneStep.java index baedbcfdbe2..34b8148c88f 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectOneStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectOneStep.java @@ -41,7 +41,6 @@ public final class SelectOneStep extends MapStep implements Traversa private final Pop pop; private final String selectKey; - private Boolean pathSelectKey = null; private Traversal.Admin selectTraversal = null; private Set keepLabels; @@ -104,17 +103,7 @@ public void modulateBy(final Traversal.Admin selectTraversal) { @Override public Set getRequirements() { - if (null == this.pathSelectKey) - return this.getSelfAndChildRequirements(TraversalHelper.getLabels(TraversalHelper.getRootTraversal(this.traversal)).contains(this.selectKey) ? - TYPICAL_GLOBAL_REQUIREMENTS_ARRAY : - TYPICAL_LOCAL_REQUIREMENTS_ARRAY); - else - return this.getSelfAndChildRequirements(this.pathSelectKey ? TYPICAL_GLOBAL_REQUIREMENTS_ARRAY : TYPICAL_LOCAL_REQUIREMENTS_ARRAY); - } - - @Override - public void setPathLabels(final Set labels) { - this.pathSelectKey = labels.contains(this.selectKey); + return this.getSelfAndChildRequirements(TraverserRequirement.OBJECT, TraverserRequirement.SIDE_EFFECTS); } @Override diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectStep.java index 3a963803960..167fa47fe40 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectStep.java @@ -48,7 +48,6 @@ public final class SelectStep extends MapStep> implement private TraversalRing traversalRing = new TraversalRing<>(); private final Pop pop; private final List selectKeys; - private Boolean pathSelectKey = null; private final Set selectKeysSet; private Set keepLabels; @@ -121,17 +120,7 @@ public void modulateBy(final Traversal.Admin selectTraversal) { @Override public Set getRequirements() { - if (null == this.pathSelectKey) - return this.getSelfAndChildRequirements(TraversalHelper.getLabels(TraversalHelper.getRootTraversal(this.traversal)).stream().filter(this.selectKeys::contains).findAny().isPresent() ? - TYPICAL_GLOBAL_REQUIREMENTS_ARRAY : - TYPICAL_LOCAL_REQUIREMENTS_ARRAY); - else - return this.getSelfAndChildRequirements(this.pathSelectKey ? TYPICAL_GLOBAL_REQUIREMENTS_ARRAY : TYPICAL_LOCAL_REQUIREMENTS_ARRAY); - } - - @Override - public void setPathLabels(final Set labels) { - this.pathSelectKey = labels.stream().filter(this.selectKeysSet::contains).findAny().isPresent(); + return this.getSelfAndChildRequirements(TraverserRequirement.OBJECT, TraverserRequirement.SIDE_EFFECTS); } @Override diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/ScopingStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/ScopingStrategy.java deleted file mode 100644 index 073f45e9165..00000000000 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/ScopingStrategy.java +++ /dev/null @@ -1,65 +0,0 @@ -/* - * 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.finalization; - -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.Scoping; -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.Set; - -/** - * ScopingStrategy will analyze the traversal for step labels (e.g. as()) and provide {@link Scoping} steps that information. - * This enables Scoping steps to avoid having to generate step label data at {@link Step#getRequirements()} and thus, - * may significantly reduce compilation times. - * - * @author Marko A. Rodriguez (http://markorodriguez.com) - */ -public final class ScopingStrategy extends AbstractTraversalStrategy implements TraversalStrategy.FinalizationStrategy { - - private static final ScopingStrategy INSTANCE = new ScopingStrategy(); - - - private ScopingStrategy() { - } - - @Override - public void apply(final Traversal.Admin traversal) { - // only operate on the root traversal and only if it contains scoping steps - if (!(traversal.getParent() instanceof EmptyStep) || - !TraversalHelper.hasStepOfAssignableClassRecursively(Scoping.class, traversal)) - return; - - // get the labels associated with the traveral - final Set labels = TraversalHelper.getLabels(traversal); - // tell all scoping steps what those labels are - for (final Scoping scoping : TraversalHelper.getStepsOfAssignableClassRecursively(Scoping.class, traversal)) { - scoping.setPathLabels(labels); - } - } - - public static final ScopingStrategy instance() { - return INSTANCE; - } -} diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java index 52c3027b9d6..c0e54db597f 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java @@ -147,6 +147,8 @@ public Set getTraverserRequirements() { for (final Step step : this.getSteps()) { this.requirements.addAll(step.getRequirements()); } + if (TraversalHelper.hasLabels(this)) + this.requirements.add(TraverserRequirement.LABELED_PATH); if (!this.getSideEffects().keys().isEmpty()) this.requirements.add(TraverserRequirement.SIDE_EFFECTS); if (null != this.getSideEffects().getSackInitialValue()) 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 567ce5339a0..5163824a706 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 @@ -549,6 +549,24 @@ public static void reIdSteps(final StepPosition stepPosition, final Traversal.Ad return traversal; } + public static boolean hasLabels(final Traversal.Admin traversal) { + for (final Step step : traversal.getSteps()) { + if (!step.getLabels().isEmpty()) + return true; + if (step instanceof TraversalParent) { + for (final Traversal.Admin local : ((TraversalParent) step).getLocalChildren()) { + if (TraversalHelper.hasLabels(local)) + return true; + } + for (final Traversal.Admin global : ((TraversalParent) step).getGlobalChildren()) { + if (TraversalHelper.hasLabels(global)) + return true; + } + } + } + return false; + } + public static Set getLabels(final Traversal.Admin traversal) { return TraversalHelper.getLabels(new HashSet<>(), traversal); } diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WhereStepTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WhereStepTest.java index 7f65f231193..fa9adeb1f7a 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WhereStepTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WhereStepTest.java @@ -56,10 +56,10 @@ public void shouldRequirePathsAccordingly() { Object[][] traversalPaths = new Object[][]{ {false, __.where(P.not(P.within("x"))).asAdmin()}, {true, __.as("x").where(P.not(P.within("x"))).asAdmin()}, - {false, __.as("a").where(P.not(P.within("x"))).asAdmin()}, + {true, __.as("a").where(P.not(P.within("x"))).asAdmin()}, {false, __.local(__.where(P.not(P.within("x")))).asAdmin()}, {true, __.as("x").local(__.where(P.not(P.within("x")))).asAdmin()}, - {false, __.as("a").local(__.where(P.not(P.within("x")))).asAdmin()}, + {false, __.local(__.where(P.not(P.within("x")))).asAdmin()}, }; for (final Object[] traversalPath : traversalPaths) { assertEquals(traversalPath[0], ((Traversal.Admin) traversalPath[1]).getTraverserRequirements().contains(TraverserRequirement.LABELED_PATH)); diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/computer/GraphComputerTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/computer/GraphComputerTest.java index 108550a56c9..40b03fed9c3 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/computer/GraphComputerTest.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/computer/GraphComputerTest.java @@ -47,6 +47,7 @@ import org.apache.tinkerpop.gremlin.structure.VertexProperty; import org.apache.tinkerpop.gremlin.structure.util.StringFactory; import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils; +import org.junit.Ignore; import org.junit.Test; import java.util.AbstractMap; @@ -2381,6 +2382,7 @@ public void shouldSucceedWithProperTraverserRequirements() throws Exception { @Test @LoadGraphWith(MODERN) + @Ignore("Labeled paths now trigger LABELED_PATH requirements -- @dkuppitz") public void shouldFailWithImproperTraverserRequirements() throws Exception { final AtomicInteger counter = new AtomicInteger(0); From f693cb771d97eb98726babc6bafb2bb1b03c338a Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Fri, 10 Mar 2017 12:31:38 -0500 Subject: [PATCH 07/17] TINKERPOP-1642 Removed some extra iteration in Parameters.getTraversals() --- .../gremlin/process/traversal/step/util/Parameters.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java index 6640e8714a0..93cf1f8fd8a 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java @@ -54,7 +54,7 @@ public final class Parameters implements Cloneable, Serializable { * {@link #set(Object...)} because when the parameter map is large the cost of iterating it repeatedly on the * high number of calls to {@link #getTraversals()} and {@link #integrateTraversals(TraversalParent)} is great. */ - private List traversals = new ArrayList<>(); + private List> traversals = new ArrayList<>(); /** * Checks for existence of key in parameter set. @@ -203,11 +203,7 @@ public void integrateTraversals(final TraversalParent step) { */ public List> getTraversals() { // stupid generics - just need to return "traversals" - final List> result = new ArrayList<>(); - for (Traversal.Admin t : traversals) { - result.add(t); - } - return result; + return (List>) (Object) traversals; } public Parameters clone() { From 88a3f60ce66ce4ff55de8ecefabfc4b22f277f62 Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Fri, 10 Mar 2017 13:27:01 -0500 Subject: [PATCH 08/17] TINKERPOP-1642 Minor refactoring to get rid of duplicate code --- .../traversal/util/TraversalHelper.java | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) 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 5163824a706..95862d0725f 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 @@ -417,7 +417,7 @@ public static boolean hasStepOfAssignableClassRecursively(final Scope scope, fin * Determine if any step in {@link Traversal} or its children match the step given the provided {@link Predicate}. * * @param predicate the match function - * @param traversal th traversal to perform the action on + * @param traversal the traversal to perform the action on * @return {@code true} if there is a match and {@code false} otherwise */ public static boolean anyStepRecursively(final Predicate predicate, final Traversal.Admin traversal) { @@ -425,18 +425,19 @@ public static boolean anyStepRecursively(final Predicate predicate, final if (predicate.test(step)) { return true; } - if (step instanceof TraversalParent) { - for (final Traversal.Admin localChild : ((TraversalParent) step).getLocalChildren()) { - if (anyStepRecursively(predicate, localChild)) return true; - } - for (final Traversal.Admin globalChild : ((TraversalParent) step).getGlobalChildren()) { - if (anyStepRecursively(predicate, globalChild)) return true; - } - } + + if (step instanceof TraversalParent) anyStepRecursively(predicate, ((TraversalParent) step)); } return false; } + /** + * Determine if any child step of a {@link TraversalParent} match the step given the provided {@link Predicate}. + * + * @param predicate the match function + * @param step the step to perform the action on + * @return {@code true} if there is a match and {@code false} otherwise + */ public static boolean anyStepRecursively(final Predicate predicate, final TraversalParent step) { for (final Traversal.Admin localChild : step.getLocalChildren()) { if (anyStepRecursively(predicate, localChild)) return true; From 1f99a5103bfaec758d657078362ac666fe47e8d2 Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Fri, 10 Mar 2017 15:11:52 -0500 Subject: [PATCH 09/17] TINKERPOP-1642 Made Mutating steps implement Scoping. This simplified PathUtil.getReferencedLabels() and reduced iterations over Parameter traversals. --- CHANGELOG.asciidoc | 1 + .../process/traversal/Parameterizing.java | 8 ++++ .../process/traversal/step/Scoping.java | 14 ++++--- .../traversal/step/map/AddEdgeStep.java | 11 +++++- .../step/map/AddVertexStartStep.java | 9 ++++- .../traversal/step/map/AddVertexStep.java | 9 ++++- .../step/sideEffect/AddPropertyStep.java | 9 ++++- .../traversal/step/util/Parameters.java | 37 ++++++++++++++++--- .../process/traversal/util/PathUtil.java | 17 +-------- 9 files changed, 85 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 6e88565136f..8dd8d9a84a5 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.5 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* `Mutating` steps now implement `Scoping` interface. * Fixed a step id compilation bug in `AddVertexStartStep`, `AddVertexStep`, `AddEdgeStep`, and `AddPropertyStep`. * De-registered metrics on Gremlin Server shutdown. * Added "help" command option on `:remote config` for plugins that support that feature in the Gremlin Console. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Parameterizing.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Parameterizing.java index 8af80b3356e..9b7e088f472 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Parameterizing.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Parameterizing.java @@ -21,9 +21,17 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters; /** + * An interface for {@link Step} implementations that hold a {@link Parameters} object, which fold in arguments from + * other steps. It is typically used on mutating steps, such as {@code addV()}, where calls to {@code property(k,v)} + * are folded in as parameters to that add vertex step. + * * @author Marko A. Rodriguez (http://markorodriguez.com) + * @author Stephen Mallette (http://stephen.genoprime.com) */ public interface Parameterizing { + /** + * Gets the parameters on the step. + */ public Parameters getParameters(); } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java index 22109bf41b8..683e6618153 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java @@ -20,23 +20,27 @@ import org.apache.tinkerpop.gremlin.process.traversal.Path; import org.apache.tinkerpop.gremlin.process.traversal.Pop; +import org.apache.tinkerpop.gremlin.process.traversal.Step; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; -import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; -import java.util.EnumSet; import java.util.Map; import java.util.Set; /** + * This interface is implemented by {@link Step} implementations that access labeled path steps, side-effects or + * {@code Map} values by key, such as {@code select('a')} step. Note that a step like {@code project()} is non-scoping + * because while it creates a {@code Map} it does not introspect them. + * * @author Marko A. Rodriguez (http://markorodriguez.com) + * @author Stephen Mallette (http://stephen.genoprime.com) */ public interface Scoping { - public static enum Variable {START, END} + public enum Variable {START, END} public default S getScopeValue(final Pop pop, final String key, final Traverser.Admin traverser) throws IllegalArgumentException { if (traverser.getSideEffects().exists(key)) - return traverser.getSideEffects().get(key); + return traverser.getSideEffects().get(key); /// final Object object = traverser.get(); if (object instanceof Map && ((Map) object).containsKey(key)) @@ -51,7 +55,7 @@ public default S getScopeValue(final Pop pop, final String key, final Traver public default S getNullableScopeValue(final Pop pop, final String key, final Traverser.Admin traverser) { if (traverser.getSideEffects().exists(key)) - return traverser.getSideEffects().get(key); + return traverser.getSideEffects().get(key); /// final Object object = traverser.get(); if (object instanceof Map && ((Map) object).containsKey(key)) diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStep.java index 13efc8e98af..3a46c0d4ecd 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStep.java @@ -23,6 +23,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traverser; import org.apache.tinkerpop.gremlin.process.traversal.step.FromToModulating; import org.apache.tinkerpop.gremlin.process.traversal.step.Mutating; +import org.apache.tinkerpop.gremlin.process.traversal.step.Scoping; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters; import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.CallbackRegistry; @@ -36,6 +37,8 @@ import org.apache.tinkerpop.gremlin.structure.util.StringFactory; import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedFactory; +import java.util.HashSet; +import java.util.Iterator; import java.util.List; import java.util.Set; @@ -43,7 +46,8 @@ * @author Marko A. Rodriguez (http://markorodriguez.com) * @author Stephen Mallette (http://stephen.genoprime.com) */ -public final class AddEdgeStep extends MapStep implements Mutating, TraversalParent, Parameterizing, FromToModulating { +public final class AddEdgeStep extends MapStep + implements Mutating, TraversalParent, Parameterizing, Scoping, FromToModulating { private static final String FROM = Graph.Hidden.hide("from"); private static final String TO = Graph.Hidden.hide("to"); @@ -66,6 +70,11 @@ public Parameters getParameters() { return this.parameters; } + @Override + public Set getScopeKeys() { + return this.parameters.getReferencedLabels(); + } + @Override public void addPropertyMutations(final Object... keyValues) { this.parameters.set(keyValues); diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexStartStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexStartStep.java index e7939dce119..064fc79c951 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexStartStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexStartStep.java @@ -24,6 +24,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traverser; import org.apache.tinkerpop.gremlin.process.traversal.TraverserGenerator; import org.apache.tinkerpop.gremlin.process.traversal.step.Mutating; +import org.apache.tinkerpop.gremlin.process.traversal.step.Scoping; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep; import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters; @@ -44,7 +45,8 @@ * @author Marko A. Rodriguez (http://markorodriguez.com) * @author Stephen Mallette (http://stephen.genoprime.com) */ -public final class AddVertexStartStep extends AbstractStep implements Mutating, TraversalParent, Parameterizing { +public final class AddVertexStartStep extends AbstractStep + implements Mutating, TraversalParent, Parameterizing, Scoping { private Parameters parameters = new Parameters(); private boolean first = true; @@ -61,6 +63,11 @@ public Parameters getParameters() { return this.parameters; } + @Override + public Set getScopeKeys() { + return this.parameters.getReferencedLabels(); + } + @Override public List> getLocalChildren() { return this.parameters.getTraversals(); diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexStep.java index 57cd6161d93..69ee6e50be1 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexStep.java @@ -22,6 +22,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; import org.apache.tinkerpop.gremlin.process.traversal.step.Mutating; +import org.apache.tinkerpop.gremlin.process.traversal.step.Scoping; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters; import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.CallbackRegistry; @@ -40,7 +41,8 @@ * @author Marko A. Rodriguez (http://markorodriguez.com) * @author Stephen Mallette (http://stephen.genoprime.com) */ -public final class AddVertexStep extends MapStep implements Mutating, TraversalParent, Parameterizing { +public final class AddVertexStep extends MapStep + implements Mutating, TraversalParent, Parameterizing, Scoping { private Parameters parameters = new Parameters(); private CallbackRegistry callbackRegistry; @@ -56,6 +58,11 @@ public Parameters getParameters() { return this.parameters; } + @Override + public Set getScopeKeys() { + return this.parameters.getReferencedLabels(); + } + @Override public List> getLocalChildren() { return this.parameters.getTraversals(); diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java index 44232f098ff..85292b794d1 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java @@ -22,6 +22,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; import org.apache.tinkerpop.gremlin.process.traversal.step.Mutating; +import org.apache.tinkerpop.gremlin.process.traversal.step.Scoping; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters; import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.CallbackRegistry; @@ -45,7 +46,8 @@ /** * @author Marko A. Rodriguez (http://markorodriguez.com) */ -public final class AddPropertyStep extends SideEffectStep implements Mutating, TraversalParent, Parameterizing { +public final class AddPropertyStep extends SideEffectStep + implements Mutating, TraversalParent, Parameterizing, Scoping { private Parameters parameters = new Parameters(); private final VertexProperty.Cardinality cardinality; @@ -64,6 +66,11 @@ public Parameters getParameters() { return this.parameters; } + @Override + public Set getScopeKeys() { + return this.parameters.getReferencedLabels(); + } + @Override public List> getLocalChildren() { return this.parameters.getTraversals(); diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java index 93cf1f8fd8a..4b38d2e092c 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java @@ -22,6 +22,7 @@ import org.apache.commons.lang.ArrayUtils; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.step.Scoping; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil; import org.apache.tinkerpop.gremlin.structure.Element; @@ -33,8 +34,10 @@ import java.util.Arrays; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.function.Supplier; /** @@ -48,6 +51,7 @@ public final class Parameters implements Cloneable, Serializable { private static final Object[] EMPTY_ARRAY = new Object[0]; private Map> parameters = new HashMap<>(); + private Set referencedLabels = new HashSet<>(); /** * A cached list of traversals that serve as parameter values. The list is cached on calls to @@ -111,14 +115,15 @@ public List get(final Object key, final Supplier defaultValue) { public Object remove(final Object key) { final List o = parameters.remove(key); - // once a key is removed, it's possible that the traversal cache will need to be regenerated + // once a key is removed, it's possible that the traversal/label cache will need to be regenerated if (IteratorUtils.anyMatch(o.iterator(), p -> p instanceof Traversal.Admin)) { traversals.clear(); traversals = new ArrayList<>(); for (final List list : this.parameters.values()) { for (final Object object : list) { if (object instanceof Traversal.Admin) { - traversals.add((Traversal.Admin) object); + final Traversal.Admin t = (Traversal.Admin) object; + addTraversal(t); } } } @@ -170,9 +175,11 @@ public void set(final Object... keyValues) { for (int i = 0; i < keyValues.length; i = i + 2) { if (keyValues[i + 1] != null) { // track the list of traversals that are present so that elsewhere in Parameters there is no need - // to iterate all values to not find any. - if (keyValues[i + 1] instanceof Traversal.Admin) - traversals.add((Traversal.Admin) keyValues[i + 1]); + // to iterate all values to not find any. also grab available labels in traversal values + if (keyValues[i + 1] instanceof Traversal.Admin) { + final Traversal.Admin t = (Traversal.Admin) keyValues[i + 1]; + addTraversal(t); + } List values = this.parameters.get(keyValues[i]); if (null == values) { @@ -206,6 +213,13 @@ public List> getTraversals() { return (List>) (Object) traversals; } + /** + * Gets a list of all labels held in parameters that have a traversal as a value. + */ + public Set getReferencedLabels() { + return referencedLabels; + } + public Parameters clone() { try { final Parameters clone = (Parameters) super.clone(); @@ -217,6 +231,8 @@ public Parameters clone() { } clone.parameters.put(entry.getKey() instanceof Traversal.Admin ? ((Traversal.Admin) entry.getKey()).clone() : entry.getKey(), values); } + + clone.referencedLabels = new HashSet<>(this.referencedLabels); return clone; } catch (final CloneNotSupportedException e) { throw new IllegalStateException(e.getMessage(), e); @@ -246,4 +262,15 @@ private static void legalPropertyKeyValueArray(final Object... propertyKeyValues throw new IllegalArgumentException("The provided key/value array must have a String, T, or Traversal on even array indices"); } } + + private void addTraversal(final Traversal.Admin t) { + traversals.add(t); + for (final Object ss : t.getSteps()) { + if (ss instanceof Scoping) { + for (String label : ((Scoping) ss).getScopeKeys()) { + referencedLabels.add(label); + } + } + } + } } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/PathUtil.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/PathUtil.java index ab978369c64..cefc62a14ce 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/PathUtil.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/PathUtil.java @@ -18,19 +18,17 @@ */ package org.apache.tinkerpop.gremlin.process.traversal.util; -import org.apache.tinkerpop.gremlin.process.traversal.Parameterizing; import org.apache.tinkerpop.gremlin.process.traversal.Step; -import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.step.Scoping; 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.step.util.Parameters; import java.util.HashSet; import java.util.Set; /** * @author Ted Wilmes (http://twilmes.org) + * @author Stephen Mallette (http://stephen.genoprime.com) */ public class PathUtil { @@ -50,19 +48,6 @@ public static Set getReferencedLabelsAfterStep(Step step) { public static Set getReferencedLabels(final Step step) { final Set referencedLabels = new HashSet<>(); - if (step instanceof Parameterizing) { // TODO: we should really make the mutation steps Scoping :| - final Parameters parameters = ((Parameterizing) step).getParameters(); - for (final Traversal.Admin trav : parameters.getTraversals()) { - for (final Object ss : trav.getSteps()) { - if (ss instanceof Scoping) { - for (String label : ((Scoping) ss).getScopeKeys()) { - referencedLabels.add(label); - } - } - } - } - } - if (step instanceof Scoping) { final Set labels = new HashSet<>(((Scoping) step).getScopeKeys()); if (step instanceof MatchStep) { From 12c7f01332d1bcb07ab1dda41d4c045403975a26 Mon Sep 17 00:00:00 2001 From: Daniel Kuppitz Date: Mon, 13 Mar 2017 16:01:12 +0100 Subject: [PATCH 10/17] Fixed VertexProgram test that verifies proper TraversalRequirements. --- .../process/computer/GraphComputerTest.java | 199 ++++++++---------- 1 file changed, 92 insertions(+), 107 deletions(-) diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/computer/GraphComputerTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/computer/GraphComputerTest.java index 40b03fed9c3..5c66673b59b 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/computer/GraphComputerTest.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/computer/GraphComputerTest.java @@ -36,6 +36,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; +import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyPath; import org.apache.tinkerpop.gremlin.process.traversal.strategy.verification.VerificationException; import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; import org.apache.tinkerpop.gremlin.process.traversal.traverser.util.TraverserSet; @@ -47,6 +48,7 @@ import org.apache.tinkerpop.gremlin.structure.VertexProperty; import org.apache.tinkerpop.gremlin.structure.util.StringFactory; import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils; +import org.javatuples.Pair; import org.junit.Ignore; import org.junit.Test; @@ -58,6 +60,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; +import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Optional; @@ -108,9 +111,6 @@ @ExceptionCoverage(exceptionClass = Graph.Exceptions.class, methods = { "graphDoesNotSupportProvidedGraphComputer" }) -@ExceptionCoverage(exceptionClass = Path.Exceptions.class, methods = { - "shouldFailWithImproperTraverserRequirements" -}) @SuppressWarnings("ThrowableResultOfMethodCallIgnored") public class GraphComputerTest extends AbstractGremlinProcessTest { @@ -2351,30 +2351,26 @@ public GraphComputer.Persist getPreferredPersist() { @LoadGraphWith(MODERN) public void shouldSucceedWithProperTraverserRequirements() throws Exception { - final AtomicInteger counter = new AtomicInteger(0); - final Map idsByName = new HashMap<>(); - final VertexProgramQ vp = VertexProgramQ.build().from("a").property("coworkers").create(); - - g.V().hasLabel("person").filter(outE("created")).valueMap(true, "name").forEachRemaining((Map map) -> - idsByName.put((String) ((List) map.get("name")).get(0), map.get(id))); + final VertexProgramQ vp = VertexProgramQ.build().property("pl").create(); + final Map> expected = new HashMap<>(); + expected.put("vadas", Collections.singletonList(2)); + expected.put("lop", Arrays.asList(2, 2, 2, 3)); + expected.put("josh", Collections.singletonList(2)); + expected.put("ripple", Arrays.asList(2, 3)); try { - g.V().as("a").out("created").in("created").program(vp).dedup() - .valueMap("name", "coworkers").forEachRemaining((Map map) -> { + g.V().repeat(__.out()).emit().program(vp).dedup() + .valueMap("name", "pl").forEachRemaining((Map map) -> { final String name = (String) ((List) map.get("name")).get(0); - final Map coworkers = (Map) ((List) map.get("coworkers")).get(0); - assertTrue(idsByName.containsKey(name)); - assertEquals(2, coworkers.size()); - idsByName.keySet().stream().filter(cn -> !cn.equals(name)).forEach(cn -> { - final Object cid = idsByName.get(cn); - assertTrue(coworkers.containsKey(cid)); - assertEquals(1L, coworkers.get(cid).longValue()); - }); - counter.incrementAndGet(); + final List pathLengths = (List) map.get("pl"); + assertTrue(expected.containsKey(name)); + final List expectedPathLengths = expected.remove(name); + assertTrue(expectedPathLengths.containsAll(pathLengths)); + assertTrue(pathLengths.containsAll(expectedPathLengths)); }); - assertEquals(3, counter.intValue()); + assertTrue(expected.isEmpty()); } catch (VerificationException ex) { assumeNoException(ex); } @@ -2382,29 +2378,11 @@ public void shouldSucceedWithProperTraverserRequirements() throws Exception { @Test @LoadGraphWith(MODERN) - @Ignore("Labeled paths now trigger LABELED_PATH requirements -- @dkuppitz") public void shouldFailWithImproperTraverserRequirements() throws Exception { - - final AtomicInteger counter = new AtomicInteger(0); - final Map idsByName = new HashMap<>(); - final VertexProgramQ vp = VertexProgramQ.build().from("a").property("coworkers"). - useTraverserRequirements(false).create(); - - g.V().hasLabel("person").filter(outE("created")).valueMap(true, "name").forEachRemaining((Map map) -> - idsByName.put((String) ((List) map.get("name")).get(0), map.get(id))); - + final VertexProgramQ vp = VertexProgramQ.build().property("pl").useTraverserRequirements(false).create(); try { - g.V().as("a").out("created").in("created").program(vp).dedup() - .valueMap("name", "coworkers").forEachRemaining((Map map) -> { - - final String name = (String) ((List) map.get("name")).get(0); - final Map coworkers = (Map) ((List) map.get("coworkers")).get(0); - assertTrue(idsByName.containsKey(name)); - assertTrue(coworkers.isEmpty()); - counter.incrementAndGet(); - }); - - assertEquals(3, counter.intValue()); + g.V().repeat(__.out()).emit().program(vp).dedup() + .forEachRemaining((Vertex v) -> assertFalse(v.property("pl").isPresent())); } catch (VerificationException ex) { assumeNoException(ex); } @@ -2413,13 +2391,16 @@ public void shouldFailWithImproperTraverserRequirements() throws Exception { private static class VertexProgramQ implements VertexProgram { private static final String VERTEX_PROGRAM_Q_CFG_PREFIX = "gremlin.vertexProgramQ"; - private static final String MAP_KEY_CFG_KEY = VERTEX_PROGRAM_Q_CFG_PREFIX + ".source"; private static final String PROPERTY_CFG_KEY = VERTEX_PROGRAM_Q_CFG_PREFIX + ".property"; + private static final String LENGTHS_KEY = VERTEX_PROGRAM_Q_CFG_PREFIX + ".lengths"; private static final String USE_TRAVERSER_REQUIREMENTS_CFG_KEY = VERTEX_PROGRAM_Q_CFG_PREFIX + ".useTraverserRequirements"; + private final static Set MEMORY_COMPUTE_KEYS = Collections.singleton( + MemoryComputeKey.of(LENGTHS_KEY, Operator.addAll, true, true) + ); + private final Set elementComputeKeys; private Configuration configuration; - private String sourceKey; private String propertyKey; private Set traverserRequirements; @@ -2427,6 +2408,43 @@ private VertexProgramQ() { elementComputeKeys = new HashSet<>(); } + public static Builder build() { + return new Builder(); + } + + static class Builder extends AbstractVertexProgramBuilder { + + private Builder() { + super(VertexProgramQ.class); + } + + @SuppressWarnings("unchecked") + @Override + public VertexProgramQ create(final Graph graph) { + if (graph != null) { + ConfigurationUtils.append(graph.configuration().subset(VERTEX_PROGRAM_Q_CFG_PREFIX), configuration); + } + return (VertexProgramQ) VertexProgram.createVertexProgram(graph, configuration); + } + + public VertexProgramQ create() { + return create(null); + } + + public Builder property(final String name) { + configuration.setProperty(PROPERTY_CFG_KEY, name); + return this; + } + + /** + * This is only configurable for the purpose of testing. In a real-world VP this would be a bad pattern. + */ + public Builder useTraverserRequirements(final boolean value) { + configuration.setProperty(USE_TRAVERSER_REQUIREMENTS_CFG_KEY, value); + return this; + } + } + @Override public void storeState(final Configuration config) { VertexProgram.super.storeState(config); @@ -2435,16 +2453,16 @@ public void storeState(final Configuration config) { } } + @Override public void loadState(final Graph graph, final Configuration config) { configuration = new BaseConfiguration(); if (config != null) { ConfigurationUtils.copy(config, configuration); } - sourceKey = configuration.getString(MAP_KEY_CFG_KEY); propertyKey = configuration.getString(PROPERTY_CFG_KEY); traverserRequirements = configuration.getBoolean(USE_TRAVERSER_REQUIREMENTS_CFG_KEY, true) - ? Collections.singleton(TraverserRequirement.LABELED_PATH) : Collections.emptySet(); + ? Collections.singleton(TraverserRequirement.PATH) : Collections.emptySet(); elementComputeKeys.add(VertexComputeKey.of(propertyKey, false)); } @@ -2454,34 +2472,36 @@ public void setup(final Memory memory) { @Override public void execute(final Vertex vertex, final Messenger messenger, final Memory memory) { - final Property haltedTraversers = vertex.property(TraversalVertexProgram.HALTED_TRAVERSERS); - if (!haltedTraversers.isPresent()) return; - final Iterator iterator = haltedTraversers.value().iterator(); - if (iterator.hasNext()) { - List> list = new ArrayList<>(); - while (iterator.hasNext()) { - final Traverser t = (Traverser) iterator.next(); - try { - final Vertex source = (Vertex) t.path(sourceKey); - if (!source.id().equals(vertex.id())) { - final Map.Entry entry = new AbstractMap.SimpleEntry<>(source.id(), t.bulk()); - list.add(entry); + if (memory.isInitialIteration()) { + final Property haltedTraversers = vertex.property(TraversalVertexProgram.HALTED_TRAVERSERS); + if (!haltedTraversers.isPresent()) return; + final Iterator iterator = haltedTraversers.value().iterator(); + if (iterator.hasNext()) { + while (iterator.hasNext()) { + final Traverser t = (Traverser) iterator.next(); + if (!(t.path() instanceof EmptyPath)) { + final int pathLength = t.path().size(); + final List> memoryValue = new LinkedList<>(); + memoryValue.add(Pair.with(vertex, pathLength)); + memory.add(LENGTHS_KEY, memoryValue); + } + } + } + } else { + if (memory.exists(LENGTHS_KEY)) { + final List> lengths = memory.get(LENGTHS_KEY); + for (final Pair pair : lengths) { + if (pair.getValue0().equals(vertex)) { + vertex.property(VertexProperty.Cardinality.list, propertyKey, pair.getValue1()); } - assertFalse(traverserRequirements.isEmpty()); - } catch (Exception ex) { - assertTrue(traverserRequirements.isEmpty()); - validateException(Path.Exceptions.stepWithProvidedLabelDoesNotExist(sourceKey), ex); } } - final Map map = new HashMap<>(list.size(), 1f); - for (Map.Entry entry : list) map.put(entry.getKey(), entry.getValue()); - vertex.property(propertyKey, map); } } @Override public boolean terminate(final Memory memory) { - return memory.isInitialIteration(); + return !memory.isInitialIteration(); } @Override @@ -2494,6 +2514,11 @@ public Set getVertexComputeKeys() { return elementComputeKeys; } + @Override + public Set getMemoryComputeKeys() { + return MEMORY_COMPUTE_KEYS; + } + @SuppressWarnings({"CloneDoesntDeclareCloneNotSupportedException", "CloneDoesntCallSuperClone"}) @Override public VertexProgram clone() { @@ -2525,46 +2550,6 @@ public boolean requiresVertexPropertyAddition() { }; } - public static Builder build() { - return new Builder(); - } - - static class Builder extends AbstractVertexProgramBuilder { - - private Builder() { - super(VertexProgramQ.class); - } - - @SuppressWarnings("unchecked") - @Override - public VertexProgramQ create(final Graph graph) { - if (graph != null) { - ConfigurationUtils.append(graph.configuration().subset(VERTEX_PROGRAM_Q_CFG_PREFIX), configuration); - } - return (VertexProgramQ) VertexProgram.createVertexProgram(graph, configuration); - } - - public VertexProgramQ create() { - return create(null); - } - - public Builder from(final String label) { - configuration.setProperty(MAP_KEY_CFG_KEY, label); - return this; - } - - public Builder property(final String name) { - configuration.setProperty(PROPERTY_CFG_KEY, name); - return this; - } - /** - * This is only configurable for the purpose of testing. In a real-world VP this would be a bad pattern. - */ - public Builder useTraverserRequirements(final boolean value) { - configuration.setProperty(USE_TRAVERSER_REQUIREMENTS_CFG_KEY, value); - return this; - } - } } -} \ No newline at end of file +} From 181558cea654ad0f7e351755bd02d952408a0944 Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Mon, 13 Mar 2017 14:50:09 -0400 Subject: [PATCH 11/17] TINKERPOP-1642 Pushed integrateChild() operations into Parameters.set() There was no need to continually iterate the traversal list and doing integrateChild() over and over again for each newly added traversal. Removing those wasteful cycles and only doing that once, when the Traversal was added helped improve performance. --- .../traversal/step/map/AddEdgeStep.java | 15 +++--- .../step/map/AddVertexStartStep.java | 6 +-- .../traversal/step/map/AddVertexStep.java | 6 +-- .../step/sideEffect/AddPropertyStep.java | 7 +-- .../traversal/step/util/Parameters.java | 19 ++------ .../decoration/ElementIdStrategy.java | 2 +- .../traversal/step/util/ParametersTest.java | 46 +++++++++---------- 7 files changed, 39 insertions(+), 62 deletions(-) diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStep.java index 3a46c0d4ecd..03a2fa70a3a 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStep.java @@ -57,7 +57,7 @@ public final class AddEdgeStep extends MapStep public AddEdgeStep(final Traversal.Admin traversal, final String edgeLabel) { super(traversal); - this.parameters.set(T.label, edgeLabel); + this.parameters.set(this, T.label, edgeLabel); } @Override @@ -77,20 +77,17 @@ public Set getScopeKeys() { @Override public void addPropertyMutations(final Object... keyValues) { - this.parameters.set(keyValues); - this.parameters.integrateTraversals(this); + this.parameters.set(this, keyValues); } @Override - public void addTo(final Traversal.Admin toObject) { - this.parameters.set(TO, toObject); - this.parameters.integrateTraversals(this); + public void addTo(final Traversal.Admin toObject) { + this.parameters.set(this, TO, toObject); } @Override - public void addFrom(final Traversal.Admin fromObject) { - this.parameters.set(FROM, fromObject); - this.parameters.integrateTraversals(this); + public void addFrom(final Traversal.Admin fromObject) { + this.parameters.set(this, FROM, fromObject); } @Override diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexStartStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexStartStep.java index 064fc79c951..7de8839bd9a 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexStartStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexStartStep.java @@ -54,8 +54,7 @@ public final class AddVertexStartStep extends AbstractStep public AddVertexStartStep(final Traversal.Admin traversal, final String label) { super(traversal); - this.parameters.set(T.label, label); - this.parameters.integrateTraversals(this); + this.parameters.set(this, T.label, label); } @Override @@ -75,8 +74,7 @@ public List> getLocalChildren() { @Override public void addPropertyMutations(final Object... keyValues) { - this.parameters.set(keyValues); - this.parameters.integrateTraversals(this); + this.parameters.set(this, keyValues); } @Override diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexStep.java index 69ee6e50be1..c35fa80b740 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexStep.java @@ -49,8 +49,7 @@ public final class AddVertexStep extends MapStep public AddVertexStep(final Traversal.Admin traversal, final String label) { super(traversal); - this.parameters.set(T.label, label); - this.parameters.integrateTraversals(this); + this.parameters.set(this, T.label, label); } @Override @@ -70,8 +69,7 @@ public List> getLocalChildren() { @Override public void addPropertyMutations(final Object... keyValues) { - this.parameters.set(keyValues); - this.parameters.integrateTraversals(this); + this.parameters.set(this, keyValues); } @Override diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java index 85292b794d1..3de8932e200 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java @@ -55,10 +55,8 @@ public final class AddPropertyStep extends SideEffectStep public AddPropertyStep(final Traversal.Admin traversal, final VertexProperty.Cardinality cardinality, final Object keyObject, final Object valueObject) { super(traversal); - this.parameters.set(T.key, keyObject); - this.parameters.set(T.value, valueObject); + this.parameters.set(this, T.key, keyObject, T.value, valueObject); this.cardinality = cardinality; - this.parameters.integrateTraversals(this); } @Override @@ -78,8 +76,7 @@ public List> getLocalChildren() { @Override public void addPropertyMutations(final Object... keyValues) { - this.parameters.set(keyValues); - this.parameters.integrateTraversals(this); + this.parameters.set(this, keyValues); } @Override diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java index 4b38d2e092c..0583ed298db 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java @@ -55,8 +55,8 @@ public final class Parameters implements Cloneable, Serializable { /** * A cached list of traversals that serve as parameter values. The list is cached on calls to - * {@link #set(Object...)} because when the parameter map is large the cost of iterating it repeatedly on the - * high number of calls to {@link #getTraversals()} and {@link #integrateTraversals(TraversalParent)} is great. + * {@link #set(TraversalParent, Object...)} because when the parameter map is large the cost of iterating it repeatedly on the + * high number of calls to {@link #getTraversals()} is great. */ private List> traversals = new ArrayList<>(); @@ -170,7 +170,7 @@ public Map> getRaw(final Object... exceptKeys) { /** * Set parameters given key/value pairs. */ - public void set(final Object... keyValues) { + public void set(final TraversalParent parent, final Object... keyValues) { Parameters.legalPropertyKeyValueArray(keyValues); for (int i = 0; i < keyValues.length; i = i + 2) { if (keyValues[i + 1] != null) { @@ -179,6 +179,7 @@ public void set(final Object... keyValues) { if (keyValues[i + 1] instanceof Traversal.Admin) { final Traversal.Admin t = (Traversal.Admin) keyValues[i + 1]; addTraversal(t); + if (parent != null) parent.integrateChild(t); } List values = this.parameters.get(keyValues[i]); @@ -193,18 +194,6 @@ public void set(final Object... keyValues) { } } - /** - * Calls {@link TraversalParent#integrateChild(Traversal.Admin)} on any traversal objects in the parameter list. - * This method requires that {@link #set(Object...)} is called prior to this method as the it will switch the - * {@code traversalsPresent} flag field if a {@link Traversal.Admin} object is present and allow this method to - * do its work. - */ - public void integrateTraversals(final TraversalParent step) { - for (Traversal.Admin t : traversals) { - step.integrateChild(t); - } - } - /** * Gets all the {@link Traversal.Admin} objects in the map of parameters. */ diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/ElementIdStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/ElementIdStrategy.java index 32437e102aa..c8f9d229a92 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/ElementIdStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/ElementIdStrategy.java @@ -111,7 +111,7 @@ public void apply(final Traversal.Admin traversal) { if (parameterizing.getParameters().contains(T.id)) parameterizing.getParameters().rename(T.id, this.idPropertyKey); else if (!parameterizing.getParameters().contains(this.idPropertyKey)) - parameterizing.getParameters().set(this.idPropertyKey, idMaker.get()); + parameterizing.getParameters().set(null, this.idPropertyKey, idMaker.get()); } }); } diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ParametersTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ParametersTest.java index 9c96c1cd6c4..ebe40af5b4f 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ParametersTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/ParametersTest.java @@ -52,7 +52,7 @@ public void shouldGetKeyValuesEmpty() { @Test public void shouldGetKeyValues() { final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "b", "bat", "c", "cat"); + parameters.set(null, "a", "axe", "b", "bat", "c", "cat"); final Object[] params = parameters.getKeyValues(mock(Traverser.Admin.class)); assertEquals(6, params.length); @@ -62,7 +62,7 @@ public void shouldGetKeyValues() { @Test public void shouldGetKeyValuesIgnoringSomeKeys() { final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "b", "bat", "c", "cat"); + parameters.set(null, "a", "axe", "b", "bat", "c", "cat"); final Object[] params = parameters.getKeyValues(mock(Traverser.Admin.class), "b"); assertEquals(4, params.length); @@ -72,7 +72,7 @@ public void shouldGetKeyValuesIgnoringSomeKeys() { @Test public void shouldGetUsingTraverserOverload() { final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "b", "bat", "c", "cat"); + parameters.set(null, "a", "axe", "b", "bat", "c", "cat"); assertEquals(Collections.singletonList("axe"), parameters.get(mock(Traverser.Admin.class), "a", () -> "x")); assertEquals(Collections.singletonList("bat"), parameters.get(mock(Traverser.Admin.class), "b", () -> "x")); @@ -83,7 +83,7 @@ public void shouldGetUsingTraverserOverload() { @Test public void shouldGetMultipleUsingTraverserOverload() { final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat"); + parameters.set(null, "a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat"); final Map> params = parameters.getRaw(); assertEquals(3, params.size()); @@ -96,7 +96,7 @@ public void shouldGetMultipleUsingTraverserOverload() { @Test public void shouldGetRaw() { final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "b", "bat", "c", "cat"); + parameters.set(null, "a", "axe", "b", "bat", "c", "cat"); final Map> params = parameters.getRaw(); assertEquals(3, params.size()); @@ -108,7 +108,7 @@ public void shouldGetRaw() { @Test public void shouldGetRawWithMulti() { final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "b", "bat", "a", "ant", "c", "cat"); + parameters.set(null, "a", "axe", "b", "bat", "a", "ant", "c", "cat"); final Map> params = parameters.getRaw(); assertEquals(3, params.size()); @@ -127,7 +127,7 @@ public void shouldGetRawEmptyAndUnmodifiable() { @Test public void shouldGetRawExcept() { final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "b", "bat", "c", "cat"); + parameters.set(null, "a", "axe", "b", "bat", "c", "cat"); final Map> params = parameters.getRaw("b"); assertEquals(2, params.size()); @@ -138,7 +138,7 @@ public void shouldGetRawExcept() { @Test public void shouldRemove() { final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "b", "bat", "c", "cat"); + parameters.set(null, "a", "axe", "b", "bat", "c", "cat"); final Map> before = parameters.getRaw(); assertEquals(3, before.size()); @@ -157,7 +157,7 @@ public void shouldRemove() { @Test public void shouldRemoveRefreshTraversalCache() { final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "b", "bat", "c", "cat", "c", mock(Traversal.Admin.class), "t", mock(Traversal.Admin.class)); + parameters.set(null, "a", "axe", "b", "bat", "c", "cat", "c", mock(Traversal.Admin.class), "t", mock(Traversal.Admin.class)); final Map> before = parameters.getRaw(); assertEquals(4, before.size()); @@ -190,7 +190,7 @@ public void shouldRemoveRefreshTraversalCache() { @Test public void shouldRename() { final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "b", "bat", "c", "cat"); + parameters.set(null, "a", "axe", "b", "bat", "c", "cat"); parameters.rename("a", "z"); @@ -204,7 +204,7 @@ public void shouldRename() { @Test public void shouldContainKey() { final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "b", "bat", "c", "cat"); + parameters.set(null, "a", "axe", "b", "bat", "c", "cat"); assertThat(parameters.contains("b"), is(true)); } @@ -212,7 +212,7 @@ public void shouldContainKey() { @Test public void shouldNotContainKey() { final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "b", "bat", "c", "cat"); + parameters.set(null, "a", "axe", "b", "bat", "c", "cat"); assertThat(parameters.contains("z"), is(false)); } @@ -220,7 +220,7 @@ public void shouldNotContainKey() { @Test public void shouldGetSetMultiple() { final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat"); + parameters.set(null, "a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat"); final Map> params = parameters.getRaw(); assertEquals(3, params.size()); @@ -234,7 +234,7 @@ public void shouldGetSetMultiple() { @Test public void shouldGetDefault() { final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "b", "bat", "c", "cat"); + parameters.set(null, "a", "axe", "b", "bat", "c", "cat"); assertEquals(Collections.singletonList("axe"), parameters.get("a", () -> "x")); assertEquals(Collections.singletonList("bat"), parameters.get("b", () -> "x")); @@ -245,7 +245,7 @@ public void shouldGetDefault() { @Test public void shouldClone() { final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat"); + parameters.set(null, "a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat"); final Parameters parametersClone = parameters.clone(); @@ -259,7 +259,7 @@ public void shouldCloneWithTraversals() { when(t.clone()).thenReturn(t); final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat", "t", t); + parameters.set(null, "a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat", "t", t); final Parameters parametersClone = parameters.clone(); @@ -270,10 +270,10 @@ public void shouldCloneWithTraversals() { @Test public void shouldBeDifferent() { final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat"); + parameters.set(null, "a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat"); final Parameters parametersDifferent = new Parameters(); - parametersDifferent.set("a", "ant", "a", "axe", "b", "bat", "b", "ball", "c", "cat"); + parametersDifferent.set(null, "a", "ant", "a", "axe", "b", "bat", "b", "ball", "c", "cat"); assertNotEquals(parameters.getRaw(), parametersDifferent.getRaw()); } @@ -281,29 +281,27 @@ public void shouldBeDifferent() { @Test public void shouldGetNoTraversals() { final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat"); + parameters.set(null, "a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat"); assertEquals(Collections.emptyList(), parameters.getTraversals()); } @Test public void shouldGetTraversals() { final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat", "t", __.outE("knows")); + parameters.set(null, "a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat", "t", __.outE("knows")); assertEquals(Collections.singletonList(__.outE("knows")), parameters.getTraversals()); } @Test public void shouldIntegrateTraversals() { - final Parameters parameters = new Parameters(); - parameters.set("a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat", "t", __.outE("knows")); - final TraversalParent mock = mock(TraversalParent.class); + final Parameters parameters = new Parameters(); // the mock can return nothing of consequence as the return isn't used in this case. we just need to // validate that the method gets called as a result of calls to set/integrateTraversals() when(mock.integrateChild(__.outE("knows").asAdmin())).thenReturn(null); - parameters.integrateTraversals(mock); + parameters.set(mock, "a", "axe", "a", "ant", "b", "bat", "b", "ball", "c", "cat", "t", __.outE("knows")); verify(mock).integrateChild(__.outE("knows").asAdmin()); } From 3182a06de4b39fb185b363444e59ce373536411e Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Fri, 17 Mar 2017 11:57:52 -0600 Subject: [PATCH 12/17] I made it so ProfileStrategy uses the MARKER model developed by @dkuppitz and myself to reduce recurssive lookups in strategies. Moving forward, we really need to move to a Traversal.metadata model as using labeled steps is a bit hackish. --- CHANGELOG.asciidoc | 1 + .../finalization/ProfileStrategy.java | 39 +++++++++---------- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 8dd8d9a84a5..926aa1804df 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.5 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* `ProfileStrategy` now uses the marker-model to reduce recrussive lookups of `ProfileSideEffectStep`. * `Mutating` steps now implement `Scoping` interface. * Fixed a step id compilation bug in `AddVertexStartStep`, `AddVertexStep`, `AddEdgeStep`, and `AddPropertyStep`. * De-registered metrics on Gremlin Server shutdown. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/ProfileStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/ProfileStrategy.java index 6d086525c1e..b5f0b86acd6 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/ProfileStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/ProfileStrategy.java @@ -18,13 +18,16 @@ */ package org.apache.tinkerpop.gremlin.process.traversal.strategy.finalization; +import org.apache.tinkerpop.gremlin.process.computer.traversal.step.map.VertexProgramStep; 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.sideEffect.ProfileSideEffectStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyStep; import org.apache.tinkerpop.gremlin.process.traversal.step.util.ProfileStep; 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.List; @@ -34,34 +37,28 @@ public final class ProfileStrategy extends AbstractTraversalStrategy implements TraversalStrategy.FinalizationStrategy { private static final ProfileStrategy INSTANCE = new ProfileStrategy(); + private static final String MARKER = Graph.Hidden.hide("gremlin.profile"); private ProfileStrategy() { } @Override public void apply(final Traversal.Admin traversal) { - if (TraversalHelper.hasStepOfAssignableClassRecursively(ProfileSideEffectStep.class, TraversalHelper.getRootTraversal(traversal).asAdmin())) { - prepTraversalForProfiling(traversal); - } - } - - // Iterate the traversal steps and inject the .profile()-steps. - private void prepTraversalForProfiling(Traversal.Admin traversal) { - // Add .profile() step after every pre-existing step. - final List steps = traversal.getSteps(); - final int numSteps = steps.size(); - for (int ii = 0; ii < numSteps; ii++) { - // Get the original step - final Step step = steps.get(ii * 2); - - // Do not inject profiling after ProfileSideEffectStep as this will be the last step on the root traversal. - if (step instanceof ProfileSideEffectStep) { - break; + if ((traversal.getParent() instanceof EmptyStep || traversal.getParent() instanceof VertexProgramStep) && + TraversalHelper.hasStepOfAssignableClassRecursively(ProfileSideEffectStep.class, traversal)) + TraversalHelper.applyTraversalRecursively(t -> t.getEndStep().addLabel(MARKER), traversal); + if (traversal.getEndStep().getLabels().contains(MARKER)) { + traversal.getEndStep().removeLabel(MARKER); + // Add .profile() step after every pre-existing step. + final List steps = traversal.getSteps(); + final int numSteps = steps.size(); + for (int i = 0; i < numSteps; i++) { + // Do not inject profiling after ProfileSideEffectStep as this will be the last step on the root traversal. + if (steps.get(i * 2) instanceof ProfileSideEffectStep) + break; + // Create and inject ProfileStep + traversal.addStep((i * 2) + 1, new ProfileStep(traversal)); } - - // Create and inject ProfileStep - ProfileStep profileStep = new ProfileStep(traversal); - traversal.addStep((ii * 2) + 1, profileStep); } } From 97141dab3ea3b187e1c1bd93bd12f8afa6a3b601 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Fri, 17 Mar 2017 13:04:58 -0600 Subject: [PATCH 13/17] added MARKER model to PathRetractionStrategy to reduce recurssive lookups of invalidating steps. Again, we really need Traversal.metdata to make this more 'pure'. --- CHANGELOG.asciidoc | 3 ++- .../optimization/PathRetractionStrategy.java | 15 +++++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 926aa1804df..57314976ec6 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -26,7 +26,8 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima TinkerPop 3.2.5 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -* `ProfileStrategy` now uses the marker-model to reduce recrussive lookups of `ProfileSideEffectStep`. +* `PathRetractionStrategy` now uses the marker-model to reduce recursive lookups of invalidating steps. +* `ProfileStrategy` now uses the marker-model to reduce recursive lookups of `ProfileSideEffectStep`. * `Mutating` steps now implement `Scoping` interface. * Fixed a step id compilation bug in `AddVertexStartStep`, `AddVertexStep`, `AddEdgeStep`, and `AddPropertyStep`. * De-registered metrics on Gremlin Server shutdown. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathRetractionStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathRetractionStrategy.java index 56f9f669a70..443e4e381fd 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathRetractionStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathRetractionStrategy.java @@ -37,6 +37,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; import org.apache.tinkerpop.gremlin.process.traversal.util.PathUtil; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper; +import org.apache.tinkerpop.gremlin.structure.Graph; import org.javatuples.Pair; import java.util.ArrayList; @@ -58,6 +59,7 @@ public final class PathRetractionStrategy extends AbstractTraversalStrategy> PRIORS = new HashSet<>(Arrays.asList( RepeatUnrollStrategy.class, MatchPredicateStrategy.class, PathProcessorStrategy.class)); + private static final String MARKER = Graph.Hidden.hide("gremlin.pathRetraction"); private final int standardBarrierSize; @@ -74,11 +76,16 @@ public void apply(final Traversal.Admin traversal) { // do not apply this strategy if there are lambdas as you can't introspect to know what path information the lambdas are using // do not apply this strategy if a PATH requirement step is being used (in the future, we can do PATH requirement lookhead to be more intelligent about its usage) // do not apply this strategy if a VertexProgramStep is present with LABELED_PATH requirements - if (TraversalHelper.anyStepRecursively(step -> step instanceof LambdaHolder || - step.getRequirements().contains(TraverserRequirement.PATH) || - (step instanceof VertexProgramStep && step.getRequirements().contains(TraverserRequirement.LABELED_PATH)), - TraversalHelper.getRootTraversal(traversal))) + if ((traversal.getParent() instanceof EmptyStep || traversal.getParent() instanceof VertexProgramStep) && + TraversalHelper.anyStepRecursively(step -> step instanceof LambdaHolder || + step.getRequirements().contains(TraverserRequirement.PATH) || + (step instanceof VertexProgramStep && step.getRequirements().contains(TraverserRequirement.LABELED_PATH)),traversal)) + TraversalHelper.applyTraversalRecursively(t -> t.getEndStep().addLabel(MARKER), traversal); + + if (traversal.getEndStep().getLabels().contains(MARKER)) { + traversal.getEndStep().removeLabel(MARKER); return; + } final boolean onGraphComputer = TraversalHelper.onGraphComputer(traversal); final Set foundLabels = new HashSet<>(); From acc8d7399e291a8c9a517d223dea846c1c67d7c3 Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Mon, 27 Mar 2017 14:03:26 -0400 Subject: [PATCH 14/17] TINKERPOP-1642 Fixed up some execution problems after rebase The logic still doesn't seem quite right but at least the tests pass and a complete build is achieved. The issue is related to new stuff added in PathRetractionStrategy on TINKERPOP-1652. --- .../strategy/optimization/PathRetractionStrategy.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathRetractionStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathRetractionStrategy.java index 443e4e381fd..10e2372d69c 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathRetractionStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathRetractionStrategy.java @@ -78,15 +78,20 @@ public void apply(final Traversal.Admin traversal) { // do not apply this strategy if a VertexProgramStep is present with LABELED_PATH requirements if ((traversal.getParent() instanceof EmptyStep || traversal.getParent() instanceof VertexProgramStep) && TraversalHelper.anyStepRecursively(step -> step instanceof LambdaHolder || - step.getRequirements().contains(TraverserRequirement.PATH) || - (step instanceof VertexProgramStep && step.getRequirements().contains(TraverserRequirement.LABELED_PATH)),traversal)) + step.getRequirements().contains(TraverserRequirement.PATH),traversal)) { TraversalHelper.applyTraversalRecursively(t -> t.getEndStep().addLabel(MARKER), traversal); + } if (traversal.getEndStep().getLabels().contains(MARKER)) { traversal.getEndStep().removeLabel(MARKER); return; } + if (TraversalHelper.anyStepRecursively(step -> step instanceof VertexProgramStep && step.getRequirements().contains(TraverserRequirement.LABELED_PATH), + TraversalHelper.getRootTraversal(traversal))) { + return; + } + final boolean onGraphComputer = TraversalHelper.onGraphComputer(traversal); final Set foundLabels = new HashSet<>(); final Set keepLabels = new HashSet<>(); From 8e90349a43c843e44b53b87aea7d7c1d9b0ccee7 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Tue, 28 Mar 2017 07:46:31 -0600 Subject: [PATCH 15/17] removed a repeated recurssion in PathRetractionStrategy using the MARKER model of strategies. --- .../optimization/PathRetractionStrategy.java | 26 ++++++++----------- .../StandardVerificationStrategy.java | 3 ++- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathRetractionStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathRetractionStrategy.java index 10e2372d69c..304161e4fb0 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathRetractionStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathRetractionStrategy.java @@ -58,7 +58,9 @@ public final class PathRetractionStrategy extends AbstractTraversalStrategy> PRIORS = new HashSet<>(Arrays.asList( - RepeatUnrollStrategy.class, MatchPredicateStrategy.class, PathProcessorStrategy.class)); + RepeatUnrollStrategy.class, + MatchPredicateStrategy.class, + PathProcessorStrategy.class)); private static final String MARKER = Graph.Hidden.hide("gremlin.pathRetraction"); private final int standardBarrierSize; @@ -76,9 +78,11 @@ public void apply(final Traversal.Admin traversal) { // do not apply this strategy if there are lambdas as you can't introspect to know what path information the lambdas are using // do not apply this strategy if a PATH requirement step is being used (in the future, we can do PATH requirement lookhead to be more intelligent about its usage) // do not apply this strategy if a VertexProgramStep is present with LABELED_PATH requirements - if ((traversal.getParent() instanceof EmptyStep || traversal.getParent() instanceof VertexProgramStep) && + if (traversal.getParent() instanceof EmptyStep && TraversalHelper.anyStepRecursively(step -> step instanceof LambdaHolder || - step.getRequirements().contains(TraverserRequirement.PATH),traversal)) { + step.getRequirements().contains(TraverserRequirement.PATH) || + (step instanceof VertexProgramStep && + step.getRequirements().contains(TraverserRequirement.LABELED_PATH)), traversal)) { TraversalHelper.applyTraversalRecursively(t -> t.getEndStep().addLabel(MARKER), traversal); } @@ -87,11 +91,6 @@ public void apply(final Traversal.Admin traversal) { return; } - if (TraversalHelper.anyStepRecursively(step -> step instanceof VertexProgramStep && step.getRequirements().contains(TraverserRequirement.LABELED_PATH), - TraversalHelper.getRootTraversal(traversal))) { - return; - } - final boolean onGraphComputer = TraversalHelper.onGraphComputer(traversal); final Set foundLabels = new HashSet<>(); final Set keepLabels = new HashSet<>(); @@ -223,9 +222,8 @@ public void apply(final Traversal.Admin traversal) { // if there is one more RepeatSteps in this traversal's lineage, preserve keep labels if (currentStep instanceof PathProcessor) { ((PathProcessor) currentStep).getKeepLabels().addAll(keeperTrail); - if (hasRepeat) { + if (hasRepeat) ((PathProcessor) currentStep).getKeepLabels().addAll(keepLabels); - } } } } @@ -238,18 +236,16 @@ private void applyToChildren(final Set keepLabels, final List keepLabels) { for (final Object s : traversal.getSteps()) { - if (s instanceof PathProcessor) { + if (s instanceof PathProcessor) addLabels((PathProcessor) s, keepLabels); - } } } private void addLabels(final PathProcessor s, final Set keepLabels) { - if (s.getKeepLabels() == null) { + if (null == s.getKeepLabels()) s.setKeepLabels(new HashSet<>(keepLabels)); - } else { + else s.getKeepLabels().addAll(new HashSet<>(keepLabels)); - } } @Override 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 aed5372688e..cad3b8eff35 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 @@ -33,6 +33,7 @@ import org.apache.tinkerpop.gremlin.structure.Graph; import java.util.Collections; +import java.util.HashSet; import java.util.Set; /** @@ -54,7 +55,7 @@ public void apply(final Traversal.Admin traversal) { } for (final Step step : traversal.getSteps()) { - for (String label : step.getLabels()) { + for (String label : new HashSet<>(step.getLabels())) { if (Graph.Hidden.isHidden(label)) step.removeLabel(label); } From af5f3364e12aadead8025f3724559d282105bfaa Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Wed, 29 Mar 2017 10:07:55 -0600 Subject: [PATCH 16/17] fixed a Parameters.clone() bug introduced with the Traversal caching work in this ticket. This fixes the @twilmes AddEdgeTest case. --- .../traversal/step/util/Parameters.java | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java index 0583ed298db..5cb60014432 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/Parameters.java @@ -58,7 +58,7 @@ public final class Parameters implements Cloneable, Serializable { * {@link #set(TraversalParent, Object...)} because when the parameter map is large the cost of iterating it repeatedly on the * high number of calls to {@link #getTraversals()} is great. */ - private List> traversals = new ArrayList<>(); + private List> traversals = new ArrayList<>(); /** * Checks for existence of key in parameter set. @@ -199,7 +199,7 @@ public void set(final TraversalParent parent, final Object... keyValues) { */ public List> getTraversals() { // stupid generics - just need to return "traversals" - return (List>) (Object) traversals; + return (List>) (Object) this.traversals; } /** @@ -213,14 +213,24 @@ public Parameters clone() { try { final Parameters clone = (Parameters) super.clone(); clone.parameters = new HashMap<>(); + clone.traversals = new ArrayList<>(); for (final Map.Entry> entry : this.parameters.entrySet()) { final List values = new ArrayList<>(); for (final Object value : entry.getValue()) { - values.add(value instanceof Traversal.Admin ? ((Traversal.Admin) value).clone() : value); + if (value instanceof Traversal.Admin) { + final Traversal.Admin traversalClone = ((Traversal.Admin) value).clone(); + clone.traversals.add(traversalClone); + values.add(traversalClone); + } else + values.add(value); } - clone.parameters.put(entry.getKey() instanceof Traversal.Admin ? ((Traversal.Admin) entry.getKey()).clone() : entry.getKey(), values); + if (entry.getKey() instanceof Traversal.Admin) { + final Traversal.Admin traversalClone = ((Traversal.Admin) entry.getKey()).clone(); + clone.traversals.add(traversalClone); + clone.parameters.put(traversalClone, values); + } else + clone.parameters.put(entry.getKey(), values); } - clone.referencedLabels = new HashSet<>(this.referencedLabels); return clone; } catch (final CloneNotSupportedException e) { @@ -253,11 +263,11 @@ private static void legalPropertyKeyValueArray(final Object... propertyKeyValues } private void addTraversal(final Traversal.Admin t) { - traversals.add(t); + this.traversals.add(t); for (final Object ss : t.getSteps()) { if (ss instanceof Scoping) { for (String label : ((Scoping) ss).getScopeKeys()) { - referencedLabels.add(label); + this.referencedLabels.add(label); } } } From 8ac09c8866df1bbbe23576b07bfda6820ba60c07 Mon Sep 17 00:00:00 2001 From: "Marko A. Rodriguez" Date: Wed, 29 Mar 2017 11:41:31 -0600 Subject: [PATCH 17/17] Found an ancient bug in B_LP_O_S_SE_SL_Traverser that only reared its ugly head with how LABELED_PATH requirements are now computed. Bug fixed and optimized LABELED_PATH requirment analysis. --- CHANGELOG.asciidoc | 1 + .../traversal/traverser/B_LP_O_S_SE_SL_Traverser.java | 2 +- .../gremlin/process/traversal/util/DefaultTraversal.java | 2 +- .../gremlin/process/traversal/util/TraversalHelper.java | 9 ++++++--- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 57314976ec6..78055af3a23 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.5 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Fixed a `NullPointerException` bug in `B_LP_O_S_SE_SL_Traverser`. * `PathRetractionStrategy` now uses the marker-model to reduce recursive lookups of invalidating steps. * `ProfileStrategy` now uses the marker-model to reduce recursive lookups of `ProfileSideEffectStep`. * `Mutating` steps now implement `Scoping` interface. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/traverser/B_LP_O_S_SE_SL_Traverser.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/traverser/B_LP_O_S_SE_SL_Traverser.java index fc1636698eb..74a049c1e35 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/traverser/B_LP_O_S_SE_SL_Traverser.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/traverser/B_LP_O_S_SE_SL_Traverser.java @@ -121,7 +121,7 @@ public boolean equals(final Object object) { && ((B_LP_O_S_SE_SL_Traverser) object).t.equals(this.t) && ((B_LP_O_S_SE_SL_Traverser) object).future.equals(this.future) && ((B_LP_O_S_SE_SL_Traverser) object).loops == this.loops - && (null == this.sack || null != this.sideEffects.getSackMerger()) + && (null == this.sack || (null != this.sideEffects && null != this.sideEffects.getSackMerger())) // hmmm... serialization in OLAP destroys the transient sideEffects && ((B_LP_O_S_SE_SL_Traverser) object).path.popEquals(Pop.last, this.path); // this should be Pop.all? } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java index c0e54db597f..c8f4b24132e 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java @@ -147,7 +147,7 @@ public Set getTraverserRequirements() { for (final Step step : this.getSteps()) { this.requirements.addAll(step.getRequirements()); } - if (TraversalHelper.hasLabels(this)) + if (!this.requirements.contains(TraverserRequirement.LABELED_PATH) && TraversalHelper.hasLabels(this)) this.requirements.add(TraverserRequirement.LABELED_PATH); if (!this.getSideEffects().keys().isEmpty()) this.requirements.add(TraverserRequirement.SIDE_EFFECTS); 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 95862d0725f..de4e36f58dd 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 @@ -45,6 +45,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.util.BulkSet; import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyStep; import org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer; +import org.apache.tinkerpop.gremlin.structure.Graph; import org.apache.tinkerpop.gremlin.structure.T; import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils; @@ -435,7 +436,7 @@ public static boolean anyStepRecursively(final Predicate predicate, final * Determine if any child step of a {@link TraversalParent} match the step given the provided {@link Predicate}. * * @param predicate the match function - * @param step the step to perform the action on + * @param step the step to perform the action on * @return {@code true} if there is a match and {@code false} otherwise */ public static boolean anyStepRecursively(final Predicate predicate, final TraversalParent step) { @@ -552,8 +553,10 @@ public static void reIdSteps(final StepPosition stepPosition, final Traversal.Ad public static boolean hasLabels(final Traversal.Admin traversal) { for (final Step step : traversal.getSteps()) { - if (!step.getLabels().isEmpty()) - return true; + for (final String label : step.getLabels()) { + if (!Graph.Hidden.isHidden(label)) + return true; + } if (step instanceof TraversalParent) { for (final Traversal.Admin local : ((TraversalParent) step).getLocalChildren()) { if (TraversalHelper.hasLabels(local))