You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by co...@apache.org on 2025/06/17 02:38:07 UTC
(tinkerpop) branch GValueManager3.8-wip updated: WIP Start pinning GValues instead of removal
This is an automated email from the ASF dual-hosted git repository.
colegreer pushed a commit to branch GValueManager3.8-wip
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
The following commit(s) were added to refs/heads/GValueManager3.8-wip by this push:
new 4442e2e3b7 WIP Start pinning GValues instead of removal
4442e2e3b7 is described below
commit 4442e2e3b7a391530bd1154582ccc32065ef7e45
Author: Cole-Greer <Co...@improving.com>
AuthorDate: Mon Jun 16 19:37:52 2025 -0700
WIP Start pinning GValues instead of removal
---
.../strategy/decoration/VertexProgramStrategy.java | 3 +
.../gremlin/process/traversal/GValueManager.java | 86 ++++++++++++++++++++--
.../strategy/optimization/CountStrategy.java | 2 +-
.../strategy/optimization/EarlyLimitStrategy.java | 5 ++
.../optimization/InlineFilterStrategy.java | 5 +-
.../strategy/optimization/OrderLimitStrategy.java | 4 +-
.../optimization/PathProcessorStrategy.java | 4 +
.../optimization/RepeatUnrollStrategy.java | 2 +-
.../traversal/strategy/GValueManagerVerifier.java | 12 +++
.../strategy/optimization/CountStrategyTest.java | 4 +-
.../optimization/OrderLimitStrategyTest.java | 2 +-
11 files changed, 114 insertions(+), 15 deletions(-)
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/strategy/decoration/VertexProgramStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/strategy/decoration/VertexProgramStrategy.java
index d28f6047c8..0dbbcee2a7 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/strategy/decoration/VertexProgramStrategy.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/strategy/decoration/VertexProgramStrategy.java
@@ -70,6 +70,9 @@ public final class VertexProgramStrategy extends AbstractTraversalStrategy<Trave
@Override
public void apply(final Traversal.Admin<?, ?> traversal) {
+ // Don't track GValues in OLAP
+ traversal.getGValueManager().reset();
+
// VertexPrograms can only execute at the root level of a Traversal and should not be applied locally prior to RemoteStrategy
if (!(traversal.isRoot())
|| traversal instanceof AbstractLambdaTraversal
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/GValueManager.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/GValueManager.java
index d4781b6220..d383a5884c 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/GValueManager.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/GValueManager.java
@@ -38,6 +38,7 @@ import java.io.Serializable;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
+import java.util.HashMap;
import java.util.HashSet;
import java.util.IdentityHashMap;
import java.util.Map;
@@ -54,7 +55,8 @@ import java.util.stream.Collectors;
public final class GValueManager implements Serializable, Cloneable {
private boolean locked = false;
- private final Map<String, GValue> gValueRegistry = new IdentityHashMap();
+ private final Map<String, GValue> gValueRegistry = new HashMap<>();
+ private final Map<String, GValue> pinnedGValues = new HashMap<>();
private final Map<Step, StepContract> stepRegistry = new IdentityHashMap();
public GValueManager() {
@@ -122,10 +124,10 @@ public final class GValueManager implements Serializable, Cloneable {
* Copy the registry state from one step to another.
*/
public <S, E> void copyRegistryState(final Step<S,E> sourceStep, final Step<S,E> targetStep) {
- if (this.locked) {
- throw Traversal.Exceptions.traversalIsLocked();
- }
if (stepRegistry.containsKey(sourceStep)) {
+ if (this.locked) {
+ throw Traversal.Exceptions.traversalIsLocked();
+ }
// todo: oh boy - should has stuff even be handled this way?
// todo: is it ok that the StepContract can be tied to two different steps? like, can happen with Traversal.clone() where you gotta copy
@@ -194,6 +196,22 @@ public final class GValueManager implements Serializable, Cloneable {
}
}
+ /**
+ * Gets the set of variable names used in this traversal which have not been pinned to specific values. Note that
+ * this set won't be consistent until {@link #lock()} is called first. Calls to this method prior to locking will
+ * force iteration through traversal steps to real-time gather then variable names.
+ */
+ public Set<String> getUnpinnedVariableNames() {
+ Set<String> variableNames = new HashSet<>();
+ if (locked) {
+ variableNames.addAll(gValueRegistry.keySet());
+ } else {
+ variableNames.addAll(getGValues().stream().map(GValue::getName).collect(Collectors.toSet()));
+ }
+ variableNames.removeAll(pinnedGValues.keySet());
+ return Collections.unmodifiableSet(variableNames);
+ }
+
/**
* Gets the set of {@link GValue} objects used in this traversal. Note that this set won't be consistent until
* {@link #lock()} is called first. Calls to this method prior to locking will force iteration through traversal
@@ -234,6 +252,13 @@ public final class GValueManager implements Serializable, Cloneable {
return !getVariableNames().isEmpty();
}
+ /**
+ * Determines whether the manager has unpinned GValues.
+ */
+ public boolean hasUnpinnedVariables() {
+ return !getUnpinnedVariableNames().isEmpty();
+ }
+
/**
* Delete all data.
*/
@@ -254,13 +279,48 @@ public final class GValueManager implements Serializable, Cloneable {
if (this.locked) throw Traversal.Exceptions.traversalIsLocked();
final StepContract removedContract = stepRegistry.remove(step);
- if (removedContract != null) {
- removeGValues(extractGValues(removedContract));
- }
+ //TODO:: should we automatically pin GValues for removed steps?
return removedContract;
}
+ /**
+ * Pins the GValues associated with the step to their current values. This should be called anytime an optimization
+ * alters the Traversal based on the current value of a GValue. This indicates that the resulting traversal is only
+ * valid for this specific value of a GValue, and is not generalizable to any parameter value.
+ *
+ * @param step the {@link Step} in which to pin associated GValues
+ * @return the pinned contract or null if there was no mapping for the key
+ * @throws IllegalStateException if the manager is locked
+ */
+ public StepContract pinGValues(final Step step) {
+ if (this.locked) throw Traversal.Exceptions.traversalIsLocked();
+ final StepContract contract = stepRegistry.get(step);
+
+ if (contract != null) {
+ pinGValues(extractGValues(contract));
+ }
+ return contract;
+ }
+
+ /**
+ * Pins any GValue with the provided name. This should be called anytime an optimization
+ * alters the Traversal based on the current value of a GValue. This indicates that the resulting traversal is only
+ * valid for this specific value of a GValue, and is not generalizable to any parameter value.
+ *
+ * @param name the name of the GValue to be pinned
+ * @return true if name matches a registered GValue which was successfully pinned, false otherwise.
+ * @throws IllegalStateException if the manager is locked
+ */
+ public boolean pinVariable(final String name) {
+ if (this.locked) throw Traversal.Exceptions.traversalIsLocked();
+ if (!gValueRegistry.containsKey(name)) {
+ return false;
+ }
+ pinnedGValues.put(name, gValueRegistry.get(name));
+ return true;
+ }
+
@Override
public GValueManager clone() {
return new GValueManager(gValueRegistry, stepRegistry);
@@ -476,6 +536,18 @@ public final class GValueManager implements Serializable, Cloneable {
}
}
+ /**
+ * Pin a collection of {@link GValue} instances from the internal registry. Iteratively processes each
+ * {@link GValue} to pin it to its current value.
+ */
+ private void pinGValues(final Collection<GValue<?>> gValues) {
+ for (GValue<?> gValue : gValues) {
+ if (gValue.isVariable()) {
+ pinnedGValues.put(gValue.getName(), gValue);
+ }
+ }
+ }
+
/**
* Recursively collects mappings of {@link Step} to associated {@link GValue} instances from the given traversal
* and its child traversals. If a {@link Step} is parameterized, its associated {@link GValue}s are extracted
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategy.java
index 50bd0fdbdd..c12260f41e 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategy.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategy.java
@@ -154,7 +154,7 @@ public final class CountStrategy extends AbstractTraversalStrategy<TraversalStra
// this, we nullify the possibility of using that GValue for downstream optimizations as it
// would tie the dynamic GValue to a statically constructed literal (i.e. the highRange) that
// only applies for this current strategy application.
- traversal.getGValueManager().remove(isStep);
+ traversal.getGValueManager().pinGValues(isStep);
if (useNotStep || dismissCountIs) {
TraversalHelper.removeStep(isStep, traversal); // IsStep
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/EarlyLimitStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/EarlyLimitStrategy.java
index 9dee35a509..0437864a9f 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/EarlyLimitStrategy.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/EarlyLimitStrategy.java
@@ -132,6 +132,11 @@ public final class EarlyLimitStrategy
}
remove = merge;
+ // optimization is dependent on the high and low values from both `step` and `other`. Both must have their
+ // GValues pinned as a result.
+ traversal.getGValueManager().pinGValues(step);
+ traversal.getGValueManager().pinGValues(other);
+
// a step is being permanently removed - remove from manager
final Step stepToRemove = merge ? insertAfter : step;
traversal.getGValueManager().remove(stepToRemove);
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategy.java
index c9d0edaccf..fd26fc8dca 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategy.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategy.java
@@ -257,7 +257,10 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver
if (process) {
final HasStep hasStep = new HasStep<>(traversal, new HasContainer(key, predicate));
TraversalHelper.replaceStep(step, hasStep, traversal);
- stepsToRemoveFromManager.forEach(s -> traversal.getGValueManager().remove(s));
+ stepsToRemoveFromManager.forEach(s -> {
+ traversal.getGValueManager().pinGValues(s);
+ traversal.getGValueManager().remove(s);
+ });
traversal.getGValueManager().register(hasStep, hasStep);
TraversalHelper.copyLabels(step, hasStep, false);
for (final String label : labels) {
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/OrderLimitStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/OrderLimitStrategy.java
index 31a64ac8d0..28cd007159 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/OrderLimitStrategy.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/OrderLimitStrategy.java
@@ -99,8 +99,8 @@ public final class OrderLimitStrategy extends AbstractTraversalStrategy<Traversa
if (null != range) {
order.setLimit(range.getHighRange());
- // remove GValueManager state because the step contract value has been used to set up order()
- traversal.getGValueManager().remove(range);
+ // pin GValues because the step contract value has been used to set up order()
+ traversal.getGValueManager().pinGValues(range);
}
}
}
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathProcessorStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathProcessorStrategy.java
index 766a087df1..4c76998a9b 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathProcessorStrategy.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/PathProcessorStrategy.java
@@ -168,7 +168,9 @@ public final class PathProcessorStrategy extends AbstractTraversalStrategy<Trave
final List<Step> byChildSteps = TraversalHelper.getStepsOfAssignableClassRecursively(Step.class, byTraversal);
for (int ix = 0; ix < byChildSteps.size(); ix++) {
manager.copyRegistryState(byChildSteps.get(ix), clonedByTraversal.getSteps().get(ix));
+ manager.pinGValues(byChildSteps.get(ix));
manager.remove(byChildSteps.get(ix));
+ clonedByTraversal.getGValueManager().pinGValues(byChildSteps.get(ix));
clonedByTraversal.getGValueManager().remove(byChildSteps.get(ix));
}
@@ -198,7 +200,9 @@ public final class PathProcessorStrategy extends AbstractTraversalStrategy<Trave
manager.copyRegistryState(localChildSteps.get(ix), localChildClone.getSteps().get(ix));
// todo: look at cases where we clone() traversal more carefully. so much to think about in these cases
+ manager.pinGValues(localChildSteps.get(ix));
manager.remove(localChildSteps.get(ix));
+ localChildClone.getGValueManager().pinGValues(localChildSteps.get(ix));
localChildClone.getGValueManager().remove(localChildSteps.get(ix));
}
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RepeatUnrollStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RepeatUnrollStrategy.java
index 0a4d0c285c..46a013c823 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RepeatUnrollStrategy.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/RepeatUnrollStrategy.java
@@ -100,7 +100,7 @@ public final class RepeatUnrollStrategy extends AbstractTraversalStrategy<Traver
manager.copyRegistryState(repeatSteps.get(ix), repeatClone.getSteps().get(ix));
// todo: PathProcessorStrategy showed some state can hang about in the manager - really need to investigate clone more
- repeatClone.getGValueManager().remove(repeatSteps.get(ix));
+ repeatClone.getGValueManager().remove(repeatSteps.get(ix)); // don't need to pin GValues as RepeatUnroll isn't based on specific GValue Values
}
TraversalHelper.insertTraversal(insertIndex, repeatClone, traversal);
diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/GValueManagerVerifier.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/GValueManagerVerifier.java
index 45564422e9..8e2dd0a386 100644
--- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/GValueManagerVerifier.java
+++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/GValueManagerVerifier.java
@@ -303,6 +303,18 @@ public class GValueManagerVerifier {
return self();
}
+ /**
+ * Verifies that and GValues in GValueManager have been pinned to indicate that optimizations have been applied
+ * which cannot be generalized for any parameter values. Pinning of GValues only applies to variables. There
+ * might yet be unnamed {@link GValue} instances which are unpinned, but since we don't bind to those and they
+ * are usually incident to something like {@code V(1, x=2, 3)} where one variable implies that all arguments
+ * must be converted to {@link GValue} you can have scenarios where there is a mix.
+ */
+ public T allGValuesArePinned() {
+ assertThat(String.format("GValueManager only contain pinned GValues but [%s] is unpinned", manager.getUnpinnedVariableNames()), manager.hasUnpinnedVariables(), is(false));
+ return self();
+ }
+
/**
* Verifies that specific steps have been cleared from GValueManager
*/
diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategyTest.java
index cc299bd177..7e10f0bdf5 100644
--- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategyTest.java
+++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategyTest.java
@@ -181,7 +181,7 @@ public class CountStrategyTest {
* instances are being introduced by CountStrategy.
*/
@RunWith(Parameterized.class)
- public static class GValueClearedBecauseItWasAccessedToInformTest {
+ public static class GValuePinnedBecauseItWasAccessedToInformTest {
@Parameterized.Parameter(value = 0)
public Traversal.Admin<?, ?> traversal;
@@ -300,7 +300,7 @@ public class CountStrategyTest {
beforeApplying().
stepsOfClassAreParameterized(true, IsStep.class).
afterApplying().
- managerIsEmpty();
+ allGValuesArePinned();
}
}
diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/OrderLimitStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/OrderLimitStrategyTest.java
index f0a975b173..1f5d71ebf2 100644
--- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/OrderLimitStrategyTest.java
+++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/OrderLimitStrategyTest.java
@@ -120,7 +120,7 @@ public class OrderLimitStrategyTest {
beforeApplying().
hasVariables(expectedNames).
afterApplying().
- managerIsEmpty();
+ allGValuesArePinned();
}
}
}