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 2019/11/13 20:15:35 UTC
[tinkerpop] 01/01: TINKERPOP-2314 Employ by(String) for Map when
possible
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 <sp...@gmail.com>
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"));
+ }
}
}