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 2015/06/17 17:32:20 UTC
incubator-tinkerpop git commit: Revised evaluation of ids with has()
TINKERPOP3-721
Repository: incubator-tinkerpop
Updated Branches:
refs/heads/master a114512bf -> 50d87ac07
Revised evaluation of ids with has() TINKERPOP3-721
Fixed problem where toString() of id() in has() was good for equality but not so good when you did g.V().has(id,lt(10)). Modified the approach so that the conversion via toString() only occurs if the user calls has() with String values. It is otherwise treated as the value that was passed in.
Project: http://git-wip-us.apache.org/repos/asf/incubator-tinkerpop/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-tinkerpop/commit/50d87ac0
Tree: http://git-wip-us.apache.org/repos/asf/incubator-tinkerpop/tree/50d87ac0
Diff: http://git-wip-us.apache.org/repos/asf/incubator-tinkerpop/diff/50d87ac0
Branch: refs/heads/master
Commit: 50d87ac0747e91ea5aab7f39357966eaad389200
Parents: a114512
Author: Stephen Mallette <sp...@genoprime.com>
Authored: Wed Jun 17 11:29:36 2015 -0400
Committer: Stephen Mallette <sp...@genoprime.com>
Committed: Wed Jun 17 11:29:36 2015 -0400
----------------------------------------------------------------------
.../tinkerpop/gremlin/process/traversal/P.java | 16 +++++++
.../traversal/step/util/HasContainer.java | 44 ++++++++++++++++----
.../traversal/step/filter/GroovyHasTest.groovy | 5 +++
.../process/traversal/step/filter/HasTest.java | 35 +++++++++++++---
4 files changed, 85 insertions(+), 15 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-tinkerpop/blob/50d87ac0/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java
----------------------------------------------------------------------
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java
index 4946b5d..ba4eabe 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java
@@ -36,14 +36,17 @@ import java.util.function.Predicate;
/**
* @author Marko A. Rodriguez (http://markorodriguez.com)
+ * @author Stephen Mallette (http://stephen.genoprime.com)
*/
public class P<V> implements Predicate<V>, Serializable, Cloneable {
protected BiPredicate<V, V> biPredicate;
protected V value;
+ protected V originalValue;
public P(final BiPredicate<V, V> biPredicate, final V value) {
this.value = value;
+ this.originalValue = value;
this.biPredicate = biPredicate;
}
@@ -51,6 +54,19 @@ public class P<V> implements Predicate<V>, Serializable, Cloneable {
return this.biPredicate;
}
+ /**
+ * Gets the original value used at time of construction of the {@code P}. This value can change its type
+ * in some cases.
+ *
+ * @see org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer
+ */
+ public V getOriginalValue() {
+ return originalValue;
+ }
+
+ /**
+ * Gets the current value to be passed to the predicate for testing.
+ */
public V getValue() {
return this.value;
}
http://git-wip-us.apache.org/repos/asf/incubator-tinkerpop/blob/50d87ac0/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java
----------------------------------------------------------------------
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java
index 797a789..b634e37 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java
@@ -38,23 +38,40 @@ public final class HasContainer implements Serializable, Cloneable, Predicate<El
private String key;
private P predicate;
+ private boolean testingIdString;
+
public HasContainer(final String key, final P<?> predicate) {
this.key = key;
this.predicate = predicate;
- // if the key being evaluated is id then the has() test can evaluate as a toString() representation of the
- // identifier. this could be done in the test() method but it seems cheaper to do the conversion once in
- // the constructor. to avoid losing the original value, the string version of the collection is maintained
- // separately
- if (this.key.equals(T.id.getAccessor()))
- this.predicate.setValue(this.predicate.getValue() instanceof Collection ? IteratorUtils.set(IteratorUtils.map(((Collection<Object>) this.predicate.getValue()).iterator(), Object::toString)) : this.predicate.getValue().toString());
+ if (!this.key.equals(T.id.getAccessor()))
+ testingIdString = false;
+ else {
+ // the values should be homogenous if a collection is submitted
+ final Object predicateValue = this.predicate.getValue();
+
+ // enforce a homogenous collection of values when testing ids
+ enforceHomogenousCollectionIfPresent(predicateValue);
+
+ // grab an instance of a value which is either the first item in a homogeneous collection or the value itself
+ final Object valueInstance = this.predicate.getValue() instanceof Collection ?
+ ((Collection) this.predicate.getValue()).toArray()[0] : this.predicate.getValue();
+
+ // if the key being evaluated is id then the has() test can evaluate as a toString() representation of the
+ // identifier. this could be done in the test() method but it seems cheaper to do the conversion once in
+ // the constructor. the original value in P is maintained separately
+ this.testingIdString = this.key.equals(T.id.getAccessor()) && valueInstance instanceof String;
+ if (this.testingIdString)
+ this.predicate.setValue(this.predicate.getValue() instanceof Collection ? IteratorUtils.set(IteratorUtils.map(((Collection<Object>) this.predicate.getValue()).iterator(), Object::toString)) : this.predicate.getValue().toString());
+ }
}
public boolean test(final Element element) {
- // it is OK to evaluate equality of ids via toString() now given that the toString() the test suite
- // enforces the value of id().toString() to be a first class representation of the identifier
+ // it is OK to evaluate equality of ids via toString(), given that the test suite enforces the value of
+ // id().toString() to be a first class representation of the identifier. a string test is only executed
+ // if the predicate value is a String. this allows stuff like: g.V().has(id,lt(10)) to work properly
if (this.key.equals(T.id.getAccessor()))
- return this.predicate.test(element.id().toString());
+ return testingIdString ? this.predicate.test(element.id().toString()) : this.predicate.test(element.id());
else if (this.key.equals(T.label.getAccessor()))
return this.predicate.test(element.label());
else if (element instanceof VertexProperty && this.key.equals(T.value.getAccessor()))
@@ -117,6 +134,15 @@ public final class HasContainer implements Serializable, Cloneable, Predicate<El
////////////
+ private void enforceHomogenousCollectionIfPresent(final Object predicateValue) {
+ if (predicateValue instanceof Collection) {
+ final Collection collection = (Collection) predicateValue;
+ Class<?> first = collection.toArray()[0].getClass();
+ if (!((Collection) predicateValue).stream().map(Object::getClass).allMatch(c -> first.equals(c)))
+ throw new IllegalArgumentException("Has comparisons on a collection of ids require ids to all be of the same type");
+ }
+ }
+
public static boolean testAll(final Element element, final List<HasContainer> hasContainers) {
for (final HasContainer hasContainer : hasContainers) {
if (!hasContainer.test(element))
http://git-wip-us.apache.org/repos/asf/incubator-tinkerpop/blob/50d87ac0/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/filter/GroovyHasTest.groovy
----------------------------------------------------------------------
diff --git a/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/filter/GroovyHasTest.groovy b/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/filter/GroovyHasTest.groovy
index 69c716a..dc21a98 100644
--- a/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/filter/GroovyHasTest.groovy
+++ b/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/filter/GroovyHasTest.groovy
@@ -81,6 +81,11 @@ public abstract class GroovyHasTest {
}
@Override
+ public Traversal<Vertex, Vertex> get_g_VX1X_out_hasXid_lt_3X(final Object v1Id, final Object v3Id) {
+ TraversalScriptHelper.compute("g.V(v1Id).out().has(T.id, P.lt(v3Id))", g, "v1Id", v1Id, "v3Id", v3Id);
+ }
+
+ @Override
public Traversal<Vertex, Vertex> get_g_V_hasXage_gt_30X() {
TraversalScriptHelper.compute("g.V.has('age', gt(30))", g);
}
http://git-wip-us.apache.org/repos/asf/incubator-tinkerpop/blob/50d87ac0/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasTest.java
----------------------------------------------------------------------
diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasTest.java
index 060154c..f7aac61 100644
--- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasTest.java
+++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasTest.java
@@ -18,6 +18,7 @@
*/
package org.apache.tinkerpop.gremlin.process.traversal.step.filter;
+import org.apache.tinkerpop.gremlin.FeatureRequirement;
import org.apache.tinkerpop.gremlin.LoadGraphWith;
import org.apache.tinkerpop.gremlin.process.AbstractGremlinProcessTest;
import org.apache.tinkerpop.gremlin.process.GremlinProcessRunner;
@@ -26,6 +27,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__;
import org.apache.tinkerpop.gremlin.structure.Edge;
import org.apache.tinkerpop.gremlin.structure.Element;
+import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.T;
import org.apache.tinkerpop.gremlin.structure.Vertex;
import org.hamcrest.CoreMatchers;
@@ -37,6 +39,7 @@ import java.util.List;
import static org.apache.tinkerpop.gremlin.LoadGraphWith.GraphData.CREW;
import static org.apache.tinkerpop.gremlin.LoadGraphWith.GraphData.MODERN;
+import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.*;
/**
@@ -60,6 +63,8 @@ public abstract class HasTest extends AbstractGremlinProcessTest {
public abstract Traversal<Vertex, Vertex> get_g_VX1X_hasXage_gt_30X(final Object v1Id);
+ public abstract Traversal<Vertex, Vertex> get_g_VX1X_out_hasXid_lt_3X(final Object v1Id, final Object v3Id);
+
public abstract Traversal<Vertex, Vertex> get_g_VX1X_out_hasIdX2X(final Object v1Id, final Object v2Id);
public abstract Traversal<Vertex, Vertex> get_g_VX1X_out_hasIdX2_3X(final Object v1Id, final Object v2Id, final Object v3Id);
@@ -162,20 +167,33 @@ public abstract class HasTest extends AbstractGremlinProcessTest {
@LoadGraphWith(MODERN)
public void g_VX1X_out_hasXid_2X() {
final Traversal<Vertex, Vertex> traversal = get_g_VX1X_out_hasIdX2X(convertToVertexId("marko"), convertToVertexId("vadas"));
- assert_g_VX1X_out_hasXid_2X(traversal);
+ assertVadasAsOnlyValueReturned(traversal);
+ }
+
+ @Test
+ @LoadGraphWith(MODERN)
+ @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_USER_SUPPLIED_IDS)
+ @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_NUMERIC_IDS)
+ public void g_VX1X_out_hasXid_lt_3X() {
+ // can only execute this on graphs with user supplied ids so that we can be assured of the lt op. it
+ // sort of assumes that ids increment, but there's no feature check for that. graphs that don't work this
+ // way with numeric ids may need to optout
+ final Traversal<Vertex, Vertex> traversal = get_g_VX1X_out_hasXid_lt_3X(convertToVertexId("marko"), convertToVertexId("lop"));
+ assertVadasAsOnlyValueReturned(traversal);
}
@Test
@LoadGraphWith(MODERN)
public void g_VX1AsStringX_out_hasXid_2AsStringX() {
final Traversal<Vertex, Vertex> traversal = get_g_VX1X_out_hasIdX2X(convertToVertexId("marko").toString(), convertToVertexId("vadas").toString());
- assert_g_VX1X_out_hasXid_2X(traversal);
+ assertVadasAsOnlyValueReturned(traversal);
}
- private void assert_g_VX1X_out_hasXid_2X(final Traversal<Vertex, Vertex> traversal) {
+ private void assertVadasAsOnlyValueReturned(final Traversal<Vertex, Vertex> traversal) {
printTraversalForm(traversal);
- assertTrue(traversal.hasNext());
+ assertThat(traversal.hasNext(), is(true));
assertEquals(convertToVertexId("vadas"), traversal.next().id());
+ assertThat(traversal.hasNext(), is(false));
}
@Test
@@ -199,8 +217,8 @@ public abstract class HasTest extends AbstractGremlinProcessTest {
protected void assert_g_VX1X_out_hasXid_2_3X(Object id2, Object id3, Traversal<Vertex, Vertex> traversal) {
printTraversalForm(traversal);
assertTrue(traversal.hasNext());
- assertThat(traversal.next().id(), CoreMatchers.anyOf(CoreMatchers.is(id2), CoreMatchers.is(id3)));
- assertThat(traversal.next().id(), CoreMatchers.anyOf(CoreMatchers.is(id2), CoreMatchers.is(id3)));
+ assertThat(traversal.next().id(), CoreMatchers.anyOf(is(id2), is(id3)));
+ assertThat(traversal.next().id(), CoreMatchers.anyOf(is(id2), is(id3)));
assertFalse(traversal.hasNext());
}
@@ -368,6 +386,11 @@ public abstract class HasTest extends AbstractGremlinProcessTest {
}
@Override
+ public Traversal<Vertex, Vertex> get_g_VX1X_out_hasXid_lt_3X(final Object v1Id, final Object v3Id) {
+ return g.V(v1Id).out().has(T.id, P.lt(v3Id));
+ }
+
+ @Override
public Traversal<Vertex, Vertex> get_g_VX1X_out_hasIdX2X(final Object v1Id, final Object v2Id) {
return g.V(v1Id).out().hasId(v2Id);
}