Cole-Greer commented on code in PR #3458:
URL: https://github.com/apache/tinkerpop/pull/3458#discussion_r3407405461
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java:
##########
@@ -49,10 +56,23 @@ public class P<V> implements Predicate<V>, Serializable,
Cloneable {
protected Map<String, V> variables = new HashMap<>();
protected Collection<V> literals = Collections.EMPTY_LIST;
private boolean isCollection = false;
+ private boolean resolvedEmpty = false;
+ private Traversal.Admin<?, ?> traversalValue;
+ private List<Traversal.Admin<?, ?>> traversalValues;
public P(final PBiPredicate<V, V> biPredicate, final V value) {
- setValue(value);
this.biPredicate = biPredicate;
+ // If the value is a DefaultGraphTraversal (the type created by
__.xxx() anonymous traversals),
+ // treat it as a child traversal rather than a literal. This handles
the case where Java's
+ // overload resolution picks P(BiPredicate, V) instead of
P(BiPredicate, Traversal) when
+ // the caller passes a GraphTraversal. We specifically check for
DefaultGraphTraversal
+ // rather than Traversal to avoid catching internal traversal types
like ConstantTraversal,
+ // ValueTraversal, and IdentityTraversal which are used as literal
values in P.
Review Comment:
I'm not sure I agree with handling Traversals other than
DefaultGraphTraversal as literal values. I can't imagine a case where I would
want to `P.eq()` a `ConstantTraversal`, and have that mean
`constantTraversal.equals(myValue)` instead of
`constantTraversal().next().equals(myValue)` like any regular `GraphTraversal`.
That said, I'm not sure when I would ever want to pass a ConstantTraversal into
a predicate.
Even if the choice is kept to exclude most of the oddball traversals, I
would think testing for `GraphTraversal` instead of `DefaultGraphTraversal`
would be more reasonable.
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java:
##########
@@ -49,10 +56,23 @@ public class P<V> implements Predicate<V>, Serializable,
Cloneable {
protected Map<String, V> variables = new HashMap<>();
protected Collection<V> literals = Collections.EMPTY_LIST;
private boolean isCollection = false;
+ private boolean resolvedEmpty = false;
+ private Traversal.Admin<?, ?> traversalValue;
+ private List<Traversal.Admin<?, ?>> traversalValues;
Review Comment:
Nit: The result types of these traversals should arguably be `V` throughout
this class. I expect that would help with a lot of the
`@SuppressWarnings("unchecked")` annotations throughout this class. There's
probably an argument that the generics are so broken in this class already that
it hardly matters, but it would be more proper this way.
```suggestion
private Traversal.Admin<?, V> traversalValue;
private List<Traversal.Admin<?, V>> traversalValues;
```
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java:
##########
@@ -453,6 +697,106 @@ public static <V> P<V> without(final Collection<V> value)
{
return new P(Contains.without, value);
}
+ /**
+ * Determines if values are equal using a child traversal resolved at
runtime.
+ *
+ * @since 4.0.0
+ */
+ public static <V> P<V> eq(final Traversal<?, ?> traversalValue) {
Review Comment:
We shouldn't assume that traversals will never end up in the original `eq(V
value)` overload, it's essentially `eq(Object value)` after erasure and java
can be unreliable when selecting overloads sometimes. We ran into that problem
quite a bit with GValue, the only value the GValue overloads really gave was
making the generics behave a bit nicer in the average case. There's plenty of
times where Java chooses to ignore the narrow overloads.
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java:
##########
@@ -224,6 +283,187 @@ public Set<GValue<?>> getGValues() {
return results;
}
+ /**
+ * Determines if this predicate holds a child traversal whose result is
resolved at runtime.
+ */
+ public boolean hasTraversal() {
+ return this.traversalValue != null || (this.traversalValues != null &&
!this.traversalValues.isEmpty());
+ }
+
+ /**
+ * Returns {@code true} if the most recent call to {@link
#resolve(Traverser.Admin)} produced no results.
+ * Steps should check this after calling {@code resolve()} and
short-circuit appropriately rather than
+ * calling {@link #test(Object)}, which would compare against {@code null}.
+ */
+ public boolean isResolvedEmpty() {
+ return this.resolvedEmpty;
+ }
+
+ /**
+ * Gets the child traversal value, if one was provided. Returns {@code
null} when this predicate uses
+ * literal values or variables.
+ */
+ public Traversal.Admin<?, ?> getTraversalValue() {
+ return this.traversalValue;
+ }
+
+ /**
+ * Gets the list of child traversal values for multi-traversal predicates
(e.g., {@code within(trav1, trav2)}).
+ * Returns {@code null} when this predicate uses a single traversal or
literal values.
+ *
+ * @since 4.0.0
+ */
+ public List<Traversal.Admin<?, ?>> getTraversalValues() {
+ return this.traversalValues;
+ }
+
+ /**
+ * Resolves the child traversal against the given traverser, replacing the
traversal value with the
+ * resolved literal(s) for this test cycle. If no traversal is present,
this method returns immediately.
+ *
+ * <p>For all predicates, only the first result from the child traversal
is used,
+ * consistent with {@code by(traversal)} semantics. For collection
predicates
+ * ({@link Contains#within}, {@link Contains#without}), the first result
should be a
+ * {@link Collection} (e.g., produced by {@code fold()}).</p>
+ *
+ * <p>When multiple traversals are present (via {@link #traversalValues}),
each traversal is evaluated
+ * independently, the first result from each is taken, and results are
combined into a single collection.
+ * This is only valid for collection predicates ({@link Contains#within},
{@link Contains#without}).</p>
+ */
+ @SuppressWarnings("unchecked")
+ public void resolve(final Traverser.Admin<?> traverser) {
+ if (this.traversalValues != null && !this.traversalValues.isEmpty()) {
+ resolveMultipleTraversals(traverser);
+ return;
+ }
+
+ if (this.traversalValue == null) return;
+
+ final Traversal.Admin<Object, Object> trav = (Traversal.Admin<Object,
Object>) (Traversal.Admin) this.traversalValue;
+ prepareChildTraversal(traverser, trav);
+
+ try {
+ if (!trav.hasNext()) {
+ // 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();
+ 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();
+ if (this.biPredicate instanceof Contains) {
+ if (firstResult instanceof Collection) {
+ this.literals = (Collection<V>) firstResult;
+ } else {
+ this.literals = Collections.singletonList((V)
firstResult);
+ }
+ this.isCollection = true;
+ } else {
+ setValue((V) firstResult);
+ }
+ }
+ } finally {
+ CloseableIterator.closeIterator(trav);
+ }
+ }
+
+ /**
+ * Resolves multiple child traversals, taking the first result from each
and combining into a collection.
+ * Only valid for collection predicates ({@link Contains}).
+ */
+ @SuppressWarnings("unchecked")
+ private void resolveMultipleTraversals(final Traverser.Admin<?> traverser)
{
+ final List<Object> allResults = new ArrayList<>();
+
+ for (final Traversal.Admin<?, ?> tv : this.traversalValues) {
+ final Traversal.Admin<Object, Object> trav =
(Traversal.Admin<Object, Object>) (Traversal.Admin) tv;
+ prepareChildTraversal(traverser, trav);
+
+ try {
+ if (trav.hasNext()) {
+ final Object firstResult = trav.next();
+ if (firstResult instanceof Collection) {
+ allResults.addAll((Collection<?>) firstResult);
+ } else {
+ allResults.add(firstResult);
+ }
+ }
+ } finally {
+ CloseableIterator.closeIterator(trav);
+ }
+ }
+
+ // 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()
Review Comment:
Nit: Do we need the ternary here? Can we just directly feed `allResults`
into `setValue()` or `this.literals`?
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java:
##########
@@ -453,6 +697,106 @@ public static <V> P<V> without(final Collection<V> value)
{
return new P(Contains.without, value);
}
+ /**
+ * Determines if values are equal using a child traversal resolved at
runtime.
+ *
+ * @since 4.0.0
+ */
+ public static <V> P<V> eq(final Traversal<?, ?> traversalValue) {
+ ChildTraversalValidator.validate(traversalValue.asAdmin());
+ return new P(Compare.eq, traversalValue.asAdmin());
+ }
+
+ /**
+ * Determines if values are not equal using a child traversal resolved at
runtime.
+ *
+ * @since 4.0.0
+ */
+ public static <V> P<V> neq(final Traversal<?, ?> traversalValue) {
+ ChildTraversalValidator.validate(traversalValue.asAdmin());
+ return new P(Compare.neq, traversalValue.asAdmin());
+ }
+
+ /**
+ * Determines if a value is less than another using a child traversal
resolved at runtime.
+ *
+ * @since 4.0.0
+ */
+ public static <V> P<V> lt(final Traversal<?, ?> traversalValue) {
+ ChildTraversalValidator.validate(traversalValue.asAdmin());
+ return new P(Compare.lt, traversalValue.asAdmin());
+ }
+
+ /**
+ * Determines if a value is less than or equal to another using a child
traversal resolved at runtime.
+ *
+ * @since 4.0.0
+ */
+ public static <V> P<V> lte(final Traversal<?, ?> traversalValue) {
+ ChildTraversalValidator.validate(traversalValue.asAdmin());
+ return new P(Compare.lte, traversalValue.asAdmin());
+ }
+
+ /**
+ * Determines if a value is greater than another using a child traversal
resolved at runtime.
+ *
+ * @since 4.0.0
+ */
+ public static <V> P<V> gt(final Traversal<?, ?> traversalValue) {
+ ChildTraversalValidator.validate(traversalValue.asAdmin());
+ return new P(Compare.gt, traversalValue.asAdmin());
+ }
+
+ /**
+ * Determines if a value is greater than or equal to another using a child
traversal resolved at runtime.
+ *
+ * @since 4.0.0
+ */
+ public static <V> P<V> gte(final Traversal<?, ?> traversalValue) {
+ ChildTraversalValidator.validate(traversalValue.asAdmin());
+ return new P(Compare.gte, traversalValue.asAdmin());
+ }
+
+ /**
+ * Determines if a value is within the results of a child traversal
resolved at runtime.
+ *
+ * @since 4.0.0
+ */
+ public static <V> P<V> within(final Traversal<?, ?> traversalValue) {
+ ChildTraversalValidator.validate(traversalValue.asAdmin());
+ return new P(Contains.within, traversalValue.asAdmin());
+ }
+
+ /**
+ * Determines if a value is within the union of results from multiple
child traversals resolved at runtime.
+ * Each traversal is evaluated independently and results are combined into
a single collection.
+ *
+ * @since 4.0.0
+ */
+ public static <V> P<V> within(final Traversal<?, ?> first, final
Traversal<?, ?> second, final Traversal<?, ?>... more) {
+ return containsTraversals(Contains.within, first, second, more);
+ }
+
+ /**
+ * Determines if a value is not within the results of a child traversal
resolved at runtime.
+ *
+ * @since 4.0.0
+ */
+ public static <V> P<V> without(final Traversal<?, ?> traversalValue) {
+ ChildTraversalValidator.validate(traversalValue.asAdmin());
+ return new P(Contains.without, traversalValue.asAdmin());
+ }
+
+ /**
+ * Determines if a value is not within the union of results from multiple
child traversals resolved at runtime.
+ * Each traversal is evaluated independently and results are combined into
a single collection.
+ *
+ * @since 4.0.0
+ */
+ public static <V> P<V> without(final Traversal<?, ?> first, final
Traversal<?, ?> second, final Traversal<?, ?>... more) {
Review Comment:
Why do we need special cases for these extra first, second, more overloads?
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java:
##########
@@ -453,6 +697,106 @@ public static <V> P<V> without(final Collection<V> value)
{
return new P(Contains.without, value);
}
+ /**
+ * Determines if values are equal using a child traversal resolved at
runtime.
+ *
+ * @since 4.0.0
+ */
+ public static <V> P<V> eq(final Traversal<?, ?> traversalValue) {
+ ChildTraversalValidator.validate(traversalValue.asAdmin());
+ return new P(Compare.eq, traversalValue.asAdmin());
+ }
+
+ /**
+ * Determines if values are not equal using a child traversal resolved at
runtime.
+ *
+ * @since 4.0.0
+ */
+ public static <V> P<V> neq(final Traversal<?, ?> traversalValue) {
+ ChildTraversalValidator.validate(traversalValue.asAdmin());
+ return new P(Compare.neq, traversalValue.asAdmin());
+ }
+
+ /**
+ * Determines if a value is less than another using a child traversal
resolved at runtime.
+ *
+ * @since 4.0.0
+ */
+ public static <V> P<V> lt(final Traversal<?, ?> traversalValue) {
+ ChildTraversalValidator.validate(traversalValue.asAdmin());
Review Comment:
It might be better to pull this validation into the P constructor or even
setValue(), in most cases P's are built via these static methods but not always.
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java:
##########
@@ -453,6 +697,106 @@ public static <V> P<V> without(final Collection<V> value)
{
return new P(Contains.without, value);
}
+ /**
Review Comment:
Nit on organization, it would be nice to interleave these overloads with
their matching pairs (all the eq's together, all the gt's together...)
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java:
##########
@@ -224,6 +283,187 @@ public Set<GValue<?>> getGValues() {
return results;
}
+ /**
+ * Determines if this predicate holds a child traversal whose result is
resolved at runtime.
+ */
+ public boolean hasTraversal() {
+ return this.traversalValue != null || (this.traversalValues != null &&
!this.traversalValues.isEmpty());
+ }
+
+ /**
+ * Returns {@code true} if the most recent call to {@link
#resolve(Traverser.Admin)} produced no results.
+ * Steps should check this after calling {@code resolve()} and
short-circuit appropriately rather than
+ * calling {@link #test(Object)}, which would compare against {@code null}.
+ */
+ public boolean isResolvedEmpty() {
+ return this.resolvedEmpty;
+ }
+
+ /**
+ * Gets the child traversal value, if one was provided. Returns {@code
null} when this predicate uses
+ * literal values or variables.
+ */
+ public Traversal.Admin<?, ?> getTraversalValue() {
+ return this.traversalValue;
+ }
+
+ /**
+ * Gets the list of child traversal values for multi-traversal predicates
(e.g., {@code within(trav1, trav2)}).
+ * Returns {@code null} when this predicate uses a single traversal or
literal values.
+ *
+ * @since 4.0.0
+ */
+ public List<Traversal.Admin<?, ?>> getTraversalValues() {
+ return this.traversalValues;
+ }
+
+ /**
+ * Resolves the child traversal against the given traverser, replacing the
traversal value with the
+ * resolved literal(s) for this test cycle. If no traversal is present,
this method returns immediately.
+ *
+ * <p>For all predicates, only the first result from the child traversal
is used,
+ * consistent with {@code by(traversal)} semantics. For collection
predicates
+ * ({@link Contains#within}, {@link Contains#without}), the first result
should be a
+ * {@link Collection} (e.g., produced by {@code fold()}).</p>
+ *
+ * <p>When multiple traversals are present (via {@link #traversalValues}),
each traversal is evaluated
+ * independently, the first result from each is taken, and results are
combined into a single collection.
+ * This is only valid for collection predicates ({@link Contains#within},
{@link Contains#without}).</p>
+ */
+ @SuppressWarnings("unchecked")
+ public void resolve(final Traverser.Admin<?> traverser) {
+ if (this.traversalValues != null && !this.traversalValues.isEmpty()) {
+ resolveMultipleTraversals(traverser);
+ return;
+ }
+
+ if (this.traversalValue == null) return;
+
+ final Traversal.Admin<Object, Object> trav = (Traversal.Admin<Object,
Object>) (Traversal.Admin) this.traversalValue;
+ prepareChildTraversal(traverser, trav);
+
+ try {
+ if (!trav.hasNext()) {
+ // 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();
+ 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();
+ if (this.biPredicate instanceof Contains) {
Review Comment:
Why can't we go through the `setValue()` path for `Contains` bipredicates?
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java:
##########
@@ -49,10 +56,23 @@ public class P<V> implements Predicate<V>, Serializable,
Cloneable {
protected Map<String, V> variables = new HashMap<>();
protected Collection<V> literals = Collections.EMPTY_LIST;
private boolean isCollection = false;
+ private boolean resolvedEmpty = false;
+ private Traversal.Admin<?, ?> traversalValue;
+ private List<Traversal.Admin<?, ?>> traversalValues;
public P(final PBiPredicate<V, V> biPredicate, final V value) {
Review Comment:
I think we should be careful to avoid the assumption that `V value` here
can't also be a `List<Traversal>` simply because that explicit overload is
present, or because the type parameter theoretically forbids it. This class
really abuses its generics, and relies heavily on type erasure to even
function. It's not exactly type safe, and generally best to treat `V` as
`Object` in most instances here.
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/ChildTraversalValidator.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.util;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Step;
+import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
+import org.apache.tinkerpop.gremlin.process.traversal.step.Mutating;
+import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent;
+
+/**
+ * Validates that child traversals do not contain mutating steps. Child
traversals used as
+ * arguments to filter predicates ({@code has()}, {@code is()}, etc.), lookup
steps
+ * ({@code V(traversal)}, {@code E(traversal)}), and mutation steps ({@code
property(traversal)})
+ * must be read-only - their purpose is to compute values, not produce side
effects.
+ * <p>
+ * Recursion walks both {@link TraversalParent#getLocalChildren()} and
+ * {@link TraversalParent#getGlobalChildren()} to detect mutations nested
inside
+ * {@code map()}, {@code union()}, {@code choose()}, {@code coalesce()}, or
any other parent step.
+ */
+public final class ChildTraversalValidator {
+
+ private ChildTraversalValidator() {
+ }
+
+ /**
+ * Validates that a child traversal contains no {@link Mutating} steps at
any nesting depth.
+ * Throws {@link IllegalArgumentException} if one is found.
+ */
+ public static void validate(final Traversal.Admin<?, ?> child) {
+ for (final Step<?, ?> step : child.getSteps()) {
Review Comment:
Nit: this should probably just use
https://github.com/apache/tinkerpop/blob/24df1d2b89e3b7d10c5f958561d05371ae1c9a75/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java#L428
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java:
##########
@@ -224,6 +283,187 @@ public Set<GValue<?>> getGValues() {
return results;
}
+ /**
+ * Determines if this predicate holds a child traversal whose result is
resolved at runtime.
+ */
+ public boolean hasTraversal() {
+ return this.traversalValue != null || (this.traversalValues != null &&
!this.traversalValues.isEmpty());
+ }
+
+ /**
+ * Returns {@code true} if the most recent call to {@link
#resolve(Traverser.Admin)} produced no results.
+ * Steps should check this after calling {@code resolve()} and
short-circuit appropriately rather than
+ * calling {@link #test(Object)}, which would compare against {@code null}.
+ */
+ public boolean isResolvedEmpty() {
+ return this.resolvedEmpty;
+ }
+
+ /**
+ * Gets the child traversal value, if one was provided. Returns {@code
null} when this predicate uses
+ * literal values or variables.
+ */
+ public Traversal.Admin<?, ?> getTraversalValue() {
+ return this.traversalValue;
+ }
+
+ /**
+ * Gets the list of child traversal values for multi-traversal predicates
(e.g., {@code within(trav1, trav2)}).
+ * Returns {@code null} when this predicate uses a single traversal or
literal values.
+ *
+ * @since 4.0.0
+ */
+ public List<Traversal.Admin<?, ?>> getTraversalValues() {
+ return this.traversalValues;
+ }
+
+ /**
+ * Resolves the child traversal against the given traverser, replacing the
traversal value with the
+ * resolved literal(s) for this test cycle. If no traversal is present,
this method returns immediately.
+ *
+ * <p>For all predicates, only the first result from the child traversal
is used,
+ * consistent with {@code by(traversal)} semantics. For collection
predicates
+ * ({@link Contains#within}, {@link Contains#without}), the first result
should be a
+ * {@link Collection} (e.g., produced by {@code fold()}).</p>
+ *
+ * <p>When multiple traversals are present (via {@link #traversalValues}),
each traversal is evaluated
+ * independently, the first result from each is taken, and results are
combined into a single collection.
+ * This is only valid for collection predicates ({@link Contains#within},
{@link Contains#without}).</p>
+ */
+ @SuppressWarnings("unchecked")
+ public void resolve(final Traverser.Admin<?> traverser) {
+ if (this.traversalValues != null && !this.traversalValues.isEmpty()) {
+ resolveMultipleTraversals(traverser);
+ return;
+ }
+
+ if (this.traversalValue == null) return;
+
+ final Traversal.Admin<Object, Object> trav = (Traversal.Admin<Object,
Object>) (Traversal.Admin) this.traversalValue;
+ prepareChildTraversal(traverser, trav);
+
+ try {
+ if (!trav.hasNext()) {
+ // 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();
+ 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();
+ if (this.biPredicate instanceof Contains) {
+ if (firstResult instanceof Collection) {
+ this.literals = (Collection<V>) firstResult;
+ } else {
+ this.literals = Collections.singletonList((V)
firstResult);
+ }
+ this.isCollection = true;
+ } else {
+ setValue((V) firstResult);
+ }
+ }
+ } finally {
+ CloseableIterator.closeIterator(trav);
+ }
+ }
+
+ /**
+ * Resolves multiple child traversals, taking the first result from each
and combining into a collection.
+ * Only valid for collection predicates ({@link Contains}).
+ */
+ @SuppressWarnings("unchecked")
+ private void resolveMultipleTraversals(final Traverser.Admin<?> traverser)
{
+ final List<Object> allResults = new ArrayList<>();
+
+ for (final Traversal.Admin<?, ?> tv : this.traversalValues) {
+ final Traversal.Admin<Object, Object> trav =
(Traversal.Admin<Object, Object>) (Traversal.Admin) tv;
+ prepareChildTraversal(traverser, trav);
+
+ try {
+ if (trav.hasNext()) {
+ final Object firstResult = trav.next();
+ if (firstResult instanceof Collection) {
+ allResults.addAll((Collection<?>) firstResult);
+ } else {
+ allResults.add(firstResult);
+ }
+ }
+ } finally {
+ CloseableIterator.closeIterator(trav);
+ }
+ }
+
+ // 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;
+ }
+
+ /**
+ * Prepares a child traversal for evaluation by splitting the current
traverser and seeding it.
+ */
+ @SuppressWarnings("unchecked")
+ private static void prepareChildTraversal(final Traverser.Admin<?>
traverser, final Traversal.Admin<Object, Object> trav) {
+ final Traverser.Admin<Object> split = (Traverser.Admin<Object>)
traverser.split();
+ split.setSideEffects(trav.getSideEffects());
+ split.setBulk(1L);
+ trav.reset();
+ trav.addStart(split);
+ }
+
+ //////////////// predicate traversal utilities
+
+ /**
+ * Recursively integrates all child traversals found in the predicate tree
into the given parent step.
+ * Handles {@link ConnectiveP} (recurses into children) and {@link NotP}
(recurses into wrapped predicate).
+ */
+ public static void integrateTraversals(final P<?> p, final TraversalParent
parent) {
Review Comment:
This feels like a bit of an odd API for `P` to surface, we already have
`collectTraversals()` below. Couldn't the parent steps use this to integrate
all of their children directly, instead of delegating this through P?
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java:
##########
@@ -49,10 +56,23 @@ public class P<V> implements Predicate<V>, Serializable,
Cloneable {
protected Map<String, V> variables = new HashMap<>();
protected Collection<V> literals = Collections.EMPTY_LIST;
private boolean isCollection = false;
+ private boolean resolvedEmpty = false;
+ private Traversal.Admin<?, ?> traversalValue;
+ private List<Traversal.Admin<?, ?>> traversalValues;
public P(final PBiPredicate<V, V> biPredicate, final V value) {
- setValue(value);
this.biPredicate = biPredicate;
+ // If the value is a DefaultGraphTraversal (the type created by
__.xxx() anonymous traversals),
+ // treat it as a child traversal rather than a literal. This handles
the case where Java's
+ // overload resolution picks P(BiPredicate, V) instead of
P(BiPredicate, Traversal) when
+ // the caller passes a GraphTraversal. We specifically check for
DefaultGraphTraversal
+ // rather than Traversal to avoid catching internal traversal types
like ConstantTraversal,
+ // ValueTraversal, and IdentityTraversal which are used as literal
values in P.
+ if (value instanceof
org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.DefaultGraphTraversal)
{
Review Comment:
I think this logic should be folded into `setValue()` instead of kept
exclusively in the constructor. I don't like the implication that `new
P(Compare.eq, __.V(5))` won't behave the same as `new P(Compare.eq,
10).setValue(__.V(5))`
--
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]
