Skip to content

Fix TraversalParent implementations for Placeholder steps #3209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Sep 25, 2025
Merged

Fix TraversalParent implementations for Placeholder steps #3209

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9b292c6
Fix TraversalParent implementations for Placeholder steps
Cole-Greer Sep 12, 2025
6b7445d
fix cloning in AbstractMergeElementStepPlaceholder
Cole-Greer Sep 18, 2025
5a23a28
fix javadoc links
Cole-Greer Sep 18, 2025
3c94201
updates from PR feedback and fix Traversal representation of elementI…
Cole-Greer Sep 24, 2025
9b6b08e
extra test case
Cole-Greer Sep 24, 2025
271d783
suggestions from code review
Cole-Greer Sep 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions ...n/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/GValueConstantTraversal.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,17 @@ public boolean equals(final Object other) {
return constantTraversal.equals(other);
}

@Override
public GValueConstantTraversal<S, E> clone() {
GValueConstantTraversal<S, E> clone = new GValueConstantTraversal<>(GValue.of(this.end.getName(), this.end.get()));
try {
clone.setGValueManager(this.getGValueManager().clone());
} catch (CloneNotSupportedException e) { //TODO:: handle properly
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There needs to be a revisit for cloning of GValues, GValueManager, and StepPlaceholders. I consider this out of scope for this PR and intend to address this soon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that there are fields in the parent class AbstractLambdaTraversal which are not cloned by this current logic such as bypassTraversal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated this method to include a call to super.clone(). I will do a further followup of cloning of GValues/StepPlaceholders in a followup PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the call to super.clone should go inside the try block

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still leaving this scope to a followup on cloning, although AbstractLambdaTraversal.clone() does not throw CloneNotSupportedException so adding it to the try does not have any effect. There's actually no way for GValue or GValueManager to throw CloneNotSupportedException either given the current implementation, so in the followup this will be corrected and the try-catch will go away altogether.

throw new RuntimeException(e);
}
return clone;
}

public ConstantTraversal<S, E> getConstantTraversal() {
return constantTraversal;
}
Expand Down
18 changes: 18 additions & 0 deletions ...re/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/TraversalParent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import org.apache.tinkerpop.gremlin.process.traversal.GValueManager;
import org.apache.tinkerpop.gremlin.process.traversal.Step;
import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
import org.apache.tinkerpop.gremlin.process.traversal.step.branch.BranchStep;
import org.apache.tinkerpop.gremlin.process.traversal.step.branch.LocalStep;
import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper;

Expand All @@ -31,14 +33,30 @@
import java.util.Set;

/**
* Indicates that a step can contain child Traversals. Any step which implements this interface should override at least
Copy link
Contributor

@andreachild andreachild Sep 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Likely out of scope for this PR but I feel it would be helpful in general for TinkerPop to have more custom annotations. In this case there could be a custom annotation like @ChildTraversal(scope=Type.LOCAL) to mark step fields that store a child traversal. Then there can be a single common method which handles the presence of child traversals in AbstractStep.java like (pseudocode):

@Override
public void setTraversal(final Traversal.Admin<?, ?> traversal) {
    this.traversal = traversal;
    // retrieve any step fields annotated with @ChildTraversal
    // for each child traversal, call integrateChild
}

And then there could also be an AbstractTravesalParent.java which has:

<S, E> List<Traversal.Admin<S, E>> getGlobalChildren() {
    // use reflection to retrieve fields annotated with @ChildTraversal(type=GLOBAL)
}

<S, E> List<Traversal.Admin<S, E>> getLocalChildren() {
    // use reflection to retrieve fields annotated with @ChildTraversal(type=LOCAL)
}

Then when adding new steps or adding child traversals to an existing step, all you would need to do is add the custom annotation and you wouldn't have to 'remember' to do the boilerplate logic.

If you agree with this suggestion it's possible to use an annotation for these affected classes only and then slowly migrate towards 100% usage of the annotation in future PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's definitely a compelling idea and would be an improvement over the status quo. I'm not sure about half starting it here in this PR though. I would rather such a change be made all at once, as I'm worried there are a few steps which will be difficult to adapt to the annotation model, and may require tweaking the design. The steps which come to mind which may be difficult are the ones like AddEdge which currently shove all of their arguments into a Parameters object and use that to manage child Traversals.

I think this might make for a good JIRA. I don't think it's prohibitively large to tackle all at once as a standalone task. There's only about 60 implementations of TraversalParent when you factor out abstract classes and such, and there's really only a handful of unique patterns within those, they are mostly simple and repetitive.

* one of getGlobalChildren() or getLocalChildren(). All TraversalParents should call integrateChild() for all child
* traversals as they are added to the step, and TraversalParents should override {@link Step::setTraversal()}, and
* call integrateChild() again for all child traversals.
*
* @author Marko A. Rodriguez (http://markorodriguez.com)
*/
public interface TraversalParent extends PopContaining, AutoCloseable {

/**
* Gets a list of all "global" child traversals for this step. A "global" traversal is one which evaluates across
* all of its parents incoming traversers at once. This is typically used in cases where the child traversal
* represents a branch of traversal flow. See {@link BranchStep} as an example.
*/
public default <S, E> List<Traversal.Admin<S, E>> getGlobalChildren() {
return Collections.emptyList();
}

/**
* Gets a list of all "local" child traversals for this step. A "local" traversal is one which is evaluated
* independently for each incoming traverser to the parent step. This is typically used in cases where the child
* is used to process or augment each traverser individually. See {@link LocalStep} or {@link ByModulating} as
* examples.
*/
public default <S, E> List<Traversal.Admin<S, E>> getLocalChildren() {
return Collections.emptyList();
}
Expand Down
26 changes: 12 additions & 14 deletions ...g/apache/tinkerpop/gremlin/process/traversal/step/map/AbstractAddEdgeStepPlaceholder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,20 +65,6 @@ public void addFrom(final Traversal.Admin<?, ?> fromObject) {
this.integrateChild(this.from);
}

@Override
public List<Traversal.Admin<S, Edge>> getLocalChildren() {
final List<Traversal.Admin<S, Edge>> childTraversals = super.getLocalChildren();
if (from != null) childTraversals.add((Traversal.Admin) from);
if (to != null) childTraversals.add((Traversal.Admin) to);
return childTraversals;
}

@Override
public void setTraversal(final Traversal.Admin<?, ?> parentTraversal) {
super.setTraversal(parentTraversal);
this.getLocalChildren().forEach(this::integrateChild);
}

@Override
public int hashCode() {
int hash = super.hashCode();
Expand All @@ -101,6 +87,18 @@ public boolean isParameterized() {
return false;
}

@Override
public List<Traversal.Admin<?, ?>> getLocalChildren() {
List<Traversal.Admin<?, ?>> childTraversals = super.getLocalChildren();
if (from != null) {
childTraversals.add(from instanceof GValueConstantTraversal ? ((GValueConstantTraversal<?, ?>) from).getConstantTraversal() : from);
}
if (to != null) {
childTraversals.add(to instanceof GValueConstantTraversal ? ((GValueConstantTraversal<?, ?>) to).getConstantTraversal() : to);
}
return childTraversals;
}

@Override
protected boolean supportsMultiProperties() {
return false;
Expand Down
26 changes: 21 additions & 5 deletions ...pache/tinkerpop/gremlin/process/traversal/step/map/AbstractAddElementStepPlaceholder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,22 +81,35 @@ public Set<String> getScopeKeys() {
}

protected void addTraversal(final Traversal.Admin<?, ?> traversal) {
integrateChild(traversal);
TraversalHelper.getStepsOfAssignableClassRecursively(Scoping.class, traversal).forEach(s -> scopeKeys.addAll(s.getScopeKeys()));
}

@Override
public List<Traversal.Admin<S, E>> getLocalChildren() {
List<Traversal.Admin<S, E>> childTraversals = new ArrayList<>();
for (List<Object> values : properties.values()) {
for (Object value : values) {
public List<Traversal.Admin<?, ?>> getLocalChildren() {
List<Traversal.Admin<?, ?>> childTraversals = new ArrayList<>();
for (Map.Entry<Object, List<Object>> entry : properties.entrySet()) {
if (entry.getKey() instanceof Traversal) {
childTraversals.add((Traversal.Admin<?, ?>) ((Traversal) entry.getKey()).asAdmin());
}
for (Object value : entry.getValue()) {
if (value instanceof Traversal) {
childTraversals.add((Traversal.Admin<S, E>) ((Traversal) value).asAdmin());
childTraversals.add((Traversal.Admin<?, ?>) ((Traversal) value).asAdmin());
}
}
}
if (label != null) {
childTraversals.add(label);
}
return childTraversals;
}

@Override
public void setTraversal(final Traversal.Admin<?, ?> parentTraversal) {
super.setTraversal(parentTraversal);
this.getLocalChildren().forEach(this::integrateChild);
}

@Override
public Set<TraverserRequirement> getRequirements() {
return this.getSelfAndChildRequirements(TraverserRequirement.OBJECT);
Expand Down Expand Up @@ -175,6 +188,9 @@ public void addProperty(Object key, Object value) {
if (key instanceof GValue) {
throw new IllegalArgumentException("GValue cannot be used as a property key");
}
if (key instanceof Traversal) {
this.integrateChild(((Traversal<?, ?>) key).asAdmin());
}
if (value instanceof GValue) {
traversal.getGValueManager().register((GValue<?>) value);
}
Expand Down
50 changes: 38 additions & 12 deletions ...che/tinkerpop/gremlin/process/traversal/step/map/AbstractMergeElementStepPlaceholder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
import org.apache.tinkerpop.gremlin.structure.T;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
Expand All @@ -55,6 +56,7 @@ public abstract class AbstractMergeElementStepPlaceholder<S, E> extends Abstract
public AbstractMergeElementStepPlaceholder(Traversal.Admin traversal, final Traversal.Admin<?, Map<Object, Object>> mergeTraversal, final boolean isStart) {
super(traversal);
this.mergeTraversal = mergeTraversal;
this.integrateChild(mergeTraversal);
this.isStart = isStart;
}

Expand All @@ -73,6 +75,27 @@ public Set<TraverserRequirement> getRequirements() {
return super.getRequirements();
}

@Override
public void setTraversal(final Traversal.Admin<?, ?> parentTraversal) {
super.setTraversal(parentTraversal);
this.getLocalChildren().forEach(this::integrateChild);
}

@Override
public List<Traversal.Admin<?, ?>> getLocalChildren() {
List<Traversal.Admin<?, ?>> localChildren = new ArrayList<>();
if (mergeTraversal != null) {
localChildren.add(mergeTraversal);
}
if (onCreateTraversal != null) {
localChildren.add(onCreateTraversal);
}
if (onMatchTraversal != null) {
localChildren.add(onMatchTraversal);
}
return localChildren;
}

@Override
public Traversal.Admin getMergeTraversal() {
if (mergeTraversal != null && mergeTraversal instanceof GValueConstantTraversal) {
Expand Down Expand Up @@ -158,27 +181,30 @@ public boolean isUsingPartitionStrategy() {
}

@Override
public void setOnMatch(final Traversal.Admin<?, Map<Object, Object>> onMatchMap) {
this.onMatchTraversal = onMatchMap;
if (onMatchMap instanceof GValueConstantTraversal && ((GValueConstantTraversal<?, Map<Object, Object>>) onMatchMap).isParameterized()) {
traversal.getGValueManager().register(((GValueConstantTraversal<?, Map<Object, Object>>) onMatchMap).getGValue());
public void setOnMatch(final Traversal.Admin<?, Map<Object, Object>> onMatchTraversal) {
this.onMatchTraversal = onMatchTraversal;
if (onMatchTraversal instanceof GValueConstantTraversal && ((GValueConstantTraversal<?, Map<Object, Object>>) onMatchTraversal).isParameterized()) {
traversal.getGValueManager().register(((GValueConstantTraversal<?, Map<Object, Object>>) onMatchTraversal).getGValue());
}
this.integrateChild(onMatchTraversal);
}

@Override
public void setOnCreate(final Traversal.Admin<?, Map<Object, Object>> onCreateMap) {
this.onCreateTraversal = onCreateMap;
if (onCreateMap instanceof GValueConstantTraversal && ((GValueConstantTraversal<?, Map<Object, Object>>) onCreateMap).isParameterized()) {
traversal.getGValueManager().register(((GValueConstantTraversal<?, Map<Object, Object>>) onCreateMap).getGValue());
public void setOnCreate(final Traversal.Admin<?, Map<Object, Object>> onCreateTraversal) {
this.onCreateTraversal = onCreateTraversal;
if (onCreateTraversal instanceof GValueConstantTraversal && ((GValueConstantTraversal<?, Map<Object, Object>>) onCreateTraversal).isParameterized()) {
traversal.getGValueManager().register(((GValueConstantTraversal<?, Map<Object, Object>>) onCreateTraversal).getGValue());
}
this.integrateChild(onCreateTraversal);
}

@Override
public void setMerge(Traversal.Admin<?, Map<Object, Object>> mergeMap) {
this.mergeTraversal = mergeMap;
if (mergeMap instanceof GValueConstantTraversal && ((GValueConstantTraversal<?, Map<Object, Object>>) mergeMap).isParameterized()) {
traversal.getGValueManager().register(((GValueConstantTraversal<?, Map<Object, Object>>) mergeMap).getGValue());
public void setMerge(Traversal.Admin<?, Map<Object, Object>> mergeTraversal) {
this.mergeTraversal = mergeTraversal;
if (mergeTraversal instanceof GValueConstantTraversal && ((GValueConstantTraversal<?, Map<Object, Object>>) mergeTraversal).isParameterized()) {
traversal.getGValueManager().register(((GValueConstantTraversal<?, Map<Object, Object>>) mergeTraversal).getGValue());
}
this.integrateChild(mergeTraversal);
}

@Override
Expand Down
9 changes: 9 additions & 0 deletions ...e/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/DateDiffStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.time.ZoneOffset;
import java.util.Collections;
import java.util.Date;
import java.util.List;
import java.util.Set;

/**
Expand Down Expand Up @@ -102,9 +103,17 @@ public Set<TraverserRequirement> getRequirements() {
return Collections.singleton(TraverserRequirement.OBJECT);
}

@Override
public List<Traversal.Admin<?, ?>> getLocalChildren() {
return dateTraversal == null ? Collections.emptyList() : List.of(dateTraversal);
}

@Override
public void setTraversal(final Traversal.Admin<?, ?> parentTraversal) {
super.setTraversal(parentTraversal);
if (dateTraversal != null) {
integrateChild(dateTraversal);
}
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ...ain/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/TraversalSelectStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ public TraversalSelectStep<S, E> clone() {
@Override
public void setTraversal(final Traversal.Admin<?, ?> parentTraversal) {
super.setTraversal(parentTraversal);
this.integrateChild(this.selectTraversal);
this.getLocalChildren().forEach(this::integrateChild);
}

@Override
Expand Down
6 changes: 5 additions & 1 deletion .../java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
import org.apache.tinkerpop.gremlin.process.traversal.lambda.ConstantTraversal;
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.Writing;
Expand Down Expand Up @@ -209,7 +210,10 @@ public Object getKey() {
@Override
public Object getValue() {
List<Object> values = parameters.get(T.value, null);
return values.isEmpty() ? null : values.get(0);
if (values.isEmpty()) {
return null;
}
return values.get(0) instanceof ConstantTraversal ? ((ConstantTraversal<?, ?>) values.get(0)).next() : values.get(0);
}

@Override
Expand Down
12 changes: 8 additions & 4 deletions ...g/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepContract.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.lambda.ConstantTraversal;
import org.apache.tinkerpop.gremlin.process.traversal.step.Deleting;
import org.apache.tinkerpop.gremlin.process.traversal.step.GValue;
import org.apache.tinkerpop.gremlin.process.traversal.step.Scoping;
Expand Down Expand Up @@ -47,14 +48,17 @@ default HashSet<PopInstruction> getPopInstructions() {
Object getKey();

/**
* Get the property value
* Gets the property value. If the value was originally passed as a {@link GValue<?>}, {@link ConstantTraversal <?>},
* or a literal value, then the literal value is returned. Otherwise, the MergeMap is returned in Traversal form.
*/
Object getValue();

/**
* Get the value as a GValue, without pinning the variable
* Gets the property value. If the value was originally passed as a {@link GValue<?>}, that is returned
* directly. If it was originally passed as a {@link ConstantTraversal <?>}, or a literal value, then the literal
* value is returned. Otherwise, the MergeMap is returned in Traversal form.
*/
default GValue<?> getValueAsGValue() {
return GValue.of(getValue());
default Object getValueWithGValue() {
return getValue();
}
}
Loading
Loading