[
https://issues.apache.org/jira/browse/TINKERPOP-1568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17346122#comment-17346122
]
ASF GitHub Bot commented on TINKERPOP-1568:
-------------------------------------------
spmallette commented on a change in pull request #1211:
URL: https://github.com/apache/tinkerpop/pull/1211#discussion_r633483080
##########
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:
It's been a long time since i looked at this code and as i go back to it
now, i recall wanting to revisit it before release as there still is a bit of a
tangle in it that i was never quite satisfied with. that revisiting didn't
happen, but i'm not sure it would have raised this issue because we don't seem
to have any test assertions to this effect. Other things that bothered me are
slowing coming back to memory the more I stare at this. I don't imagine more
breaking changes, just some additional refactoring.
While not ideal, is there a possible workaround for you, where you could
alter your strategy to get the `Graph` from the parent? Maybe just do:
`Traversal.Admin<?,?> root = TraversalHelper.getRootTraversal(traversal)` for
now. I've tested this approach with a contrived strategy for TinkerGraph and it
works in that case.
https://issues.apache.org/jira/browse/TINKERPOP-2568
--
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)
