[TINKERPOP-2128] Added ExplainStrategy #2367
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## 3.6-dev #2367 +/- ##
=============================================
- Coverage 75.20% 75.19% -0.01%
+ Complexity 12322 12321 -1
=============================================
Files 1057 1058 +1
Lines 63450 63463 +13
Branches 6935 6938 +3
=============================================
+ Hits 47716 47721 +5
- Misses 13169 13171 +2
- Partials 2565 2571 +6 ☔ View full report in Codecov by Sentry. |
| public void shouldModifyTraversal() { | ||
| final Traversal<Vertex, ?> t = g.withStrategies(ExplainStrategy.instance()).V().limit(1).count().is(0); | ||
|
|
||
| final String explanation = (String) t.next(); |
There was a problem hiding this comment.
The casting to String here is similar to the problem stated in https://issues.apache.org/jira/browse/TINKERPOP-2932 . Essentially, strategies that can remove steps from the end of the traversal will cause the S, E type parameters to become incorrect. This could end up being confusing for users. I'm fine with this trade-off as long as other potential implementations have even worse useability.
| # under the License. | ||
|
|
||
| @StepClassIntegrated | ||
| Feature: Step - ExplainStrategy |
There was a problem hiding this comment.
A test with more steps in the traversal should be added.
| @@ -5097,6 +5097,17 @@ new transaction immediately following the `commit()` that raises the events. The | |||
| may also not behave as "snapshots" at the time of their creation as they are "live" references to actual database | |||
| elements. | |||
|
|
|||
| === ExplainStrategy | |||
There was a problem hiding this comment.
this change seems to create a new and additional way to do explain in Java where only this new one works in non-JVM languages? there's always going to be little differences among the various languages but this one seems really fundamental. everyone knows the explain() and I don't think there should be another way to do it.
I see the complexity of the explain() signature in trying to make this change - it returns TraversalExplanation rather than Traversal and is effectively a form of terminal step.
I don't know if this makes this change much more elegant, but couldn't you remove the user need to know about ExplainStrategy if Traversal.explain() added it and then self-iterated to return the TraversalExplanation?
In Java, I suppose you would self-iterate a clone so that you could still do:
gremlin> t = g.V().out();[]
gremlin> t.explain()
==>Traversal Explanation
==========================================================================================
Original Traversal [GraphStep(vertex,[]), VertexStep(OUT,vertex)]
ConnectiveStrategy [D] [GraphStep(vertex,[]), VertexStep(OUT,vertex)]
...
Final Traversal [TinkerGraphStep(vertex,[]), VertexStep(OUT,vertex)]
gremlin> t
==>v[3]
==>v[2]
==>v[4]
==>v[5]
==>v[3]
==>v[3]
On the idea of a completely different approach, I can't help wondering if "explain" isn't a form of GraphOp, like transactions are (i.e. a special form of Bytecode that trigger a particular command on the server). Did you consider that?
Added
ExplainStrategywhich allows to get traversal explanation for remote traversals and GLV's.https://issues.apache.org/jira/browse/TINKERPOP-2128