[
https://issues.apache.org/jira/browse/TINKERPOP-1568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17346336#comment-17346336
]
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.
##########
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:
Yes, thanks, that workaround works.
--
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:
[email protected]
> 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)
