xiazcy opened a new pull request, #3458: URL: https://github.com/apache/tinkerpop/pull/3458
## Summary Allow child traversals as arguments to `has()`, `is()`, `V()`, `E()`, `property()`, `where(P)`, and all `P`/`TextP` predicates. The child traversal is evaluated per-traverser at runtime, enabling dynamic value resolution. **JIRA:** - [TINKERPOP-2586](https://issues.apache.org/jira/browse/TINKERPOP-2586): Traversal objects inside `within()` silently return empty instead of executing or erroring — **fully addressed** (traversals are now executed) - [TINKERPOP-2777](https://issues.apache.org/jira/browse/TINKERPOP-2777): `V()` cannot accept traversals like `select()` for data-driven lookups — **fully addressed** - [TINKERPOP-3005](https://issues.apache.org/jira/browse/TINKERPOP-3005): `property()` should accept a traversal producing a Map — **fully addressed** - [TINKERPOP-1463](https://issues.apache.org/jira/browse/TINKERPOP-1463): `has(key, traversal)` doesn't work as expected — **partially addressed** (dynamic filtering works; runtime folding into index lookups is deferred as a future optimization) ### What it enables ```groovy // Data-driven vertex lookup (TINKERPOP-2777) g.inject(['1','2','3']).as('a').V(__.select('a')) // Dynamic filtering g.V().has("age", P.gt(__.V(vid).values("age"))) // Multi-source dynamic filtering g.V().has("name", P.within(__.V(vid1).values("name"), __.V(vid2).values("name"))) // Computed property mutation (TINKERPOP-3005) g.V(vid).property(__.V(other).project("friendCount","software").by(__.out("knows").count()).by(__.out("created").values("name"))) ``` --- ## Review Guide This is a large change (68 files, ~7k lines) but structured in layers. **Review bottom-up:** | # | Commit | Layer | What to look for | |---|--------|-------|-----------------| | 1 | `5f37d11` | Predicate resolution | Core: `P.resolve()`, empty-result semantics, `AndP` short-circuit, `ConnectiveP`/`NotP` delegation | | 2 | `6a547d2` | Step implementations | How steps use `P.resolve()`. `HasStep` single path, `GraphStep` idTraversal, `AddPropertyStep` mapForm, `AcceptsChildPredicateTraversal` marker | | 3 | `e80423a` | Grammar | `Gremlin.g4` `nestedTraversal` alternatives. Visitors delegate to API (not `addStep()`) | | 4 | `1b21d15` | GLVs | `within()`/`without()` serialization. .NET typed overloads | | 5 | `793e7a8` | Tests + docs | Cucumber `.feature` files define behavioral contract. Reference doc updates | | 6 | `400f5ae` | Cleanup | Dead code removal from HasContainer unification. Test quality fixes | ### Architecture (single resolution path) ``` has("age", __.V(1).values("age")) │ DSL wraps: has("age", P.eq(traversal)) │ ▼ HasContainer(key, P.eq(traversal)) │ ┌───────────────┴───────────────┐ │ HasStep.resolvePredicate() │ │ P.resolve(traverser) │ │ hc.test(element) │ └────────────────────────────────┘ ``` ### Key decisions to scrutinize 1. **Single resolution path** - At the DSL level, `has(key, traversal)` wraps the traversal in `P.eq(traversal)` before creating the `HasContainer`. This means HasStep only has one code path for traversal resolution: call `P.resolve(traverser)` on the predicate, then `hc.test(element)`. The GremlinLang serialization still records the original `has(key, traversal)` form (verified by round-trip tests). This is a new 4.0.0 feature, so no existing provider code is affected. Providers implementing custom strategies must check `HasContainer.hasTraversal()` before folding into index lookups. 2. **Empty-result semantics** - When a child traversal produces zero results, behavior depends on predicate type. Collection predicates (`within`/`without`) resolve to an empty collection and delegate to `Contains.test()` which applies correct set-theoretic logic: `within([])` is always false, `without([])` is always true. Scalar predicates (`eq`/`gt`/`lt`/etc.) set `resolvedEmpty=true` and steps short-circuit to false since there is no meaningful comparison value. A null result (traversal produces `null`) is distinct from an empty result: it resolves normally and `null` is used as the comparison value, consistent with existing `P.eq(null)` semantics. 3. **AndP short-circuits, OrP doesn't** - `AndP.resolve()` overrides `ConnectiveP.resolve()` to stop resolving children at the first scalar predicate that resolves empty. This is sound because `and(unsatisfiable, X)` is always false regardless of X. `OrP` does NOT short-circuit: a non-empty resolution does not guarantee the predicate will pass `test()`, so later children must already be resolved when their turn comes. Note: short-circuiting means side-effecting child traversals (e.g., ones using `aggregate`) in later AndP operands may not execute. This matches standard boolean short-circuit semantics. 4. **`P.resolve()` mutates in place** - `resolve()` overwrites `literals`, `isCollection`, and `resolvedEmpty` fields on the `P` instance. In OLTP this is safe since resolve and test happen sequentially per traverser. In OLAP, `HasStep.clone()` deep-clones predicates per worker, providing thread isolation. The mutation pattern is a known limitation documented for future improvement (return an immutable resolved value instead). Predicate trees of arbitrary depth (AndP containing OrP containing NotP, etc.) are handled by recursive resolution and recursive cloning. 5. **Mutation guard** - `ChildTraversalValidator` rejects any child traversal containing a step that implements the `Mutating` interface (addV, addE, drop, property). This runs at construction time (DSL methods and P factory methods) and again at strategy time via `ChildTraversalVerificationStrategy`. Aggregating side-effects (`aggregate`, `store`, `group`) are NOT blocked because they don't modify graph state. Whether they should be restricted is a question for reviewers. 6. **Provider contract** - `HasContainer.hasTraversal()` is the single check point providers must use before folding. Providers that fold HasContainers into index-backed steps without this check will attempt to use an unresolved `P` value (returns `null` before `resolve()` is called) as an index key, producing wrong results. `TinkerGraphStepStrategy` demonstrates the pattern: skip traversal-bearing HasSteps with `continue` so that subsequent literal HasSteps can still be folded. Providers with custom `GraphStep` replacements must also copy the `idTraversal` field during strategy replacement, otherwise `V(traversal)` falls through to returning all vertices. 7. **`choose()` restriction** - `choose(P)` and `choose().option(P, ...)` reject traversal-bearing predicates at construction time. This is a deliberate scope decision. `PredicateTraversal` (which evaluates option-key predicates in `BranchStep`) has access to the traverser and could technically call `P.resolve()` before `P.test()`, but extending the branching infrastructure is out of scope for this feature. Users needing dynamic branching can use the `choose(Traversal, ...)` form where the branch predicate is a full traversal (e.g., `choose(__.is(P.gt(traversal)), trueChoice, falseChoice)`). This works because `IsStep` handles resolution normally. Restricting `is()` when used inside `choose()`'s branch traversal is not feasible since `choose()` receives it as an opaque `Traversal.Admin` and does not inspect internal steps. ### What's NOT in this PR - `P.resolve()` thread-safety refactoring (future) - `instanceof DefaultGraphTraversal` cleanup (deferred) - Runtime index folding for deterministic child traversals - `FilterRankingStrategy` cost awareness ### Compatibility - Additive overloads only. No existing step or predicate signature changed. - Serialized GremlinLang form is unchanged: `has(key, traversal)` still serializes as `has(key, traversal)`, verified by `GremlinLangTraversalRoundTripTest` and `GremlinQueryParserTraversalTest`. --- ## Testing All `gremlin-core` unit tests pass (9132, 0 failures) and the full TinkerGraph Gherkin suite passes (2172 scenarios, 0 failures) against the current `master`. | Category | Coverage | |----------|----------| | Behavioral contract | `HasTraversal.feature`, `IsTraversal.feature`, `WhereTraversal.feature`, `VETraversal.feature`, `PropertyTraversal.feature`, `ChildTraversalVerification.feature` | | Internal mechanics | `PTraversalTest`, `CloneIndependenceTraversalTest`, `HasContainerTest`, `ChildTraversalValidatorTest`, `ChildTraversalVerificationStrategyTest` | | Grammar + serialization | `GremlinQueryParserTraversalTest`, `GremlinLangTraversalRoundTripTest` | | Strategy correctness | `TinkerGraphStepStrategyTraversalTest` | ## Checklist - [x] CHANGELOG entry added - [x] Reference documentation updated (`the-traversal.asciidoc`) - [x] Upgrade documentation added (`release-4.x.x.asciidoc`) - [x] All GLVs updated (Python, .NET, Go, JavaScript) - [x] Full `mvn clean install` passes - [x] No new external dependencies introduced -- 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]
