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 febdda55012106bbb38d9213bbb1a3bca0fab7ed Author: Yang Xia <[email protected]> AuthorDate: Thu Jun 11 23:39:58 2026 -0700 Update has() to use P.eq and update validation to use marker interface, along with misc updates --- CHANGELOG.asciidoc | 2 +- docs/src/reference/the-traversal.asciidoc | 12 +- docs/src/upgrade/release-4.x.x.asciidoc | 8 +- .../tinkerpop/gremlin/process/traversal/NotP.java | 12 +- .../tinkerpop/gremlin/process/traversal/P.java | 34 ++-- .../traversal/dsl/graph/GraphTraversal.java | 10 +- .../step/AcceptsChildPredicateTraversal.java | 34 ++++ .../process/traversal/step/filter/AllStep.java | 3 +- .../process/traversal/step/filter/AnyStep.java | 3 +- .../process/traversal/step/filter/HasStep.java | 173 ++++----------------- .../process/traversal/step/filter/IsStep.java | 3 +- .../process/traversal/step/filter/NoneStep.java | 3 +- .../traversal/step/filter/WherePredicateStep.java | 3 +- .../process/traversal/step/map/GraphStep.java | 3 +- .../traversal/step/sideEffect/AddPropertyStep.java | 50 ++++-- .../step/sideEffect/AddPropertyStepContract.java | 4 +- .../sideEffect/AddPropertyStepPlaceholder.java | 16 +- .../process/traversal/step/util/HasContainer.java | 38 +---- .../ChildTraversalVerificationStrategy.java | 17 +- .../gremlin/process/traversal/util/AndP.java | 20 +++ .../gremlin/process/traversal/PTraversalTest.java | 139 ++++++++++++++++- .../step/CloneIndependenceTraversalTest.java | 17 +- .../traversal/step/util/HasContainerTest.java | 36 ++--- 23 files changed, 365 insertions(+), 275 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 66dc0980b1..f48f88edd8 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -35,7 +35,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima * Added `hasLabel(Traversal)` overload with grammar support for dynamic label filtering. * Added traversal-bearing predicate support to `where(P)` — resolves child traversal and tests against current value when `P.hasTraversal()` is true. * Added rejection of traversal-bearing predicates in `choose(P)` — use `choose(__.is(P.op(traversal)), ...)` form instead. -* Added runtime Map validation for `property(traversal)` — rejects non-Map results to prevent sentinel key leakage. +* Added runtime Map validation for `property(traversal)` — rejects results that are not a `Map` of property key/value pairs. * Added mixed traversal/literal detection in `P.within()` and `P.without()` — throws `IllegalArgumentException` with guidance to wrap literals in `__.constant()`. * Removed `uuid` dependency from `gremlin-javascript` in favor of the built-in `globalThis.crypto.randomUUID()`. * Added streaming HTTP response support to `gremlin-driver` for incremental result deserialization over GraphBinary. diff --git a/docs/src/reference/the-traversal.asciidoc b/docs/src/reference/the-traversal.asciidoc index bb24250651..52fb52a29f 100644 --- a/docs/src/reference/the-traversal.asciidoc +++ b/docs/src/reference/the-traversal.asciidoc @@ -4050,16 +4050,18 @@ g.addV().property(set, null) The `property()` step also accepts a traversal that produces a `Map` of key-value pairs to set as properties. The child traversal must be read-only — mutating steps are rejected. If the traversal does not produce a `Map`, the result -is rejected. +is rejected. Each entry in the resulting Map becomes a separate property on the element, allowing multiple properties +to be set in a single step. [gremlin-groovy,modern] ---- -g.V(2).property(__.V(2).project('friendCount').by(__.both('knows').count())) <1> -g.V(2).valueMap() +g.V(4).property(__.V(1).project('friendCount','createdSoftware').by(__.out('knows').count()).by(__.out('created').values('name'))) <1> +g.V(4).valueMap('friendCount','createdSoftware') ---- -<1> Set properties on vertex 2 from a Map produced by a child traversal. Here the `project()` step creates a Map with -key "friendCount" and a value computed from the graph. +<1> Set two properties on vertex 4 (josh) from a Map produced by a child traversal. The `project()` step builds a Map +with keys "friendCount" and "createdSoftware", whose values are computed from vertex 1's (marko's) relationships — +number of friends and name of software created. Both properties are applied to vertex 4 in one operation. *Additional References* diff --git a/docs/src/upgrade/release-4.x.x.asciidoc b/docs/src/upgrade/release-4.x.x.asciidoc index 45555d65dc..83752c31ac 100644 --- a/docs/src/upgrade/release-4.x.x.asciidoc +++ b/docs/src/upgrade/release-4.x.x.asciidoc @@ -54,8 +54,8 @@ Affected steps: `has()`, `hasLabel()`, `V()`, `E()`, `property()`, `is()`, `wher // Dynamic property comparison g.V().has("age", P.gt(__.V(1).values("age"))) -// Dynamic vertex lookup via select() -g.V(1).id().as("x").V().has("name","josh").V(__.select("x")).values("name") +// Dynamic vertex lookup: resolve the ids of marko's friends, then look them up by id +g.V(1).V(__.out("knows").id()).values("name") // Multi-source filtering with within() g.V().has("name", P.within(__.V(1).out("knows").values("name").fold(), __.constant("peter"))) @@ -544,8 +544,8 @@ if (hasContainer.hasTraversal()) continue; // skip — dynamic value, cannot fo ---- `GraphStep.processHasContainerIds()` already includes this guard. Providers that implement their own `HasContainer` -folding strategy should add the same check. Without it, `getValue()` and `getBiPredicate()` will throw -`IllegalStateException` on traversal-bearing containers. +folding strategy should add the same check. Without it, the container's value is read before it has been resolved +against a traverser, so an unresolved value would be folded into the index lookup and produce incorrect results. ==== Graph Driver Providers diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/NotP.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/NotP.java index 50ea9c4be8..aca1d241e4 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/NotP.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/NotP.java @@ -59,11 +59,19 @@ public class NotP<V> extends P<V> { } /** - * Returns the original unwrapped P contained within this NotP, as double negation cancels out. + * Returns the inner predicate wrapped by this NotP. + */ + public P<V> getWrapped() { + return this.originalP; + } + + /** + * Returns the original unwrapped P, since double negation cancels out. + * @apiNote Functionally identical to {@link #getWrapped()}, but fulfills the {@link java.util.function.Predicate#negate()} contract. */ @Override public P<V> negate() { - return originalP; + return getWrapped(); } public P<V> clone() { 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 810a67e372..5f0606bd07 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 @@ -344,9 +344,19 @@ public class P<V> implements Predicate<V>, Serializable, Cloneable { try { if (!trav.hasNext()) { - this.resolvedEmpty = true; + // No results from the child traversal. For collection predicates (within/without) this is a + // legitimate empty set: within(empty) -> false, without(empty) -> true. Resolve to an empty + // collection and let Contains.test() apply the correct semantics rather than short-circuiting. + // For scalar predicates (eq/gt/lt/etc.) there is no comparison value, so flag as resolved-empty + // and let the step short-circuit (cannot satisfy). this.literals = Collections.emptyList(); - this.isCollection = false; + if (this.biPredicate instanceof Contains) { + this.resolvedEmpty = false; + this.isCollection = true; + } else { + this.resolvedEmpty = true; + this.isCollection = false; + } } else { this.resolvedEmpty = false; final Object firstResult = trav.next(); @@ -392,14 +402,14 @@ public class P<V> implements Predicate<V>, Serializable, Cloneable { } } - this.resolvedEmpty = allResults.isEmpty(); - if (allResults.isEmpty()) { - this.literals = Collections.emptyList(); - this.isCollection = false; - } else { - this.literals = (Collection<V>) (Collection<?>) allResults; - this.isCollection = true; - } + // Multi-traversal resolution is only valid for collection predicates (within/without). An empty + // combined result is a legitimate empty set, so resolve to an empty collection and let Contains.test() + // apply the correct semantics (within(empty) -> false, without(empty) -> true) instead of short-circuiting. + this.resolvedEmpty = false; + this.isCollection = true; + this.literals = allResults.isEmpty() + ? Collections.emptyList() + : (Collection<V>) (Collection<?>) allResults; } /** @@ -426,7 +436,7 @@ public class P<V> implements Predicate<V>, Serializable, Cloneable { integrateTraversals(child, parent); } } else if (p instanceof NotP) { - integrateTraversals(((NotP<?>) p).negate(), parent); + integrateTraversals(((NotP<?>) p).getWrapped(), parent); } else if (p.getTraversalValue() != null) { parent.integrateChild(p.getTraversalValue()); } else if (p.getTraversalValues() != null) { @@ -446,7 +456,7 @@ public class P<V> implements Predicate<V>, Serializable, Cloneable { collectTraversals(child, traversals); } } else if (p instanceof NotP) { - collectTraversals(((NotP<?>) p).negate(), traversals); + collectTraversals(((NotP<?>) p).getWrapped(), traversals); } else if (p.getTraversalValue() != null) { traversals.add(p.getTraversalValue()); } else if (p.getTraversalValues() != null) { diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java index fcf46d23f4..2b8ec3eca7 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java @@ -2720,7 +2720,7 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { if (null == traversal) return has(propertyKey, (Object) null); ChildTraversalValidator.validate(traversal.asAdmin()); this.asAdmin().getGremlinLang().addStep(Symbols.has, propertyKey, traversal); - final HasContainer hasContainer = new HasContainer(propertyKey, traversal.asAdmin()); + final HasContainer hasContainer = new HasContainer(propertyKey, P.eq(traversal.asAdmin())); return this.asAdmin().addStep(new HasStep(this.asAdmin(), hasContainer)); } @@ -2742,7 +2742,7 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { ChildTraversalValidator.validate(traversal.asAdmin()); this.asAdmin().getGremlinLang().addStep(Symbols.has, accessor, traversal); - final HasContainer hasContainer = new HasContainer(accessor.getAccessor(), traversal.asAdmin()); + final HasContainer hasContainer = new HasContainer(accessor.getAccessor(), P.eq(traversal.asAdmin())); return this.asAdmin().addStep(new HasStep(this.asAdmin(), hasContainer)); } @@ -2795,7 +2795,7 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { ChildTraversalValidator.validate(traversal.asAdmin()); this.asAdmin().getGremlinLang().addStep(Symbols.has, label, propertyKey, traversal); TraversalHelper.addHasContainer(this.asAdmin(), new HasContainer(T.label.getAccessor(), P.eq(label))); - final HasContainer hasContainer = new HasContainer(propertyKey, traversal.asAdmin()); + final HasContainer hasContainer = new HasContainer(propertyKey, P.eq(traversal.asAdmin())); return this.asAdmin().addStep(new HasStep(this.asAdmin(), hasContainer)); } @@ -2907,7 +2907,7 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { if (null == traversal) return hasLabel((String) null); ChildTraversalValidator.validate(traversal.asAdmin()); this.asAdmin().getGremlinLang().addStep(Symbols.hasLabel, traversal); - final HasContainer hasContainer = new HasContainer(T.label.getAccessor(), traversal.asAdmin()); + final HasContainer hasContainer = new HasContainer(T.label.getAccessor(), P.eq(traversal.asAdmin())); return this.asAdmin().addStep(new HasStep(this.asAdmin(), hasContainer)); } @@ -3961,7 +3961,7 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> { if (null == mapTraversal) return this; ChildTraversalValidator.validate(mapTraversal.asAdmin()); this.asAdmin().getGremlinLang().addStep(Symbols.property, mapTraversal); - this.asAdmin().addStep(new AddPropertyStepPlaceholder(this.asAdmin(), null, "~traversalMap", mapTraversal.asAdmin())); + this.asAdmin().addStep(new AddPropertyStepPlaceholder(this.asAdmin(), null, null, mapTraversal.asAdmin(), true)); return this; } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/AcceptsChildPredicateTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/AcceptsChildPredicateTraversal.java new file mode 100644 index 0000000000..fef3841441 --- /dev/null +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/AcceptsChildPredicateTraversal.java @@ -0,0 +1,34 @@ +/* + * 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.step; + +/** + * Marker interface for steps that accept user-supplied child traversals embedded as predicate or lookup + * arguments (for example {@code has(key, traversal)}, {@code is(P.gt(traversal))}, + * {@code V(traversal)}, or {@code property(traversal)}). + * <p> + * The {@link org.apache.tinkerpop.gremlin.process.traversal.strategy.verification.ChildTraversalVerificationStrategy} + * validates the local children of every step implementing this interface, rejecting child traversals that + * contain mutating steps. New steps that accept such traversals should implement this interface so they are + * validated automatically rather than relying on a hardcoded step list. + * + * @since 4.0.0 + */ +public interface AcceptsChildPredicateTraversal { +} diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AllStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AllStep.java index ddabdfb798..f17795dc0b 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AllStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AllStep.java @@ -21,6 +21,7 @@ package org.apache.tinkerpop.gremlin.process.traversal.step.filter; import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.step.AcceptsChildPredicateTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; import org.apache.tinkerpop.gremlin.structure.util.StringFactory; @@ -32,7 +33,7 @@ import java.util.Iterator; import java.util.List; import java.util.Set; -public final class AllStep<S, S2> extends FilterStep<S> implements TraversalParent { +public final class AllStep<S, S2> extends FilterStep<S> implements TraversalParent, AcceptsChildPredicateTraversal { private P<S2> predicate; diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AnyStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AnyStep.java index a518829ea9..ed0216934c 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AnyStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/AnyStep.java @@ -21,6 +21,7 @@ package org.apache.tinkerpop.gremlin.process.traversal.step.filter; import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.step.AcceptsChildPredicateTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; import org.apache.tinkerpop.gremlin.structure.util.StringFactory; @@ -32,7 +33,7 @@ import java.util.Iterator; import java.util.List; import java.util.Set; -public final class AnyStep<S, S2> extends FilterStep<S> implements TraversalParent { +public final class AnyStep<S, S2> extends FilterStep<S> implements TraversalParent, AcceptsChildPredicateTraversal { private P<S2> predicate; 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 fc39da51f3..1b56dbdfeb 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 @@ -21,29 +21,26 @@ package org.apache.tinkerpop.gremlin.process.traversal.step.filter; import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.step.AcceptsChildPredicateTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.Configuring; import org.apache.tinkerpop.gremlin.process.traversal.step.HasContainerHolder; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer; import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters; import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; -import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil; import org.apache.tinkerpop.gremlin.structure.Element; import org.apache.tinkerpop.gremlin.structure.Property; -import org.apache.tinkerpop.gremlin.structure.T; -import org.apache.tinkerpop.gremlin.structure.util.CloseableIterator; import org.apache.tinkerpop.gremlin.structure.util.StringFactory; import java.util.ArrayList; import java.util.Collections; -import java.util.Iterator; import java.util.List; import java.util.Set; /** * @author Marko A. Rodriguez (http://markorodriguez.com) */ -public class HasStep<S extends Element> extends FilterStep<S> implements HasContainerHolder<S, S>, Configuring, TraversalParent { +public class HasStep<S extends Element> extends FilterStep<S> implements HasContainerHolder<S, S>, Configuring, TraversalParent, AcceptsChildPredicateTraversal { private final Parameters parameters = new Parameters(); private List<HasContainer> hasContainers; @@ -54,7 +51,7 @@ public class HasStep<S extends Element> extends FilterStep<S> implements HasCont this.hasContainers = new ArrayList<>(); for (final HasContainer hc : hasContainers) { this.hasContainers.add(hc); - extractAndIntegrateTraversals(hc); + collectChildTraversals(hc); } } @@ -87,12 +84,13 @@ public class HasStep<S extends Element> extends FilterStep<S> implements HasCont } /** - * Tests all HasContainers against an Element, handling traversal-bearing containers by evaluating - * child traversals against the current traverser. + * Tests all HasContainers against an Element. Traversal-bearing containers carry a {@code P} predicate + * (e.g. {@code P.eq(traversal)}) whose child traversal is resolved against the current traverser before + * the standard {@link HasContainer#test(Element)} is applied. */ private boolean testAllWithTraversals(final Traverser.Admin<S> traverser, final Element element) { for (final HasContainer hc : this.hasContainers) { - if (!testHasContainer(traverser, hc, element)) { + if (!resolvePredicate(hc, traverser) || !hc.test(element)) { return false; } } @@ -100,151 +98,41 @@ 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. + * Tests all HasContainers against a Property. See {@link #testAllWithTraversals(Traverser.Admin, Element)}. */ - @SuppressWarnings("unchecked") private boolean testAllWithTraversals(final Traverser.Admin<S> traverser, final Property property) { for (final HasContainer hc : this.hasContainers) { - if (hc.hasTraversal()) { - if (hc.getTraversalValue() != null) { - 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; - if (!hc.test(property)) return false; - } else { - if (!hc.test(property)) return false; - } - } else { - if (!hc.test(property)) return false; + if (!resolvePredicate(hc, traverser) || !hc.test(property)) { + return false; } } return true; } /** - * Tests a single HasContainer against an Element, resolving traversals if present. + * Resolves a container's predicate traversal (if any) against the current traverser, mutating the + * predicate with the resolved value for this test cycle. + * + * @return {@code false} if the predicate resolved empty (no comparison value), meaning the element + * cannot match and the caller should short-circuit; {@code true} otherwise. */ - @SuppressWarnings("unchecked") - private boolean testHasContainer(final Traverser.Admin<S> traverser, final HasContainer hc, final Element element) { + private boolean resolvePredicate(final HasContainer hc, final Traverser.Admin<S> traverser) { if (hc.hasTraversal()) { - if (hc.getTraversalValue() != null) { - // 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 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); - if (hc.getPredicate().isResolvedEmpty()) return false; - return hc.test(element); - } - } - // No traversal: use existing test path - return hc.test(element); - } - - /** - * Gets the property value from an Element for the given key, handling T.id and T.label accessors. - */ - private Object getPropertyValue(final Element element, final String key) { - if (key != null) { - if (key.equals(T.id.getAccessor())) { - return element.id(); - } - if (key.equals(T.label.getAccessor())) { - return element.label(); - } - } - final Iterator<? extends Property> itty = element.properties(key); - try { - if (itty.hasNext()) { - return itty.next().value(); - } - } finally { - CloseableIterator.closeIterator(itty); - } - return null; - } - - /** - * Gets the property value from a Property for the given key, handling T.value and T.key accessors. - */ - private Object getPropertyValueFromProperty(final Property property, final String key) { - if (key != null) { - if (key.equals(T.value.getAccessor())) { - return property.value(); - } - if (key.equals(T.key.getAccessor())) { - return property.key(); - } - } - if (property instanceof Element) { - return getPropertyValue((Element) property, key); - } - return null; - } - - /** - * Collects all results from a child traversal evaluated against the current traverser. - * Uses the same prepare + iterate pattern as {@link TraversalUtil#applyAll(Traverser.Admin, Traversal.Admin)} - * to avoid ambiguous overload resolution when S extends Element. - */ - @SuppressWarnings("unchecked") - private List<Object> collectTraversalResults(final Traverser.Admin<S> traverser, - final Traversal.Admin<?, ?> childTraversal) { - final List<Object> results = new ArrayList<>(); - TraversalUtil.prepare(traverser, (Traversal.Admin) childTraversal); - while (childTraversal.hasNext()) { - results.add(childTraversal.next()); + hc.getPredicate().resolve(traverser); + return !hc.getPredicate().isResolvedEmpty(); } - return results; + return true; } /** - * Extracts child traversals from a HasContainer and integrates them as children of this step. - * Handles direct traversal values on the container as well as traversals embedded within - * predicates (including ConnectiveP which may have traversals on child predicates). + * Collects the child traversals embedded within a HasContainer's predicate (including + * {@link org.apache.tinkerpop.gremlin.process.traversal.util.ConnectiveP}, which may carry traversals on + * child predicates) into {@link #childTraversals}. Parenting is deferred to + * {@link #setTraversal(Traversal.Admin)} which is the single integration point. */ - private void extractAndIntegrateTraversals(final HasContainer hc) { - if (hc.getTraversalValue() != null) { - this.childTraversals.add(hc.getTraversalValue()); - this.integrateChild(hc.getTraversalValue()); - } + private void collectChildTraversals(final HasContainer hc) { if (hc.getPredicate() != null && hc.getPredicate().hasTraversal()) { - final List<Traversal.Admin<?, ?>> predicateTraversals = new ArrayList<>(); - P.collectTraversals(hc.getPredicate(), predicateTraversals); - for (final Traversal.Admin<?, ?> t : predicateTraversals) { - this.childTraversals.add(t); - this.integrateChild(t); - } + P.collectTraversals(hc.getPredicate(), this.childTraversals); } } @@ -266,7 +154,13 @@ public class HasStep<S extends Element> extends FilterStep<S> implements HasCont @Override public void addHasContainer(final HasContainer hasContainer) { this.hasContainers.add(hasContainer); - extractAndIntegrateTraversals(hasContainer); + // A container may be added after this step has already been parented (e.g. by a strategy), so collect + // its child traversals and integrate just those new ones immediately. + final int before = this.childTraversals.size(); + collectChildTraversals(hasContainer); + for (int i = before; i < this.childTraversals.size(); i++) { + this.integrateChild(this.childTraversals.get(i)); + } } @SuppressWarnings("unchecked") @@ -288,9 +182,6 @@ public class HasStep<S extends Element> extends FilterStep<S> implements HasCont for (final HasContainer hasContainer : this.hasContainers) { final HasContainer clonedHc = hasContainer.clone(); clone.hasContainers.add(clonedHc); - if (clonedHc.getTraversalValue() != null) { - clone.childTraversals.add(clonedHc.getTraversalValue()); - } if (clonedHc.getPredicate() != null && clonedHc.getPredicate().hasTraversal()) { P.collectTraversals(clonedHc.getPredicate(), clone.childTraversals); } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/IsStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/IsStep.java index 04d492733e..e49c7e3537 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/IsStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/IsStep.java @@ -21,6 +21,7 @@ package org.apache.tinkerpop.gremlin.process.traversal.step.filter; import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.step.AcceptsChildPredicateTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; import org.apache.tinkerpop.gremlin.structure.util.StringFactory; @@ -34,7 +35,7 @@ import java.util.Set; * @author Daniel Kuppitz (http://gremlin.guru) * @author Marko A. Rodriguez (http://markorodriguez.com) */ -public final class IsStep<S> extends FilterStep<S> implements IsStepContract<S>, TraversalParent { +public final class IsStep<S> extends FilterStep<S> implements IsStepContract<S>, TraversalParent, AcceptsChildPredicateTraversal { private P<S> predicate; diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/NoneStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/NoneStep.java index b303d30c6e..6344e3a2b8 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/NoneStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/NoneStep.java @@ -21,6 +21,7 @@ package org.apache.tinkerpop.gremlin.process.traversal.step.filter; import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.step.AcceptsChildPredicateTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; import org.apache.tinkerpop.gremlin.structure.util.StringFactory; @@ -32,7 +33,7 @@ import java.util.Iterator; import java.util.List; import java.util.Set; -public final class NoneStep<S, S2> extends FilterStep<S> implements TraversalParent { +public final class NoneStep<S, S2> extends FilterStep<S> implements TraversalParent, AcceptsChildPredicateTraversal { private P<S2> predicate; diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java index a3bd10ea48..6a2a3a5770 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java @@ -22,6 +22,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Pop; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; +import org.apache.tinkerpop.gremlin.process.traversal.step.AcceptsChildPredicateTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.ByModulating; import org.apache.tinkerpop.gremlin.process.traversal.step.PathProcessor; import org.apache.tinkerpop.gremlin.process.traversal.step.Scoping; @@ -45,7 +46,7 @@ import java.util.Set; /** * @author Marko A. Rodriguez (http://markorodriguez.com) */ -public final class WherePredicateStep<S> extends FilterStep<S> implements Scoping, PathProcessor, ByModulating, TraversalParent { +public final class WherePredicateStep<S> extends FilterStep<S> implements Scoping, PathProcessor, ByModulating, TraversalParent, AcceptsChildPredicateTraversal { protected String startKey; protected List<String> selectKeys; diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java index 043c99c99f..c000c7c164 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java @@ -24,6 +24,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.Contains; 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.AcceptsChildPredicateTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.GValue; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep; @@ -52,7 +53,7 @@ import java.util.function.Supplier; * @author Marko A. Rodriguez (http://markorodriguez.com) * @author Pieter Martin */ -public class GraphStep<S, E extends Element> extends AbstractStep<S, E> implements GraphStepContract<S, E>, TraversalParent { +public class GraphStep<S, E extends Element> extends AbstractStep<S, E> implements GraphStepContract<S, E>, TraversalParent, AcceptsChildPredicateTraversal { protected Parameters parameters = new Parameters(); protected final Class<E> returnClass; diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java index 0c7f30c919..7b5acbf44a 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java @@ -49,15 +49,34 @@ import java.util.Set; */ public class AddPropertyStep<S extends Element> extends SideEffectStep<S> implements AddPropertyStepContract<S> { + /** + * Internal placeholder key used for the single-argument {@code property(traversal)} (Map-producing) form, + * where the property keys are supplied by the traversal's resulting Map rather than by a caller-provided key. + * This value is never used for dispatch (see {@link #mapForm}); it exists only to give the parameter a + * stable, non-null key within {@link org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters}. + */ + static final String MAP_VALUE_TRAVERSAL_KEY = "~traversalMap"; + private Parameters internalParameters = new Parameters(); private Parameters withConfiguration = new Parameters(); private final VertexProperty.Cardinality cardinality; + /** + * Marks the single-argument {@code property(traversal)} form whose child traversal must produce a Map of + * property key/value pairs. Dispatch is driven by this flag rather than by inspecting the property key, so a + * caller-supplied key that happens to collide with {@link #MAP_VALUE_TRAVERSAL_KEY} is handled normally. + */ + private final boolean mapForm; private CallbackRegistry<Event.ElementPropertyChangedEvent> callbackRegistry; public AddPropertyStep(final Traversal.Admin traversal, final VertexProperty.Cardinality cardinality, final Object keyObject, final Object valueObject) { + this(traversal, cardinality, keyObject, valueObject, false); + } + + public AddPropertyStep(final Traversal.Admin traversal, final VertexProperty.Cardinality cardinality, final Object keyObject, final Object valueObject, final boolean mapForm) { super(traversal); this.internalParameters.set(this, T.key, keyObject, T.value, valueObject); this.cardinality = cardinality; + this.mapForm = mapForm; } @Override @@ -134,23 +153,24 @@ public class AddPropertyStep<S extends Element> extends SideEffectStep<S> implem final Element element = traverser.get(); final Object[] vertexPropertyKeyValues = this.internalParameters.getKeyValues(traverser, T.key, T.value); - // Map-producing form: property(traversal) where traversal produces a Map - if (results.size() == 1 && results.get(0) instanceof Map) { - final Map<?, ?> map = (Map<?, ?>) results.get(0); - for (final Map.Entry<?, ?> entry : map.entrySet()) { - if (!(entry.getKey() instanceof String)) { - throw new IllegalArgumentException( - "Property key must be a String but found " + entry.getKey().getClass().getSimpleName()); + // Map-producing form: single-argument property(traversal) where the traversal produces a Map. + // Only the dedicated map form expands a Map into per-entry properties; for the key+traversal form a + // Map result is treated as an ordinary property value. + if (this.mapForm) { + if (results.size() == 1 && results.get(0) instanceof Map) { + final Map<?, ?> map = (Map<?, ?>) results.get(0); + for (final Map.Entry<?, ?> entry : map.entrySet()) { + if (!(entry.getKey() instanceof String)) { + throw new IllegalArgumentException( + "Property key must be a String but found " + entry.getKey().getClass().getSimpleName()); + } + final String mapKey = (String) entry.getKey(); + applyPropertyMutation(traverser, mapKey, entry.getValue(), vertexPropertyKeyValues); } - final String mapKey = (String) entry.getKey(); - applyPropertyMutation(traverser, mapKey, entry.getValue(), vertexPropertyKeyValues); + return; } - return; - } - // Runtime validation for property(traversal) — the Map-producing form uses sentinel key "~traversalMap". - // If the traversal did NOT produce a Map, reject it to prevent the sentinel key from leaking as a real property. - if ("~traversalMap".equals(key)) { + // The map form requires a Map result. Reject anything else rather than guessing. final Object result = results.get(0); throw new IllegalArgumentException( "property(traversal) requires the traversal to produce a Map, but got: " + @@ -282,7 +302,7 @@ public class AddPropertyStep<S extends Element> extends SideEffectStep<S> implem @Override public int hashCode() { - final int hash = super.hashCode() ^ this.internalParameters.hashCode() ^ this.withConfiguration.hashCode(); + int hash = super.hashCode() ^ this.internalParameters.hashCode() ^ this.withConfiguration.hashCode() ^ Boolean.hashCode(this.mapForm); return (null != this.cardinality) ? (hash ^ cardinality.hashCode()) : hash; } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepContract.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepContract.java index c6547ad425..70f430db4a 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepContract.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepContract.java @@ -19,6 +19,7 @@ package org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect; import org.apache.tinkerpop.gremlin.process.traversal.Step; +import org.apache.tinkerpop.gremlin.process.traversal.step.AcceptsChildPredicateTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.Configuring; import org.apache.tinkerpop.gremlin.process.traversal.step.Deleting; import org.apache.tinkerpop.gremlin.process.traversal.step.GValue; @@ -33,7 +34,8 @@ import java.util.HashSet; import java.util.List; public interface AddPropertyStepContract<S> extends Step<S, S>, TraversalParent, Scoping, PropertiesHolder, - Writing<Event.ElementPropertyChangedEvent>, Deleting<Event.ElementPropertyChangedEvent>, Configuring { + Writing<Event.ElementPropertyChangedEvent>, Deleting<Event.ElementPropertyChangedEvent>, Configuring, + AcceptsChildPredicateTraversal { /** * Concrete implementations of this contract that can be referenced as TinkerPop implementations. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepPlaceholder.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepPlaceholder.java index 3bb1c087cf..3dd2734cf3 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepPlaceholder.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepPlaceholder.java @@ -62,8 +62,18 @@ public class AddPropertyStepPlaceholder<S extends Element> extends SideEffectSte private Parameters withConfiguration = new Parameters(); + /** + * Marks the single-argument {@code property(traversal)} (Map-producing) form. + */ + private boolean mapForm; + public AddPropertyStepPlaceholder(final Traversal.Admin traversal, final VertexProperty.Cardinality cardinality, final Object keyObject, final Object valueObject) { + this(traversal, cardinality, keyObject, valueObject, false); + } + + public AddPropertyStepPlaceholder(final Traversal.Admin traversal, final VertexProperty.Cardinality cardinality, final Object keyObject, final Object valueObject, final boolean mapForm) { super(traversal); + this.mapForm = mapForm; if (keyObject instanceof GValue) { throw new IllegalArgumentException("GValue is not allowed for property keys"); } @@ -128,13 +138,14 @@ public class AddPropertyStepPlaceholder<S extends Element> extends SideEffectSte return Objects.equals(key, that.key) && Objects.equals(value, that.value) && cardinality == that.cardinality && + mapForm == that.mapForm && Objects.equals(properties, that.properties) && Objects.equals(withConfiguration, that.withConfiguration); } @Override public int hashCode() { - return Objects.hash(super.hashCode(), key, value, cardinality, properties, withConfiguration); + return Objects.hash(super.hashCode(), key, value, cardinality, mapForm, properties, withConfiguration); } @Override @@ -176,6 +187,7 @@ public class AddPropertyStepPlaceholder<S extends Element> extends SideEffectSte public AddPropertyStepPlaceholder<S> clone() { final AddPropertyStepPlaceholder<S> clone = (AddPropertyStepPlaceholder<S>) super.clone(); clone.cardinality = cardinality; + clone.mapForm = mapForm; // Attempt to deep clone key for Traversal and GValue. Shallow copy is fine if key is a String or enum if (this.key instanceof Traversal) { @@ -218,7 +230,7 @@ public class AddPropertyStepPlaceholder<S extends Element> extends SideEffectSte @Override public AddPropertyStep<S> asConcreteStep() { - AddPropertyStep<S> step = new AddPropertyStep<>(traversal, cardinality, key, value instanceof GValue ? ((GValue<?>) value).get() : value); + AddPropertyStep<S> step = new AddPropertyStep<>(traversal, cardinality, key, value instanceof GValue ? ((GValue<?>) value).get() : value, mapForm); for (final Map.Entry<Object, List<Object>> entry : properties.entrySet()) { for (Object value : entry.getValue()) { diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java index df3b2f1b13..c6d0d1fe29 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainer.java @@ -19,7 +19,6 @@ package org.apache.tinkerpop.gremlin.process.traversal.step.util; import org.apache.tinkerpop.gremlin.process.traversal.P; -import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.structure.Element; import org.apache.tinkerpop.gremlin.structure.Property; import org.apache.tinkerpop.gremlin.structure.T; @@ -40,7 +39,6 @@ public class HasContainer implements Serializable, Cloneable, Predicate<Element> private String key; private P predicate; - private Traversal.Admin<?, ?> traversalValue; private final boolean testingIdString; @@ -50,18 +48,6 @@ public class HasContainer implements Serializable, Cloneable, Predicate<Element> this.testingIdString = isStringTestable(); } - /** - * Constructs a {@code HasContainer} with a child traversal whose result is resolved at runtime - * against the current traverser. The predicate is set to {@code null} and will be constructed - * dynamically during evaluation (e.g., as {@code P.within(results)}). - */ - public HasContainer(final String key, final Traversal.Admin<?, ?> traversalValue) { - this.key = key; - this.predicate = null; - this.traversalValue = traversalValue; - this.testingIdString = false; - } - public final boolean test(final Element element) { // it is OK to evaluate equality of ids via toString(), given that the test suite enforces the value of // id().toString() to be a first class representation of the identifier. a string test is only executed @@ -118,9 +104,6 @@ public class HasContainer implements Serializable, Cloneable, Predicate<Element> } public final String toString() { - if (this.traversalValue != null) { - return Objects.toString(this.key) + ".traversal(" + this.traversalValue + ")"; - } return Objects.toString(this.key) + '.' + this.predicate; } @@ -128,9 +111,6 @@ public class HasContainer implements Serializable, Cloneable, Predicate<Element> try { final HasContainer clone = (HasContainer) super.clone(); clone.predicate = this.predicate != null ? this.predicate.clone() : null; - if (this.traversalValue != null) { - clone.traversalValue = this.traversalValue.clone(); - } return clone; } catch (final CloneNotSupportedException e) { throw new IllegalStateException(e.getMessage(), e); @@ -155,31 +135,19 @@ public class HasContainer implements Serializable, Cloneable, Predicate<Element> } public final BiPredicate<?, ?> getBiPredicate() { - if (this.predicate == null) - throw new IllegalStateException("Cannot access BiPredicate on a traversal-bearing HasContainer; check hasTraversal() first"); return this.predicate.getBiPredicate(); } public final Object getValue() { - if (this.predicate == null) - throw new IllegalStateException("Cannot access value on a traversal-bearing HasContainer; check hasTraversal() first"); return this.predicate.getValue(); } /** - * Determines if this {@code HasContainer} holds a child traversal, either directly - * via the {@code traversalValue} field or embedded within the predicate. + * Determines if this {@code HasContainer}'s predicate holds a child traversal whose result is resolved + * at runtime (e.g. {@code P.eq(traversal)} or {@code P.within(traversal)}). */ public boolean hasTraversal() { - return this.traversalValue != null || (this.predicate != null && this.predicate.hasTraversal()); - } - - /** - * Gets the child traversal value, if one was provided. Returns {@code null} when this - * {@code HasContainer} uses a literal predicate. - */ - public Traversal.Admin<?, ?> getTraversalValue() { - return this.traversalValue; + return this.predicate != null && this.predicate.hasTraversal(); } //////////// diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/ChildTraversalVerificationStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/ChildTraversalVerificationStrategy.java index 730de40913..240cc08a85 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/ChildTraversalVerificationStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/verification/ChildTraversalVerificationStrategy.java @@ -21,14 +21,8 @@ package org.apache.tinkerpop.gremlin.process.traversal.strategy.verification; import org.apache.tinkerpop.gremlin.process.traversal.Step; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.step.AcceptsChildPredicateTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; -import org.apache.tinkerpop.gremlin.process.traversal.step.filter.AllStep; -import org.apache.tinkerpop.gremlin.process.traversal.step.filter.AnyStep; -import org.apache.tinkerpop.gremlin.process.traversal.step.filter.HasStep; -import org.apache.tinkerpop.gremlin.process.traversal.step.filter.IsStep; -import org.apache.tinkerpop.gremlin.process.traversal.step.filter.NoneStep; -import org.apache.tinkerpop.gremlin.process.traversal.step.map.GraphStep; -import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.AddPropertyStepContract; import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.util.ChildTraversalValidator; @@ -49,7 +43,7 @@ public final class ChildTraversalVerificationStrategy @Override public void apply(final Traversal.Admin<?, ?> traversal) { for (final Step<?, ?> step : traversal.getSteps()) { - if (step instanceof TraversalParent && hasChildTraversalsToValidate(step)) { + if (step instanceof AcceptsChildPredicateTraversal && step instanceof TraversalParent) { for (final Traversal.Admin<?, ?> child : ((TraversalParent) step).getLocalChildren()) { try { ChildTraversalValidator.validate(child); @@ -61,13 +55,6 @@ public final class ChildTraversalVerificationStrategy } } - private boolean hasChildTraversalsToValidate(final Step<?, ?> step) { - return step instanceof HasStep || step instanceof IsStep || - step instanceof AllStep || step instanceof AnyStep || step instanceof NoneStep || - (step instanceof GraphStep && ((GraphStep<?, ?>) step).getIdTraversal() != null) || - step instanceof AddPropertyStepContract; - } - public static ChildTraversalVerificationStrategy instance() { return INSTANCE; } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/AndP.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/AndP.java index c6b860fd11..1275e08f04 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/AndP.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/AndP.java @@ -20,6 +20,7 @@ package org.apache.tinkerpop.gremlin.process.traversal.util; import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.PBiPredicate; +import org.apache.tinkerpop.gremlin.process.traversal.Traverser; import org.apache.tinkerpop.gremlin.structure.util.StringFactory; import java.io.Serializable; @@ -56,6 +57,25 @@ public final class AndP<V> extends ConnectiveP<V> { return new OrP<>(this.predicates); } + /** + * Resolves child predicates with short-circuiting. Because a conjunction fails as soon as any child + * predicate cannot be satisfied, resolution stops at the first child that resolves empty (i.e. a scalar + * predicate whose child traversal produced no comparison value). This avoids evaluating the remaining + * child traversals, which may be expensive. Collection predicates (within/without) never resolve empty + * (they resolve to an empty collection), so they do not trigger the short-circuit. + */ + @Override + public void resolve(final Traverser.Admin<?> traverser) { + // No super.resolve(): a connective predicate carries no child traversal of its own; only its + // operands do. Resolving each operand directly is sufficient (and enables the short-circuit below). + for (final P<V> p : this.predicates) { + if (p.hasTraversal()) { + p.resolve(traverser); + if (p.isResolvedEmpty()) return; + } + } + } + @Override public boolean isResolvedEmpty() { // AND short-circuits: if any child resolved empty, the conjunction cannot be satisfied 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 010965c3f9..09e36b09c9 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 @@ -331,10 +331,12 @@ public class PTraversalTest { @SuppressWarnings("unchecked") @Test public void shouldHandleAllEmptyResults() { - // within(__.limit(0), __.limit(0)) where both produce nothing + // within(__.limit(0), __.limit(0)) where both produce nothing. A collection predicate resolves to + // an empty set rather than flagging resolved-empty, so within() simply matches nothing. final P<Object> p = P.within(__.limit(0).asAdmin(), __.limit(0).asAdmin()); p.resolve(createTraverser("start")); - assertThat(p.isResolvedEmpty(), is(true)); + assertThat(p.isResolvedEmpty(), is(false)); + assertThat(p.test(1), is(false)); } @SuppressWarnings("unchecked") @@ -397,4 +399,137 @@ public class PTraversalTest { assertThat(collected.size(), is(3)); } } + + /** + * Covers the empty-result semantics of collection (within/without) versus scalar predicates and the + * behavior of connective predicates (AndP/OrP) and NotP when their operands carry child traversals. + * <p> + * Regression coverage for: within(empty) -> false, without(empty) -> true (collection predicates resolve + * to an empty collection rather than short-circuiting), scalar predicates remain resolved-empty, and the + * shared-state safety of resolving the same predicate across many traversers sequentially. + */ + public static class EmptyResolutionAndConnectiveTest { + + private Traverser.Admin<?> createTraverser(final Object value) { + return new B_O_Traverser<>(value, 1L); + } + + @SuppressWarnings("unchecked") + @Test + public void shouldPassWithoutWhenTraversalResolvesEmpty() { + // without(empty set) -> nothing to exclude -> everything passes + final P<Object> p = P.without(__.<Object>limit(0).asAdmin()); + p.resolve(createTraverser("anything")); + assertThat(p.isResolvedEmpty(), is(false)); + assertThat(p.test("anything"), is(true)); + assertThat(p.test(42), is(true)); + } + + @SuppressWarnings("unchecked") + @Test + public void shouldFailWithinWhenTraversalResolvesEmpty() { + // within(empty set) -> nothing matches -> everything fails + final P<Object> p = P.within(__.<Object>limit(0).asAdmin()); + p.resolve(createTraverser("anything")); + assertThat(p.isResolvedEmpty(), is(false)); + assertThat(p.test("anything"), is(false)); + assertThat(p.test(42), is(false)); + } + + @SuppressWarnings("unchecked") + @Test + public void shouldPassWithoutWhenFoldedTraversalIsEmpty() { + // explicit empty collection via fold() of nothing + final P<Object> p = P.without(__.<Object>limit(0).fold().asAdmin()); + p.resolve(createTraverser("anything")); + assertThat(p.test("anything"), is(true)); + } + + @SuppressWarnings("unchecked") + @Test + public void shouldFailWithinWhenMultiTraversalResolvesAllEmpty() { + final P<Object> p = P.within(__.<Object>limit(0).asAdmin(), __.<Object>limit(0).asAdmin()); + p.resolve(createTraverser("anything")); + assertThat(p.isResolvedEmpty(), is(false)); + assertThat(p.test(1), is(false)); + } + + @SuppressWarnings("unchecked") + @Test + public void shouldRemainResolvedEmptyForScalarPredicateWithEmptyTraversal() { + // eq(empty) has no comparison value -> resolved empty so the step can short-circuit + final P<Object> p = P.eq(__.<Object>limit(0).asAdmin()); + p.resolve(createTraverser("anything")); + assertThat(p.isResolvedEmpty(), is(true)); + } + + @SuppressWarnings("unchecked") + @Test + public void shouldResolveAndPWithTraversalOperands() { + final P<Object> p = (P<Object>) (P) P.gt(__.constant(10).asAdmin()).and(P.lt(__.constant(20).asAdmin())); + p.resolve(createTraverser("start")); + assertThat(p.test(15), is(true)); + assertThat(p.test(10), is(false)); + assertThat(p.test(25), is(false)); + } + + @SuppressWarnings("unchecked") + @Test + public void shouldShortCircuitAndPResolveWhenScalarChildEmpty() { + // first child resolves empty (no comparison value); AndP cannot be satisfied + final P<Object> p = (P<Object>) (P) P.eq(__.<Object>limit(0).asAdmin()).and(P.gt(__.constant(5).asAdmin())); + p.resolve(createTraverser("start")); + assertThat(p.isResolvedEmpty(), is(true)); + } + + @SuppressWarnings("unchecked") + @Test + public void shouldResolveOrPWithTraversalOperands() { + final P<Object> p = (P<Object>) (P) P.eq(__.constant(1).asAdmin()).or(P.eq(__.constant(2).asAdmin())); + p.resolve(createTraverser("start")); + assertThat(p.test(1), is(true)); + assertThat(p.test(2), is(true)); + assertThat(p.test(3), is(false)); + } + + @SuppressWarnings("unchecked") + @Test + public void shouldReturnFalseForOrPWhenResolvedNonEmptyButNonMatching() { + // a non-empty within() resolution that simply does not contain the test value must still return false + final P<Object> p = (P<Object>) (P) P.within(__.inject(1, 2, 3).fold().asAdmin()) + .or(P.within(__.inject(4, 5, 6).fold().asAdmin())); + p.resolve(createTraverser("start")); + assertThat(p.test(2), is(true)); + assertThat(p.test(5), is(true)); + assertThat(p.test(9), is(false)); + } + + @SuppressWarnings("unchecked") + @Test + public void shouldResolveNotPWrappingTraversalPredicate() { + final P<Object> p = P.eq(__.constant(42).asAdmin()).negate(); + p.resolve(createTraverser("start")); + assertThat(p.test(42), is(false)); + assertThat(p.test(99), is(true)); + } + + @Test + public void shouldExposeWrappedPredicateFromNotP() { + final P<Object> inner = P.eq(__.constant(42).asAdmin()); + final NotP<Object> notP = new NotP<>(inner); + assertThat(notP.getWrapped() == inner, is(true)); + } + + @SuppressWarnings("unchecked") + @Test + public void shouldProduceConsistentResultsAcrossManySequentialResolves() { + // resolve() mutates the predicate per traverser; verify repeated resolve+test cycles are stable + final P<Object> p = P.gt(__.constant(10).asAdmin()); + for (int i = 0; i < 1000; i++) { + p.resolve(createTraverser("start" + i)); + assertThat(p.test(11), is(true)); + assertThat(p.test(5), is(false)); + } + } + } } diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/CloneIndependenceTraversalTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/CloneIndependenceTraversalTest.java index 237a5d7884..09af6da179 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/CloneIndependenceTraversalTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/CloneIndependenceTraversalTest.java @@ -18,6 +18,7 @@ */ package org.apache.tinkerpop.gremlin.process.traversal.step; +import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.HasStep; @@ -48,9 +49,9 @@ public class CloneIndependenceTraversalTest { @Test public void shouldProduceIndependentCloneForHasStepWithTraversalValue() { - // Create a HasStep with a traversal-bearing HasContainer + // Create a HasStep with a traversal-bearing HasContainer (P.eq(traversal)) final Traversal.Admin<?, ?> childTraversal = __.constant("marko").asAdmin(); - final HasContainer hc = new HasContainer("name", childTraversal); + final HasContainer hc = new HasContainer("name", P.eq(childTraversal)); final HasStep<Vertex> original = new HasStep<>(EmptyTraversal.instance(), hc); // Clone the step @@ -70,8 +71,8 @@ public class CloneIndependenceTraversalTest { final List<HasContainer> originalContainers = original.getHasContainers(); final List<HasContainer> cloneContainers = clone.getHasContainers(); assertThat(cloneContainers.get(0), is(not(sameInstance(originalContainers.get(0))))); - assertThat(cloneContainers.get(0).getTraversalValue(), - is(not(sameInstance(originalContainers.get(0).getTraversalValue())))); + assertThat(cloneContainers.get(0).getPredicate().getTraversalValue(), + is(not(sameInstance(originalContainers.get(0).getPredicate().getTraversalValue())))); } @Test @@ -95,8 +96,8 @@ public class CloneIndependenceTraversalTest { @Test public void shouldProduceIndependentCloneForHasStepWithMultipleContainers() { // Create a HasStep with multiple HasContainers, some with traversals - final HasContainer hc1 = new HasContainer("name", __.constant("marko").asAdmin()); - final HasContainer hc2 = new HasContainer("age", __.constant(29).asAdmin()); + final HasContainer hc1 = new HasContainer("name", P.eq(__.constant("marko").asAdmin())); + final HasContainer hc2 = new HasContainer("age", P.eq(__.constant(29).asAdmin())); final HasStep<Vertex> original = new HasStep<>(EmptyTraversal.instance(), hc1, hc2); // Clone the step @@ -111,8 +112,8 @@ public class CloneIndependenceTraversalTest { for (int i = 0; i < originalContainers.size(); i++) { assertThat(cloneContainers.get(i), is(not(sameInstance(originalContainers.get(i))))); - assertThat(cloneContainers.get(i).getTraversalValue(), - is(not(sameInstance(originalContainers.get(i).getTraversalValue())))); + assertThat(cloneContainers.get(i).getPredicate().getTraversalValue(), + is(not(sameInstance(originalContainers.get(i).getPredicate().getTraversalValue())))); } } diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainerTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainerTest.java index 2164b7feba..4049b20eb6 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainerTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/HasContainerTest.java @@ -25,14 +25,15 @@ import org.junit.Test; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.notNullValue; -import static org.hamcrest.CoreMatchers.nullValue; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.core.IsNot.not; import static org.hamcrest.core.IsSame.sameInstance; import static org.junit.Assert.assertEquals; /** - * Tests for {@link HasContainer}, including traversal-bearing constructor and lifecycle methods. + * Tests for {@link HasContainer}, including traversal-bearing predicates and lifecycle methods. + * A traversal-bearing container holds a {@link P} whose value is resolved at runtime from a child traversal + * (e.g. {@code P.eq(traversal)}); the container exposes this via {@link HasContainer#hasTraversal()}. */ public class HasContainerTest { @@ -43,32 +44,32 @@ public class HasContainerTest { } @Test - public void shouldReturnTrueForHasTraversalWithTraversalValue() { + public void shouldReturnTrueForHasTraversalWithTraversalPredicate() { final Traversal.Admin<?, ?> traversal = __.identity().asAdmin(); - final HasContainer hc = new HasContainer("name", traversal); + final HasContainer hc = new HasContainer("name", P.eq(traversal)); assertThat(hc.hasTraversal(), is(true)); } @Test - public void shouldSetFieldsCorrectlyWithTraversalConstructor() { + public void shouldSetFieldsCorrectlyWithTraversalPredicate() { final Traversal.Admin<?, ?> traversal = __.identity().asAdmin(); - final HasContainer hc = new HasContainer("age", traversal); + final HasContainer hc = new HasContainer("age", P.eq(traversal)); assertEquals("age", hc.getKey()); - assertThat(hc.getPredicate(), is(nullValue())); - assertThat(hc.getTraversalValue(), is(notNullValue())); - assertThat(hc.getTraversalValue(), is(sameInstance(traversal))); + assertThat(hc.getPredicate(), is(notNullValue())); + assertThat(hc.getPredicate().getTraversalValue(), is(sameInstance(traversal))); } @Test - public void shouldCloneProduceIndependentDeepCopyOfTraversalValue() { + public void shouldCloneProduceIndependentDeepCopyOfTraversalPredicate() { final Traversal.Admin<?, ?> traversal = __.identity().asAdmin(); - final HasContainer original = new HasContainer("name", traversal); + final HasContainer original = new HasContainer("name", P.eq(traversal)); final HasContainer clone = original.clone(); - // clone should have a traversalValue that is not the same instance - assertThat(clone.getTraversalValue(), is(notNullValue())); - assertThat(clone.getTraversalValue(), is(not(sameInstance(original.getTraversalValue())))); + // clone's predicate should carry a traversal that is not the same instance + assertThat(clone.getPredicate().getTraversalValue(), is(notNullValue())); + assertThat(clone.getPredicate().getTraversalValue(), + is(not(sameInstance(original.getPredicate().getTraversalValue())))); // key should be equal assertEquals(original.getKey(), clone.getKey()); @@ -83,14 +84,7 @@ public class HasContainerTest { final HasContainer clone = original.clone(); assertThat(clone.hasTraversal(), is(false)); - assertThat(clone.getTraversalValue(), is(nullValue())); assertEquals("name", clone.getKey()); assertEquals(P.eq("marko"), clone.getPredicate()); } - - @Test - public void shouldReturnNullTraversalValueForLiteralConstructor() { - final HasContainer hc = new HasContainer("name", P.eq("marko")); - assertThat(hc.getTraversalValue(), is(nullValue())); - } }
