This is an automated email from the ASF dual-hosted git repository. colegreer pushed a commit to branch fixTraversalBoxingInPlaceholders in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 7364aa64c333ffd845a7d858b670102e0d8f3499 Author: Cole-Greer <[email protected]> AuthorDate: Fri Sep 26 12:21:58 2025 -0700 CTR avoid boxing property values as Traversals in AddPropertyStepPlaceholder --- .../step/sideEffect/AddPropertyStepContract.java | 10 ++-- .../sideEffect/AddPropertyStepPlaceholder.java | 60 ++++++++++------------ .../traversal/step/TraversalParentTest.java | 18 +++---- 3 files changed, 41 insertions(+), 47 deletions(-) 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 af4587eef4..143c684beb 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,7 +19,6 @@ 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; @@ -48,15 +47,14 @@ public interface AddPropertyStepContract<S> extends Step<S, S>, TraversalParent, Object getKey(); /** - * 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 value is returned in Traversal form. + * Gets the property value. If the value was originally passed as a {@link GValue<?>}, the variable will become pinned + * and the literal value returned. */ Object getValue(); /** - * 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 value is returned in Traversal form. + * Gets the property value. If the value was originally passed as a {@link GValue<?>} it is returned as-is, without + * pinning the variable. */ default Object getValueWithGValue() { return getValue(); 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 c1835b0b9d..7818b85a2a 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 @@ -20,11 +20,8 @@ package org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect; 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.lambda.GValueConstantTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.GValue; import org.apache.tinkerpop.gremlin.process.traversal.step.GValueHolder; -import org.apache.tinkerpop.gremlin.process.traversal.step.util.AbstractStep; import org.apache.tinkerpop.gremlin.process.traversal.step.util.GValueHelper; import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.CallbackRegistry; import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.Event; @@ -39,7 +36,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.NoSuchElementException; import java.util.Objects; import java.util.Set; @@ -53,7 +49,7 @@ public class AddPropertyStepPlaceholder<S extends Element> extends SideEffectSte /** * property value */ - private Traversal.Admin<?, ?> value; + private Object value; /** * cardinality of the property */ @@ -74,13 +70,11 @@ public class AddPropertyStepPlaceholder<S extends Element> extends SideEffectSte } if (valueObject instanceof GValue) { traversal.getGValueManager().register((GValue<?>) valueObject); - this.value = new GValueConstantTraversal<>((GValue<?>) valueObject); - } else if (valueObject instanceof Traversal) { - this.value = ((Traversal<?, ?>) valueObject).asAdmin(); - } else { - this.value = new ConstantTraversal<>(valueObject); } - this.integrateChild(this.value); + this.value = valueObject; + if (this.value instanceof Traversal) { + this.integrateChild(((Traversal<?, ?>) this.value).asAdmin()); + } this.cardinality = cardinality; } @@ -105,8 +99,8 @@ public class AddPropertyStepPlaceholder<S extends Element> extends SideEffectSte if (key != null && key instanceof Traversal) { childTraversals.add(((Traversal<?, ?>) key).asAdmin()); } - if (value != null) { - childTraversals.add(value); + if (value != null && value instanceof Traversal) { + childTraversals.add(((Traversal<?, ?>) value).asAdmin()); } return childTraversals; } @@ -147,34 +141,27 @@ public class AddPropertyStepPlaceholder<S extends Element> extends SideEffectSte if (value == null) { return null; } - if (value instanceof GValueConstantTraversal) { - traversal.getGValueManager().pinVariable(((GValueConstantTraversal<?, ?>) value).getGValue().getName()); - return value.next(); - } - if (value instanceof ConstantTraversal) { - return value.next(); + if (value instanceof GValue) { + traversal.getGValueManager().pinVariable(((GValue<?>) value).getName()); + return ((GValue<?>) value).get(); } return value; } /** - * Get the value as a GValue, without pinning the variable + * Get the value as-is, without unboxing GValues if present, and without pinning the variable */ @Override public Object getValueWithGValue() { - if (value instanceof GValueConstantTraversal) { - return ((GValueConstantTraversal<?, ?>) value).getGValue(); - } - return getValue(); // Don't need to worry about pinning variable as GValue case is already covered + return value; } @Override public AddPropertyStepPlaceholder<S> clone() { final AddPropertyStepPlaceholder<S> clone = (AddPropertyStepPlaceholder<S>) super.clone(); clone.cardinality = cardinality; - clone.value = value.clone(); - // Attempt to deep clone keys for Traversal and GValue. Shallow copy is fine if key is a String or enum + // 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) { clone.key = ((Traversal<?, ?>) this.key).asAdmin().clone(); } else if (this.key instanceof GValue) { @@ -183,6 +170,15 @@ public class AddPropertyStepPlaceholder<S extends Element> extends SideEffectSte clone.key = this.key; } + // Attempt to deep clone value for Traversal and GValue. + if (this.value instanceof Traversal) { + clone.value = ((Traversal<?, ?>) this.value).asAdmin().clone(); + } else if (this.value instanceof GValue) { + clone.value = ((GValue<?>) this.value).clone(); + } else { + clone.value = this.value; + } + // deep clone properties clone.properties = new HashMap<>(); for (Map.Entry<Object, List<Object>> entry : this.properties.entrySet()) { @@ -206,7 +202,7 @@ public class AddPropertyStepPlaceholder<S extends Element> extends SideEffectSte @Override public AddPropertyStep<S> asConcreteStep() { - AddPropertyStep<S> step = new AddPropertyStep<>(traversal, cardinality, key, value instanceof GValueConstantTraversal ? ((GValueConstantTraversal<?, ?>) value).getConstantTraversal() : value); + AddPropertyStep<S> step = new AddPropertyStep<>(traversal, cardinality, key, value instanceof GValue ? ((GValue<?>) value).get() : value); for (final Map.Entry<Object, List<Object>> entry : properties.entrySet()) { for (Object value : entry.getValue()) { @@ -220,7 +216,7 @@ public class AddPropertyStepPlaceholder<S extends Element> extends SideEffectSte @Override public boolean isParameterized() { - if (value instanceof GValueConstantTraversal && ((GValueConstantTraversal<?, ?>) value).isParameterized()) { + if (value instanceof GValue && ((GValue<?>) value).isVariable()) { return true; } for (List<Object> list : properties.values()) { @@ -233,8 +229,8 @@ public class AddPropertyStepPlaceholder<S extends Element> extends SideEffectSte @Override public void updateVariable(String name, Object value) { - if (value instanceof GValueConstantTraversal && name.equals(((GValueConstantTraversal<?, ?>) value).getGValue().getName())) { - this.value = new GValueConstantTraversal<>(GValue.of(name, value)); + if (this.value instanceof GValue && name.equals(((GValue<?>) this.value).getName())) { + this.value = GValue.of(name, value); } for (final Map.Entry<Object, List<Object>> entry : properties.entrySet()) { for (final Object propertyVal : entry.getValue()) { @@ -248,8 +244,8 @@ public class AddPropertyStepPlaceholder<S extends Element> extends SideEffectSte @Override public Collection<GValue<?>> getGValues() { Set<GValue<?>> gValues = GValueHelper.getGValuesFromProperties(properties); - if (value instanceof GValueConstantTraversal && ((GValueConstantTraversal<?, ?>) value).isParameterized()) { - gValues.add(((GValueConstantTraversal<?, ?>) value).getGValue()); + if (value instanceof GValue && ((GValue<?>) value).isVariable()) { + gValues.add((GValue<?>) value); } return gValues; } diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/TraversalParentTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/TraversalParentTest.java index d251da52a1..f209b11ac5 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/TraversalParentTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/TraversalParentTest.java @@ -291,7 +291,7 @@ public class TraversalParentTest { {AddPropertyStepContract.class, g.inject(1).property("key", "value"), List.of(), - List.of(new ConstantTraversal<>("value")), // constant Value's are internally boxed in Traversals + List.of(), null, null }, {AddPropertyStepContract.class, @@ -309,7 +309,7 @@ public class TraversalParentTest { {AddPropertyStepContract.class, g.addV().property("name", "value", "metaKey", "metaValue"), List.of(), - List.of(new ConstantTraversal<>("value")), // constant Value's are internally boxed in Traversals + List.of(), null, null }, {AddPropertyStepContract.class, @@ -327,7 +327,7 @@ public class TraversalParentTest { {AddPropertyStepContract.class, g.addV().property("name", "value", "metaKey1", "metaValue1", "metaKey2", "metaValue2"), List.of(), - List.of(new ConstantTraversal<>("value")), // constant Value's are internally boxed in Traversals + List.of(), null, null }, {MergeStepContract.class, @@ -982,23 +982,23 @@ public class TraversalParentTest { private void verifyExpectedParents(List<Traversal.Admin<?,?>> expectedGlobalChildren, List<Traversal.Admin<?,?>> expectedLocalChildren, String messageSuffix) { List<Step<?,?>> steps = TraversalHelper.getStepsOfAssignableClass(stepClass, traversal); - assertTrue(String.format("Expected a single step of class %s to test, found %d %s", stepClass.getName(), steps.size(), messageSuffix), steps.size() == 1); - assertTrue(String.format("Expected step to implement TraversalParent %s", messageSuffix), steps.get(0) instanceof TraversalParent); + assertTrue(String.format("Expected a single step of class %s to test, found %d for Traversal [%s] %s", stepClass.getName(), steps.size(), traversal, messageSuffix), steps.size() == 1); + assertTrue(String.format("Expected step to implement TraversalParent for Traversal [%s] %s", traversal, messageSuffix), steps.get(0) instanceof TraversalParent); TraversalParent testStep = (TraversalParent) steps.get(0); - assertThat(String.format("getGlobalChildren() did not produce the expected results %s", messageSuffix), testStep.getGlobalChildren(), containsInAnyOrder(expectedGlobalChildren.toArray())); - assertThat(String.format("getLocalChildren() did not produce the expected results %s", messageSuffix), testStep.getLocalChildren(), containsInAnyOrder(expectedLocalChildren.toArray())); + assertThat(String.format("getGlobalChildren() did not produce the expected results for Traversal [%s] %s", traversal, messageSuffix), testStep.getGlobalChildren(), containsInAnyOrder(expectedGlobalChildren.toArray())); + assertThat(String.format("getLocalChildren() did not produce the expected results for Traversal [%s] %s", traversal, messageSuffix), testStep.getLocalChildren(), containsInAnyOrder(expectedLocalChildren.toArray())); for (Traversal.Admin<?, ?> child : testStep.getGlobalChildren()) { if (!(child instanceof AbstractLambdaTraversal)) { // Use reference equality to ensure parent points to the correct object. It's not enough that the parents contents match. - assertTrue(String.format("Expected child traversal %s to have correctly assigned parent %s", child.toString(), messageSuffix), testStep == child.getParent()); + assertTrue(String.format("Expected child traversal %s to have correctly assigned parent for Traversal [%s] %s", child.toString(), traversal, messageSuffix), testStep == child.getParent()); } } for (Traversal.Admin<?, ?> child : testStep.getLocalChildren()) { if (!(child instanceof AbstractLambdaTraversal)) { // Use reference equality to ensure parent points to the correct object. It's not enough that the parents contents match. - assertTrue(String.format("Expected child traversal %s to have correctly assigned parent %s", child.toString(), messageSuffix), testStep == child.getParent()); + assertTrue(String.format("Expected child traversal %s to have correctly assigned parent for Traversal [%s] %s", child.toString(), traversal, messageSuffix), testStep == child.getParent()); } } }
