You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by sp...@apache.org on 2023/02/03 20:30:14 UTC
[tinkerpop] branch TINKERPOP-2865 created (now a6c53350a3)
This is an automated email from the ASF dual-hosted git repository.
spmallette pushed a change to branch TINKERPOP-2865
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
at a6c53350a3 TINKERPOP-2865 Inject partition steps after last has in chain
This branch includes the following new commits:
new a6c53350a3 TINKERPOP-2865 Inject partition steps after last has in chain
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
[tinkerpop] 01/01: TINKERPOP-2865 Inject partition steps after last has in chain
Posted by sp...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
spmallette pushed a commit to branch TINKERPOP-2865
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit a6c53350a393a7bdef19e61114a45182690d1c8a
Author: Stephen Mallette <st...@amazon.com>
AuthorDate: Fri Feb 3 15:29:33 2023 -0500
TINKERPOP-2865 Inject partition steps after last has in chain
---
CHANGELOG.asciidoc | 2 +-
.../strategy/decoration/PartitionStrategy.java | 24 +++++++++----
.../strategy/decoration/PartitionStrategyTest.java | 40 ++++++++++++++++++----
3 files changed, 53 insertions(+), 13 deletions(-)
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index 8b0deefa0e..cb74b50863 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -23,7 +23,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
[[release-3-5-6]]
=== TinkerPop 3.5.6 (Release Date: NOT OFFICIALLY RELEASED YET)
-
+* Changed `PartitionStrategy` to force its filters to the end of the chain for `Vertex` and `Edge` read steps, thus preserving order of the `has()`.
[[release-3-5-5]]
=== TinkerPop 3.5.5 (Release Date: January 16, 2023)
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/PartitionStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/PartitionStrategy.java
index d3d6147e10..47d03364e7 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/PartitionStrategy.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/PartitionStrategy.java
@@ -139,16 +139,28 @@ public final class PartitionStrategy extends AbstractTraversalStrategy<Traversal
// no need to add has after mutating steps because we want to make it so that the write partition can
// be independent of the read partition. in other words, i don't need to be able to read from a partition
- // in order to write to it.
+ // in order to write to it. Seems like ElementStep isn't necessary here? a Property can't be loaded that
+ // isn't within the partition so element() could only ever traverse back to something within the partition.
final List<Step> stepsToInsertHasAfter = new ArrayList<>();
stepsToInsertHasAfter.addAll(TraversalHelper.getStepsOfAssignableClass(GraphStep.class, traversal));
stepsToInsertHasAfter.addAll(TraversalHelper.getStepsOfAssignableClass(VertexStep.class, traversal));
stepsToInsertHasAfter.addAll(TraversalHelper.getStepsOfAssignableClass(EdgeOtherVertexStep.class, traversal));
stepsToInsertHasAfter.addAll(TraversalHelper.getStepsOfAssignableClass(EdgeVertexStep.class, traversal));
- // all steps that return a vertex need to have has(partitionKey,within,partitionValues) injected after it
- stepsToInsertHasAfter.forEach(step -> TraversalHelper.insertAfterStep(
- new HasStep(traversal, new HasContainer(partitionKey, P.within(new ArrayList<>(readPartitions)))), step, traversal));
+ // all steps that return a vertex need to have has(partitionKey,within,partitionValues) injected after it.
+ // attempt to put the partition filter at the end of other has() following that step so that the order of
+ // the has() is maintained on the chance that order up to that point is somehow intentional. this seems
+ // only relevant to Vertex/Edge steps and not properties where this order would seemingly have less
+ // significance to read performance
+ stepsToInsertHasAfter.forEach(step -> {
+ // find the last has() following the insert step
+ Step insertAfter = step;
+ while (insertAfter.getNextStep() instanceof HasStep) {
+ insertAfter = insertAfter.getNextStep();
+ }
+ TraversalHelper.insertAfterStep(
+ new HasStep(traversal, new HasContainer(partitionKey, P.within(new ArrayList<>(readPartitions)))), insertAfter, traversal);
+ });
if (vertexFeatures.isPresent()) {
final List<PropertiesStep> propertiesSteps = TraversalHelper.getStepsOfAssignableClass(PropertiesStep.class, traversal);
@@ -215,8 +227,8 @@ public final class PartitionStrategy extends AbstractTraversalStrategy<Traversal
final List<Step> stepsToInsertPropertyMutations = traversal.getSteps().stream().filter(step ->
step instanceof AddEdgeStep || step instanceof AddVertexStep ||
- step instanceof AddEdgeStartStep || step instanceof AddVertexStartStep ||
- (includeMetaProperties && step instanceof AddPropertyStep)
+ step instanceof AddEdgeStartStep || step instanceof AddVertexStartStep ||
+ (includeMetaProperties && step instanceof AddPropertyStep)
).collect(Collectors.toList());
stepsToInsertPropertyMutations.forEach(step -> {
diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/PartitionStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/PartitionStrategyTest.java
index 7bc80b326f..c5d4d7e5c9 100644
--- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/PartitionStrategyTest.java
+++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/PartitionStrategyTest.java
@@ -19,6 +19,7 @@
package org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration;
import org.apache.tinkerpop.gremlin.process.traversal.Contains;
+import org.apache.tinkerpop.gremlin.process.traversal.Step;
import org.apache.tinkerpop.gremlin.process.traversal.Translator;
import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.DefaultGraphTraversal;
@@ -46,11 +47,13 @@ import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
+import static org.hamcrest.core.IsInstanceOf.instanceOf;
+import static org.hamcrest.core.IsIterableContaining.hasItem;
+import static org.hamcrest.core.IsNot.not;
import static org.junit.Assert.assertEquals;
import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.core.IsCollectionContaining.hasItem;
import static org.junit.Assert.fail;
-import static org.mockito.Matchers.any;
+import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
@@ -87,7 +90,13 @@ public class PartitionStrategyTest {
{create().in().out().addE("test").to("x"), 2, true},
{create().out().out().out(), 3, false},
{create().in().out().in(), 3, false},
- {create().inE().outV().inE().outV(), 4, false}});
+ {create().inE().outV().inE().outV(), 4, false},
+ {create().bothV().hasLabel("person"), 2, false},
+ {create().inV().hasLabel("person").has("name"), 2, false}, // just 2 coz has("name") is TraversalFilterStep and not a has() that goes in a container :/
+ {create().outV().hasLabel("person").out(), 3, false},
+ {create().outV().hasLabel("person").out().hasLabel("software"), 4, false},
+ {create().in().hasLabel("person").out().outE().hasLabel("knows").groupCount(), 5, false},
+ {create().out().inE().otherV().hasLabel("person"), 4, false}});
}
@Parameterized.Parameter(value = 0)
@@ -137,9 +146,28 @@ public class PartitionStrategyTest {
steps.forEach(s -> {
assertEquals(repr, 1, s.getHasContainers().size());
final HasContainer hasContainer = (HasContainer) s.getHasContainers().get(0);
- assertEquals(repr, "p", hasContainer.getKey());
- assertEquals(repr, keySet, hasContainer.getValue());
- assertEquals(repr, Contains.within, hasContainer.getBiPredicate());
+
+ // if it is a partition has() then ensure it is not followed by has(). if it is something else, let it
+ // be followed by partition has()
+ if (hasContainer.getKey().equals("p")) {
+ assertEquals(repr, keySet, hasContainer.getValue());
+ assertEquals(repr, Contains.within, hasContainer.getBiPredicate());
+
+ // no has() after the partition filter
+ assertThat(s.getNextStep(), not(instanceOf(HasStep.class)));
+ } else {
+ // last has() in the sequence should be the partition has()
+ Step insertAfter = s;
+ while (insertAfter.getNextStep() instanceof HasStep) {
+ insertAfter = insertAfter.getNextStep();
+ }
+
+ final HasContainer insertAfterHasContainer = (HasContainer) ((HasStep) insertAfter).getHasContainers().get(0);
+
+ assertEquals(repr, "p", insertAfterHasContainer.getKey());
+ assertEquals(repr, keySet, insertAfterHasContainer.getValue());
+ assertEquals(repr, Contains.within, insertAfterHasContainer.getBiPredicate());
+ }
});
}