This is an automated email from the ASF dual-hosted git repository. spmallette pushed a commit to branch GValueManager3.8-wip in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 1d344550b9ff02beaaa218a56b78e5fb2fc5bf49 Author: Stephen Mallette <[email protected]> AuthorDate: Wed Jun 11 09:58:41 2025 -0400 wip - more tests --- .../gremlin/process/traversal/GValueManager.java | 22 +- .../tinkerpop/gremlin/process/traversal/P.java | 2 +- .../optimization/InlineFilterStrategy.java | 69 +++++-- .../process/traversal/util/TraversalHelper.java | 2 +- .../traversal/strategy/GValueManagerVerifier.java | 26 ++- .../optimization/InlineFilterStrategyTest.java | 227 +++++++++++++-------- 6 files changed, 238 insertions(+), 110 deletions(-) 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 a9483c95af..5f98ec4f38 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 @@ -32,6 +32,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.stepContract.Predicat import org.apache.tinkerpop.gremlin.process.traversal.step.stepContract.RangeContract; import org.apache.tinkerpop.gremlin.process.traversal.step.stepContract.StepContract; import org.apache.tinkerpop.gremlin.process.traversal.step.stepContract.TailContract; +import org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer; import org.apache.tinkerpop.gremlin.structure.Vertex; import java.io.Serializable; @@ -120,7 +121,17 @@ public final class GValueManager implements Serializable, Cloneable { throw Traversal.Exceptions.traversalIsLocked(); } if (stepRegistry.containsKey(sourceStep)) { - stepRegistry.put(targetStep, stepRegistry.get(sourceStep)); + // todo: oh boy + // gotta merge HasContainerHolder since it already exists + if (targetStep instanceof HasContainerHolder && stepRegistry.containsKey(targetStep) && + sourceStep instanceof HasContainerHolder) { + final HasContainerHolder targetHasContainer = (HasContainerHolder) stepRegistry.get(targetStep); + final HasContainerHolder sourceHasContainer = (HasContainerHolder) stepRegistry.get(sourceStep); + sourceHasContainer.getHasContainers().forEach(targetHasContainer::addHasContainer); + stepRegistry.put(targetStep, targetHasContainer); + } else { + stepRegistry.put(targetStep, stepRegistry.get(sourceStep)); + } } } @@ -133,6 +144,11 @@ public final class GValueManager implements Serializable, Cloneable { return (T) stepRegistry.get(step); } + public Collection<GValue<?>> getGValues(final Step step) { + if (!isParameterized(step)) return Collections.emptyList(); + return extractGValues(getStepContract(step)); + } + /** * Determines whether the given step is parameterized by checking its presence * in the step registry or the predicate registry. @@ -145,6 +161,10 @@ public final class GValueManager implements Serializable, Cloneable { return this.stepRegistry.containsKey(step); } + public Set<Step> getSteps() { + return Collections.unmodifiableSet(stepRegistry.keySet()); + } + /** * Gets the set of variable names 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 diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java index 6dee62840d..e6fa1d65d9 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java @@ -306,7 +306,7 @@ public class P<V> implements Predicate<V>, Serializable, Cloneable { return gValueRegistry; } - protected class GValueRegistry<T> implements Serializable { + public class GValueRegistry<T> implements Serializable { private Map<P, GValue<T>> gValueRegistry = new IdentityHashMap<>(); public GValueRegistry() {} 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 d2713218c5..c9d0edaccf 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 @@ -22,10 +22,12 @@ package org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization; import org.apache.tinkerpop.gremlin.process.computer.traversal.strategy.optimization.GraphFilterStrategy; import org.apache.tinkerpop.gremlin.process.traversal.Compare; import org.apache.tinkerpop.gremlin.process.traversal.Contains; +import org.apache.tinkerpop.gremlin.process.traversal.GValueManager; import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Step; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; +import org.apache.tinkerpop.gremlin.process.traversal.step.GValue; import org.apache.tinkerpop.gremlin.process.traversal.step.LambdaHolder; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.AndStep; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.DedupGlobalStep; @@ -37,6 +39,8 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.filter.RangeGlobalSte import org.apache.tinkerpop.gremlin.process.traversal.step.filter.TraversalFilterStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.MatchStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.VertexStep; +import org.apache.tinkerpop.gremlin.process.traversal.step.stepContract.DefaultEdgeLabelContract; +import org.apache.tinkerpop.gremlin.process.traversal.step.stepContract.StepContract; import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyStep; import org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer; import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy; @@ -92,15 +96,15 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver final Iterator<FilterStep> filterStepIterator = TraversalHelper.getStepsOfAssignableClass(FilterStep.class, traversal).iterator(); while (!changed && filterStepIterator.hasNext()) { final FilterStep<?> step = filterStepIterator.next(); - changed = step instanceof HasStep && InlineFilterStrategy.processHasStep((HasStep) step, traversal) || - step instanceof TraversalFilterStep && InlineFilterStrategy.processTraversalFilterStep((TraversalFilterStep) step, traversal) || - step instanceof OrStep && InlineFilterStrategy.processOrStep((OrStep) step, traversal) || - step instanceof AndStep && InlineFilterStrategy.processAndStep((AndStep) step, traversal); + changed = step instanceof HasStep && processHasStep((HasStep) step, traversal) || + step instanceof TraversalFilterStep && processTraversalFilterStep((TraversalFilterStep) step, traversal) || + step instanceof OrStep && processOrStep((OrStep) step, traversal) || + step instanceof AndStep && processAndStep((AndStep) step, traversal); } if (!changed && traversal.isRoot()) { final Iterator<MatchStep> matchStepIterator = TraversalHelper.getStepsOfClass(MatchStep.class, traversal).iterator(); while (!changed && matchStepIterator.hasNext()) { - if (InlineFilterStrategy.processMatchStep(matchStepIterator.next(), traversal)) + if (processMatchStep(matchStepIterator.next(), traversal)) changed = true; } } @@ -111,20 +115,26 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver /////////////////////////// private static boolean processHasStep(final HasStep<?> step, final Traversal.Admin<?, ?> traversal) { + final GValueManager manager = traversal.getGValueManager(); if (step.getPreviousStep() instanceof HasStep) { final HasStep<?> previousStep = (HasStep<?>) step.getPreviousStep(); final List<HasContainer> hasContainers = new ArrayList<>(step.getHasContainers()); for (final HasContainer hasContainer : hasContainers) { previousStep.addHasContainer(hasContainer); } + + // merged has() steps so GValue state from later has() steps must be merged into the previous has() + manager.copyRegistryState((Step) step, previousStep); + TraversalHelper.copyLabels(step, previousStep, false); - traversal.removeStep(step); + TraversalHelper.removeStep(step, traversal); return true; } else if (step.getPreviousStep() instanceof VertexStep && ((VertexStep) step.getPreviousStep()).returnsEdge() && 0 == ((VertexStep) step.getPreviousStep()).getEdgeLabels().length) { final VertexStep<Edge> previousStep = (VertexStep<Edge>) step.getPreviousStep(); final List<String> edgeLabels = new ArrayList<>(); + final List<GValue> edgeLabelGValues = new ArrayList<>(); for (final HasContainer hasContainer : new ArrayList<>(step.getHasContainers())) { if (hasContainer.getKey().equals(T.label.getAccessor())) { if (hasContainer.getBiPredicate() == Compare.eq && @@ -132,19 +142,28 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver edgeLabels.isEmpty()) { edgeLabels.add((String) hasContainer.getValue()); step.removeHasContainer(hasContainer); + if (hasContainer.getPredicate().isParameterized()) { + edgeLabelGValues.addAll(hasContainer.getPredicate().getGValueRegistry().getGValues()); + } } else if (hasContainer.getBiPredicate() == Contains.within && hasContainer.getValue() instanceof Collection && ((Collection) hasContainer.getValue()).containsAll(edgeLabels)) { edgeLabels.addAll((Collection<String>) hasContainer.getValue()); step.removeHasContainer(hasContainer); + if (hasContainer.getPredicate().isParameterized()) { + edgeLabelGValues.addAll(hasContainer.getPredicate().getGValueRegistry().getGValues()); + } } else if (hasContainer.getPredicate() instanceof OrP && edgeLabels.isEmpty()) { boolean removeContainer = true; final List<P<?>> orps = ((OrP) hasContainer.getPredicate()).getPredicates(); final List<String> newEdges = new ArrayList<>(); for (int i = 0; i < orps.size(); i++) { - if (orps.get(i).getBiPredicate() == Compare.eq && orps.get(i).getValue() instanceof String) + if (orps.get(i).getBiPredicate() == Compare.eq && orps.get(i).getValue() instanceof String) { newEdges.add((String) orps.get(i).getValue()); - else { + if (hasContainer.getPredicate().isParameterized()) { + edgeLabelGValues.addAll(hasContainer.getPredicate().getGValueRegistry().getGValues()); + } + } else { removeContainer = false; break; } @@ -162,7 +181,13 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver TraversalHelper.copyLabels(previousStep, newVertexStep, false); if (step.getHasContainers().isEmpty()) { TraversalHelper.copyLabels(step, newVertexStep, false); - traversal.removeStep(step); + TraversalHelper.removeStep(step, traversal); + } + + if (!edgeLabelGValues.isEmpty()) { + final GValue<String>[] edgeLabelGValuesArray = edgeLabelGValues.stream().toArray(GValue[]::new); + final StepContract contract = new DefaultEdgeLabelContract<>(edgeLabelGValuesArray); + manager.register(newVertexStep, contract); } return true; } @@ -171,7 +196,7 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver return false; } - private static final boolean processTraversalFilterStep(final TraversalFilterStep<?> step, final Traversal.Admin<?, ?> traversal) { + private static boolean processTraversalFilterStep(final TraversalFilterStep<?> step, final Traversal.Admin<?, ?> traversal) { final Traversal.Admin<?, ?> childTraversal = step.getLocalChildren().get(0); if (TraversalHelper.hasAllStepsOfClass(childTraversal, FilterStep.class) && !TraversalHelper.hasStepOfClass(childTraversal, @@ -182,19 +207,26 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver final Step<?, ?> finalStep = childTraversal.getEndStep(); TraversalHelper.insertTraversal((Step) step, childTraversal, traversal); TraversalHelper.copyLabels(step, finalStep, false); - traversal.removeStep(step); + TraversalHelper.removeStep(step, traversal); return true; } return false; } - private static final boolean processOrStep(final OrStep<?> step, final Traversal.Admin<?, ?> traversal) { + /** + * Processes an {@link OrStep} within a traversal and attempts to optimize it by replacing it with + * simpler constructs if possible. The method evaluates the local child traversals of the provided + * {@code OrStep} and determines if the {@code OrStep} can be simplified into a single step such as + * a {@link HasStep}. + */ + private static boolean processOrStep(final OrStep<?> step, final Traversal.Admin<?, ?> traversal) { boolean process = true; String key = null; P predicate = null; final List<String> labels = new ArrayList<>(); + final List<Step> stepsToRemoveFromManager = new ArrayList<>(); for (final Traversal.Admin<?, ?> childTraversal : step.getLocalChildren()) { - InlineFilterStrategy.instance().apply(childTraversal); // todo: this may be a bad idea, but I can't seem to find a test case to break it + InlineFilterStrategy.instance().apply(childTraversal); for (final Step<?, ?> childStep : childTraversal.getSteps()) { if (childStep instanceof HasStep) { P p = null; @@ -213,6 +245,7 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver predicate = null == predicate ? p : predicate.or(p); } labels.addAll(childStep.getLabels()); + stepsToRemoveFromManager.add(childStep); } else { process = false; break; @@ -224,6 +257,8 @@ 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)); + traversal.getGValueManager().register(hasStep, hasStep); TraversalHelper.copyLabels(step, hasStep, false); for (final String label : labels) { hasStep.addLabel(label); @@ -233,7 +268,7 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver return false; } - private static final boolean processAndStep(final AndStep<?> step, final Traversal.Admin<?, ?> traversal) { + private static boolean processAndStep(final AndStep<?> step, final Traversal.Admin<?, ?> traversal) { boolean process = true; for (final Traversal.Admin<?, ?> childTraversal : step.getLocalChildren()) { if (!TraversalHelper.hasAllStepsOfClass(childTraversal, FilterStep.class) || @@ -257,13 +292,13 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver } if (null != finalStep) TraversalHelper.copyLabels(step, finalStep, false); - traversal.removeStep(step); + TraversalHelper.removeStep(step, traversal); return true; } return false; } - private static final boolean processMatchStep(final MatchStep<?, ?> step, final Traversal.Admin<?, ?> traversal) { + private static boolean processMatchStep(final MatchStep<?, ?> step, final Traversal.Admin<?, ?> traversal) { if (step.getPreviousStep() instanceof EmptyStep) return false; boolean changed = false; @@ -286,7 +321,7 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver } } if (step.getGlobalChildren().isEmpty()) - traversal.removeStep(step); + TraversalHelper.removeStep(step, traversal); return changed; } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java index aefada9d8b..6f9952ee41 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/TraversalHelper.java @@ -175,7 +175,7 @@ public final class TraversalHelper { } /** - * Replace a step with a new step. + * Replace a step with a new step. When a step is replaced, it is also removed from the {@link GValueManager}. * * @param removeStep the step to remove * @param insertStep the step to insert 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 3989d3cadf..0053e29937 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 @@ -26,6 +26,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.step.GValue; import org.apache.tinkerpop.gremlin.process.traversal.step.stepContract.EdgeLabelContract; import org.apache.tinkerpop.gremlin.process.traversal.step.stepContract.RangeContract; +import org.apache.tinkerpop.gremlin.process.traversal.step.stepContract.StepContract; import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.FilterRankingStrategy; import org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversalStrategies; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper; @@ -70,7 +71,7 @@ public class GValueManagerVerifier { } else { // If there are additional strategies, combine them with one provided strategies = new TraversalStrategy[additionalStrategies.size() + 1]; - strategies[0] = FilterRankingStrategy.instance(); + strategies[0] = strategy; int i = 1; for (TraversalStrategy ts : additionalStrategies) { @@ -98,7 +99,9 @@ public class GValueManagerVerifier { } /** - * Applies the strategies and returns the verifier + * Applies the strategies and returns the verifier while ensuring that the step state is consistent where + * every step in the manager is in the traversal and every step that is a parameterized {@link StepContract} + * is in the manager. */ public AfterVerifier<S, E> afterApplying() { // Capture pre-strategy state @@ -114,9 +117,28 @@ public class GValueManagerVerifier { } traversal.setStrategies(traversalStrategies); traversal.applyStrategies(); + + verifyStateIsConsistent(); return new AfterVerifier<>(traversal, preVariables, preStepVariables, preGValues); } + + /** + * Verifies that the manager is not referencing any steps that aren't in the traversal after strategy + * application. + */ + private void verifyStateIsConsistent() { + // Get all steps in the traversal including nested ones + final List<Step> allSteps = TraversalHelper.getStepsOfAssignableClassRecursively(Step.class, traversal); + final Set<Step> allStepsSet = new java.util.HashSet<>(allSteps); + final GValueManager manager = traversal.getGValueManager(); + + // Check that manager only references valid steps + for (Step step : manager.getSteps()) { + assertThat("GValueManager references step not in traversal", + allStepsSet.contains(step), is(true)); + } + } } /** diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategyTest.java index 7596f49dc8..a58084dc91 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategyTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategyTest.java @@ -19,6 +19,7 @@ package org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization; +import org.apache.tinkerpop.gremlin.process.traversal.GValueManager; import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.Translator; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; @@ -26,8 +27,10 @@ import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategies; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; +import org.apache.tinkerpop.gremlin.process.traversal.step.GValue; import org.apache.tinkerpop.gremlin.process.traversal.step.filter.HasStep; import org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer; +import org.apache.tinkerpop.gremlin.process.traversal.strategy.GValueManagerVerifier; import org.apache.tinkerpop.gremlin.process.traversal.strategy.decoration.ConnectiveStrategy; import org.apache.tinkerpop.gremlin.process.traversal.translator.GroovyTranslator; import org.apache.tinkerpop.gremlin.process.traversal.util.DefaultTraversalStrategies; @@ -35,6 +38,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.util.EmptyTraversal; import org.apache.tinkerpop.gremlin.structure.T; import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyGraph; import org.junit.Test; +import org.junit.experimental.runners.Enclosed; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -65,105 +69,152 @@ import static org.junit.Assert.assertEquals; /** * @author Marko A. Rodriguez (http://markorodriguez.com) */ -@RunWith(Parameterized.class) +@RunWith(Enclosed.class) public class InlineFilterStrategyTest { private static final Translator.ScriptTranslator translator = GroovyTranslator.of("__"); - @Parameterized.Parameter(value = 0) - public Traversal.Admin original; + @RunWith(Parameterized.class) + public static class StandardTest { + @Parameterized.Parameter(value = 0) + public Traversal.Admin original; - @Parameterized.Parameter(value = 1) - public Traversal optimized; + @Parameterized.Parameter(value = 1) + public Traversal optimized; - @Parameterized.Parameter(value = 2) - public List<TraversalStrategy> extras; + @Parameterized.Parameter(value = 2) + public List<TraversalStrategy> extras; - @Test - public void doTest() { - final String repr = translator.translate(original.getBytecode()).getScript(); - final TraversalStrategies strategies = new DefaultTraversalStrategies(); - strategies.addStrategies(InlineFilterStrategy.instance()); + @Parameterized.Parameters(name = "{0}") + public static Iterable<Object[]> generateTestParameters() { - extras.forEach(strategies::addStrategies); + return Arrays.asList(new Object[][]{ + {has("age", 10).as("a").has("name", "marko").as("b").coin(0.5).as("c"), addHas(__.start(), "age", eq(10), "name", eq("marko")).as("a", "b").coin(0.5).as("c"), Collections.emptyList()}, + // + {filter(out("knows")), filter(out("knows")), Collections.emptyList()}, + {filter(has("age", gt(10))).as("a"), has("age", gt(10)).as("a"), Collections.emptyList()}, + {filter(has("age", gt(10)).as("b")).as("a"), has("age", gt(10)).as("b", "a"), Collections.emptyList()}, + {filter(has("age", gt(10)).as("a")), has("age", gt(10)).as("a"), Collections.emptyList()}, + {filter(and(has("age", gt(10)).as("a"), has("name", "marko"))), addHas(__.start(), "age", gt(10), "name", eq("marko")).as("a"), Collections.emptyList()}, + {filter(out("created").and().out("knows").or().in("knows")), or(and(out("created"), out("knows")), __.in("knows")), Collections.singletonList(ConnectiveStrategy.instance())}, + {filter(out("created").and().out("knows").or().in("knows")).and(hasLabel("person")), or(and(out("created"), out("knows")), __.in("knows")).hasLabel("person"), Collections.singletonList(ConnectiveStrategy.instance())}, + // + {or(has("name", "marko"), has("age", 32)), or(has("name", "marko"), has("age", 32)), Collections.emptyList()}, + {or(has("name", "marko"), has("name", "bob")), has("name", eq("marko").or(eq("bob"))), Collections.emptyList()}, + {or(has("name", "marko"), has("name")), or(has("name", "marko"), has("name")), Collections.emptyList()}, + {or(has("age", 10), and(has("age", gt(20)), has("age", lt(100)))), has("age", eq(10).or(gt(20).and(lt(100)))), Collections.emptyList()}, + {or(has("name", "marko"), filter(has("name", "bob"))), has("name", eq("marko").or(eq("bob"))), Collections.emptyList()}, + {or(has("name", "marko"), filter(or(filter(has("name", "bob")), has("name", "stephen")))), has("name", eq("marko").or(eq("bob").or(eq("stephen")))), Collections.emptyList()}, + {or(has("name", "marko").as("a"), filter(or(filter(has("name", "bob")).as("b"), has("name", "stephen").as("c")))), has("name", eq("marko").or(eq("bob").or(eq("stephen")))).as("a", "b", "c"), Collections.emptyList()}, + {or(and(has("age",gt(20)), has("age",lt(30))), and(has("age",gt(35)), has("age",lt(40)))), has("age", gt(20).and(lt(30)).or(gt(35).and(lt(40)))), Collections.emptyList()}, + // + {has("name", "marko").and().has("name", "marko").and().has("name", "marko"), has("name", "marko").has("name", "marko").has("name", "marko"), Collections.emptyList()}, + {filter(has("name", "marko")).and().filter(has("name", "marko")).and().filter(has("name", "marko")), has("name", "marko").has("name", "marko").has("name", "marko"), Collections.emptyList()}, + {V().has("name", "marko").and().has("name", "marko").and().has("name", "marko"), V().has("name", "marko").has("name", "marko").has("name", "marko"), Collections.emptyList()}, + {V().filter(has("name", "marko")).and().filter(has("name", "marko")).and().filter(has("name", "marko")), V().has("name", "marko").has("name", "marko").has("name", "marko"), Collections.emptyList()}, + {has("name", "marko").and().has("name", "marko").and().has("name", "marko"), has("name", "marko").has("name", "marko").has("name", "marko"), Collections.singletonList(ConnectiveStrategy.instance())}, + {V().filter(has("name", "marko")).and().filter(has("name", "marko")).and().filter(has("name", "marko")), and(V().has("name", "marko"), has("name", "marko"), has("name", "marko")), Collections.singletonList(ConnectiveStrategy.instance())}, + {V().has("name", "marko").and().has("name", "marko").and().has("name", "marko"), and(V().has("name", "marko"), has("name", "marko"), has("name", "marko")), Collections.singletonList(ConnectiveStrategy.instance())}, + {EmptyGraph.instance().traversal().V().filter(has("name", "marko")).and().filter(has("name", "marko")).and().filter(has("name", "marko")), EmptyGraph.instance().traversal().V().has("name", "marko").has("name", "marko").has("name", "marko"), Collections.singletonList(ConnectiveStrategy.instance())}, + {EmptyGraph.instance().traversal().V().has("name", "marko").and().has("name", "marko").and().has("name", "marko"), EmptyGraph.instance().traversal().V().has("name", "marko").has("name", "marko").has("name", "marko"), Collections.singletonList(ConnectiveStrategy.instance())}, + // + {and(has("age", gt(10)), filter(has("age", 22))), addHas(__.start(), "age", gt(10), "age", eq(22)), Collections.emptyList()}, + {and(has("age", gt(10)).as("a"), filter(has("age", 22).as("b")).as("c")).as("d"), addHas(__.start(), "age", gt(10), "age", eq(22)).as("a", "b", "c", "d"), Collections.emptyList()}, + {and(has("age", gt(10)).as("a"), and(filter(has("age", 22).as("b")).as("c"), has("name", "marko").as("d"))), addHas(__.start(), "age", gt(10), "age", eq(22), "name", eq("marko")).as("a", "b", "c", "d"), Collections.emptyList()}, + {and(has("age", gt(10)).as("a"), and(has("name", "stephen").as("b"), has("name", "marko").as("c")).as("d")).as("e"), addHas(__.start(), "age", gt(10), "name", eq("stephen"), "name", eq("marko")).as("a", "b", "c", "d", "e"), Collections.emptyList()}, + {and(has("age", gt(10)), and(out("knows"), has("name", "marko"))), has("age", gt(10)).and(out("knows"), has("name", "marko")), Collections.emptyList()}, + {and(has("age", gt(20)), or(has("age", lt(10)), has("age", gt(100)))), addHas(__.start(), "age", gt(20), "age", lt(10).or(gt(100))), Collections.emptyList()}, + {and(has("age", gt(20)).as("a"), or(has("age", lt(10)), has("age", gt(100)).as("b"))), addHas(__.start(), "age", gt(20), "age", lt(10).or(gt(100))).as("a", "b"), Collections.emptyList()}, + {and(has("age", gt(20)).as("a"), or(has("age", lt(10)).as("c"), has("age", gt(100)).as("b"))), addHas(__.start(), "age", gt(20), "age", lt(10).or(gt(100))).as("a", "b", "c"), Collections.emptyList()}, + // + {V().match(as("a").has("age", 10), as("a").filter(has("name")).as("b")), V().has("age", 10).as("a").match(as("a").has("name").as("b")), Collections.emptyList()}, + {match(as("a").has("age", 10), as("a").filter(has("name")).as("b")), match(as("a").has("age", 10), as("a").has("name").as("b")), Collections.emptyList()}, + {match(as("a").has("age", 10).both().as("b"), as("b").out().as("c")), match(as("a").has("age", 10).both().as("b"), as("b").out().as("c")), Collections.emptyList()}, + {__.map(match(as("a").has("age", 10), as("a").filter(has("name")).as("b"))), __.map(match(as("a").has("age", 10), as("a").has("name").as("b"))), Collections.emptyList()}, + {V().match(as("a").has("age", 10)), V().has("age", 10).as("a"), Collections.emptyList()}, + {V().match(as("a").has("age", 10).has("name", "marko").as("b")), V().has("age", 10).has("name", "marko").as("a", "b"), Collections.emptyList()}, + {V().match(as("a").has("age", 10).has("name", "marko").as("b"), as("a").out("knows").as("c")), V().has("age", 10).has("name", "marko").as("a", "b").match(as("a").out("knows").as("c")), Collections.emptyList()}, + {V().match(as("a").out("knows").as("c"), as("a").has("age", 10).has("name", "marko").as("b")), V().has("age", 10).has("name", "marko").as("a", "b").match(as("a").out("knows").as("c")), Collections.emptyList()}, + {V().match(as("a").out("knows").as("c"), as("a").has("age", 10).has("name", "marko").as("b"), as("a").has("name", "bob")), V().has("age", 10).has("name", "marko").has("name", "bob").as("a", "b").match(as("a").out("knows").as("c")), Collections.emptyList()}, + {V().match(as("a").has("age", 10).as("b"), as("a").filter(has("name")).as("b")), V().has("age", 10).as("a", "b").match(as("a").has("name").as("b")), Collections.emptyList()}, + // + {filter(dedup()), filter(dedup()), Collections.emptyList()}, + {filter(filter(drop())), filter(drop()), Collections.emptyList()}, + {and(has("name"), limit(10).has("age")), and(has("name"), limit(10).has("age")), Collections.emptyList()}, + {filter(tail(10).as("a")), filter(tail(10).as("a")), Collections.emptyList()}, + // + {outE().hasLabel("knows").inV(), outE("knows").inV(), Collections.emptyList()}, + {outE().hasLabel("knows").hasLabel("created").inV(), outE("knows").hasLabel("created").inV(), Collections.emptyList()}, + {outE().or(hasLabel("knows"), hasLabel("created")).inV(), outE("knows", "created").inV(), Collections.emptyList()}, + {outE().or(hasLabel("knows").as("a"), hasLabel("created").as("b")).as("c").inV(), outE("knows", "created").as("a", "b", "c").inV(), Collections.emptyList()}, + {outE().hasLabel(P.eq("knows").or(P.gt("created"))).has("weight", gt(1.0)).inV(), addHas(outE(), T.label.getAccessor(), P.eq("knows").or(P.gt("created")), "weight", gt(1.0)).inV(), Collections.emptyList()}, + {outE().hasLabel(P.eq("knows").or(P.eq("created"))).has("weight", gt(1.0)).inV(), outE("knows", "created").has("weight", gt(1.0)).inV(), Collections.emptyList()}, + {outE().hasLabel(P.within("knows", "created")).inV(), outE("knows", "created").inV(), Collections.emptyList()}, + }); + } - this.original.asAdmin().setStrategies(strategies); - this.original.asAdmin().applyStrategies(); - assertEquals(repr, this.optimized, this.original); - } + @Test + public void doTest() { + final String repr = translator.translate(original.getBytecode()).getScript(); - @Parameterized.Parameters(name = "{0}") - public static Iterable<Object[]> generateTestParameters() { - - return Arrays.asList(new Object[][]{ - {has("age", 10).as("a").has("name", "marko").as("b").coin(0.5).as("c"), addHas(__.start(), "age", eq(10), "name", eq("marko")).as("a", "b").coin(0.5).as("c"), Collections.emptyList()}, - // - {filter(out("knows")), filter(out("knows")), Collections.emptyList()}, - {filter(has("age", gt(10))).as("a"), has("age", gt(10)).as("a"), Collections.emptyList()}, - {filter(has("age", gt(10)).as("b")).as("a"), has("age", gt(10)).as("b", "a"), Collections.emptyList()}, - {filter(has("age", gt(10)).as("a")), has("age", gt(10)).as("a"), Collections.emptyList()}, - {filter(and(has("age", gt(10)).as("a"), has("name", "marko"))), addHas(__.start(), "age", gt(10), "name", eq("marko")).as("a"), Collections.emptyList()}, - {filter(out("created").and().out("knows").or().in("knows")), or(and(out("created"), out("knows")), __.in("knows")), Collections.singletonList(ConnectiveStrategy.instance())}, - {filter(out("created").and().out("knows").or().in("knows")).and(hasLabel("person")), or(and(out("created"), out("knows")), __.in("knows")).hasLabel("person"), Collections.singletonList(ConnectiveStrategy.instance())}, - // - {or(has("name", "marko"), has("age", 32)), or(has("name", "marko"), has("age", 32)), Collections.emptyList()}, - {or(has("name", "marko"), has("name", "bob")), has("name", eq("marko").or(eq("bob"))), Collections.emptyList()}, - {or(has("name", "marko"), has("name")), or(has("name", "marko"), has("name")), Collections.emptyList()}, - {or(has("age", 10), and(has("age", gt(20)), has("age", lt(100)))), has("age", eq(10).or(gt(20).and(lt(100)))), Collections.emptyList()}, - {or(has("name", "marko"), filter(has("name", "bob"))), has("name", eq("marko").or(eq("bob"))), Collections.emptyList()}, - {or(has("name", "marko"), filter(or(filter(has("name", "bob")), has("name", "stephen")))), has("name", eq("marko").or(eq("bob").or(eq("stephen")))), Collections.emptyList()}, - {or(has("name", "marko").as("a"), filter(or(filter(has("name", "bob")).as("b"), has("name", "stephen").as("c")))), has("name", eq("marko").or(eq("bob").or(eq("stephen")))).as("a", "b", "c"), Collections.emptyList()}, - {or(and(has("age",gt(20)), has("age",lt(30))), and(has("age",gt(35)), has("age",lt(40)))), has("age", gt(20).and(lt(30)).or(gt(35).and(lt(40)))), Collections.emptyList()}, - // - {has("name", "marko").and().has("name", "marko").and().has("name", "marko"), has("name", "marko").has("name", "marko").has("name", "marko"), Collections.emptyList()}, - {filter(has("name", "marko")).and().filter(has("name", "marko")).and().filter(has("name", "marko")), has("name", "marko").has("name", "marko").has("name", "marko"), Collections.emptyList()}, - {V().has("name", "marko").and().has("name", "marko").and().has("name", "marko"), V().has("name", "marko").has("name", "marko").has("name", "marko"), Collections.emptyList()}, - {V().filter(has("name", "marko")).and().filter(has("name", "marko")).and().filter(has("name", "marko")), V().has("name", "marko").has("name", "marko").has("name", "marko"), Collections.emptyList()}, - {has("name", "marko").and().has("name", "marko").and().has("name", "marko"), has("name", "marko").has("name", "marko").has("name", "marko"), Collections.singletonList(ConnectiveStrategy.instance())}, - {V().filter(has("name", "marko")).and().filter(has("name", "marko")).and().filter(has("name", "marko")), and(V().has("name", "marko"), has("name", "marko"), has("name", "marko")), Collections.singletonList(ConnectiveStrategy.instance())}, - {V().has("name", "marko").and().has("name", "marko").and().has("name", "marko"), and(V().has("name", "marko"), has("name", "marko"), has("name", "marko")), Collections.singletonList(ConnectiveStrategy.instance())}, - {EmptyGraph.instance().traversal().V().filter(has("name", "marko")).and().filter(has("name", "marko")).and().filter(has("name", "marko")), EmptyGraph.instance().traversal().V().has("name", "marko").has("name", "marko").has("name", "marko"), Collections.singletonList(ConnectiveStrategy.instance())}, - {EmptyGraph.instance().traversal().V().has("name", "marko").and().has("name", "marko").and().has("name", "marko"), EmptyGraph.instance().traversal().V().has("name", "marko").has("name", "marko").has("name", "marko"), Collections.singletonList(ConnectiveStrategy.instance())}, - // - {and(has("age", gt(10)), filter(has("age", 22))), addHas(__.start(), "age", gt(10), "age", eq(22)), Collections.emptyList()}, - {and(has("age", gt(10)).as("a"), filter(has("age", 22).as("b")).as("c")).as("d"), addHas(__.start(), "age", gt(10), "age", eq(22)).as("a", "b", "c", "d"), Collections.emptyList()}, - {and(has("age", gt(10)).as("a"), and(filter(has("age", 22).as("b")).as("c"), has("name", "marko").as("d"))), addHas(__.start(), "age", gt(10), "age", eq(22), "name", eq("marko")).as("a", "b", "c", "d"), Collections.emptyList()}, - {and(has("age", gt(10)).as("a"), and(has("name", "stephen").as("b"), has("name", "marko").as("c")).as("d")).as("e"), addHas(__.start(), "age", gt(10), "name", eq("stephen"), "name", eq("marko")).as("a", "b", "c", "d", "e"), Collections.emptyList()}, - {and(has("age", gt(10)), and(out("knows"), has("name", "marko"))), has("age", gt(10)).and(out("knows"), has("name", "marko")), Collections.emptyList()}, - {and(has("age", gt(20)), or(has("age", lt(10)), has("age", gt(100)))), addHas(__.start(), "age", gt(20), "age", lt(10).or(gt(100))), Collections.emptyList()}, - {and(has("age", gt(20)).as("a"), or(has("age", lt(10)), has("age", gt(100)).as("b"))), addHas(__.start(), "age", gt(20), "age", lt(10).or(gt(100))).as("a", "b"), Collections.emptyList()}, - {and(has("age", gt(20)).as("a"), or(has("age", lt(10)).as("c"), has("age", gt(100)).as("b"))), addHas(__.start(), "age", gt(20), "age", lt(10).or(gt(100))).as("a", "b", "c"), Collections.emptyList()}, - // - {V().match(as("a").has("age", 10), as("a").filter(has("name")).as("b")), V().has("age", 10).as("a").match(as("a").has("name").as("b")), Collections.emptyList()}, - {match(as("a").has("age", 10), as("a").filter(has("name")).as("b")), match(as("a").has("age", 10), as("a").has("name").as("b")), Collections.emptyList()}, - {match(as("a").has("age", 10).both().as("b"), as("b").out().as("c")), match(as("a").has("age", 10).both().as("b"), as("b").out().as("c")), Collections.emptyList()}, - {__.map(match(as("a").has("age", 10), as("a").filter(has("name")).as("b"))), __.map(match(as("a").has("age", 10), as("a").has("name").as("b"))), Collections.emptyList()}, - {V().match(as("a").has("age", 10)), V().has("age", 10).as("a"), Collections.emptyList()}, - {V().match(as("a").has("age", 10).has("name", "marko").as("b")), V().has("age", 10).has("name", "marko").as("a", "b"), Collections.emptyList()}, - {V().match(as("a").has("age", 10).has("name", "marko").as("b"), as("a").out("knows").as("c")), V().has("age", 10).has("name", "marko").as("a", "b").match(as("a").out("knows").as("c")), Collections.emptyList()}, - {V().match(as("a").out("knows").as("c"), as("a").has("age", 10).has("name", "marko").as("b")), V().has("age", 10).has("name", "marko").as("a", "b").match(as("a").out("knows").as("c")), Collections.emptyList()}, - {V().match(as("a").out("knows").as("c"), as("a").has("age", 10).has("name", "marko").as("b"), as("a").has("name", "bob")), V().has("age", 10).has("name", "marko").has("name", "bob").as("a", "b").match(as("a").out("knows").as("c")), Collections.emptyList()}, - {V().match(as("a").has("age", 10).as("b"), as("a").filter(has("name")).as("b")), V().has("age", 10).as("a", "b").match(as("a").has("name").as("b")), Collections.emptyList()}, - // - {filter(dedup()), filter(dedup()), Collections.emptyList()}, - {filter(filter(drop())), filter(drop()), Collections.emptyList()}, - {and(has("name"), limit(10).has("age")), and(has("name"), limit(10).has("age")), Collections.emptyList()}, - {filter(tail(10).as("a")), filter(tail(10).as("a")), Collections.emptyList()}, - // - {outE().hasLabel("knows").inV(), outE("knows").inV(), Collections.emptyList()}, - {outE().hasLabel("knows").hasLabel("created").inV(), outE("knows").hasLabel("created").inV(), Collections.emptyList()}, - {outE().or(hasLabel("knows"), hasLabel("created")).inV(), outE("knows", "created").inV(), Collections.emptyList()}, - {outE().or(hasLabel("knows").as("a"), hasLabel("created").as("b")).as("c").inV(), outE("knows", "created").as("a", "b", "c").inV(), Collections.emptyList()}, - {outE().hasLabel(P.eq("knows").or(P.gt("created"))).has("weight", gt(1.0)).inV(), addHas(outE(), T.label.getAccessor(), P.eq("knows").or(P.gt("created")), "weight", gt(1.0)).inV(), Collections.emptyList()}, - {outE().hasLabel(P.eq("knows").or(P.eq("created"))).has("weight", gt(1.0)).inV(), outE("knows", "created").has("weight", gt(1.0)).inV(), Collections.emptyList()}, - {outE().hasLabel(P.within("knows", "created")).inV(), outE("knows", "created").inV(), Collections.emptyList()}, - }); + GValueManagerVerifier.verify(original.asAdmin(), InlineFilterStrategy.instance(), extras) + .afterApplying() + .managerIsEmpty(); + + assertEquals(repr, this.optimized, this.original); + } + + private static GraphTraversal.Admin<?, ?> addHas(final GraphTraversal<?, ?> traversal, final Object... hasKeyValues) { + final HasStep<?> hasStep = new HasStep<>((Traversal.Admin) traversal); + for (int i = 0; i < hasKeyValues.length; i = i + 2) { + hasStep.addHasContainer(new HasContainer((String) hasKeyValues[i], (P) hasKeyValues[i + 1])); + } + return traversal.asAdmin().addStep(hasStep); + } } - private static GraphTraversal.Admin<?, ?> addHas(final GraphTraversal<?, ?> traversal, final Object... hasKeyValues) { - final HasStep<?> hasStep = new HasStep<>((Traversal.Admin) traversal); - for (int i = 0; i < hasKeyValues.length; i = i + 2) { - hasStep.addHasContainer(new HasContainer((String) hasKeyValues[i], (P) hasKeyValues[i + 1])); + /** + * Tests that {@link GValueManager} is properly maintaining state when using InlineFilterStrategy. + */ + @RunWith(Parameterized.class) + public static class GValueManagerVerifierTest { + + @Parameterized.Parameter(value = 0) + public Traversal.Admin<?, ?> traversal; + + @Parameterized.Parameters(name = "{0}") + public static Iterable<Object[]> generateTestParameters() { + return Arrays.asList(new Object[][]{ + // Test filter with has step using GValue + { filter(has("age", GValue.of("x", 10))).asAdmin() }, + { filter(has("age", gt(GValue.ofInteger("x", 20)))).asAdmin() }, + { filter(has("age", lt(GValue.ofInteger("x", 30)))).asAdmin() }, + + // Test or conditions with GValue + { or(has("name", GValue.of("x", "marko")), has("age", GValue.of("y", 32))).asAdmin() }, + { or(has("name", GValue.of("x", "marko")), has("name", GValue.of("y", "bob"))).asAdmin() }, + + // Test and conditions with GValue + { and(has("age", gt(GValue.ofInteger("x", 10))), filter(has("age", GValue.of("y", 22)))).asAdmin() }, + { and(has("age", gt(GValue.ofInteger("x", 10))).as("a"), filter(has("age", GValue.of("y", 22)).as("b"))).asAdmin() }, + + // Test complex conditions with GValue + { or(and(has("age", gt(GValue.ofInteger("x", 20))), has("age", lt(GValue.ofInteger("y", 30)))), + and(has("age", gt(GValue.ofInteger("z", 35))), has("age", lt(GValue.ofInteger("w", 40))))).asAdmin() }, + + // Test edge label with GValue + { outE().hasLabel(GValue.of("x", "knows")).inV().asAdmin() }, + { outE().hasLabel(GValue.of("x", "knows"), GValue.of("y", "created")).inV().asAdmin() }, + }); + } + + @Test + public void shouldMaintainGValueManagerState() { + // Use GValueManagerVerifier to verify the state before and after applying the strategy + GValueManagerVerifier.verify(traversal, InlineFilterStrategy.instance()). + beforeApplying(). + stepsOfClassAreParameterized(true, HasStep.class). + afterApplying(). + variablesArePreserved(); } - return traversal.asAdmin().addStep(hasStep); } }
