JanusGraph · sjudeng · Jan 6, 2018 · Dec 10, 2017 · Dec 10, 2017 · Dec 11, 2017
diff --git a/janusgraph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/optimize/HasStepFolder.java b/janusgraph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/optimize/HasStepFolder.java
@@ -15,6 +15,7 @@
 package org.janusgraph.graphdb.tinkerpop.optimize;
 
 import org.apache.tinkerpop.gremlin.process.traversal.P;
+import org.apache.tinkerpop.gremlin.process.traversal.lambda.ElementValueTraversal;
 import org.apache.tinkerpop.gremlin.process.traversal.step.map.GraphStep;
 import org.apache.tinkerpop.gremlin.process.traversal.step.map.NoOpBarrierStep;
 import org.apache.tinkerpop.gremlin.process.traversal.util.AndP;
@@ -32,7 +33,6 @@
 import org.apache.tinkerpop.gremlin.process.traversal.step.filter.RangeGlobalStep;
 import org.apache.tinkerpop.gremlin.process.traversal.step.map.OrderGlobalStep;
 import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.IdentityStep;
-import org.apache.tinkerpop.gremlin.process.traversal.step.util.ElementValueComparator;
 import org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer;
 
 import org.javatuples.Pair;
@@ -76,17 +76,24 @@ static boolean validJanusGraphHas(Iterable<HasContainer> has) {
 
     static boolean validJanusGraphOrder(OrderGlobalStep orderGlobalStep, Traversal rootTraversal,
                                           boolean isVertexOrder) {
-        for (Pair<Traversal.Admin<Object, Comparable>, Comparator<Comparable>> comp : (List<Pair<Traversal.Admin<Object, Comparable>, Comparator<Comparable>>>) orderGlobalStep.getComparators()) {
-            if (!(comp.getValue1() instanceof ElementValueComparator)) return false;
-            ElementValueComparator evc = (ElementValueComparator) comp.getValue1();
-            if (!(evc.getValueComparator() instanceof Order)) return false;
-
-            JanusGraphTransaction tx = JanusGraphTraversalUtil.getTx(rootTraversal.asAdmin());
-            String key = evc.getPropertyKey();
-            PropertyKey propertyKey = tx.getPropertyKey(key);
-            if (propertyKey == null || !(Comparable.class.isAssignableFrom(propertyKey.dataType()))) return false;
-            if (isVertexOrder && propertyKey.cardinality() != Cardinality.SINGLE) return false;
+        final List<Pair<Traversal.Admin, Object>> comparators = orderGlobalStep.getComparators();
+        for(Pair<Traversal.Admin, Object> comp : comparators) {
+            if (comp.getValue0() instanceof ElementValueTraversal &&
+                comp.getValue1() instanceof Order) {
+                final String key = ((ElementValueTraversal) comp.getValue0()).getPropertyKey();
+                final JanusGraphTransaction tx = JanusGraphTraversalUtil.getTx(rootTraversal.asAdmin());
+                final PropertyKey pKey = tx.getPropertyKey(key);
+                if(pKey == null
+                    || !(Comparable.class.isAssignableFrom(pKey.dataType()))
+                    || (isVertexOrder && pKey.cardinality() != Cardinality.SINGLE)) {
+                    return false;
+                }
+            } else {
+                // do not fold comparators that include nested traversals that are not simple ElementValues
+                return false;
+            }
         }
+
         return true;
     }
 
@@ -118,10 +125,8 @@ static void foldInIds(final HasStepFolder janusgraphStep, final Traversal.Admin<
                             ids.addAll(Arrays.asList(graphStep.getIds()));
                         }
                     }
-                    // clear ids to allow folding in ids from next HasContainer if relevant
-                    graphStep.clearIds();
                 });
-                graphStep.addIds(ids);
+
                 if (!removableHasContainers.isEmpty()) {
                     removableHasContainers.forEach(hasContainerHolder::removeHasContainer);
                 }
@@ -178,9 +183,10 @@ static void foldInOrder(final HasStepFolder janusgraphStep, final Traversal.Admi
         if (lastOrder != null) {
             if (validJanusGraphOrder(lastOrder, rootTraversal, isVertexOrder)) {
                 //Add orders to HasStepFolder
-                for (Pair<Traversal.Admin<Object, Comparable>, Comparator<Comparable>> comp : (List<Pair<Traversal.Admin<Object, Comparable>, Comparator<Comparable>>>) ((OrderGlobalStep) lastOrder).getComparators()) {
-                    ElementValueComparator evc = (ElementValueComparator) comp.getValue1();
-                    janusgraphStep.orderBy(evc.getPropertyKey(), (Order) evc.getValueComparator());
+                for (Pair<Traversal.Admin<Object, Comparable>, Comparator<Object>> comp :
+                    (List<Pair<Traversal.Admin<Object, Comparable>, Comparator<Object>>>) ((OrderGlobalStep) lastOrder).getComparators()) {
+                    ElementValueTraversal evt = (ElementValueTraversal) comp.getValue0();
+                    janusgraphStep.orderBy(evt.getPropertyKey(), (Order) comp.getValue1());
                 }
                 lastOrder.getLabels().forEach(janusgraphStep::addLabel);
                 traversal.removeStep(lastOrder);
@@ -208,6 +214,32 @@ public OrderEntry(String key, Order order) {
             this.key = key;
             this.order = order;
         }
+
+        @Override
+        public boolean equals(Object o) {
+            if (this == o) return true;
+            if (o == null || getClass() != o.getClass()) return false;
+
+            OrderEntry that = (OrderEntry) o;
+
+            if (key != null ? !key.equals(that.key) : that.key != null) return false;
+            return order == that.order;
+        }
+
+        @Override
+        public int hashCode() {
+            int result = key != null ? key.hashCode() : 0;
+            result = 31 * result + (order != null ? order.hashCode() : 0);
+            return result;
+        }
+
+        @Override
+        public String toString() {
+            return "OrderEntry{" +
+                "key='" + key + '\'' +
+                ", order=" + order +
+                '}';
+        }
     }
 
     static <E extends Ranging> void foldInRange(final HasStepFolder janusgraphStep, final Traversal.Admin<?, ?> traversal) {

diff --git a/janusgraph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/optimize/JanusGraphStep.java b/janusgraph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/optimize/JanusGraphStep.java
@@ -72,7 +72,7 @@ public JanusGraphStep(final GraphStep<S, E> originalStep) {
     @Override
     public String toString() {
         return this.hasContainers.isEmpty() ?
-                super.toString() : StringFactory.stepString(this, Arrays.toString(this.ids), this.hasContainers);
+                super.toString() : StringFactory.stepString(this, Arrays.toString(this.ids), this.hasContainers, this.orders);
     }
 
     @Override

diff --git a/janusgraph-test/src/main/java/org/janusgraph/graphdb/JanusGraphTest.java b/janusgraph-test/src/main/java/org/janusgraph/graphdb/JanusGraphTest.java
@@ -3508,9 +3508,16 @@ public void testTinkerPopOptimizationStrategies() {
         assertNumStep(10, 1, gts.V(sv[0]).local(__.outE("knows").limit(10)), JanusGraphVertexStep.class);
         assertNumStep(10, 1, gts.V(sv[0]).local(__.outE("knows").range(10, 20)), LocalStep.class);
         assertNumStep(numV, 2, gts.V(sv[0]).outE("knows").order().by("weight", decr), JanusGraphVertexStep.class, OrderGlobalStep.class);
-        assertNumStep(10, 1, gts.V(sv[0]).local(__.outE("knows").order().by("weight", decr).limit(10)), LocalStep.class);
+        // Ensure the LocalStep is dropped because the Order can be folded in the JanusGraphVertexStep which in turn
+        // will allow JanusGraphLocalQueryOptimizationStrategy to drop the LocalStep as the local ordering will be
+        // provided by the single JanusGraphVertex step
+        assertNumStep(10, 0, gts.V(sv[0]).local(__.outE("knows").order().by("weight", decr).limit(10)), LocalStep.class);
         assertNumStep(numV / 5, 2, gts.V(sv[0]).outE("knows").has("weight", 1).order().by("weight", incr), JanusGraphVertexStep.class, OrderGlobalStep.class);
-        assertNumStep(10, 1, gts.V(sv[0]).local(__.outE("knows").has("weight", 1).order().by("weight", incr).limit(10)), LocalStep.class);
+        assertNumStep(10, 0, gts.V(sv[0]).local(__.outE("knows").has("weight", 1).order().by("weight", incr).limit(10)), LocalStep.class);
+        // Note that for this test, the upper offset of the range will be folded into the JanusGraphVertexStep
+        // by JanusGraphLocalQueryOptimizationStrategy, but not the lower offset. The RangeGlobalStep will in turn be kept
+        // to enforce this lower bound and the LocalStep will be left as is as the local behavior will have not been
+        // entirely subsumed by the JanusGraphVertexStep
         assertNumStep(5, 1, gts.V(sv[0]).local(__.outE("knows").has("weight", 1).has("weight", 1).order().by("weight", incr).range(10, 15)), LocalStep.class);
         assertNumStep(1, 1, gts.V(sv[0]).outE("knows").filter(__.inV().is(vs[50])), JanusGraphVertexStep.class);
         assertNumStep(1, 1, gts.V(sv[0]).outE("knows").filter(__.otherV().is(vs[50])), JanusGraphVertexStep.class);
@@ -3520,7 +3527,7 @@ public void testTinkerPopOptimizationStrategies() {
         //Property
         assertNumStep(numV / 5, 1, gts.V(sv[0]).properties("names").has("weight", 1), JanusGraphPropertiesStep.class);
         assertNumStep(numV, 1, gts.V(sv[0]).properties("names"), JanusGraphPropertiesStep.class);
-        assertNumStep(10, 1, gts.V(sv[0]).local(__.properties("names").order().by("weight", decr).limit(10)), LocalStep.class);
+        assertNumStep(10, 0, gts.V(sv[0]).local(__.properties("names").order().by("weight", decr).limit(10)), LocalStep.class);
         assertNumStep(numV, 2, gts.V(sv[0]).outE("knows").values("weight"), JanusGraphVertexStep.class, JanusGraphPropertiesStep.class);
 
 
@@ -3540,15 +3547,16 @@ public void testTinkerPopOptimizationStrategies() {
         assertNumStep(superV * (numV / 5 * 2), 2, gts.V().has("id", sid).outE("knows").has("weight", P.gte(1)).has("weight", P.lt(3)), JanusGraphStep.class, JanusGraphVertexStep.class);
         assertNumStep(superV * (numV / 5 * 2), 2, gts.V().has("id", sid).outE("knows").has("weight", P.between(1, 3)), JanusGraphStep.class, JanusGraphVertexStep.class);
         assertNumStep(superV * 10, 2, gts.V().has("id", sid).local(__.outE("knows").has("weight", P.gte(1)).has("weight", P.lt(3)).limit(10)), JanusGraphStep.class, JanusGraphVertexStep.class);
-        assertNumStep(superV * 10, 2, gts.V().has("id", sid).local(__.outE("knows").has("weight", P.between(1, 3)).order().by("weight", decr).limit(10)), JanusGraphStep.class, LocalStep.class);
-
+        assertNumStep(superV * 10, 1, gts.V().has("id", sid).local(__.outE("knows").has("weight", P.between(1, 3)).order().by("weight", decr).limit(10)), JanusGraphStep.class);
+        assertNumStep(superV * 10, 0, gts.V().has("id", sid).local(__.outE("knows").has("weight", P.between(1, 3)).order().by("weight", decr).limit(10)), LocalStep.class);
         clopen(option(USE_MULTIQUERY), true);
         gts = graph.traversal();
 
         assertNumStep(superV * (numV / 5), 2, gts.V().has("id", sid).outE("knows").has("weight", 1), JanusGraphStep.class, JanusGraphVertexStep.class);
         assertNumStep(superV * (numV / 5 * 2), 2, gts.V().has("id", sid).outE("knows").has("weight", P.between(1, 3)), JanusGraphStep.class, JanusGraphVertexStep.class);
         assertNumStep(superV * 10, 2, gts.V().has("id", sid).local(__.outE("knows").has("weight", P.gte(1)).has("weight", P.lt(3)).limit(10)), JanusGraphStep.class, JanusGraphVertexStep.class);
-        assertNumStep(superV * 10, 2, gts.V().has("id", sid).local(__.outE("knows").has("weight", P.between(1, 3)).order().by("weight", decr).limit(10)), JanusGraphStep.class, LocalStep.class);
+        assertNumStep(superV * 10, 1, gts.V().has("id", sid).local(__.outE("knows").has("weight", P.between(1, 3)).order().by("weight", decr).limit(10)), JanusGraphStep.class);
+        assertNumStep(superV * 10, 0, gts.V().has("id", sid).local(__.outE("knows").has("weight", P.between(1, 3)).order().by("weight", decr).limit(10)), LocalStep.class);
         assertNumStep(superV * numV, 2, gts.V().has("id", sid).values("names"), JanusGraphStep.class, JanusGraphPropertiesStep.class);
 
         //Verify traversal metrics when all reads are from cache (i.e. no backend queries)
@@ -3582,19 +3590,19 @@ private static void assertNumStep(int expectedResults, int expectedSteps, GraphT
             traversal.next();
             num++;
         }
-//        System.out.println(traversal);
+
         assertEquals(expectedResults, num);
 
         //Verify that steps line up with what is expected after JanusGraph's optimizations are applied
         List<Step> steps = traversal.asAdmin().getSteps();
         Set<Class<? extends Step>> expSteps = Sets.newHashSet(expectedStepTypes);
         int numSteps = 0;
         for (Step s : steps) {
-//            System.out.println(s.getClass());
             if (s.getClass().equals(GraphStep.class) || s.getClass().equals(StartStep.class)) continue;
 
-            assertTrue(s.getClass().getName(), expSteps.contains(s.getClass()));
-            numSteps++;
+            if (expSteps.contains(s.getClass())) {
+                numSteps++;
+            }
         }
         assertEquals(expectedSteps, numSteps);
     }