TINKERPOP-3200 Make repeat() act as a global parent (#3247)

Co-authored-by: Cole-Greer <[email protected]>

Author: kenhuuu

CHANGELOG.asciidoc

@@ -102,6 +102,7 @@ This release also includes changes from <<release-3-7-XXX, 3.7.XXX>>.
 * Removed the `@RemoteOnly` testing tag in Gherkin as lambda tests have all been moved to the Java test suite.
 * Updated gremlin-javascript to use GraphBinary as default instead of GraphSONv3
 * Added the `asNumber()` step to perform number conversion.
+* Changed `repeat()` to make `repeatTraversal` global rather than a mix of local and global.
 * Renamed many types in the grammar for consistent use of terms "Literal", "Argument", and "Varargs".
 * Changed `gremlin-net` so that System.Text.Json is only listed as an explicit dependency when it is not available from the framework.
 * Fixed translation of numeric literals for Go losing type definitions.

docs/src/dev/provider/gremlin-semantics.asciidoc

@@ -2034,7 +2034,7 @@ link:https://tinkerpop.apache.org/docs/x.y.z/reference/#project-step[reference]
 [[repeat-step]]
 === repeat()
 
-*Description:* Iteratively applies a traversal (the "loop body") to each incoming traverser until a stopping
+*Description:* Iteratively applies a traversal (the "loop body") to all incoming traversers until a stopping
 condition is met. Optionally, it can emit traversers on each iteration according to an emit predicate. The
 repeat step supports loop naming and a loop counter via `loops()`.
 
@@ -2075,6 +2075,9 @@ predicates are evaluated before the first iteration (pre) or after each iteratio
 `do/while` semantics respectively:
   - Pre-check / pre-emit: when the modulator appears before `repeat(...)`.
   - Post-check / post-emit: when the modulator appears after `repeat(...)`.
+- Global traversal scope: The `repeatTraversal` is a global child. This means all traversers entering the repeat body
+are processed together as a unified stream with global semantics. `Barrier` (`order()`, `sample()`, etc.) steps within
+the repeat traversal operate across all traversers collectively rather than in isolation per traverser.
 - Loop counter semantics:
   - The loop counter for a given named or unnamed repeat is incremented once per completion of the loop body (i.e.,
 after the body finishes), not before. Therefore, `loops()` reflects the number of completed iterations.

docs/src/upgrade/release-3.8.x.asciidoc

@@ -262,6 +262,104 @@ gremlin> g.inject([Float.MAX_VALUE, Float.MAX_VALUE], [Double.MAX_VALUE, Double.
 
 See link:https://issues.apache.org/jira/browse/TINKERPOP-3115[TINKERPOP-3115]
 
+==== repeat() Step Global Children Semantics Change
+
+The `repeat()` step has been updated to treat the repeat traversal as a global child in all cases. Previously, the
+repeat traversal behaved as a hybrid between local and global semantics, which could lead to unexpected results in
+certain scenarios. The repeat traversal started off as a local child but as traversers were added back per iteration,
+it behaved more like a global child.
+
+With this change, the repeat traversal now consistently operates with global semantics, meaning that all traversers
+are processed together rather than being processed per traverser. This provides more predictable behavior and aligns
+with the semantics of other steps.
+
+[source,text]
+----
+// In 3.7.x and earlier, the order would be local to the first traverser.
+// Notice how the results are grouped by marko, then vadas, then lop
+gremlin> g.withoutStrategies(RepeatUnrollStrategy).V(1, 2, 3).
+......1> repeat(both().simplePath().order().by("name")).times(2).path().by("name")
+==>[marko,lop,josh]
+==>[marko,josh,lop]
+==>[marko,lop,peter]
+==>[marko,josh,ripple]
+==>[vadas,marko,josh]
+==>[vadas,marko,lop]
+==>[lop,marko,josh]
+==>[lop,josh,marko]
+==>[lop,josh,ripple]
+==>[lop,marko,vadas]
+
+// In 3.8.0, the repeat now consistently uses global semantics
+// The traversers from the final iteration are ordered first then by the traversers from previous iterations
+gremlin> g.withoutStrategies(RepeatUnrollStrategy).V(1, 2, 3).
+......1> repeat(both().simplePath().order().by("name")).times(2).path().by("name")
+==>[marko,lop,josh]
+==>[vadas,marko,josh]
+==>[lop,marko,josh]
+==>[marko,josh,lop]
+==>[vadas,marko,lop]
+==>[lop,josh,marko]
+==>[marko,lop,peter]
+==>[marko,josh,ripple]
+==>[lop,josh,ripple]
+==>[lop,marko,vadas]
+----
+
+This change may affect traversals that relied on the previous hybrid behavior, particularly those using side effects
+or barrier steps within `repeat()`. Review any traversals using `repeat()` with steps like `aggregate()`, `store()`,
+or other barrier steps to ensure they produce the expected results. 
+
+If you would like `repeat()` to behave similarly to how it did in 3.7.x, then you should wrap the repeat inside a
+`local()`. The following example demonstrates this:
+
+[source,text]
+----
+// In 3.7.x
+gremlin> g.V().repeat(both().simplePath().order().by("name")).times(2).path().by("name")
+==>[marko,lop,josh]
+==>[marko,josh,lop]
+==>[marko,lop,peter]
+==>[marko,josh,ripple]
+==>[vadas,marko,josh]
+==>[vadas,marko,lop]
+==>[lop,marko,josh]
+==>[lop,josh,marko]
+==>[lop,josh,ripple]
+==>[lop,marko,vadas]
+==>[josh,marko,lop]
+==>[josh,lop,marko]
+==>[josh,lop,peter]
+==>[josh,marko,vadas]
+==>[ripple,josh,lop]
+==>[ripple,josh,marko]
+==>[peter,lop,josh]
+==>[peter,lop,marko]
+
+// In 3.8.0, placing the repeat inside a local will again cause the repeat traversal to apply per traverser (locally)
+gremlin> g.V().local(repeat(both().simplePath().order().by("name")).times(2)).path().by("name")
+==>[marko,lop,josh]
+==>[marko,josh,lop]
+==>[marko,lop,peter]
+==>[marko,josh,ripple]
+==>[vadas,marko,josh]
+==>[vadas,marko,lop]
+==>[lop,marko,josh]
+==>[lop,josh,marko]
+==>[lop,josh,ripple]
+==>[lop,marko,vadas]
+==>[josh,marko,lop]
+==>[josh,lop,marko]
+==>[josh,lop,peter]
+==>[josh,marko,vadas]
+==>[ripple,josh,lop]
+==>[ripple,josh,marko]
+==>[peter,lop,josh]
+==>[peter,lop,marko]
+----
+
+See: link:https://issues.apache.org/jira/browse/TINKERPOP-3200[TINKERPOP-3200]
+
 ==== Prefer OffsetDateTime
 
 The default implementation for date type in Gremlin is now changed from the `java.util.Date` to the more encompassing
@@ -985,35 +1083,6 @@ gremlin> g.withoutStrategies(RepeatUnrollStrategy).V().values('name').repeat(inj
 ==>peter
 ----
 
-Another example is the usage of `aggregate()` inside `repeat()`. The following results returned from the `modern` graph 
-demonstrate the change of semantics if the `aggregate()` in `repeat()` were to be unrolled:
-
-[source,text]
-----
-gremlin> g.V().both().aggregate('x').both().aggregate('x').limit(10)
-==>v[1]
-==>v[1]
-==>v[1]
-==>v[1]
-==>v[1]
-==>v[1]
-==>v[1]
-==>v[4]
-==>v[4]
-==>v[4]
-gremlin> g.withoutStrategies(RepeatUnrollStrategy).V().repeat(both().aggregate('x')).times(2).limit(10)
-==>v[1]
-==>v[1]
-==>v[1]
-==>v[4]
-==>v[6]
-==>v[5]
-==>v[3]
-==>v[3]
-==>v[2]
-==>v[4]
-----
-
 Other examples of affected traversals include (but are not limited to):
 
 [source,groovy]
@@ -1196,6 +1265,62 @@ The `ChooseStep` now provides a `ChooseSemantics` enum which helps indicate if t
 
 See: link:https://issues.apache.org/jira/browse/TINKERPOP-3178[TINKERPOP-3178]
 
+===== repeat() Step Global Children Semantics Change
+
+The `RepeatStep` has been updated to consistently treat the repeat traversal as a global child rather than using
+hybrid local/global semantics. This change affects how the repeat traversal processes traversers and interacts with
+the parent traversal.
+
+Previously, `RepeatStep` would start with local semantics for the first iteration and then switch to global semantics
+for the subsequent iterations, which created inconsistencies in how side effects and barriers behaved within the repeat
+traversal. The biggest change will be to `Barrier` steps in the repeat traversal as they will now have access to all
+the starting traversers.
+
+[source,text]
+----
+// In 3.7.x and earlier, the order would be local to the first traverser.
+// Notice how the results are grouped by marko, then vadas, then lop
+gremlin> g.withoutStrategies(RepeatUnrollStrategy).V(1, 2, 3).
+......1> repeat(both().simplePath().order().by("name")).times(2).path().by("name")
+==>[marko,lop,josh]
+==>[marko,josh,lop]
+==>[marko,lop,peter]
+==>[marko,josh,ripple]
+==>[vadas,marko,josh]
+==>[vadas,marko,lop]
+==>[lop,marko,josh]
+==>[lop,josh,marko]
+==>[lop,josh,ripple]
+==>[lop,marko,vadas]
+
+// In 3.8.0, the aggregate now consistently uses global semantics
+// The traversers are now ordered so the traversers from the final iteration are ordered first then by
+// the traversers from previous iterations
+gremlin> g.withoutStrategies(RepeatUnrollStrategy).V(1, 2, 3).
+......1> repeat(both().simplePath().order().by("name")).times(2).path().by("name")
+==>[marko,lop,josh]
+==>[vadas,marko,josh]
+==>[lop,marko,josh]
+==>[marko,josh,lop]
+==>[vadas,marko,lop]
+==>[lop,josh,marko]
+==>[marko,lop,peter]
+==>[marko,josh,ripple]
+==>[lop,josh,ripple]
+==>[lop,marko,vadas]
+----
+
+Providers implementing custom optimizations or strategies around `RepeatStep` should verify that their
+implementations account for the repeat traversal being a global child. This particularly affects:
+
+- Strategies that analyze or transform repeat traversals
+- Optimizations that depend on the scope semantics of child traversals
+
+The last point about optimizations may be particularly important for providers that have memory constraints as this
+change may bring about higher memory usage due to more traversers needing to be held in memory.
+
+See: link:https://issues.apache.org/jira/browse/TINKERPOP-3200[TINKERPOP-3200]
+
 ===== Prefer OffsetDateTime
 
 The default implementation for date type in Gremlin is now changed from the deprecated `java.util.Date` to the more

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatStep.java

@@ -21,9 +21,12 @@
 import org.apache.tinkerpop.gremlin.process.traversal.Step;
 import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
 import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
+import org.apache.tinkerpop.gremlin.process.traversal.step.Barrier;
 import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent;
 import org.apache.tinkerpop.gremlin.process.traversal.step.util.ComputerAwareStep;
 import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
+import org.apache.tinkerpop.gremlin.process.traversal.util.FastNoSuchElementException;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper;
 import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
 import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
 import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
@@ -43,6 +46,7 @@ public final class RepeatStep<S> extends ComputerAwareStep<S, S> implements Trav
     private Traversal.Admin<S, S> repeatTraversal = null;
     private Traversal.Admin<S, ?> untilTraversal = null;
     private Traversal.Admin<S, ?> emitTraversal = null;
+    private boolean first = true;
     private String loopName = null;
     public boolean untilFirst = false;
     public boolean emitFirst = false;
@@ -206,20 +210,20 @@ protected Iterator<Traverser.Admin<S>> standardAlgorithm() throws NoSuchElementE
             throw new IllegalStateException("The repeat()-traversal was not defined: " + this);
 
         while (true) {
-            if (this.repeatTraversal.getEndStep().hasNext()) {
+            if (!first && this.repeatTraversal.getEndStep().hasNext()) {
                 return this.repeatTraversal.getEndStep();
             } else {
-                final Traverser.Admin<S> start = this.starts.next();
-                start.initialiseLoops(this.getId(), this.loopName);
-                if (doUntil(start, true)) {
-                    start.resetLoops();
-                    return IteratorUtils.of(start);
-                }
-                this.repeatTraversal.addStart(start);
-                if (doEmit(start, true)) {
-                    final Traverser.Admin<S> emitSplit = start.split();
-                    emitSplit.resetLoops();
-                    return IteratorUtils.of(emitSplit);
+                this.first = false;
+                if (TraversalHelper.hasStepOfAssignableClassRecursively(Barrier.class, repeatTraversal)) {
+                    // If the repeatTraversal has a Barrier then make sure that all starts are added to the
+                    // repeatTraversal before it is iterated so that RepeatStep always has "global" children.
+                    if (!this.starts.hasNext())
+                        throw FastNoSuchElementException.instance();
+                    while (this.starts.hasNext()) {
+                        processTraverser(this.starts.next());
+                    }
+                } else {
+                    return processTraverser(this.starts.next());
                 }
             }
         }
@@ -249,6 +253,21 @@ protected Iterator<Traverser.Admin<S>> computerAlgorithm() throws NoSuchElementE
         }
     }
 
+    private Iterator<Traverser.Admin<S>> processTraverser(final Traverser.Admin<S> start) {
+        start.initialiseLoops(this.getId(), this.loopName);
+        if (doUntil(start, true)) {
+            start.resetLoops();
+            return IteratorUtils.of(start);
+        }
+        this.repeatTraversal.addStart(start);
+        if (doEmit(start, true)) {
+            final Traverser.Admin<S> emitSplit = start.split();
+            emitSplit.resetLoops();
+            return IteratorUtils.of(emitSplit);
+        }
+        return Collections.emptyIterator();
+    }
+
     /////////////////////////
 
     public static <A, B, C extends Traversal<A, B>> C addRepeatToTraversal(final C traversal, final Traversal.Admin<B, B> repeatTraversal) {