Cole-Greer commented on code in PR #3241:
URL: https://github.com/apache/tinkerpop/pull/3241#discussion_r2437917792
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/RangeGlobalStep.java:
##########
@@ -55,40 +64,72 @@ public RangeGlobalStep(final Traversal.Admin traversal,
final long low, final lo
protected boolean filter(final Traverser.Admin<S> traverser) {
if (this.bypass) return true;
- if (this.high != -1 && this.counter.get() >= this.high) {
+ final String counterKey = getCounterKey(traverser);
+ final AtomicLong counter = counters.computeIfAbsent(counterKey, k ->
new AtomicLong(0L));
+
+ if (this.high != -1 && counter.get() >= this.high) {
+ if (this.getTraversal().getParent() instanceof RepeatStep) {
Review Comment:
This check should probably apply recursively to grandparents etc. Otherwise
nesting a RangeGlobalStep inside a global parent leads to failure to "reset" on
each iteration:
```
gremlin> g.V().repeat(limit(1)).times(2)
==>v[1]
gremlin> g.V().repeat(union(limit(1))).times(2)
gremlin>
```
##########
docs/src/reference/the-traversal.asciidoc:
##########
@@ -4051,6 +4054,9 @@ g.V().valueMap().select('location').range(local, 1,
2).unfold() <3>
<2> `List<String>` for each vertex containing the second location.
<3> `String` for each vertex containing the second location (use `unfold()` to
extract single elements from singleton collections).
+NOTE: When using `range()` inside <<repeat-step,`repeat()`>>, each repeat
iteration maintains its own counter.
+See the <<repeat-step,repeat step documentation>> for examples and details.
+
Review Comment:
A similar call out for `skip()` is warranted here.
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/RangeGlobalStep.java:
##########
@@ -55,40 +64,72 @@ public RangeGlobalStep(final Traversal.Admin traversal,
final long low, final lo
protected boolean filter(final Traverser.Admin<S> traverser) {
if (this.bypass) return true;
- if (this.high != -1 && this.counter.get() >= this.high) {
+ final String counterKey = getCounterKey(traverser);
+ final AtomicLong counter = counters.computeIfAbsent(counterKey, k ->
new AtomicLong(0L));
+
+ if (this.high != -1 && counter.get() >= this.high) {
+ if (this.getTraversal().getParent() instanceof RepeatStep) {
+ return false;
+ }
throw FastNoSuchElementException.instance();
}
long avail = traverser.bulk();
- if (this.counter.get() + avail <= this.low) {
+ if (counter.get() + avail <= this.low) {
// Will not surpass the low w/ this traverser. Skip and filter the
whole thing.
- this.counter.getAndAdd(avail);
+ counter.getAndAdd(avail);
return false;
}
// Skip for the low and trim for the high. Both can happen at once.
long toSkip = 0;
- if (this.counter.get() < this.low) {
- toSkip = this.low - this.counter.get();
+ if (counter.get() < this.low) {
+ toSkip = this.low - counter.get();
}
long toTrim = 0;
- if (this.high != -1 && this.counter.get() + avail >= this.high) {
- toTrim = this.counter.get() + avail - this.high;
+ if (this.high != -1 && counter.get() + avail >= this.high) {
+ toTrim = counter.get() + avail - this.high;
}
long toEmit = avail - toSkip - toTrim;
- this.counter.getAndAdd(toSkip + toEmit);
+ counter.getAndAdd(toSkip + toEmit);
traverser.setBulk(toEmit);
return true;
}
+ private String getCounterKey(Traverser.Admin<S> traverser) {
+ List<String> counterKeyParts = new ArrayList<>();
+ Traversal.Admin<Object, Object> traversal = this.getTraversal();
+ if (traversal.getParent() instanceof RepeatStep) {
Review Comment:
Note above comment about recursively checking parents
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalExplanationTest.java:
##########
@@ -131,7 +131,7 @@ public void shouldApplyStrategiesCorrectly() {
}
assertEquals(4, found);
//
- //System.out.println(traversal.explain().prettyPrint(160));
+ System.out.println(traversal.explain().prettyPrint(170));
Review Comment:
Was this an intentional change?
##########
tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphRepeatRangeGlobalTest.java:
##########
@@ -0,0 +1,759 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.tinkergraph.structure;
+
+import java.util.List;
+import java.util.function.Function;
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.tinkerpop.gremlin.process.computer.Computer;
+import org.apache.tinkerpop.gremlin.process.traversal.P;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal;
+import
org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__;
+import
org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.RepeatUnrollStrategy;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.out;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+
+public class TinkerGraphRepeatRangeGlobalTest {
+ private static GraphTraversalSource gSource;
+ private static GraphTraversalSource gComputerSource;
+
+ @BeforeClass
+ public static void setup() {
+ gSource =
TinkerGraph.open().traversal().withoutStrategies(RepeatUnrollStrategy.class);
+ load(gSource);
+ gComputerSource = gSource.withComputer(Computer.compute().workers(3));
+ }
+
+ @Test
+ public void testBasicRepeatLimit() {
+ testTraversals(s -> s.V().has("id", "l1-0")
+ .repeat(__.limit(1).out("knows"))
+ .times(2),
+ s -> s.V().has("id", "l1-0")
+ .limit(1).out("knows")
+ .limit(1).out("knows"));
+ }
+
+ @Test
+ public void testBasicRepeatLimitIncreasedLimit() {
+ testTraversals(s -> s.V().has("id", "l1-0")
+ .repeat(__.limit(2).out("knows"))
+ .times(2),
+ s -> s.V().has("id", "l1-0")
+ .limit(2).out("knows")
+ .limit(2).out("knows"));
+
+ }
+
+ @Test
+ public void testRepeatOutLimit() {
+ testTraversals(s -> s.V().has("id", "l2-0")
+ // adding order inside repeat breaks things
+ .repeat(__.out("knows").limit(2))
+ .times(2),
+ s -> s.V().has("id", "l2-0")
+ .out("knows").limit(2)
+ .out("knows").limit(2));
+ }
+
+ @Test
+ public void testRepeatWithBothAndLimit() {
+ testTraversals(s -> s.V().has("id", "l3-0")
+ .repeat(__.both("knows").order().by("id").limit(3))
+ .times(2),
+ s -> s.V().has("id", "l3-0")
+ .both("knows").order().by("id").limit(3)
+ .both("knows").order().by("id").limit(3));
+ }
+
+ @Test
+ public void testRepeatWithFilterAndLimit() {
+ testTraversals(s -> s.V().has("id", "l2-0")
+ .repeat(__.out("knows").has("id",
P.neq("l4-3")).order().limit(2))
+ .times(2),
+ s -> s.V().has("id", "l2-0")
+ .out("knows").has("id", P.neq("l4-3")).order().limit(2)
+ .out("knows").has("id",
P.neq("l4-3")).order().limit(2));
+ }
+
+ @Test
+ public void testChainedRepeatLimit() {
+ testTraversals(s -> s.V().has("id", "l2-0")
+
.repeat(__.order().by("id").limit(1).out("knows")).times(2)
+
.repeat(__.order().by("id").limit(1).in("knows")).times(2),
+ s -> s.V().has("id", "l2-0")
+ .order().by("id").limit(1).out("knows")
+ .order().by("id").limit(1).out("knows")
+ .order().by("id").limit(1).in("knows")
+ .order().by("id").limit(1).in("knows"));
+ }
+
+ @Test
+ public void testChainedRepeatLimitIncreasedLimit() {
+ testTraversals(s -> s.V().has("id", "l2-0")
+
.repeat(__.order().by("id").limit(2).out("knows")).times(2)
+
.repeat(__.order().by("id").limit(3).in("knows")).times(2),
+ s -> s.V().has("id", "l2-0")
+ .order().by("id").limit(2).out("knows")
+ .order().by("id").limit(2).out("knows")
+ .order().by("id").limit(3).in("knows")
+ .order().by("id").limit(3).in("knows"));
+ }
+
+ @Test
+ public void testNestedRepeatLimit() {
+ testTraversals(s -> s.V().has("id", "l3-0")
+ .repeat(__.order().by("id").limit(1).out("knows")
+ .repeat(__.order().by("id").limit(1).in("knows"))
+ .times(2))
+ .times(2), s -> s.V().has("id", "l3-0")
+ .order().by("id").limit(1).out("knows")
+ .order().by("id").limit(1).in("knows")
+ .order().by("id").limit(1).in("knows")
+ .order().by("id").limit(1).out("knows")
+ .order().by("id").limit(1).in("knows")
+ .order().by("id").limit(1).in("knows"));
+ }
+
+ @Test
+ public void testNestedRepeatLimitIncreasedLimit() {
+ testTraversals(s -> s.V().has("id", "l3-0")
+ .repeat(__.order().by("id").limit(2).out("knows")
+ .repeat(__.order().by("id").limit(3).in("knows"))
+ .times(2))
+ .times(2), s -> s.V().has("id", "l3-0")
+ .order().by("id").limit(2).out("knows")
+ .order().by("id").limit(3).in("knows")
+ .order().by("id").limit(3).in("knows")
+ .order().by("id").limit(2).out("knows")
+ .order().by("id").limit(3).in("knows")
+ .order().by("id").limit(3).in("knows"));
+ }
+
+ @Test
+ public void testTripleNestedRepeatLimit() {
+ testTraversals(s -> s.V().has("id", "l1-0")
+ .repeat(__.limit(1).out("knows")
+ .repeat(__.limit(1).in("knows")
+ .repeat(__.limit(1).out("knows"))
+ .times(2))
+ .times(2))
+ .times(2),
+ s -> s.V().has("id", "l1-0")
+ .limit(1).out("knows")
+ .limit(1).in("knows")
+ .limit(1).out("knows")
+ .limit(1).out("knows")
+ .limit(1).in("knows")
+ .limit(1).out("knows")
+ .limit(1).out("knows")
+ .limit(1).out("knows")
+ .limit(1).in("knows")
+ .limit(1).out("knows")
+ .limit(1).out("knows")
+ .limit(1).in("knows")
+ .limit(1).out("knows")
+ .limit(1).out("knows"));
+ }
+
+ @Test
+ public void testTripleNestedRepeatLimitIncreasedLimit() {
+ testTraversals(s -> s.V().has("id", "l1-0")
+ .repeat(__.limit(2).out("knows")
+ .repeat(__.limit(3).in("knows")
+ .repeat(__.limit(4).out("knows"))
+ .times(2))
+ .times(2))
+ .times(2),
+ s -> s.V().has("id", "l1-0")
+ .limit(2).out("knows")
+ .limit(3).in("knows")
+ .limit(4).out("knows")
+ .limit(4).out("knows")
+ .limit(3).in("knows")
+ .limit(4).out("knows")
+ .limit(4).out("knows")
+ .limit(2).out("knows")
+ .limit(3).in("knows")
+ .limit(4).out("knows")
+ .limit(4).out("knows")
+ .limit(3).in("knows")
+ .limit(4).out("knows")
+ .limit(4).out("knows"));
+ }
+
+ @Test
+ public void testAggregateRepeatLimit() {
+ testTraversals(s -> s.V().has("id", "l1-0")
+ .repeat(__.limit(1).out("knows").aggregate("x"))
+ .times(2)
+ .cap("x"),
+ s -> s.V().has("id", "l1-0")
+ .limit(1).out("knows").aggregate("x")
+ .limit(1).out("knows").aggregate("x")
+ .cap("x"));
+ }
+
+ @Test
+ public void testAggregateRepeatLimitIncreasedLimit() {
+ testTraversals(s -> s.V().has("id", "l1-0")
+ .repeat(__.limit(3).out("knows").aggregate("x"))
+ .times(2)
+ .cap("x"),
+ s -> s.V().has("id", "l1-0")
+ .limit(3).out("knows").aggregate("x")
+ .limit(3).out("knows").aggregate("x")
+ .cap("x"));
+ }
+
+ @Test
+ public void testUnionRepeatLimit() {
+ testTraversals(s -> s.V().has("id", "l1-0")
+ .union(out().limit(1), out().out().limit(1))
+ .repeat(__.limit(1)).times(1),
+ s -> s.V().has("id", "l1-0")
+ .union(out().limit(1), out().out().limit(1))
+ .limit(1));
+ }
+
+ @Test
+ public void testUnionRepeatLimitIncreasedLimit() {
+ testTraversals(s -> s.V().has("id", "l1-0")
+ .union(out().limit(1), out().out().limit(1))
+ .repeat(__.limit(3)).times(1),
+ s -> s.V().has("id", "l1-0")
+ .union(out().limit(1), out().out().limit(1))
+ .limit(3));
+ }
+
+ @Test
+ public void testBasicRepeatRange() {
+ testTraversals(s -> s.V().has("id", "l1-0")
+ .repeat(__.range(0, 1).out("knows"))
+ .times(2),
+ s -> s.V().has("id", "l1-0")
+ .range(0, 1).out("knows")
+ .range(0, 1).out("knows"));
+ }
+
+ @Test
+ public void testBasicRepeatRangeIncreasedLimit() {
+ testTraversals(s -> s.V().has("id", "l1-0")
+ .repeat(__.range(0, 2).out("knows"))
+ .times(2),
+ s -> s.V().has("id", "l1-0")
+ .range(0, 2).out("knows")
+ .range(0, 2).out("knows"));
+ }
+
+ @Test
+ public void testRepeatOutRange() {
+ testTraversals(s -> s.V().has("id", "l2-0")
+ .repeat(__.out("knows").range(0, 2))
+ .times(2),
+ s -> s.V().has("id", "l2-0")
+ .out("knows").range(0, 2)
+ .out("knows").range(0, 2));
+ }
+
+ @Test
+ public void testRepeatWithBothAndRange() {
+ testTraversals(s -> s.V().has("id", "l3-0")
+ .repeat(__.both("knows").order().by("id").range(0, 3))
+ .times(2),
+ s -> s.V().has("id", "l3-0")
+ .both("knows").order().by("id").range(0, 3)
+ .both("knows").order().by("id").range(0, 3));
+ }
+
+ @Test
+ public void testRepeatWithFilterAndRange() {
+ testTraversals(s -> s.V().has("id", "l2-0")
+ .repeat(__.out("knows").has("id",
P.neq("l4-3")).order().range(0, 2))
+ .times(2),
+ s -> s.V().has("id", "l2-0")
+ .out("knows").has("id",
P.neq("l4-3")).order().range(0, 2)
+ .out("knows").has("id",
P.neq("l4-3")).order().range(0, 2));
+ }
+
+ @Test
+ public void testChainedRepeatRange() {
+ testTraversals(s -> s.V().has("id", "l2-0")
+ .repeat(__.order().by("id").range(0,
1).out("knows")).times(2)
+ .repeat(__.order().by("id").range(0,
1).in("knows")).times(2),
+ s -> s.V().has("id", "l2-0")
+ .order().by("id").range(0, 1).out("knows")
+ .order().by("id").range(0, 1).out("knows")
+ .order().by("id").range(0, 1).in("knows")
+ .order().by("id").range(0, 1).in("knows"));
+ }
+
+ @Test
+ public void testChainedRepeatRangeIncreasedLimit() {
+ testTraversals(s -> s.V().has("id", "l2-0")
+ .repeat(__.order().by("id").range(0,
2).out("knows")).times(2)
+ .repeat(__.order().by("id").range(0,
3).in("knows")).times(2),
+ s -> s.V().has("id", "l2-0")
+ .order().by("id").range(0, 2).out("knows")
+ .order().by("id").range(0, 2).out("knows")
+ .order().by("id").range(0, 3).in("knows")
+ .order().by("id").range(0, 3).in("knows"));
+ }
+
+ @Test
+ public void testNestedRepeatRange() {
+ testTraversals(s -> s.V().has("id", "l3-0")
+ .repeat(__.order().by("id").range(0, 1).out("knows")
+ .repeat(__.order().by("id").range(0, 1).in("knows"))
+ .times(2))
+ .times(2), s -> s.V().has("id", "l3-0")
+ .order().by("id").range(0, 1).out("knows")
+ .order().by("id").range(0, 1).in("knows")
+ .order().by("id").range(0, 1).in("knows")
+ .order().by("id").range(0, 1).out("knows")
+ .order().by("id").range(0, 1).in("knows")
+ .order().by("id").range(0, 1).in("knows"));
+ }
+
+ @Test
+ public void testNestedRepeatRangeIncreasedLimit() {
+ testTraversals(s -> s.V().has("id", "l3-0")
+ .repeat(__.order().by("id").range(0, 2).out("knows")
+ .repeat(__.order().by("id").range(0, 3).in("knows"))
+ .times(2))
+ .times(2), s -> s.V().has("id", "l3-0")
+ .order().by("id").range(0, 2).out("knows")
+ .order().by("id").range(0, 3).in("knows")
+ .order().by("id").range(0, 3).in("knows")
+ .order().by("id").range(0, 2).out("knows")
+ .order().by("id").range(0, 3).in("knows")
+ .order().by("id").range(0, 3).in("knows"));
+ }
+
+ @Test
+ public void testTripleNestedRepeatRange() {
+ testTraversals(s -> s.V().has("id", "l1-0")
+ .repeat(__.range(0, 1).out("knows")
+ .repeat(__.range(0, 1).in("knows")
+ .repeat(__.range(0, 1).out("knows"))
+ .times(2))
+ .times(2))
+ .times(2),
+ s -> s.V().has("id", "l1-0")
+ .range(0, 1).out("knows")
+ .range(0, 1).in("knows")
+ .range(0, 1).out("knows")
+ .range(0, 1).out("knows")
+ .range(0, 1).in("knows")
+ .range(0, 1).out("knows")
+ .range(0, 1).out("knows")
+ .range(0, 1).out("knows")
+ .range(0, 1).in("knows")
+ .range(0, 1).out("knows")
+ .range(0, 1).out("knows")
+ .range(0, 1).in("knows")
+ .range(0, 1).out("knows")
+ .range(0, 1).out("knows"));
+ }
+
+ @Test
+ public void testTripleNestedRepeatRangeIncreasedLimit() {
+ testTraversals(s -> s.V().has("id", "l1-0")
+ .repeat(__.range(0, 2).out("knows")
+ .repeat(__.range(0, 3).in("knows")
+ .repeat(__.range(0, 4).out("knows"))
+ .times(2))
+ .times(2))
+ .times(2),
+ s -> s.V().has("id", "l1-0")
+ .range(0, 2).out("knows")
+ .range(0, 3).in("knows")
+ .range(0, 4).out("knows")
+ .range(0, 4).out("knows")
+ .range(0, 3).in("knows")
+ .range(0, 4).out("knows")
+ .range(0, 4).out("knows")
+ .range(0, 2).out("knows")
+ .range(0, 3).in("knows")
+ .range(0, 4).out("knows")
+ .range(0, 4).out("knows")
+ .range(0, 3).in("knows")
+ .range(0, 4).out("knows")
+ .range(0, 4).out("knows"));
+ }
+
+ @Test
+ public void testAggregateRepeatRange() {
+ testTraversals(s -> s.V().has("id", "l1-0")
+ .repeat(__.range(0, 1).out("knows").aggregate("x"))
+ .times(2)
+ .cap("x"),
+ s -> s.V().has("id", "l1-0")
+ .range(0, 1).out("knows").aggregate("x")
+ .range(0, 1).out("knows").aggregate("x")
+ .cap("x"));
+ }
+
+ @Test
+ public void testAggregateRepeatRangeIncreasedLimit() {
+ testTraversals(s -> s.V().has("id", "l1-0")
+ .repeat(__.range(0, 3).out("knows").aggregate("x"))
+ .times(2)
+ .cap("x"),
+ s -> s.V().has("id", "l1-0")
+ .range(0, 3).out("knows").aggregate("x")
+ .range(0, 3).out("knows").aggregate("x")
+ .cap("x"));
+ }
+
+ @Test
+ public void testUnionRepeatRange() {
+ testTraversals(s -> s.V().has("id", "l1-0")
+ .union(out().range(0, 1), out().out().range(0, 1))
+ .repeat(__.range(0, 1)).times(1),
+ s -> s.V().has("id", "l1-0")
+ .union(out().range(0, 1), out().out().range(0, 1))
+ .range(0, 1));
+ }
+
+ @Test
+ public void testUnionRepeatRangeIncreasedLimit() {
+ testTraversals(s -> s.V().has("id", "l1-0")
+ .union(out().range(0, 1), out().out().range(0, 1))
+ .repeat(__.range(0, 3)).times(1),
+ s -> s.V().has("id", "l1-0")
+ .union(out().range(0, 1), out().out().range(0, 1))
+ .range(0, 3));
+ }
+
+ private void testTraversals(Function<GraphTraversalSource,
GraphTraversal<Vertex, Vertex>> repeatFunction,
+ Function<GraphTraversalSource,
GraphTraversal<Vertex, Vertex>> nonRepeatFunction) {
+ List<?> repeatTraversal = repeatFunction.apply(gSource).toList();
+ List<?> withComputerTraversal =
repeatFunction.apply(gComputerSource).toList();
+ List<?> nonRepeatTraversal = nonRepeatFunction.apply(gSource).toList();
+ assertFalse(repeatTraversal.isEmpty());
+ assertTrue(CollectionUtils.isEqualCollection(repeatTraversal,
nonRepeatTraversal));
+ // due to non-sequential processing of computer algorithm, can not
assert equality but only size
+ assertEquals(repeatTraversal.size(), withComputerTraversal.size());
+ }
+
+ private static void load(GraphTraversalSource g) {
Review Comment:
Do you think there would be value in giving this graph a name and adding it
to TinkerFactory such that it is reusable elsewhere, or is it too specific for
the exact behaviour you are trying to test here?
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/filter/Range.feature:
##########
@@ -411,3 +411,181 @@ Feature: Step - range()
| result |
| l[d[2].i] |
| l[d[5].i] |
+
+ Scenario:
g_withoutStrategiesXRepeatUnrollStrategyX_VX5X_repeatXlimitX1X_inX_timesX2X_valuesXnameX
+ Given the modern graph
+ And using the parameter vid5 defined as "v[ripple].id"
+ And the traversal of
+ """
+
g.withoutStrategies(RepeatUnrollStrategy).V(vid5).repeat(limit(1).in()).times(2).values("name")
Review Comment:
Nit: The usages of `withoutStrategies(RepeatUnrollStrategy)` in these tests
seems redundant following the resolution of
https://issues.apache.org/jira/browse/TINKERPOP-3192
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/RangeGlobalStep.java:
##########
@@ -55,40 +64,72 @@ public RangeGlobalStep(final Traversal.Admin traversal,
final long low, final lo
protected boolean filter(final Traverser.Admin<S> traverser) {
if (this.bypass) return true;
- if (this.high != -1 && this.counter.get() >= this.high) {
+ final String counterKey = getCounterKey(traverser);
+ final AtomicLong counter = counters.computeIfAbsent(counterKey, k ->
new AtomicLong(0L));
+
+ if (this.high != -1 && counter.get() >= this.high) {
+ if (this.getTraversal().getParent() instanceof RepeatStep) {
Review Comment:
I'm curious if we can get away with dropping this if statement altogether
and always returning false instead of throwing a FastNoSuchElementException
regardless of if this step is nested in a repeat. A quick test shows that with
the exception of `g_V_playlist_paths` in Paths.feature (which seems to hang
indefinitely), all of the remaining feature tests are unaffected by such a
change. I'm not quite sure why that one scenario is failing, but that may be a
path to a preferable solution if it can be resolved.
It's notable that most filter steps (especially TailGlobalStep) don't throw
NoSuchElementExceptions and instead solely rely on returning false from
`filter()` to indicate a result should be filtered.
##########
docs/src/reference/the-traversal.asciidoc:
##########
@@ -4162,6 +4168,23 @@ In OLAP, where the atomic unit of computing is the
vertex and its local "star gr
anonymous traversals do not leave the confines of the vertex's star graph. In
other words, they can not traverse to
an adjacent vertex's properties or edges.
+==== Using limit() and range() inside repeat()
Review Comment:
Nit: include `skip()` as well. It also may be worth clarifying that this
only affects the globally scoped versions of these steps, as the local forms
implicitly reset on each traverser.
##########
docs/src/upgrade/release-3.8.x.asciidoc:
##########
@@ -830,6 +843,38 @@ g.V().repeat(both()).times(2).dedup()
See: link:https://issues.apache.org/jira/browse/TINKERPOP-3192[TINKERPOP-3192]
+==== Modified limit() and range() Semantics in repeat()
+
+The semantics of `limit()` and `range()` steps inside `repeat()` has been
modified to ensure consistent semantics across
Review Comment:
Nit: skip()
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/traverser/NL_SL_Traverser.java:
##########
@@ -0,0 +1,249 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.process.traversal.traverser;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.Stack;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.commons.collections.map.ReferenceMap;
+import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
+import
org.apache.tinkerpop.gremlin.process.traversal.traverser.util.LabelledCounter;
+
+import static
org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement.NESTED_LOOP;
+import static
org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement.SINGLE_LOOP;
+
+/**
+ * Intermediate helper interface for {@link Traverser}s that support single OR
nested loops (but not both).
Review Comment:
I'm a bit confused by this sentence (may be due to my limited understanding
of loops tracking in Traversers). I'm not understanding when a Traverser would
support nested loops but not single loops. Just looking at the names of the
traversers, all of the NL traversers also contain SL. Additionally, the way
RepeatStep adds requirements, any Traversal with NL requirements will always
contain SL requirements as well.
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/RangeGlobalStep.java:
##########
@@ -129,6 +170,9 @@ public boolean equals(final Object other) {
@Override
public Set<TraverserRequirement> getRequirements() {
+ if (this.getTraversal().getParent() instanceof RepeatStep) {
Review Comment:
The tests all seem to pass with this code removed (I only ran tests through
tinkergraph-gremlin but that should be sufficient). The getRequirements()
method in RepeatStep should already take care of these cases, as the presence
of a single RepeatStep in a traversal will add a `SINGLE_LOOP` requirement, and
if a RepeatStep wraps a child which itself contains a repeat, it will add
`NESTED_LOOP`. That alone should guarantee these requirements will be present
anytime it is needed for this step.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
