This is an automated email from the ASF dual-hosted git repository. spmallette pushed a commit to branch TINKERPOP-2351 in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 40d50eaa126f378f9cf752bdfe5949a1f00f1eaa Author: Stephen Mallette <[email protected]> AuthorDate: Thu Mar 19 16:30:19 2020 -0400 TINKERPOP-2351 Fixed bug in Order when enum is a key in Map --- CHANGELOG.asciidoc | 1 + .../tinkerpop/gremlin/process/traversal/Order.java | 20 +++++-- .../gremlin/process/traversal/OrderTest.java | 3 + gremlin-test/features/map/Order.feature | 17 +++++- .../process/traversal/step/map/OrderTest.java | 65 +++++++++++++++++----- 5 files changed, 86 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 7b9ec50..c7db3b4 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -27,6 +27,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima * Added `trustStoreType` such that keystore and truststore can be of different types in the Java driver. * Added session support to gremlin-javascript. * Modified Gremlin Server to close the session when the channel itself is closed. +* Fixed bug in `Order` where comparisons of `enum` types wouldn't compare with `String` values. * Added `maxWaitForClose` configuration option to the Java driver. * Deprecated `maxWaitForSessionClose` in the Java driver. * Remove invalid service descriptors from gremlin-shaded diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Order.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Order.java index 618cdfd..8a24b1b 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Order.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Order.java @@ -98,9 +98,13 @@ public enum Order implements Comparator<Object> { asc { @Override public int compare(final Object first, final Object second) { - return first instanceof Number && second instanceof Number - ? NumberHelper.compare((Number) first, (Number) second) - : Comparator.<Comparable>naturalOrder().compare((Comparable) first, (Comparable) second); + // need to convert enum to string representations for comparison or else you can get cast exceptions. + // this typically happens when sorting local on the keys of maps that contain T + final Object f = first instanceof Enum<?> ? ((Enum<?>) first).name() : first; + final Object s = second instanceof Enum<?> ? ((Enum<?>) second).name() : second; + return f instanceof Number && s instanceof Number + ? NumberHelper.compare((Number) f, (Number) s) + : Comparator.<Comparable>naturalOrder().compare((Comparable) f, (Comparable) s); } @Override @@ -117,9 +121,13 @@ public enum Order implements Comparator<Object> { desc { @Override public int compare(final Object first, final Object second) { - return first instanceof Number && second instanceof Number - ? NumberHelper.compare((Number) second, (Number) first) - : Comparator.<Comparable>reverseOrder().compare((Comparable) first, (Comparable) second); + // need to convert enum to string representations for comparison or else you can get cast exceptions. + // this typically happens when sorting local on the keys of maps that contain T + final Object f = first instanceof Enum<?> ? ((Enum<?>) first).name() : first; + final Object s = second instanceof Enum<?> ? ((Enum<?>) second).name() : second; + return f instanceof Number && s instanceof Number + ? NumberHelper.compare((Number) s, (Number) f) + : Comparator.<Comparable>reverseOrder().compare((Comparable) f, (Comparable) s); } @Override diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/OrderTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/OrderTest.java index 0db2039..b2d3084 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/OrderTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/OrderTest.java @@ -18,6 +18,7 @@ */ package org.apache.tinkerpop.gremlin.process.traversal; +import org.apache.tinkerpop.gremlin.structure.T; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -58,6 +59,8 @@ public class OrderTest { {Order.desc, Arrays.asList(100L, 1L, -1L, 0L), Arrays.asList(100L, 1L, 0L, -1L)}, {Order.asc, Arrays.asList(100, 1, -1, 0), Arrays.asList(-1, 0, 1, 100)}, {Order.desc, Arrays.asList(100, 1, -1, 0), Arrays.asList(100, 1, 0, -1)}, + {Order.asc, Arrays.asList("b", "a", T.id, "c", "d"), Arrays.asList("a", "b", "c", "d", T.id)}, + {Order.desc, Arrays.asList("b", "a", T.id, "c", "d"), Arrays.asList(T.id, "d", "c", "b", "a")}, {Order.incr, Arrays.asList("b", "a", "c", "d"), Arrays.asList("a", "b", "c", "d")}, {Order.decr, Arrays.asList("b", "a", "c", "d"), Arrays.asList("d", "c", "b", "a")}, {Order.incr, Arrays.asList(formatter.parse("1-Jan-2018"), formatter.parse("1-Jan-2020"), formatter.parse("1-Jan-2008")), diff --git a/gremlin-test/features/map/Order.feature b/gremlin-test/features/map/Order.feature index 1d4331f..8debc4f 100644 --- a/gremlin-test/features/map/Order.feature +++ b/gremlin-test/features/map/Order.feature @@ -367,4 +367,19 @@ Feature: Step - order() Then nothing should happen because """ TODO - """ \ No newline at end of file + """ + + Scenario: g_VX1X_valueMapXtrueX_orderXlocalX_byXkeys_descXunfold + Given the modern graph + And using the parameter v1Id defined as "v[marko].id" + And the traversal of + """ + g.V(v1Id).valueMap(true).order(Scope.local).by(Column.keys, Order.desc).unfold() + """ + When iterated to list + Then the result should be ordered + | result | + | m[{"name":["marko"]}] | + | m[{"t[label]":"person"}] | + | m[{"t[id]":"v[marko].id"}] | + | m[{"age":[29]}] | \ No newline at end of file diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/OrderTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/OrderTest.java index b2fafb9..aaf87a9 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/OrderTest.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/OrderTest.java @@ -42,9 +42,13 @@ import java.util.Map; import static org.apache.tinkerpop.gremlin.LoadGraphWith.GraphData.GRATEFUL; import static org.apache.tinkerpop.gremlin.LoadGraphWith.GraphData.MODERN; +import static org.apache.tinkerpop.gremlin.process.traversal.Order.desc; import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.outE; +import static org.apache.tinkerpop.gremlin.structure.Column.keys; +import static org.hamcrest.core.Is.is; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -104,6 +108,8 @@ public abstract class OrderTest extends AbstractGremlinProcessTest { public abstract Traversal<Vertex, Map.Entry<String, Number>> get_g_V_hasLabelXpersonX_group_byXnameX_byXoutE_weight_sumX_unfold_order_byXvalues_descX(); + public abstract Traversal<Vertex, Map.Entry<Object, Object>> get_g_VX1X_valueMapXtrueX_orderXlocalX_byXkeys_descXunfold(final Object v1Id); + @Test @LoadGraphWith(MODERN) public void g_V_name_order() { @@ -449,6 +455,34 @@ public abstract class OrderTest extends AbstractGremlinProcessTest { assertFalse(traversal.hasNext()); } + @Test + @LoadGraphWith(MODERN) + public void g_VX1X_valueMapXtrueX_orderXlocalX_byXkeys_descXunfold() { + final Traversal<Vertex, ?> traversal = get_g_VX1X_valueMapXtrueX_orderXlocalX_byXkeys_descXunfold(convertToVertexId(graph, "marko")); + printTraversalForm(traversal); + + final Object name = traversal.next(); + assertEquals("name", getKey(name)); + final Object label = traversal.next(); + assertEquals(T.label, getKey(label)); + final Object id = traversal.next(); + assertEquals(T.id, getKey(id)); + final Object age = traversal.next(); + assertEquals("age", getKey(age)); + + assertThat(traversal.hasNext(), is(false)); + } + + public Object getKey(final Object kv) { + // remotes return LinkedHashMap and embedded returns Map.Entry :/ + if (kv instanceof Map.Entry) + return ((Map.Entry) kv).getKey(); + else if (kv instanceof Map) + return ((Map) kv).keySet().iterator().next(); + else + throw new IllegalStateException("Returned value should be a Map or Map.Entry but got: " + kv.getClass().getName()); + } + public static class Traversals extends OrderTest { @Override @@ -483,7 +517,7 @@ public abstract class OrderTest extends AbstractGremlinProcessTest { @Override public Traversal<Vertex, Double> get_g_V_outE_order_byXweight_descX_weight() { - return g.V().outE().order().by("weight", Order.desc).values("weight"); + return g.V().outE().order().by("weight", desc).values("weight"); } @Override @@ -507,27 +541,27 @@ public abstract class OrderTest extends AbstractGremlinProcessTest { map.put(3, (int) v.get().value("age") * 3); map.put(4, (int) v.get().value("age")); return map; - }).order(Scope.local).by(Column.values, Order.desc).by(Column.keys, Order.asc); + }).order(Scope.local).by(Column.values, desc).by(keys, Order.asc); } @Override public Traversal<Vertex, Vertex> get_g_V_order_byXoutE_count_descX() { - return g.V().order().by(outE().count(), Order.desc); + return g.V().order().by(outE().count(), desc); } @Override public Traversal<Vertex, Map<String, List<Vertex>>> get_g_V_group_byXlabelX_byXname_order_byXdescX_foldX() { - return g.V().<String, List<Vertex>>group().by(T.label).by(__.values("name").order().by(Order.desc).fold()); + return g.V().<String, List<Vertex>>group().by(T.label).by(__.values("name").order().by(desc).fold()); } @Override public Traversal<Vertex, List<Double>> get_g_V_localXbothE_weight_foldX_order_byXsumXlocalX_descX() { - return g.V().local(__.bothE().<Double>values("weight").fold()).order().by(__.sum(Scope.local), Order.desc); + return g.V().local(__.bothE().<Double>values("weight").fold()).order().by(__.sum(Scope.local), desc); } @Override public Traversal<Vertex, Map<String, Object>> get_g_V_asXvX_mapXbothE_weight_foldX_sumXlocalX_asXsX_selectXv_sX_order_byXselectXsX_descX() { - return g.V().as("v").map(__.bothE().<Double>values("weight").fold()).sum(Scope.local).as("s").select("v", "s").order().by(__.select("s"), Order.desc); + return g.V().as("v").map(__.bothE().<Double>values("weight").fold()).sum(Scope.local).as("s").select("v", "s").order().by(__.select("s"), desc); } @Override @@ -542,32 +576,32 @@ public abstract class OrderTest extends AbstractGremlinProcessTest { @Override public Traversal<Vertex, String> get_g_V_hasLabelXpersonX_order_byXvalueXageX_descX_name() { - return g.V().hasLabel("person").order().<Vertex>by(v -> v.value("age"), Order.desc).values("name"); + return g.V().hasLabel("person").order().<Vertex>by(v -> v.value("age"), desc).values("name"); } @Override public Traversal<Vertex, String> get_g_V_properties_order_byXkey_descX_key() { - return g.V().properties().order().by(T.key, Order.desc).key(); + return g.V().properties().order().by(T.key, desc).key(); } @Override public Traversal<Vertex, Vertex> get_g_V_hasXsong_name_OHBOYX_outXfollowedByX_outXfollowedByX_order_byXperformancesX_byXsongType_descX() { - return g.V().has("song", "name", "OH BOY").out("followedBy").out("followedBy").order().by("performances").by("songType", Order.desc); + return g.V().has("song", "name", "OH BOY").out("followedBy").out("followedBy").order().by("performances").by("songType", desc); } @Override public Traversal<Vertex, String> get_g_V_both_hasLabelXpersonX_order_byXage_descX_limitX5X_name() { - return g.V().both().hasLabel("person").order().by("age", Order.desc).limit(5).values("name"); + return g.V().both().hasLabel("person").order().by("age", desc).limit(5).values("name"); } @Override public Traversal<Vertex, String> get_g_V_both_hasLabelXpersonX_order_byXage_descX_name() { - return g.V().both().hasLabel("person").order().by("age", Order.desc).values("name"); + return g.V().both().hasLabel("person").order().by("age", desc).values("name"); } @Override public Traversal<Vertex, String> get_g_V_hasLabelXsongX_order_byXperformances_descX_byXnameX_rangeX110_120X_name() { - return g.V().hasLabel("song").order().by("performances", Order.desc).by("name").range(110, 120).values("name"); + return g.V().hasLabel("song").order().by("performances", desc).by("name").range(110, 120).values("name"); } @Override @@ -577,7 +611,12 @@ public abstract class OrderTest extends AbstractGremlinProcessTest { @Override public Traversal<Vertex, Map.Entry<String, Number>> get_g_V_hasLabelXpersonX_group_byXnameX_byXoutE_weight_sumX_unfold_order_byXvalues_descX() { - return g.V().hasLabel("person").group().by("name").by(outE().values("weight").sum()).<Map.Entry<String, Number>>unfold().order().by(Column.values, Order.desc); + return g.V().hasLabel("person").group().by("name").by(outE().values("weight").sum()).<Map.Entry<String, Number>>unfold().order().by(Column.values, desc); + } + + @Override + public Traversal<Vertex, Map.Entry<Object, Object>> get_g_VX1X_valueMapXtrueX_orderXlocalX_byXkeys_descXunfold(final Object v1Id) { + return g.V(v1Id).valueMap(true).order(Scope.local).by(keys, desc).unfold(); } } }
