Issue 155: Updated JanusGraphStepStrategy to fold hasId steps into JanusGraphStep

[twilmes]

This PR updates the JanusGraphStepStrategy to fold hasId steps into JanusGraphStep. This was causing issues with bulk loading via the BulkLoaderVertexProgram. HasIdStep was not folded in so full graph scans were being triggered on id lookups. This did not effect traversals of the form g.V(id), just g.V().hasId(id).

I made a few other small updates as part of this. First, it appeared that JanusGraphStepStrategyTest had some spurious annotations on it that were causing it to not run. I removed those so it will be run now and added in a test case for hasId folding. I also removed unnecessary publics from the HasStepFolder interface.

[twilmes]

Pushed another commit that refactors the JanusGraphStepStrategyTest to the TinkerPop strategy testing style which I think is easier to follow and provides better coverage.

[pluradj]

This query throws an exception:

gremlin> g.V().has("name", "marko").or().has("name", "vadas")
JanusGraph does not support the given predicate: org.apache.tinkerpop.gremlin.process.traversal.util.OrP$OrBiPredicate@5349b246
Type ':help' or ':h' for help.
Display stack trace? [yN]y
java.lang.IllegalArgumentException: JanusGraph does not support the given predicate: org.apache.tinkerpop.gremlin.process.traversal.util.OrP$OrBiPredicate@5349b246
	at org.janusgraph.graphdb.query.JanusGraphPredicate$Converter.convert(JanusGraphPredicate.java:116)
	at org.janusgraph.graphdb.tinkerpop.optimize.JanusGraphStep.lambda$new$49(JanusGraphStep.java:64)
	at org.apache.tinkerpop.gremlin.process.traversal.step.map.GraphStep.processNextStart(GraphStep.java:136)
	at org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep.hasNext(AbstractStep.java:143)
	at org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversal.hasNext(DefaultTraversal.java:179)
	at org.apache.tinkerpop.gremlin.console.Console$_closure3.doCall(Console.groovy:220)

Traversal strategy seems ok

Original Traversal                          [GraphStep(vertex,[]), HasStep([name.eq(marko)]), OrStep, HasStep([name.eq(vadas)])]

ConnectiveStrategy                    [D]   [GraphStep(vertex,[]), OrStep([[HasStep([name.eq(vadas)])], [HasStep([name.eq(marko)])]])]
IncidentToAdjacentStrategy            [O]   [GraphStep(vertex,[]), OrStep([[HasStep([name.eq(vadas)])], [HasStep([name.eq(marko)])]])]
InlineFilterStrategy                  [O]   [GraphStep(vertex,[]), HasStep([name.or(eq(vadas), eq(marko))])]
MatchPredicateStrategy                [O]   [GraphStep(vertex,[]), HasStep([name.or(eq(vadas), eq(marko))])]
RepeatUnrollStrategy                  [O]   [GraphStep(vertex,[]), HasStep([name.or(eq(vadas), eq(marko))])]
PathRetractionStrategy                [O]   [GraphStep(vertex,[]), HasStep([name.or(eq(vadas), eq(marko))])]
AdjacentToIncidentStrategy            [O]   [GraphStep(vertex,[]), HasStep([name.or(eq(vadas), eq(marko))])]
RangeByIsCountStrategy                [O]   [GraphStep(vertex,[]), HasStep([name.or(eq(vadas), eq(marko))])]
FilterRankingStrategy                 [O]   [GraphStep(vertex,[]), HasStep([name.or(eq(vadas), eq(marko))])]
LazyBarrierStrategy                   [O]   [GraphStep(vertex,[]), HasStep([name.or(eq(vadas), eq(marko))])]
AdjacentVertexFilterOptimizerStrategy [P]   [GraphStep(vertex,[]), HasStep([name.or(eq(vadas), eq(marko))])]
JanusGraphLocalQueryOptimizerStrategy [P]   [GraphStep(vertex,[]), HasStep([name.or(eq(vadas), eq(marko))])]
JanusGraphStepStrategy                [P]   [JanusGraphStep([],[name.or(eq(vadas), eq(marko))])]
ProfileStrategy                       [F]   [JanusGraphStep([],[name.or(eq(vadas), eq(marko))])]
StandardVerificationStrategy          [V]   [JanusGraphStep([],[name.or(eq(vadas), eq(marko))])]

Final Traversal                             [JanusGraphStep([],[name.or(eq(vadas), eq(marko))])]

The same query works on TinkerGraph with TP 3.2.3, and on JanusGraph (with the WARN) before applying this PR.

[twilmes]

Good catch, I was folding in HasContainers too aggressively. Found this other issue though which looks like it's occurring straight off of master. I'll see if I can fix as part of this ticket.

gremlin> g.V().has("name","marko").or().has("name","vadas")
11:40:35 WARN  org.janusgraph.graphdb.transaction.StandardJanusGraphTx  - Query requires iterating over all vertices [()]. For better performance, use indexes
==>v[4160]
==>v[4200]
gremlin> g.V().has("name","marko").or().has("name","vadas").count()
11:40:38 WARN  org.janusgraph.graphdb.transaction.StandardJanusGraphTx  - Query requires iterating over all vertices [()]. For better performance, use indexes
==>v[4160]
==>v[8296]
==>v[4272]
==>v[4264]
==>v[4200]
==>v[4280]
gremlin>

[janusgraph-bot]

Committer of one or more commits is not listed as a CLA signer, either individual or as a member of an organization.

[twilmes]

Pushed a fix so that foldId only folds id lookups and foldHasContainers handles other predicates. That previous issue I noted is actually expected behavior given that the query uses the infix notation of the 'OrStep' so no changes were required. I did find cases where further predicate folding should occur but entered that as issue #163.

[pluradj]

Thanks for opening up the other issue.
The latest push looks good to me.
VOTE: +1

[amcp]

lgtm, i like the incremental improvement on existing code you do as well.

[sjudeng]

@twilmes Sorry I know this PR is merged but could you explain a little on the refactoring of JanusGraphStepStrategyTest you did here?

Before this update JanusGraphStepStrategyTest was run with ProcessStandardSuite and resulted in 605 tests being run. With this update the class is no longer run with ProcessStandardSuite and so only the 15 tests in the class are run.

I'm interested because one of the issues in updating to TinkerPop 3.2.4 had been test errors/failures in ProfileTest when run through JanusGraphStepStrategyTest. Now that's no longer run so the error is gone. On the one hand I'm happy the error is gone but on the other hand I feel compelled to double check that something isn't being hidden here. For what it's worth ProfileTest is already run (I think without error) in numerous other tests suites, including InMemoryJanusGraphProcessTest, BerkeleyJanusGraphProcessTest, etc..

Update: I am having ProfileTest issues with 3.2.4 elsewhere so this question is moot on that basis. Still curious to make sure nothing was lost in JanusGraphStepStrategyTest though.

[NkululekoThangelane]

Hi is there any work happening regarding a Web interface for JanusGraph

[sjudeng]

@NkululekoThangelane Gremlin Server provides a web service for executing queries. But if you can post your question on the user list or chat room you might get more information on this (or other options).