This is an automated email from the ASF dual-hosted git repository. spmallette pushed a commit to branch TINKERPOP-2314 in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit ff2c6c56d6c80db89382e43ed56a50c0b16851bb Author: stephen <[email protected]> AuthorDate: Wed Nov 13 15:14:36 2019 -0500 TINKERPOP-2314 Employ by(String) for Map when possible Also improve errors around incorrect types that might be called using this modulator overload. --- CHANGELOG.asciidoc | 4 ++ docs/src/upgrade/release-3.4.x.asciidoc | 66 ++++++++++++++++++ .../traversal/lambda/ElementValueTraversal.java | 24 ++++++- .../lambda/ElementValueTraversalTest.java | 78 ++++++++++++++++++++++ gremlin-test/features/map/Project.feature | 18 ++++- .../process/traversal/step/map/ProjectTest.java | 22 ++++++ 6 files changed, 210 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 45bda6a..6b54aee 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -25,6 +25,8 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima 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. @@ -488,6 +490,8 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima * Bump to Tornado 5.x for gremlin-python. * Deprecated `TraversalStrategies.applyStrategies()`. * Reverted: Modified Java driver to use IP address rather than hostname to create connections. +* 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. [[release-3-3-9]] === TinkerPop 3.3.9 (Release Date: October 14, 2019) 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 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima 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.Traverser; 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 @@ public final class ElementValueTraversal<V> extends AbstractLambdaTraversal<Elem @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 @@ Feature: Step - project() | 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.process.traversal.dsl.graph.__; 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 class ProjectTest extends AbstractGremlinProcessTest { 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 @@ public abstract class ProjectTest extends AbstractGremlinProcessTest { 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 abstract class ProjectTest extends AbstractGremlinProcessTest { 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")); + } } }
