This is an automated email from the ASF dual-hosted git repository. xiazcy pushed a commit to branch steps-taking-traversal-poc in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit d8dd2ae0de67390624b855176b659fcd7edb885f Author: Yang Xia <[email protected]> AuthorDate: Mon May 25 14:40:30 2026 -0700 Add optimizations into has and P --- .../tinkerpop/gremlin/process/traversal/P.java | 59 +++++++++++++--------- .../process/traversal/step/filter/HasStep.java | 44 +++++++++++----- .../gremlin/process/traversal/PTraversalTest.java | 56 +++++++++++++------- .../test/features/filter/HasTraversal.feature | 4 ++ 4 files changed, 108 insertions(+), 55 deletions(-) diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java index dbaa10f7da..29d882fa9b 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java @@ -24,6 +24,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.step.util.BulkSet; import org.apache.tinkerpop.gremlin.process.traversal.util.AndP; import org.apache.tinkerpop.gremlin.process.traversal.util.ConnectiveP; +import org.apache.tinkerpop.gremlin.structure.util.CloseableIterator; import org.apache.tinkerpop.gremlin.process.traversal.util.OrP; import java.io.Serializable; @@ -336,8 +337,6 @@ public class P<V> implements Predicate<V>, Serializable, Cloneable { if (this.traversalValue == null) return; - // Use prepare + iteration directly to avoid ambiguous overload resolution of applyAll - // when S=Object (Traverser.Admin<Object> matches both the Traverser.Admin<S> and S overloads). final Traversal.Admin<Object, Object> trav = (Traversal.Admin<Object, Object>) (Traversal.Admin) this.traversalValue; final Traverser.Admin<Object> split = (Traverser.Admin<Object>) traverser.split(); split.setSideEffects(trav.getSideEffects()); @@ -345,29 +344,35 @@ public class P<V> implements Predicate<V>, Serializable, Cloneable { trav.reset(); trav.addStart(split); - final List<Object> results = new ArrayList<>(); - while (trav.hasNext()) { - results.add(trav.next()); - } - - this.resolvedEmpty = results.isEmpty(); + this.isCollection = false; - if (results.isEmpty()) { - this.literals = Collections.emptyList(); - this.isCollection = false; - } else if (this.biPredicate instanceof Contains) { - // Collection predicates (within, without) accept multiple results - this.literals = (Collection<V>) (Collection<?>) results; - this.isCollection = true; - } else { - // Single-value predicates (Compare, Text, etc.) require exactly one result - if (results.size() > 1) { - throw new IllegalArgumentException( - "Predicate " + this.biPredicate.getPredicateName() + - " requires exactly one value but traversal produced " + results.size()); + try { + if (this.biPredicate instanceof Contains) { + // Collection predicates (within, without) need all results + final List<Object> results = new ArrayList<>(); + while (trav.hasNext()) { + results.add(trav.next()); + } + this.resolvedEmpty = results.isEmpty(); + if (!results.isEmpty()) { + this.literals = (Collection<V>) (Collection<?>) results; + this.isCollection = true; + } else { + this.literals = Collections.emptyList(); + } + } else { + // Single-value predicates (Compare, Text, etc.) only need the first result. + // Stop immediately — consistent with by(traversal) and has(key, traversal). + if (!trav.hasNext()) { + this.resolvedEmpty = true; + this.literals = Collections.emptyList(); + } else { + this.resolvedEmpty = false; + this.literals = Collections.singleton((V) trav.next()); + } } - this.literals = Collections.singleton((V) results.get(0)); - this.isCollection = false; + } finally { + CloseableIterator.closeIterator(trav); } } @@ -387,8 +392,12 @@ public class P<V> implements Predicate<V>, Serializable, Cloneable { trav.reset(); trav.addStart(split); - while (trav.hasNext()) { - allResults.add(trav.next()); + try { + while (trav.hasNext()) { + allResults.add(trav.next()); + } + } finally { + CloseableIterator.closeIterator(trav); } } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasStep.java index 65b7eebc89..fc39da51f3 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/HasStep.java @@ -103,16 +103,25 @@ public class HasStep<S extends Element> extends FilterStep<S> implements HasCont * Tests all HasContainers against a Property, handling traversal-bearing containers by evaluating * child traversals against the current traverser. */ + @SuppressWarnings("unchecked") private boolean testAllWithTraversals(final Traverser.Admin<S> traverser, final Property property) { for (final HasContainer hc : this.hasContainers) { if (hc.hasTraversal()) { - // For properties with traversals, we need to handle the traversal resolution if (hc.getTraversalValue() != null) { - final List<Object> results = collectTraversalResults(traverser, hc.getTraversalValue()); - if (results.isEmpty()) return false; - // Get the property value to test against — takes first result, ignores extras - final Object propertyValue = getPropertyValueFromProperty(property, hc.getKey()); - if (!P.eq(results.get(0)).test(propertyValue)) return false; + final Traversal.Admin<Object, Object> trav = (Traversal.Admin<Object, Object>) (Traversal.Admin) hc.getTraversalValue(); + final Traverser.Admin<Object> split = (Traverser.Admin<Object>) (Traverser.Admin) traverser.split(); + split.setSideEffects(trav.getSideEffects()); + split.setBulk(1L); + trav.reset(); + trav.addStart(split); + try { + if (!trav.hasNext()) return false; + final Object result = trav.next(); + final Object propertyValue = getPropertyValueFromProperty(property, hc.getKey()); + if (!P.eq(result).test(propertyValue)) return false; + } finally { + CloseableIterator.closeIterator(trav); + } } else if (hc.getPredicate() != null && hc.getPredicate().hasTraversal()) { hc.getPredicate().resolve(traverser); if (hc.getPredicate().isResolvedEmpty()) return false; @@ -130,16 +139,27 @@ public class HasStep<S extends Element> extends FilterStep<S> implements HasCont /** * Tests a single HasContainer against an Element, resolving traversals if present. */ + @SuppressWarnings("unchecked") private boolean testHasContainer(final Traverser.Admin<S> traverser, final HasContainer hc, final Element element) { if (hc.hasTraversal()) { if (hc.getTraversalValue() != null) { - // Direct traversal value: evaluate and use P.eq(first result) — takes first, ignores extras + // Direct traversal value: evaluate and use P.eq(first result) — takes first, ignores extras. // Consistent with by(traversal) which also takes the first result silently. - final List<Object> results = collectTraversalResults(traverser, hc.getTraversalValue()); - if (results.isEmpty()) return false; - final Object propertyValue = getPropertyValue(element, hc.getKey()); - if (propertyValue == null) return false; - return P.eq(results.get(0)).test(propertyValue); + final Traversal.Admin<Object, Object> trav = (Traversal.Admin<Object, Object>) (Traversal.Admin) hc.getTraversalValue(); + final Traverser.Admin<Object> split = (Traverser.Admin<Object>) (Traverser.Admin) traverser.split(); + split.setSideEffects(trav.getSideEffects()); + split.setBulk(1L); + trav.reset(); + trav.addStart(split); + try { + if (!trav.hasNext()) return false; + final Object result = trav.next(); + final Object propertyValue = getPropertyValue(element, hc.getKey()); + if (propertyValue == null) return false; + return P.eq(result).test(propertyValue); + } finally { + CloseableIterator.closeIterator(trav); + } } else if (hc.getPredicate() != null && hc.getPredicate().hasTraversal()) { // Predicate with embedded traversal: resolve then test normally hc.getPredicate().resolve(traverser); diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/PTraversalTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/PTraversalTest.java index 3e7af19d8b..40200c8eaa 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/PTraversalTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/PTraversalTest.java @@ -102,40 +102,60 @@ public class PTraversalTest { // --- Single-value predicates should throw on multiple results --- - @Test(expected = IllegalArgumentException.class) - public void shouldRejectMultipleResultsForEq() { - final P<Object> p = P.eq(__.inject(1, 2).asAdmin()); + // --- Single-value predicates take first result, ignore extras (consistent with by()) --- + + @SuppressWarnings("unchecked") + @Test + public void shouldTakeFirstResultForEq() { + final P<Object> p = P.eq(__.union(__.constant(1), __.constant(2)).asAdmin()); p.resolve(createTraverser("start")); + assertThat(p.test(1), is(true)); + assertThat(p.test(2), is(false)); } - @Test(expected = IllegalArgumentException.class) - public void shouldRejectMultipleResultsForNeq() { - final P<Object> p = P.neq(__.inject(1, 2).asAdmin()); + @SuppressWarnings("unchecked") + @Test + public void shouldTakeFirstResultForNeq() { + final P<Object> p = P.neq(__.union(__.constant(1), __.constant(2)).asAdmin()); p.resolve(createTraverser("start")); + assertThat(p.test(1), is(false)); + assertThat(p.test(2), is(true)); } - @Test(expected = IllegalArgumentException.class) - public void shouldRejectMultipleResultsForGt() { - final P<Object> p = P.gt(__.inject(10, 20).asAdmin()); + @SuppressWarnings("unchecked") + @Test + public void shouldTakeFirstResultForGt() { + final P<Object> p = P.gt(__.union(__.constant(10), __.constant(20)).asAdmin()); p.resolve(createTraverser("start")); + assertThat(p.test(11), is(true)); + assertThat(p.test(10), is(false)); } - @Test(expected = IllegalArgumentException.class) - public void shouldRejectMultipleResultsForLt() { - final P<Object> p = P.lt(__.inject(10, 20).asAdmin()); + @SuppressWarnings("unchecked") + @Test + public void shouldTakeFirstResultForLt() { + final P<Object> p = P.lt(__.union(__.constant(10), __.constant(20)).asAdmin()); p.resolve(createTraverser("start")); + assertThat(p.test(9), is(true)); + assertThat(p.test(10), is(false)); } - @Test(expected = IllegalArgumentException.class) - public void shouldRejectMultipleResultsForGte() { - final P<Object> p = P.gte(__.inject(10, 20).asAdmin()); + @SuppressWarnings("unchecked") + @Test + public void shouldTakeFirstResultForGte() { + final P<Object> p = P.gte(__.union(__.constant(10), __.constant(20)).asAdmin()); p.resolve(createTraverser("start")); + assertThat(p.test(10), is(true)); + assertThat(p.test(9), is(false)); } - @Test(expected = IllegalArgumentException.class) - public void shouldRejectMultipleResultsForLte() { - final P<Object> p = P.lte(__.inject(10, 20).asAdmin()); + @SuppressWarnings("unchecked") + @Test + public void shouldTakeFirstResultForLte() { + final P<Object> p = P.lte(__.union(__.constant(10), __.constant(20)).asAdmin()); p.resolve(createTraverser("start")); + assertThat(p.test(10), is(true)); + assertThat(p.test(11), is(false)); } // --- Collection predicates should accept multiple results --- diff --git a/gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/filter/HasTraversal.feature b/gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/filter/HasTraversal.feature index 3f2f924af2..8479eeac33 100644 --- a/gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/filter/HasTraversal.feature +++ b/gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/filter/HasTraversal.feature @@ -32,6 +32,8 @@ Feature: Step - has() with traversal arguments | v[marko] | @GraphComputerVerificationMidVNotSupported + # has(key, traversal) with multi-result child traversal — takes first result (order-dependent) + @InsertionOrderingRequired Scenario: g_V_hasXname_VXvid1X_outXknowsX_valuesXnameXX Given the modern graph And using the parameter vid1 defined as "v[marko].id" @@ -44,7 +46,9 @@ Feature: Step - has() with traversal arguments | result | | v[vadas] | + # has(key, traversal) with multi-result child traversal (age) — takes first result (order-dependent) @GraphComputerVerificationMidVNotSupported + @InsertionOrderingRequired Scenario: g_V_hasXage_VXvid1X_outXknowsX_valuesXageXX Given the modern graph And using the parameter vid1 defined as "v[marko].id"
