You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "kenhuuu (via GitHub)" <gi...@apache.org> on 2025/10/20 16:07:02 UTC
[PR] TINKERPOP-3200 Made repeat() act as a global parent [tinkerpop]
kenhuuu opened a new pull request, #3247:
URL: https://github.com/apache/tinkerpop/pull/3247
https://issues.apache.org/jira/browse/TINKERPOP-3200
Test tag @GraphComputerVerificationOrderingNotSupported temporarily added as there is an unrelated bug in OLAP repeat() that prevents these from being tested in the same way as OLTP.
VOTE +1
<!--
Thanks for contributing! Reminders:
+ TARGET the earliest branch where you want the change
3.7-dev -> 3.7.5 (non-breaking only)
3.8-dev -> 3.8.0
master -> 4.0.0
+ Committers will MERGE the PR forward to newer versions
+ ADD entry to the CHANGELOG.asciidoc for the targeted version
Do not reference a JIRA number there
+ ADD JIRA number to title and link in description
+ PRs requires 3 +1s from committers OR
1 +1 and 7 day wait to merge.
+ MORE details: https://s.apache.org/rtnal
-->
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3200 Made repeat() act as a global parent [tinkerpop]
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on PR #3247:
URL: https://github.com/apache/tinkerpop/pull/3247#issuecomment-3443944183
VOTE +1
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3200 Made repeat() act as a global parent [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3247:
URL: https://github.com/apache/tinkerpop/pull/3247#discussion_r2462158182
##########
docs/src/upgrade/release-3.8.x.asciidoc:
##########
Review Comment:
Can you please check if this example in the release docs is still applicable?
> 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:
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3200 Made repeat() act as a global parent [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #3247:
URL: https://github.com/apache/tinkerpop/pull/3247#discussion_r2466025833
##########
tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/TinkerWorld.java:
##########
@@ -304,6 +304,20 @@ public GraphTraversalSource getGraphTraversalSource(final LoadGraphWith.GraphDat
}})));
}
+ @Override
+ public void beforeEachScenario(Scenario scenario) {
+ this.world.beforeEachScenario(scenario);
+
+ switch (scenario.getName()) {
Review Comment:
What does this do now if used with `GraphComputer`? Is this something meant to be fixed later? If not meant to be supported should there be a computer verification that prevents the pattern with a good error message?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3200 Made repeat() act as a global parent [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3247:
URL: https://github.com/apache/tinkerpop/pull/3247#discussion_r2450145510
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatStep.java:
##########
@@ -206,25 +210,40 @@ 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.getStepsOfAssignableClassRecursively(Barrier.class, repeatTraversal).isEmpty()) {
Review Comment:
Would it be better to use `TraversalHelper. hasStepOfAssignableClassRecursively` so that it returns when a Barrier is first encountered instead of going through the entire traversal tree?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3200 Made repeat() act as a global parent [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #3247:
URL: https://github.com/apache/tinkerpop/pull/3247#discussion_r2466914248
##########
tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/TinkerWorld.java:
##########
@@ -304,6 +304,20 @@ public GraphTraversalSource getGraphTraversalSource(final LoadGraphWith.GraphDat
}})));
}
+ @Override
+ public void beforeEachScenario(Scenario scenario) {
+ this.world.beforeEachScenario(scenario);
+
+ switch (scenario.getName()) {
Review Comment:
i think it would be good to include a comment here pointing to the bug so that it's clear in the code there's an issue to relate it to.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3200 Made repeat() act as a global parent [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #3247:
URL: https://github.com/apache/tinkerpop/pull/3247#discussion_r2448845959
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/integrated/Paths.feature:
##########
@@ -103,11 +98,9 @@ Feature: Step - paths
When iterated to list
Then the result should be unordered
| result |
- | m[{"song":"TOMORROW IS A LONG TIME","artists":["Bob_Dylan"]}] |
- | m[{"song":"JOHN BROWN","artists":["Bob_Dylan"]}] |
+ | m[{"song":"I WANT YOU","artists":["Bob_Dylan"]}] |
Review Comment:
Is this change purely a result of `order().by(shuffle)` now independently scrambling each iteration instead of previous hybrid local/global semantics which essentially wouldn't order the first iteration?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3200 Made repeat() act as a global parent [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #3247:
URL: https://github.com/apache/tinkerpop/pull/3247#discussion_r2466786994
##########
tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/TinkerWorld.java:
##########
@@ -304,6 +304,20 @@ public GraphTraversalSource getGraphTraversalSource(final LoadGraphWith.GraphDat
}})));
}
+ @Override
+ public void beforeEachScenario(Scenario scenario) {
+ this.world.beforeEachScenario(scenario);
+
+ switch (scenario.getName()) {
Review Comment:
> What does this do now if used with GraphComputer?
It will return the wrong result. It seems as though the ordering is not applied globally.
> Is this something meant to be fixed later?
Yes, https://issues.apache.org/jira/browse/TINKERPOP-3208
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3200 Made repeat() act as a global parent [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu merged PR #3247:
URL: https://github.com/apache/tinkerpop/pull/3247
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3200 Made repeat() act as a global parent [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3247:
URL: https://github.com/apache/tinkerpop/pull/3247#discussion_r2452821826
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatStep.java:
##########
@@ -206,25 +210,40 @@ 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.getStepsOfAssignableClassRecursively(Barrier.class, repeatTraversal).isEmpty()) {
+ // 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());
}
}
}
}
+ private Iterator<Traverser.Admin<S>> processTraverser(final Traverser.Admin<S> start) {
Review Comment:
Nit: private method below public methods
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3200 Made repeat() act as a global parent [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on PR #3247:
URL: https://github.com/apache/tinkerpop/pull/3247#issuecomment-3438035834
VOTE +1
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3200 Made repeat() act as a global parent [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #3247:
URL: https://github.com/apache/tinkerpop/pull/3247#discussion_r2452467520
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/branch/RepeatStep.java:
##########
@@ -206,25 +210,40 @@ 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.getStepsOfAssignableClassRecursively(Barrier.class, repeatTraversal).isEmpty()) {
Review Comment:
Yea, that would be better. Thanks for pointing this out.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3200 Made repeat() act as a global parent [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3247:
URL: https://github.com/apache/tinkerpop/pull/3247#discussion_r2452807372
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/branch/Repeat.feature:
##########
Review Comment:
Would be good to add tests that cover a wider variety of the types of steps used inside repeat. For example addV, dedup, property, tail, etc.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3200 Made repeat() act as a global parent [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #3247:
URL: https://github.com/apache/tinkerpop/pull/3247#discussion_r2449980073
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/integrated/Paths.feature:
##########
@@ -103,11 +98,9 @@ Feature: Step - paths
When iterated to list
Then the result should be unordered
| result |
- | m[{"song":"TOMORROW IS A LONG TIME","artists":["Bob_Dylan"]}] |
- | m[{"song":"JOHN BROWN","artists":["Bob_Dylan"]}] |
+ | m[{"song":"I WANT YOU","artists":["Bob_Dylan"]}] |
Review Comment:
That's mostly it. The full BFS nature of it now means it found a shorter way to get to that until() condition.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org