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 <[email protected]>
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();
}
}
}
