Fix TraversalParent implementations for Placeholder steps #3209

Cole-Greer · 2025-09-17T22:54:04Z

The TraversalParent methods were not consistently implemented in the new placeholder steps. This commit adds proper implementations in all TraversalParent placeholder steps, and introduces TraversalParentTest.

TraversalParentTest takes traversals with TraversalParents and verifies that getGlobalChildren() and getLocalChildren() are producing the expected children, and that those children have correctly set their parents. These checks are done before and after strategy application. This is primarily intended for placeholder steps to ensure that TraversalParent remains correctly implemented in both placeholder and concrete steps, however it is also useful to verify against interference from strategies.

The new tests identified an existing deficiency DateDiffStep which was also addressed.

VOTE +1

The TraversalParent methods were not consistently implemented in the new placeholder steps. This commit adds proper implementations in all TraversalParent placeholder steps, and introduces TraversalParentTest. TraversalParentTest takes traversals with TraversalParents and verifies that getGlobalChildren() and getLocalChildren() are producing the expected children, and that those children have correctly set their parents. These checks are done before and after strategy application. This is primarily intended for placeholder steps to ensure that TraversalParent remains correctly implemented in both placeholder and concrete steps, however it is also useful to verify against interference from strategies. The new tests identified an existing deficiency DateDiffStep which was also addressed

Cole-Greer · 2025-09-17T22:54:51Z

...main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/GValueConstantTraversal.java

+        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


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.

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

...-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/TraversalParent.java

spmallette · 2025-09-18T11:15:47Z

taking some build errors it seems - also, i'd think you need to backport changes for DateDiffStep and TraversalSelectStep.

Cole-Greer · 2025-09-18T16:42:01Z

taking some build errors it seems - also, i'd think you need to backport changes for DateDiffStep and TraversalSelectStep.

Ahh, new issues snuck their in with a last minute tweak to merge. Should be good now.

I'll CTR the fixes to DateDiffStep and TraversalSelectStep back to 3.7-dev separately.

andreachild · 2025-09-22T23:26:11Z

.../org/apache/tinkerpop/gremlin/process/traversal/step/map/AbstractAddEdgeStepPlaceholder.java

    }

+    @Override
+    public List<Traversal.Admin<?, ?>> getLocalChildren() {


Is the parent interface generic contract which is defined as List<Traversal.Admin<S, E>> getLocalChildren too narrow since it needs to be modified here?

The TraversalParent interface itself does not use generics. The S and E type variables are only bound on the getLocalChildren method itself. That means that the S and E defined in public default <S, E> List<Traversal.Admin<S, E>> getLocalChildren() are not the same S and E defined in this implementing step classes.

They should be independent, the addEdge step here uses <S, Edge> (can start with anything and always produces an Edge), but it's child traversals do not produce edges, the from/to traversals will produce a Vertex or some ID type, the label traversal will produce a String...

I think it would be most clear if TraversalParent was updated to just return List<Traversal.Admin<?, ?>> instead, however I considered that out of scope for this PR.

andreachild · 2025-09-23T03:02:39Z

...-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/TraversalParent.java

 import java.util.Set;

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


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.

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.

andreachild · 2025-09-23T03:08:09Z

...g/apache/tinkerpop/gremlin/process/traversal/step/map/AbstractAddElementStepPlaceholder.java

            throw new IllegalArgumentException("GValue cannot be used as a property key");
        }
+        if (key instanceof Traversal) {
+            this.integrateChild(((Traversal<?, ?>) key).asAdmin());


Should integrateChild also be called by setLabel method if the label is a traversal?

Yes it should, same goes for setElementId. That appears to be a gap in the testing. I will make that fix and add those cases.

andreachild · 2025-09-23T03:14:27Z

...apache/tinkerpop/gremlin/process/traversal/step/map/AbstractMergeElementStepPlaceholder.java

Looking at the clone logic, it appears that properties can contain traversals. If that is the case then addProperty should also call integrateChild.

andreachild · 2025-09-23T03:15:35Z

...g/apache/tinkerpop/gremlin/process/traversal/step/map/AbstractAddElementStepPlaceholder.java

            throw new IllegalArgumentException("GValue cannot be used as a property key");
        }
+        if (key instanceof Traversal) {
+            this.integrateChild(((Traversal<?, ?>) key).asAdmin());


Does anything need to be done to 'de-integrate' a child traversal if a property is removed?

andreachild · 2025-09-23T03:21:32Z

.../org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepContract.java

    /**
-     * 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.


What is a MergeMap in this context?

andreachild · 2025-09-23T03:23:19Z

...g/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepPlaceholder.java

+            this.value = ((Traversal<?, ?>) valueObject).asAdmin();
+            this.integrateChild(((Traversal<?, ?>) valueObject).asAdmin());
+        } else {
+            this.value = new ConstantTraversal<>(valueObject);


Do ConstantTraversals not need to be integrated as a child traversal?

andreachild · 2025-09-23T03:28:17Z

...apache/tinkerpop/gremlin/process/traversal/step/map/AbstractMergeElementStepPlaceholder.java

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


Can you clarify why setTraversal needs to call integrateChild but the constructor which calls super(traversal) does not need to call integrateChild?

Cole-Greer commented Sep 17, 2025

View reviewed changes

spmallette reviewed Sep 18, 2025

View reviewed changes

...-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/TraversalParent.java Outdated Show resolved Hide resolved

fix cloning in AbstractMergeElementStepPlaceholder

6b7445d

fix javadoc links

5a23a28

andreachild reviewed Sep 22, 2025

View reviewed changes

andreachild reviewed Sep 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix TraversalParent implementations for Placeholder steps #3209

Fix TraversalParent implementations for Placeholder steps #3209

Cole-Greer commented Sep 17, 2025 •

edited

Loading

Uh oh!

Cole-Greer Sep 17, 2025

Uh oh!

andreachild Sep 22, 2025

Uh oh!

Uh oh!

spmallette commented Sep 18, 2025

Uh oh!

Cole-Greer commented Sep 18, 2025

Uh oh!

andreachild Sep 22, 2025

Uh oh!

Cole-Greer Sep 23, 2025

Uh oh!

andreachild Sep 23, 2025 •

edited

Loading

Uh oh!

Cole-Greer Sep 23, 2025

Uh oh!

andreachild Sep 23, 2025

Uh oh!

Cole-Greer Sep 23, 2025

Uh oh!

andreachild Sep 23, 2025

Uh oh!

andreachild Sep 23, 2025

Uh oh!

andreachild Sep 23, 2025

Uh oh!

andreachild Sep 23, 2025

Uh oh!

andreachild Sep 23, 2025

Uh oh!

Uh oh!

Fix TraversalParent implementations for Placeholder steps #3209

Are you sure you want to change the base?

Fix TraversalParent implementations for Placeholder steps #3209

Conversation

Cole-Greer commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

spmallette commented Sep 18, 2025

Uh oh!

Cole-Greer commented Sep 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreachild Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Cole-Greer commented Sep 17, 2025 •

edited

Loading

andreachild Sep 23, 2025 •

edited

Loading