[jira] [Commented] (TINKERPOP-1568) Change strategy application order

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/TINKERPOP-1568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17345697#comment-17345697 ] 

ASF GitHub Bot commented on TINKERPOP-1568:
-------------------------------------------

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


> Change strategy application order
> ---------------------------------
>
>                 Key: TINKERPOP-1568
>                 URL: https://issues.apache.org/jira/browse/TINKERPOP-1568
>             Project: TinkerPop
>          Issue Type: Improvement
>          Components: process
>    Affects Versions: 3.2.3
>            Reporter: Bryn Cooke
>            Assignee: Stephen Mallette
>            Priority: Major
>              Labels: breaking
>             Fix For: 3.5.0
>
>
> Given a traversal with the structure:
> a(b(),c(d()))
> Strategies are applied in the order:
> {noformat}
> StrategyA on a
> StrategyB on a
> StrategyA on b
> StrategyB on b
> StrategyA on c
> StrategyB on c
> StrategyA on d
> StrategyB on d
> {noformat}
> This prevents strategies from performing global operations across the traversal and all decedents effectively as children will not have been processed by preceding strategies yet.
> Say you want a strategy that compresses the entire traversal in to a string for sending over the wire, you want this to happen after everything else, but traversals with children will not have had their children processed.
> Ideally strategy application would be as follows:
> {noformat}
> StrategyA on a
> StrategyA on b
> StrategyA on c
> StrategyA on d
> StrategyB on a
> StrategyB on b
> StrategyB on c
> StrategyB on d
> {noformat}
> That way strategy B can check if it is being applied to the root traversal and if it is it knows that A has been applied globally.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)