[GitHub] [tinkerpop] li-boxuan commented on a change in pull request #1211: TINKERPOP-1568 Changed Strategy Application Methodology

[GitBox <gi...@apache.org>]

li-boxuan commented on a change in pull request #1211:
URL: https://github.com/apache/tinkerpop/pull/1211#discussion_r633090186



##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversal.java
##########
@@ -121,34 +121,54 @@ public TraverserGenerator getTraverserGenerator() {
     public void applyStrategies() throws IllegalStateException {
         if (this.locked) throw Traversal.Exceptions.traversalIsLocked();
         TraversalHelper.reIdSteps(this.stepPosition, this);
-        this.strategies.applyStrategies(this);
-        boolean hasGraph = null != this.graph;
-        for (int i = 0, j = this.steps.size(); i < j; i++) { // "foreach" can lead to ConcurrentModificationExceptions
-            final Step step = this.steps.get(i);
-            if (step instanceof TraversalParent) {
-                for (final Traversal.Admin<?, ?> globalChild : ((TraversalParent) step).getGlobalChildren()) {
-                    globalChild.setStrategies(this.strategies);
-                    globalChild.setSideEffects(this.sideEffects);
-                    if (hasGraph) globalChild.setGraph(this.graph);
-                    globalChild.applyStrategies();
-                }
-                for (final Traversal.Admin<?, ?> localChild : ((TraversalParent) step).getLocalChildren()) {
-                    localChild.setStrategies(this.strategies);
-                    localChild.setSideEffects(this.sideEffects);
-                    if (hasGraph) localChild.setGraph(this.graph);
-                    localChild.applyStrategies();
-                }
+        final boolean hasGraph = null != this.graph;
+
+        // we only want to apply strategies on the top-level step or if we got some graphcomputer stuff going on.
+        // seems like in that case, the "top-level" of the traversal is really held by the VertexProgramStep which
+        // needs to have strategies applied on "pure" copies of the traversal it is holding (i think). it further
+        // seems that we need three recursions over the traversal hierarchy to ensure everything "works", where
+        // strategy application requires top-level strategies and side-effects pushed into each child and then after
+        // application of the strategies we need to call applyStrategies() on all the children to ensure that their
+        // steps get reId'd and traverser requirements are set.
+        if (isRoot() || this.getParent() instanceof VertexProgramStep) {
+
+            // note that prior to applying strategies to children we used to set side-effects and strategies of all
+            // children to that of the parent. under this revised model of strategy application from TINKERPOP-1568
+            // it doesn't appear to be necessary to do that (at least from the perspective of the test suite). by,
+            // moving side-effect setting after actual recursive strategy application we save a loop and by
+            // consequence also fix a problem where strategies might reset something in sideeffects which seems to
+            // happen in TranslationStrategy.
+            final Iterator<TraversalStrategy<?>> strategyIterator = this.strategies.toIterator();
+            while (strategyIterator.hasNext()) {
+                final TraversalStrategy<?> strategy = strategyIterator.next();
+                TraversalHelper.applyTraversalRecursively(strategy::apply, this);

Review comment:
       @spmallette When applying it recursively, shouldn't we also do `if (hasGraph) globalChild/localChild.setGraph(this.graph);` like before? Some [JanusGraph tests are failing](https://github.com/JanusGraph/janusgraph/pull/2619#discussion_r633085171) when upgrading to TinkerPop 3.5.0, likely due to this.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org