TINKERPOP-2314 Employ by(String) for Map when possible
Also improve errors around incorrect types that might be called using this modulator overload.
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index 45bda6a..9672b59 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -25,6 +25,8 @@
This release also includes changes from <<release-3-3-10, 3.3.10>>.
+* Expanded the use of `by(String)` modulator so that it can work on `Map` as well as `Element`.
+* Improved error messaging for `by(String)` so that it is more clear as to what the problem is
* Bump to Netty 4.1.42
* GraphBinary: Introduced our own `Buffer` API as a way to wrap Netty's Buffer API. Moved `GraphBinaryReader`, `GraphBinaryWriter` and `TypeSerializer<T>` to gremlin-core.
diff --git a/docs/src/recipes/appendix.asciidoc b/docs/src/recipes/appendix.asciidoc
index 3d2f7b6..dbeaaae 100644
--- a/docs/src/recipes/appendix.asciidoc
+++ b/docs/src/recipes/appendix.asciidoc
@@ -188,13 +188,13 @@
project("ab", "ba").
by(inE("link").where(outV().as("a")).count()).
by(outE("link").where(inV().as("a")).count()).
- sack(sum).by(select("ab")).
- sack(div).by(select("ba")).
+ sack(sum).by("ab").
+ sack(div).by("ba").
project("a", "b", "#(a,b)", "#(b,a)", "#(a,b) / #(b,a)").
by(select("a")).
by(select("b")).
- by(select("ab")).
- by(select("ba")).
+ by("ab").
+ by("ba").
by(sack())
----
@@ -272,7 +272,7 @@
map(group().by(label).by(unfold())).as("b").
select("a","b").
group().
- by(select("a")).
+ by("a").
by(select("b").
group().
by(select(keys)).
diff --git a/docs/src/recipes/centrality.asciidoc b/docs/src/recipes/centrality.asciidoc
index 59e6a4d..d6c0cc9 100644
--- a/docs/src/recipes/centrality.asciidoc
+++ b/docs/src/recipes/centrality.asciidoc
@@ -34,7 +34,7 @@
g.V().group().by().by(outE().count()) <3>
g.V().project("v","degree").by().by(bothE().count()) <4>
g.V().project("v","degree").by().by(bothE().count()). <5>
- order().by(select("degree"), desc).
+ order().by("degree", desc).
limit(4)
----
diff --git a/docs/src/recipes/duplicate-edge.asciidoc b/docs/src/recipes/duplicate-edge.asciidoc
index 9af411d..481b7ab 100644
--- a/docs/src/recipes/duplicate-edge.asciidoc
+++ b/docs/src/recipes/duplicate-edge.asciidoc
@@ -38,7 +38,7 @@
project("a","b"). <1>
by().by(inV().path().by().by(label)).
group(). <2>
- by(select("b")).
+ by("b").
by(select("a").fold()).
unfold(). <3>
select(values). <4>
diff --git a/docs/src/upgrade/release-3.4.x.asciidoc b/docs/src/upgrade/release-3.4.x.asciidoc
index 8fce2e7..9a64523 100644
--- a/docs/src/upgrade/release-3.4.x.asciidoc
+++ b/docs/src/upgrade/release-3.4.x.asciidoc
@@ -28,6 +28,72 @@
Please see the link:https://github.com/apache/tinkerpop/blob/3.4.5/CHANGELOG.asciidoc#release-3-4-5[changelog] for a
complete list of all the modifications that are part of this release.
+=== Upgrading for Users
+
+==== by(String) Modulator
+
+It is quite common to use the `by(String)` modulator when doing some for of selection operation where the `String` is
+the key to the value of the current `Traverser`, demonstrated as follows:
+
+[source,text]
+----
+gremlin> g.V().project('name').by('name')
+==>[name:marko]
+==>[name:vadas]
+==>[name:lop]
+==>[name:josh]
+==>[name:ripple]
+==>[name:peter]
+gremlin> g.V().order().by('name').values('name')
+==>josh
+==>lop
+==>marko
+==>peter
+==>ripple
+==>vadas
+----
+
+Of course, this approach usually only works when the current `Traverser` is an `Element`. If it is not an element, the
+error is swift and severe:
+
+[source,text]
+----
+gremlin> g.V().valueMap().project('x').by('name')
+java.util.LinkedHashMap cannot be cast to org.apache.tinkerpop.gremlin.structure.Element
+Type ':help' or ':h' for help.
+Display stack trace? [yN]n
+----
+
+and while it is perhaps straightforward to see the problem in the above example, it is not always clear exactly where
+the mistake is. The above example is the typical misuse of `by(String)` and comes when one tries to treat a `Map` the
+same way as an `Element` (which is quite reasonable).
+
+In 3.4.5, the issue of using `by(String)` on a `Map` and the error messaging have been resolved as follows:
+
+[source,text]
+----
+gremlin> g.V().valueMap().project('x').by('name')
+==>[x:[marko]]
+==>[x:[vadas]]
+==>[x:[lop]]
+==>[x:[josh]]
+==>[x:[ripple]]
+==>[x:[peter]]
+gremlin> g.V().elementMap().order().by('name')
+==>[id:4,label:person,name:josh,age:32]
+==>[id:3,label:software,name:lop,lang:java]
+==>[id:1,label:person,name:marko,age:29]
+==>[id:6,label:person,name:peter,age:35]
+==>[id:5,label:software,name:ripple,lang:java]
+==>[id:2,label:person,name:vadas,age:27]
+gremlin> g.V().values('name').project('x').by('name')
+The by("name") modulator can only be applied to a traverser that is an Element or a Map - it is being applied to [marko] a String class instead
+Type ':help' or ':h' for help.
+Display stack trace? [yN]n
+----
+
+See: link:https://issues.apache.org/jira/browse/TINKERPOP-2314[TINKERPOP-2314]
+
=== Upgrading for Providers
==== Graph Driver Providers
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ElementValueTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ElementValueTraversal.java
index fbfff62..73f73a6 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ElementValueTraversal.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ElementValueTraversal.java
@@ -22,9 +22,15 @@
import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
import org.apache.tinkerpop.gremlin.structure.Element;
+import java.util.Map;
import java.util.Objects;
/**
+ * More efficiently extracts a value from an {@link Element} or {@code Map} to avoid strategy application costs. Note
+ * that as of 3.4.5 this class is now poorly named as it was originally designed to work with {@link Element} only.
+ * In future 3.5.0 this class will likely be renamed but to ensure that we get this revised functionality for
+ * {@code Map} without introducing a breaking change this name will remain the same.
+ *
* @author Marko A. Rodriguez (http://markorodriguez.com)
*/
public final class ElementValueTraversal<V> extends AbstractLambdaTraversal<Element, V> {
@@ -48,7 +54,23 @@
@Override
public void addStart(final Traverser.Admin<Element> start) {
- this.value = null == this.bypassTraversal ? start.get().value(this.propertyKey) : TraversalUtil.apply(start, this.bypassTraversal);
+ if (null == this.bypassTraversal) {
+ // playing a game of type erasure here.....technically we can process either Map or Element here in this
+ // case after 3.4.5. and obviously users get weird errors along those lines here anyway. will fix up the
+ // generics in 3.5.0 when we can take some breaking changes. seemed like this feature would make Gremlin
+ // a lot nicer and given the small footprint of the change this seemed like a good hack to take.
+ final Object o = start.get();
+ if (o instanceof Element)
+ this.value = ((Element) o).value(propertyKey);
+ else if (o instanceof Map)
+ this.value = (V) ((Map) o).get(propertyKey);
+ else
+ throw new IllegalStateException(String.format(
+ "The by(\"%s\") modulator can only be applied to a traverser that is an Element or a Map - it is being applied to [%s] a %s class instead",
+ propertyKey, o, o.getClass().getSimpleName()));
+ } else {
+ this.value = TraversalUtil.apply(start, this.bypassTraversal);
+ }
}
public String getPropertyKey() {
diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ElementValueTraversalTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ElementValueTraversalTest.java
new file mode 100644
index 0000000..2752a3d
--- /dev/null
+++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ElementValueTraversalTest.java
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.process.traversal.lambda;
+
+import org.apache.tinkerpop.gremlin.process.traversal.traverser.B_O_Traverser;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.VertexProperty;
+import org.junit.Test;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class ElementValueTraversalTest {
+
+ @Test
+ public void shouldWorkOnVertex() {
+ final ElementValueTraversal<Integer> t = new ElementValueTraversal<>("age");
+ final Vertex v = mock(Vertex.class);
+ when(v.value("age")).thenReturn(29);
+ t.addStart(new B_O_Traverser(v, 1).asAdmin());
+ assertEquals(29, t.next().intValue());
+ }
+
+ @Test
+ public void shouldWorkOnEdge() {
+ final ElementValueTraversal<Double> t = new ElementValueTraversal<>("weight");
+ final Edge e = mock(Edge.class);
+ when(e.value("weight")).thenReturn(1.0d);
+ t.addStart(new B_O_Traverser(e, 1).asAdmin());
+ assertEquals(1.0d, t.next(), 0.00001d);
+ }
+
+ @Test
+ public void shouldWorkOnVertexProperty() {
+ final ElementValueTraversal<Integer> t = new ElementValueTraversal<>("age");
+ final VertexProperty vp = mock(VertexProperty.class);
+ when(vp.value("age")).thenReturn(29);
+ t.addStart(new B_O_Traverser(vp, 1).asAdmin());
+ assertEquals(29, t.next().intValue());
+ }
+
+ @Test
+ public void shouldWorkOnMap() {
+ final ElementValueTraversal<Integer> t = new ElementValueTraversal<>("age");
+ final Map<String,Integer> m = new HashMap<>();
+ m.put("age", 29);
+ t.addStart(new B_O_Traverser(m, 1).asAdmin());
+ assertEquals(29, t.next().intValue());
+ }
+
+ @Test(expected = IllegalStateException.class)
+ public void shouldThrowExceptionWhenTryingUnsupportedType() {
+ final ElementValueTraversal<Integer> t = new ElementValueTraversal<>("age");
+ t.addStart(new B_O_Traverser(29, 1).asAdmin());
+ t.next();
+ }
+}
diff --git a/gremlin-test/features/map/Project.feature b/gremlin-test/features/map/Project.feature
index 8974312..f3666b8 100644
--- a/gremlin-test/features/map/Project.feature
+++ b/gremlin-test/features/map/Project.feature
@@ -52,4 +52,20 @@
| lop |
| lop |
| lop |
- | ripple |
\ No newline at end of file
+ | ripple |
+
+ Scenario: g_V_valueMap_projectXxX_byXselectXnameXX
+ Given the modern graph
+ And the traversal of
+ """
+ g.V().valueMap().project("x").by(__.select("name"))
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | m[{"x":["marko"]}] |
+ | m[{"x":["josh"]}] |
+ | m[{"x":["vadas"]}] |
+ | m[{"x":["peter"]}] |
+ | m[{"x":["lop"]}] |
+ | m[{"x":["ripple"]}] |
\ No newline at end of file
diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/ProjectTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/ProjectTest.java
index 6bfae90..af71500 100644
--- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/ProjectTest.java
+++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/ProjectTest.java
@@ -27,6 +27,7 @@
import org.apache.tinkerpop.gremlin.structure.Vertex;
import org.junit.Test;
+import java.util.Collections;
import java.util.List;
import java.util.Map;
@@ -42,6 +43,8 @@
public abstract Traversal<Vertex, String> get_g_V_outXcreatedX_projectXa_bX_byXnameX_byXinXcreatedX_countX_order_byXselectXbX__descX_selectXaX();
+ public abstract Traversal<Vertex, Map<String, Object>> get_g_V_valueMap_projectXxX_byXselectXnameXX();
+
@Test
@LoadGraphWith(MODERN)
public void g_V_hasLabelXpersonX_projectXa_bX_byXoutE_countX_byXageX() {
@@ -67,6 +70,20 @@
assertEquals("ripple", names.get(3));
}
+ @Test
+ @LoadGraphWith(MODERN)
+ public void g_V_valueMap_projectXxX_byXselectXnameXX() {
+ final Traversal<Vertex, Map<String, Object>> traversal = get_g_V_valueMap_projectXxX_byXselectXnameXX();
+ printTraversalForm(traversal);
+ checkResults(makeMapList(1,
+ "x", Collections.singletonList("marko"),
+ "x", Collections.singletonList("vadas"),
+ "x", Collections.singletonList("lop"),
+ "x", Collections.singletonList("josh"),
+ "x", Collections.singletonList("ripple"),
+ "x", Collections.singletonList("peter")), traversal);
+ }
+
public static class Traversals extends ProjectTest {
@Override
@@ -78,5 +95,10 @@
public Traversal<Vertex, String> get_g_V_outXcreatedX_projectXa_bX_byXnameX_byXinXcreatedX_countX_order_byXselectXbX__descX_selectXaX() {
return g.V().out("created").project("a", "b").by("name").by(__.in("created").count()).order().by(__.select("b"), Order.desc).select("a");
}
+
+ @Override
+ public Traversal<Vertex, Map<String, Object>> get_g_V_valueMap_projectXxX_byXselectXnameXX() {
+ return g.V().valueMap().project("x").by(__.select("name"));
+ }
}
}
