Skip to content

Commit 4ccf5a4

Browse files
authored
Merge pull request JanusGraph#869 from twilmes/merge
Merge from 0.2
2 parents 64b8b83 + 4ac4a63 commit 4ccf5a4

File tree

4 files changed

+133
-58
lines changed

4 files changed

+133
-58
lines changed

janusgraph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/optimize/HasStepFolder.java

Lines changed: 49 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package org.janusgraph.graphdb.tinkerpop.optimize;
1616

1717
import org.apache.tinkerpop.gremlin.process.traversal.P;
18+
import org.apache.tinkerpop.gremlin.process.traversal.lambda.ElementValueTraversal;
1819
import org.apache.tinkerpop.gremlin.process.traversal.step.map.GraphStep;
1920
import org.apache.tinkerpop.gremlin.process.traversal.step.map.NoOpBarrierStep;
2021
import org.apache.tinkerpop.gremlin.process.traversal.util.AndP;
@@ -32,7 +33,6 @@
3233
import org.apache.tinkerpop.gremlin.process.traversal.step.filter.RangeGlobalStep;
3334
import org.apache.tinkerpop.gremlin.process.traversal.step.map.OrderGlobalStep;
3435
import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.IdentityStep;
35-
import org.apache.tinkerpop.gremlin.process.traversal.step.util.ElementValueComparator;
3636
import org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer;
3737

3838
import org.javatuples.Pair;
@@ -76,17 +76,24 @@ static boolean validJanusGraphHas(Iterable<HasContainer> has) {
7676

7777
static boolean validJanusGraphOrder(OrderGlobalStep orderGlobalStep, Traversal rootTraversal,
7878
boolean isVertexOrder) {
79-
for (Pair<Traversal.Admin<Object, Comparable>, Comparator<Comparable>> comp : (List<Pair<Traversal.Admin<Object, Comparable>, Comparator<Comparable>>>) orderGlobalStep.getComparators()) {
80-
if (!(comp.getValue1() instanceof ElementValueComparator)) return false;
81-
ElementValueComparator evc = (ElementValueComparator) comp.getValue1();
82-
if (!(evc.getValueComparator() instanceof Order)) return false;
83-
84-
JanusGraphTransaction tx = JanusGraphTraversalUtil.getTx(rootTraversal.asAdmin());
85-
String key = evc.getPropertyKey();
86-
PropertyKey propertyKey = tx.getPropertyKey(key);
87-
if (propertyKey == null || !(Comparable.class.isAssignableFrom(propertyKey.dataType()))) return false;
88-
if (isVertexOrder && propertyKey.cardinality() != Cardinality.SINGLE) return false;
79+
final List<Pair<Traversal.Admin, Object>> comparators = orderGlobalStep.getComparators();
80+
for(Pair<Traversal.Admin, Object> comp : comparators) {
81+
if (comp.getValue0() instanceof ElementValueTraversal &&
82+
comp.getValue1() instanceof Order) {
83+
final String key = ((ElementValueTraversal) comp.getValue0()).getPropertyKey();
84+
final JanusGraphTransaction tx = JanusGraphTraversalUtil.getTx(rootTraversal.asAdmin());
85+
final PropertyKey pKey = tx.getPropertyKey(key);
86+
if(pKey == null
87+
|| !(Comparable.class.isAssignableFrom(pKey.dataType()))
88+
|| (isVertexOrder && pKey.cardinality() != Cardinality.SINGLE)) {
89+
return false;
90+
}
91+
} else {
92+
// do not fold comparators that include nested traversals that are not simple ElementValues
93+
return false;
94+
}
8995
}
96+
9097
return true;
9198
}
9299

@@ -118,10 +125,8 @@ static void foldInIds(final HasStepFolder janusgraphStep, final Traversal.Admin<
118125
ids.addAll(Arrays.asList(graphStep.getIds()));
119126
}
120127
}
121-
// clear ids to allow folding in ids from next HasContainer if relevant
122-
graphStep.clearIds();
123128
});
124-
graphStep.addIds(ids);
129+
125130
if (!removableHasContainers.isEmpty()) {
126131
removableHasContainers.forEach(hasContainerHolder::removeHasContainer);
127132
}
@@ -178,9 +183,10 @@ static void foldInOrder(final HasStepFolder janusgraphStep, final Traversal.Admi
178183
if (lastOrder != null) {
179184
if (validJanusGraphOrder(lastOrder, rootTraversal, isVertexOrder)) {
180185
//Add orders to HasStepFolder
181-
for (Pair<Traversal.Admin<Object, Comparable>, Comparator<Comparable>> comp : (List<Pair<Traversal.Admin<Object, Comparable>, Comparator<Comparable>>>) ((OrderGlobalStep) lastOrder).getComparators()) {
182-
ElementValueComparator evc = (ElementValueComparator) comp.getValue1();
183-
janusgraphStep.orderBy(evc.getPropertyKey(), (Order) evc.getValueComparator());
186+
for (Pair<Traversal.Admin<Object, Comparable>, Comparator<Object>> comp :
187+
(List<Pair<Traversal.Admin<Object, Comparable>, Comparator<Object>>>) ((OrderGlobalStep) lastOrder).getComparators()) {
188+
ElementValueTraversal evt = (ElementValueTraversal) comp.getValue0();
189+
janusgraphStep.orderBy(evt.getPropertyKey(), (Order) comp.getValue1());
184190
}
185191
lastOrder.getLabels().forEach(janusgraphStep::addLabel);
186192
traversal.removeStep(lastOrder);
@@ -208,6 +214,32 @@ public OrderEntry(String key, Order order) {
208214
this.key = key;
209215
this.order = order;
210216
}
217+
218+
@Override
219+
public boolean equals(Object o) {
220+
if (this == o) return true;
221+
if (o == null || getClass() != o.getClass()) return false;
222+
223+
OrderEntry that = (OrderEntry) o;
224+
225+
if (key != null ? !key.equals(that.key) : that.key != null) return false;
226+
return order == that.order;
227+
}
228+
229+
@Override
230+
public int hashCode() {
231+
int result = key != null ? key.hashCode() : 0;
232+
result = 31 * result + (order != null ? order.hashCode() : 0);
233+
return result;
234+
}
235+
236+
@Override
237+
public String toString() {
238+
return "OrderEntry{" +
239+
"key='" + key + '\'' +
240+
", order=" + order +
241+
'}';
242+
}
211243
}
212244

213245
static <E extends Ranging> void foldInRange(final HasStepFolder janusgraphStep, final Traversal.Admin<?, ?> traversal) {

janusgraph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/optimize/JanusGraphStep.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ public JanusGraphStep(final GraphStep<S, E> originalStep) {
7272
@Override
7373
public String toString() {
7474
return this.hasContainers.isEmpty() ?
75-
super.toString() : StringFactory.stepString(this, Arrays.toString(this.ids), this.hasContainers);
75+
super.toString() : StringFactory.stepString(this, Arrays.toString(this.ids), this.hasContainers, this.orders);
7676
}
7777

7878
@Override

janusgraph-test/src/main/java/org/janusgraph/graphdb/JanusGraphTest.java

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3508,9 +3508,16 @@ public void testTinkerPopOptimizationStrategies() {
35083508
assertNumStep(10, 1, gts.V(sv[0]).local(__.outE("knows").limit(10)), JanusGraphVertexStep.class);
35093509
assertNumStep(10, 1, gts.V(sv[0]).local(__.outE("knows").range(10, 20)), LocalStep.class);
35103510
assertNumStep(numV, 2, gts.V(sv[0]).outE("knows").order().by("weight", decr), JanusGraphVertexStep.class, OrderGlobalStep.class);
3511-
assertNumStep(10, 1, gts.V(sv[0]).local(__.outE("knows").order().by("weight", decr).limit(10)), LocalStep.class);
3511+
// Ensure the LocalStep is dropped because the Order can be folded in the JanusGraphVertexStep which in turn
3512+
// will allow JanusGraphLocalQueryOptimizationStrategy to drop the LocalStep as the local ordering will be
3513+
// provided by the single JanusGraphVertex step
3514+
assertNumStep(10, 0, gts.V(sv[0]).local(__.outE("knows").order().by("weight", decr).limit(10)), LocalStep.class);
35123515
assertNumStep(numV / 5, 2, gts.V(sv[0]).outE("knows").has("weight", 1).order().by("weight", incr), JanusGraphVertexStep.class, OrderGlobalStep.class);
3513-
assertNumStep(10, 1, gts.V(sv[0]).local(__.outE("knows").has("weight", 1).order().by("weight", incr).limit(10)), LocalStep.class);
3516+
assertNumStep(10, 0, gts.V(sv[0]).local(__.outE("knows").has("weight", 1).order().by("weight", incr).limit(10)), LocalStep.class);
3517+
// Note that for this test, the upper offset of the range will be folded into the JanusGraphVertexStep
3518+
// by JanusGraphLocalQueryOptimizationStrategy, but not the lower offset. The RangeGlobalStep will in turn be kept
3519+
// to enforce this lower bound and the LocalStep will be left as is as the local behavior will have not been
3520+
// entirely subsumed by the JanusGraphVertexStep
35143521
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);
35153522
assertNumStep(1, 1, gts.V(sv[0]).outE("knows").filter(__.inV().is(vs[50])), JanusGraphVertexStep.class);
35163523
assertNumStep(1, 1, gts.V(sv[0]).outE("knows").filter(__.otherV().is(vs[50])), JanusGraphVertexStep.class);
@@ -3520,7 +3527,7 @@ public void testTinkerPopOptimizationStrategies() {
35203527
//Property
35213528
assertNumStep(numV / 5, 1, gts.V(sv[0]).properties("names").has("weight", 1), JanusGraphPropertiesStep.class);
35223529
assertNumStep(numV, 1, gts.V(sv[0]).properties("names"), JanusGraphPropertiesStep.class);
3523-
assertNumStep(10, 1, gts.V(sv[0]).local(__.properties("names").order().by("weight", decr).limit(10)), LocalStep.class);
3530+
assertNumStep(10, 0, gts.V(sv[0]).local(__.properties("names").order().by("weight", decr).limit(10)), LocalStep.class);
35243531
assertNumStep(numV, 2, gts.V(sv[0]).outE("knows").values("weight"), JanusGraphVertexStep.class, JanusGraphPropertiesStep.class);
35253532

35263533

@@ -3540,15 +3547,16 @@ public void testTinkerPopOptimizationStrategies() {
35403547
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);
35413548
assertNumStep(superV * (numV / 5 * 2), 2, gts.V().has("id", sid).outE("knows").has("weight", P.between(1, 3)), JanusGraphStep.class, JanusGraphVertexStep.class);
35423549
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);
3543-
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);
3544-
3550+
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);
3551+
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);
35453552
clopen(option(USE_MULTIQUERY), true);
35463553
gts = graph.traversal();
35473554

35483555
assertNumStep(superV * (numV / 5), 2, gts.V().has("id", sid).outE("knows").has("weight", 1), JanusGraphStep.class, JanusGraphVertexStep.class);
35493556
assertNumStep(superV * (numV / 5 * 2), 2, gts.V().has("id", sid).outE("knows").has("weight", P.between(1, 3)), JanusGraphStep.class, JanusGraphVertexStep.class);
35503557
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);
3551-
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);
3558+
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);
3559+
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);
35523560
assertNumStep(superV * numV, 2, gts.V().has("id", sid).values("names"), JanusGraphStep.class, JanusGraphPropertiesStep.class);
35533561

35543562
//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
35823590
traversal.next();
35833591
num++;
35843592
}
3585-
// System.out.println(traversal);
3593+
35863594
assertEquals(expectedResults, num);
35873595

35883596
//Verify that steps line up with what is expected after JanusGraph's optimizations are applied
35893597
List<Step> steps = traversal.asAdmin().getSteps();
35903598
Set<Class<? extends Step>> expSteps = Sets.newHashSet(expectedStepTypes);
35913599
int numSteps = 0;
35923600
for (Step s : steps) {
3593-
// System.out.println(s.getClass());
35943601
if (s.getClass().equals(GraphStep.class) || s.getClass().equals(StartStep.class)) continue;
35953602

3596-
assertTrue(s.getClass().getName(), expSteps.contains(s.getClass()));
3597-
numSteps++;
3603+
if (expSteps.contains(s.getClass())) {
3604+
numSteps++;
3605+
}
35983606
}
35993607
assertEquals(expectedSteps, numSteps);
36003608
}

0 commit comments

Comments
 (0)