You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by ok...@apache.org on 2016/09/29 16:39:28 UTC

tinkerpop git commit: Added support for or(has(x), has(y)) to be has(x.or(y)) in InlineFilterStrategy. While doing this, I found a bug in ConnectiveP steps where nested equivalents were not being inlined. That bug has been fixed. Added test cases to PTest

Repository: tinkerpop
Updated Branches:
  refs/heads/TINKERPOP-1470 [created] 6da9151ef


Added support for or(has(x),has(y)) to be has(x.or(y)) in InlineFilterStrategy. While doing this, I found a bug in ConnectiveP steps where nested equivalents were not being inlined. That bug has been fixed. Added test cases to PTest to demonstrate proper inlining and nesting of ConnectivePs. I left two TODOs. One regarding match()-pulls that are labeled (interfering with FilterRankStrategy) and one regarding, has(x).has(y) being turned into has(x.and(y)). The reason why the latter isn't done now as it may greatly mess up providers who just rely on eq() for index lookups and are not smart enough to look into and()/or() predicates.


Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo
Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/6da9151e
Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/6da9151e
Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/6da9151e

Branch: refs/heads/TINKERPOP-1470
Commit: 6da9151ef19c998688b78309e69fddb274ddf9d6
Parents: e331164
Author: Marko A. Rodriguez <ok...@gmail.com>
Authored: Thu Sep 29 10:39:20 2016 -0600
Committer: Marko A. Rodriguez <ok...@gmail.com>
Committed: Thu Sep 29 10:39:20 2016 -0600

----------------------------------------------------------------------
 CHANGELOG.asciidoc                              |   3 +-
 .../upgrade/release-3.2.x-incubating.asciidoc   |  18 ++
 .../optimization/InlineFilterStrategy.java      |  45 +++-
 .../gremlin/process/traversal/util/AndP.java    |   8 +-
 .../process/traversal/util/ConnectiveP.java     |   3 +-
 .../gremlin/process/traversal/util/OrP.java     |   9 +-
 .../gremlin/process/traversal/PTest.java        | 245 ++++++++++---------
 .../optimization/InlineFilterStrategyTest.java  |  18 ++
 .../TinkerGraphStepStrategyTest.java            |   3 +-
 9 files changed, 233 insertions(+), 119 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/6da9151e/CHANGELOG.asciidoc
----------------------------------------------------------------------
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index 47d4bbf..19381c3 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -26,6 +26,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
 TinkerPop 3.2.3 (Release Date: NOT OFFICIALLY RELEASED YET)
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
+* Fixed a bug in `ConnectiveP` where nested equivalent connectives should be inlined.
 * Fixed a bug in `IncidentToAdjacentStrategy` where `TreeStep` traversals were allowed.
 * Fixed a end-step label bug in `MatchPredicateStrategy`.
 * Fixed a bug in `MatchPredicateStrategy` where inlined traversals did not have strategies applied to it.
@@ -33,7 +34,7 @@ TinkerPop 3.2.3 (Release Date: NOT OFFICIALLY RELEASED YET)
 * Added `TraversalHelper.copyLabels()` for copying (or moving) labels form one step to another.
 * Added `TraversalHelper.applySingleLevelStrategies()` which will apply a subset of strategies but not walk the child tree.
 * Added the concept that hidden labels using during traversal compilation are removed at the end during `StandardVerificationStrategy`. (*breaking*)
-* Added `InlineFilterStrategy` which will determine if a `TraversalFilterStep`, `AndStep`, `MatchStep` children are filters and if so, inline them.
+* Added `InlineFilterStrategy` which will determine if a `TraversalFilterStep`, `AndStep`, `OrStep`, `MatchStep` children are filters and if so, inline them.
 * Removed `IdentityRemovalStrategy` from the default listing as its not worth the clock cycles.
 * Removed the "!" symbol in `NotStep.toString()` as it is confusing and the `NotStep`-name is sufficient.
 * Fixed a bug in `TraversalVertexProgram` (OLAP) around ordering and connectives (i.e. `and()` and `or()`).

http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/6da9151e/docs/src/upgrade/release-3.2.x-incubating.asciidoc
----------------------------------------------------------------------
diff --git a/docs/src/upgrade/release-3.2.x-incubating.asciidoc b/docs/src/upgrade/release-3.2.x-incubating.asciidoc
index a6be1c6..6e65b72 100644
--- a/docs/src/upgrade/release-3.2.x-incubating.asciidoc
+++ b/docs/src/upgrade/release-3.2.x-incubating.asciidoc
@@ -137,6 +137,15 @@ uses of these exception builders as they will be removed in the future.
 
 See: link:https://issues.apache.org/jira/browse/TINKERPOP-944[TINKERPOP-944]
 
+Hidden Step Labels for Compilation Only
++++++++++++++++++++++++++++++++++++++++
+
+In order for `SubgraphStrategy` to work, it was necessary to have multi-level children communicate with one another
+via hidden step labels. It was decided that hidden step labels are for compilation purposes only and will be removed
+prior to traversal evaluation. This is a valid decision given that hidden labels for graph system providers are
+not allowed to be used by users. Likewise, hidden labels for steps should not be allowed be used by
+users as well.
+
 PropertyMapStep with Selection Traversal
 ++++++++++++++++++++++++++++++++++++++++
 
@@ -147,6 +156,15 @@ and if so, use that in their introspection for respective strategies. This model
 See: link:https://issues.apache.org/jira/browse/TINKERPOP-1456[TINKERPOP-1456],
 link:https://issues.apache.org/jira/browse/TINKERPOP-844[TINKERPOP-844]
 
+ConnectiveP Nesting Inlined
++++++++++++++++++++++++++++
+
+There was a bug in `ConnectiveP` (`AndP`/`OrP`), where `eq(1).and(eq(2).and(eq(3)))` was `OrP(eq(1),AndP(eq(2),eq(3)))`
+instead of unnested/inlined as `OrP(eq(1),eq(2),eq(3))`. Likewise, for `AndP`. If a provider was leveraging `ConnectiveP`
+predicates for their custom steps (e.g. graph- or vertex-centric index lookups), then they should be aware of the inlining
+and can simplify any and/or-tree walking code in their respective `ProviderOptimizationStrategy`.
+
+See: link:https://issues.apache.org/jira/browse/TINKERPOP-1470[TINKERPOP-1470]
 
 TinkerPop 3.2.2
 ---------------

http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/6da9151e/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategy.java
----------------------------------------------------------------------
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 7452b3b..0b7a5cc 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
@@ -20,6 +20,7 @@
 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.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;
@@ -29,10 +30,12 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.filter.DedupGlobalSte
 import org.apache.tinkerpop.gremlin.process.traversal.step.filter.DropStep;
 import org.apache.tinkerpop.gremlin.process.traversal.step.filter.FilterStep;
 import org.apache.tinkerpop.gremlin.process.traversal.step.filter.HasStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.filter.OrStep;
 import org.apache.tinkerpop.gremlin.process.traversal.step.filter.RangeGlobalStep;
 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.util.EmptyStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer;
 import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy;
 import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper;
 
@@ -89,6 +92,46 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver
                     traversal.removeStep(step);
                 }
             }
+            // or(has(x,y),has(x,z)) --> has(x,y.or(z))
+            for (final OrStep<?> step : TraversalHelper.getStepsOfClass(OrStep.class, traversal)) {
+                boolean process = true;
+                String key = null;
+                P predicate = null;
+                final List<String> labels = new ArrayList<>();
+                for (final Traversal.Admin<?, ?> childTraversal : step.getLocalChildren()) {
+                    this.apply(childTraversal); // todo: this may be a bad idea, but I can't seem to find a test case to break it
+                    for (final Step<?, ?> childStep : childTraversal.getSteps()) {
+                        if (childStep instanceof HasStep) {
+                            for (final HasContainer hasContainer : ((HasStep<?>) childStep).getHasContainers()) {
+                                if (null == key)
+                                    key = hasContainer.getKey();
+                                else if (!hasContainer.getKey().equals(key)) {
+                                    process = false;
+                                    break;
+                                }
+                                predicate = null == predicate ?
+                                        hasContainer.getPredicate() :
+                                        predicate.or(hasContainer.getPredicate());
+                            }
+                            labels.addAll(childStep.getLabels());
+                        } else {
+                            process = false;
+                            break;
+                        }
+                    }
+                    if (!process)
+                        break;
+                }
+                if (process) {
+                    changed = true;
+                    final HasStep hasStep = new HasStep<>(traversal, new HasContainer(key, predicate));
+                    TraversalHelper.replaceStep(step, hasStep, traversal);
+                    TraversalHelper.copyLabels(step, hasStep, false);
+                    for (final String label : labels) {
+                        hasStep.addLabel(label);
+                    }
+                }
+            }
             // and(x,y) --> x.y
             for (final AndStep<?> step : TraversalHelper.getStepsOfClass(AndStep.class, traversal)) {
                 boolean process = true;
@@ -139,6 +182,7 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver
                                 TraversalHelper.applySingleLevelStrategies(traversal, matchTraversal, InlineFilterStrategy.class);
                                 step.removeGlobalChild(matchTraversal);
                                 step.getPreviousStep().addLabel(startLabel);
+                                // TODO: matchTraversal.getEndStep().addLabel(startLabel); (perhaps insert an identity so filter rank can push has()-left)
                                 if (null != endLabel) matchTraversal.getEndStep().addLabel(endLabel);
                                 TraversalHelper.insertTraversal((Step) step.getPreviousStep(), matchTraversal, traversal);
                             }
@@ -149,7 +193,6 @@ public final class InlineFilterStrategy extends AbstractTraversalStrategy<Traver
                 }
             }
         }
-
     }
 
     private static final String determineStartLabelForHasPullOut(final MatchStep<?, ?> matchStep) {

http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/6da9151e/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/AndP.java
----------------------------------------------------------------------
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/AndP.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/AndP.java
index 8a7784f..cb001ff 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/AndP.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/AndP.java
@@ -34,6 +34,9 @@ public final class AndP<V> extends ConnectiveP<V> {
 
     public AndP(final List<P<V>> predicates) {
         super(predicates);
+        for (final P<V> p : predicates) {
+            this.and(p);
+        }
         this.biPredicate = new AndBiPredicate(this);
     }
 
@@ -49,7 +52,10 @@ public final class AndP<V> extends ConnectiveP<V> {
     public P<V> and(final Predicate<? super V> predicate) {
         if (!(predicate instanceof P))
             throw new IllegalArgumentException("Only P predicates can be and'd together");
-        this.predicates.add((P<V>) predicate);   // TODO: clone and add?
+        else if (predicate instanceof AndP)
+            this.predicates.addAll(((AndP) predicate).getPredicates());
+        else
+            this.predicates.add((P<V>) predicate);
         return this;
     }
 

http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/6da9151e/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/ConnectiveP.java
----------------------------------------------------------------------
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/ConnectiveP.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/ConnectiveP.java
index 70f008a..855fc06 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/ConnectiveP.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/ConnectiveP.java
@@ -31,13 +31,12 @@ import java.util.function.Predicate;
  */
 public abstract class ConnectiveP<V> extends P<V> {
 
-    protected List<P<V>> predicates;
+    protected List<P<V>> predicates = new ArrayList<>();
 
     public ConnectiveP(final List<P<V>> predicates) {
         super(null, null);
         if (predicates.size() < 2)
             throw new IllegalArgumentException("The provided " + this.getClass().getSimpleName() + " array must have at least two arguments: " + predicates.size());
-        this.predicates = new ArrayList<>(predicates); // to avoid Arrays.asList() and unmodifiable exceptions
     }
 
     @Deprecated

http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/6da9151e/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/OrP.java
----------------------------------------------------------------------
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/OrP.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/OrP.java
index 8bc4387..f7363b4 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/OrP.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/OrP.java
@@ -22,7 +22,6 @@ import org.apache.tinkerpop.gremlin.process.traversal.P;
 import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
 
 import java.io.Serializable;
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 import java.util.function.BiPredicate;
@@ -35,6 +34,9 @@ public final class OrP<V> extends ConnectiveP<V> {
 
     public OrP(final List<P<V>> predicates) {
         super(predicates);
+        for (final P<V> p : predicates) {
+            this.or(p);
+        }
         this.biPredicate = new OrBiPredicate(this);
     }
 
@@ -50,7 +52,10 @@ public final class OrP<V> extends ConnectiveP<V> {
     public P<V> or(final Predicate<? super V> predicate) {
         if (!(predicate instanceof P))
             throw new IllegalArgumentException("Only P predicates can be or'd together");
-        this.predicates.add((P<V>) predicate);   // TODO: clone and add?
+        else if (predicate instanceof OrP)
+            this.predicates.addAll(((OrP) predicate).getPredicates());
+        else
+            this.predicates.add((P<V>) predicate);
         return this;
     }
 

http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/6da9151e/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/PTest.java
----------------------------------------------------------------------
diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/PTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/PTest.java
index 3810c96..6ec33cc 100644
--- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/PTest.java
+++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/PTest.java
@@ -19,8 +19,11 @@
 package org.apache.tinkerpop.gremlin.process.traversal;
 
 import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__;
+import org.apache.tinkerpop.gremlin.process.traversal.util.AndP;
+import org.apache.tinkerpop.gremlin.process.traversal.util.OrP;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.experimental.runners.Enclosed;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
 
@@ -29,135 +32,157 @@ import java.util.Arrays;
 import java.util.Random;
 import java.util.function.Predicate;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
 
 /**
  * @author Daniel Kuppitz (http://gremlin.guru)
  */
-@RunWith(Parameterized.class)
+@RunWith(Enclosed.class)
 public class PTest {
 
-    @Parameterized.Parameters(name = "{0}.test({1}) = {2}")
-    public static Iterable<Object[]> data() {
-        return new ArrayList<>(Arrays.asList(new Object[][]{
-                {P.eq(0), 0, true},
-                {P.eq(0), 1, false},
-                {P.neq(0), 0, false},
-                {P.neq(0), 1, true},
-                {P.gt(0), -1, false},
-                {P.gt(0), 0, false},
-                {P.gt(0), 1, true},
-                {P.lt(0), -1, true},
-                {P.lt(0), 0, false},
-                {P.lt(0), 1, false},
-                {P.gte(0), -1, false},
-                {P.gte(0), 0, true},
-                {P.gte(0), 1, true},
-                {P.lte(0), -1, true},
-                {P.lte(0), 0, true},
-                {P.lte(0), 1, false},
-                {P.between(1, 10), 0, false},
-                {P.between(1, 10), 1, true},
-                {P.between(1, 10), 9, true},
-                {P.between(1, 10), 10, false},
-                {P.inside(1, 10), 0, false},
-                {P.inside(1, 10), 1, false},
-                {P.inside(1, 10), 9, true},
-                {P.inside(1, 10), 10, false},
-                {P.outside(1, 10), 0, true},
-                {P.outside(1, 10), 1, false},
-                {P.outside(1, 10), 9, false},
-                {P.outside(1, 10), 10, false},
-                {P.within(1, 2, 3), 0, false},
-                {P.within(1, 2, 3), 1, true},
-                {P.within(1, 2, 3), 10, false},
-                {P.within(Arrays.asList(1, 2, 3)), 0, false},
-                {P.within(Arrays.asList(1, 2, 3)), 1, true},
-                {P.within(Arrays.asList(1, 2, 3)), 10, false},
-                {P.without(1, 2, 3), 0, true},
-                {P.without(1, 2, 3), 1, false},
-                {P.without(1, 2, 3), 10, true},
-                {P.without(Arrays.asList(1, 2, 3)), 0, true},
-                {P.without(Arrays.asList(1, 2, 3)), 1, false},
-                {P.without(Arrays.asList(1, 2, 3)), 10, true},
-                {P.between("m", "n").and(P.neq("marko")), "marko", false},
-                {P.between("m", "n").and(P.neq("marko")), "matthias", true},
-                {P.between("m", "n").or(P.eq("daniel")), "marko", true},
-                {P.between("m", "n").or(P.eq("daniel")), "daniel", true},
-                {P.between("m", "n").or(P.eq("daniel")), "stephen", false},
-        }));
-    }
+    @RunWith(Parameterized.class)
+    public static class ParameterizedTest {
+
+        @Parameterized.Parameters(name = "{0}.test({1}) = {2}")
+        public static Iterable<Object[]> data() {
+            return new ArrayList<>(Arrays.asList(new Object[][]{
+                    {P.eq(0), 0, true},
+                    {P.eq(0), 1, false},
+                    {P.neq(0), 0, false},
+                    {P.neq(0), 1, true},
+                    {P.gt(0), -1, false},
+                    {P.gt(0), 0, false},
+                    {P.gt(0), 1, true},
+                    {P.lt(0), -1, true},
+                    {P.lt(0), 0, false},
+                    {P.lt(0), 1, false},
+                    {P.gte(0), -1, false},
+                    {P.gte(0), 0, true},
+                    {P.gte(0), 1, true},
+                    {P.lte(0), -1, true},
+                    {P.lte(0), 0, true},
+                    {P.lte(0), 1, false},
+                    {P.between(1, 10), 0, false},
+                    {P.between(1, 10), 1, true},
+                    {P.between(1, 10), 9, true},
+                    {P.between(1, 10), 10, false},
+                    {P.inside(1, 10), 0, false},
+                    {P.inside(1, 10), 1, false},
+                    {P.inside(1, 10), 9, true},
+                    {P.inside(1, 10), 10, false},
+                    {P.outside(1, 10), 0, true},
+                    {P.outside(1, 10), 1, false},
+                    {P.outside(1, 10), 9, false},
+                    {P.outside(1, 10), 10, false},
+                    {P.within(1, 2, 3), 0, false},
+                    {P.within(1, 2, 3), 1, true},
+                    {P.within(1, 2, 3), 10, false},
+                    {P.within(Arrays.asList(1, 2, 3)), 0, false},
+                    {P.within(Arrays.asList(1, 2, 3)), 1, true},
+                    {P.within(Arrays.asList(1, 2, 3)), 10, false},
+                    {P.without(1, 2, 3), 0, true},
+                    {P.without(1, 2, 3), 1, false},
+                    {P.without(1, 2, 3), 10, true},
+                    {P.without(Arrays.asList(1, 2, 3)), 0, true},
+                    {P.without(Arrays.asList(1, 2, 3)), 1, false},
+                    {P.without(Arrays.asList(1, 2, 3)), 10, true},
+                    {P.between("m", "n").and(P.neq("marko")), "marko", false},
+                    {P.between("m", "n").and(P.neq("marko")), "matthias", true},
+                    {P.between("m", "n").or(P.eq("daniel")), "marko", true},
+                    {P.between("m", "n").or(P.eq("daniel")), "daniel", true},
+                    {P.between("m", "n").or(P.eq("daniel")), "stephen", false},
+            }));
+        }
 
-    @Parameterized.Parameter(value = 0)
-    public P predicate;
+        @Parameterized.Parameter(value = 0)
+        public P predicate;
 
-    @Parameterized.Parameter(value = 1)
-    public Object value;
+        @Parameterized.Parameter(value = 1)
+        public Object value;
 
-    @Parameterized.Parameter(value = 2)
-    public boolean expected;
+        @Parameterized.Parameter(value = 2)
+        public boolean expected;
 
-    @Test
-    public void shouldTest() {
-        assertEquals(expected, predicate.test(value));
-        assertEquals(!expected, predicate.clone().negate().test(value));
-        assertEquals(!expected, P.not(predicate).test(value));
-    }
-
-    @Before
-    public void init() {
-        final Object pv = predicate.getValue();
-        final Random r = new Random();
-        assertNotNull(predicate.getBiPredicate());
-        predicate.setValue(r.nextDouble());
-        assertNotNull(predicate.getValue());
-        predicate.setValue(pv);
-        assertEquals(pv, predicate.getValue());
-        assertNotNull(predicate.hashCode());
-        assertEquals(predicate, predicate.clone());
-        assertNotEquals(__.identity(), predicate);
-
-        boolean thrown = true;
-        try {
-            predicate.and(new CustomPredicate());
-            thrown = false;
-        } catch (IllegalArgumentException ex) {
-            assertEquals("Only P predicates can be and'd together", ex.getMessage());
-        } finally {
-            assertTrue(thrown);
+        @Test
+        public void shouldTest() {
+            assertEquals(expected, predicate.test(value));
+            assertEquals(!expected, predicate.clone().negate().test(value));
+            assertEquals(!expected, P.not(predicate).test(value));
         }
 
-        thrown = true;
-        try {
-            predicate.or(new CustomPredicate());
-            thrown = false;
-        } catch (IllegalArgumentException ex) {
-            assertEquals("Only P predicates can be or'd together", ex.getMessage());
-        } finally {
-            assertTrue(thrown);
+        @Before
+        public void init() {
+            final Object pv = predicate.getValue();
+            final Random r = new Random();
+            assertNotNull(predicate.getBiPredicate());
+            predicate.setValue(r.nextDouble());
+            assertNotNull(predicate.getValue());
+            predicate.setValue(pv);
+            assertEquals(pv, predicate.getValue());
+            assertNotNull(predicate.hashCode());
+            assertEquals(predicate, predicate.clone());
+            assertNotEquals(__.identity(), predicate);
+
+            boolean thrown = true;
+            try {
+                predicate.and(new CustomPredicate());
+                thrown = false;
+            } catch (IllegalArgumentException ex) {
+                assertEquals("Only P predicates can be and'd together", ex.getMessage());
+            } finally {
+                assertTrue(thrown);
+            }
+
+            thrown = true;
+            try {
+                predicate.or(new CustomPredicate());
+                thrown = false;
+            } catch (IllegalArgumentException ex) {
+                assertEquals("Only P predicates can be or'd together", ex.getMessage());
+            } finally {
+                assertTrue(thrown);
+            }
         }
-    }
 
-    private class CustomPredicate implements Predicate {
+        private class CustomPredicate implements Predicate {
 
-        @Override
-        public boolean test(Object o) {
-            return false;
-        }
+            @Override
+            public boolean test(Object o) {
+                return false;
+            }
 
-        @Override
-        public Predicate and(Predicate other) {
-            return null;
-        }
+            @Override
+            public Predicate and(Predicate other) {
+                return null;
+            }
 
-        @Override
-        public Predicate negate() {
-            return null;
+            @Override
+            public Predicate negate() {
+                return null;
+            }
+
+            @Override
+            public Predicate or(Predicate other) {
+                return null;
+            }
         }
+    }
 
-        @Override
-        public Predicate or(Predicate other) {
-            return null;
+    public static class ConnectiveTest {
+
+        @Test
+        public void shouldComposeCorrectly() {
+            assertEquals(P.eq(1), P.eq(1));
+            assertEquals(P.eq(1).and(P.eq(2)), new AndP<>(Arrays.asList(P.eq(1), P.eq(2))));
+            assertEquals(P.eq(1).and(P.eq(2).and(P.eq(3))), new AndP<>(Arrays.asList(P.eq(1), P.eq(2), P.eq(3))));
+            assertEquals(P.eq(1).and(P.eq(2).and(P.eq(3).and(P.eq(4)))), new AndP<>(Arrays.asList(P.eq(1), P.eq(2), P.eq(3), P.eq(4))));
+            assertEquals(P.eq(1).or(P.eq(2).or(P.eq(3).or(P.eq(4)))), new OrP<>(Arrays.asList(P.eq(1), P.eq(2), P.eq(3), P.eq(4))));
+            assertEquals(P.eq(1).or(P.eq(2).and(P.eq(3).or(P.eq(4)))), new OrP<>(Arrays.asList(P.eq(1), new AndP<>(Arrays.asList(P.eq(2), new OrP<>(Arrays.asList(P.eq(3), P.eq(4))))))));
+            assertEquals(P.eq(1).and(P.eq(2).or(P.eq(3).and(P.eq(4)))), new AndP<>(Arrays.asList(P.eq(1), new OrP<>(Arrays.asList(P.eq(2), new AndP<>(Arrays.asList(P.eq(3), P.eq(4))))))));
+            assertEquals(P.eq(1).and(P.eq(2).and(P.eq(3).or(P.eq(4)))), new AndP<>(Arrays.asList(P.eq(1), P.eq(2), new OrP<>(Arrays.asList(P.eq(3), P.eq(4))))));
         }
     }
 }

http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/6da9151e/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/InlineFilterStrategyTest.java
----------------------------------------------------------------------
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 aeae593..9530c82 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
@@ -29,6 +29,9 @@ import org.junit.runners.Parameterized;
 
 import java.util.Arrays;
 
+import static org.apache.tinkerpop.gremlin.process.traversal.P.eq;
+import static org.apache.tinkerpop.gremlin.process.traversal.P.gt;
+import static org.apache.tinkerpop.gremlin.process.traversal.P.lt;
 import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.V;
 import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.and;
 import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.as;
@@ -39,6 +42,7 @@ import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.has;
 import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.limit;
 import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.map;
 import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.match;
+import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.or;
 import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.out;
 import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.tail;
 import static org.junit.Assert.assertEquals;
@@ -46,6 +50,8 @@ import static org.junit.Assert.assertEquals;
 /**
  * @author Marko A. Rodriguez (http://markorodriguez.com)
  */
+
+
 @RunWith(Parameterized.class)
 public class InlineFilterStrategyTest {
 
@@ -74,9 +80,21 @@ public class InlineFilterStrategyTest {
                 {filter(has("age", P.gt(10)).as("a")), has("age", P.gt(10)).as("a")},
                 {filter(and(has("age", P.gt(10)).as("a"), has("name", "marko"))), has("age", P.gt(10)).as("a").has("name", "marko")},
                 //
+                {or(has("name", "marko"), has("age", 32)), or(has("name", "marko"), has("age", 32))},
+                {or(has("name", "marko"), has("name", "bob")), has("name", eq("marko").or(eq("bob")))},
+                {or(has("name", "marko"), has("name")), or(has("name", "marko"), has("name"))},
+                {or(has("age", 10), and(has("age", gt(20)), has("age", lt(100)))), has("age", eq(10).or(gt(20).and(lt(100))))},
+                {or(has("name", "marko"), filter(has("name", "bob"))), has("name", eq("marko").or(eq("bob")))},
+                {or(has("name", "marko"), filter(or(filter(has("name", "bob")), has("name", "stephen")))), has("name", eq("marko").or(eq("bob").or(eq("stephen"))))},
+                {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")},
+                //
                 {and(has("age", P.gt(10)), filter(has("age", 22))), has("age", P.gt(10)).has("age", 22)},
+                {and(has("age", P.gt(10)).as("a"), filter(has("age", 22).as("b")).as("c")).as("d"), has("age", P.gt(10)).as("a").has("age", 22).as("b", "c", "d")},
                 {and(has("age", P.gt(10)).as("a"), and(filter(has("age", 22).as("b")).as("c"), has("name", "marko").as("d"))), has("age", P.gt(10)).as("a").has("age", 22).as("b", "c").has("name", "marko").as("d")},
+                {and(has("age", P.gt(10)).as("a"), and(has("name","stephen").as("b"), has("name", "marko").as("c")).as("d")).as("e"), has("age", P.gt(10)).as("a").has("name","stephen").as("b").has("name","marko").as("c","d","e")},
                 {and(has("age", P.gt(10)), and(out("knows"), has("name", "marko"))), has("age", P.gt(10)).and(out("knows"), has("name", "marko"))},
+                {and(has("age", P.gt(20)), or(has("age", lt(10)), has("age", gt(100)))), has("age", gt(20)).has("age", lt(10).or(gt(100)))},
+                {and(has("age", P.gt(20)).as("a"), or(has("age", lt(10)), has("age", gt(100)).as("b"))), has("age", gt(20)).as("a").has("age", lt(10).or(gt(100))).as("b")},
                 //
                 {V().match(as("a").has("age", 10), as("a").filter(has("name")).as("b")), V().as("a").has("age", 10).match(as("a").has("name").as("b"))},
                 {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"))},

http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/6da9151e/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategyTest.java
----------------------------------------------------------------------
diff --git a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategyTest.java b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategyTest.java
index d2bc0ea..9f97466 100644
--- a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategyTest.java
+++ b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/process/traversal/strategy/optimization/TinkerGraphStepStrategyTest.java
@@ -48,7 +48,6 @@ import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.filter
 import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.has;
 import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.not;
 import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.properties;
-import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.values;
 import static org.junit.Assert.assertEquals;
 
 /**
@@ -111,7 +110,7 @@ public class TinkerGraphStepStrategyTest {
                 {__.V().as("a").dedup().has("name", "marko").or(has("age"), has("age", gt(32))).filter(has("name", "bob")).has("lang", "java"),
                         g_V("name", eq("marko"), "name", eq("bob"), "lang", eq("java")).as("a").or(has("age"), has("age", gt(32))).dedup(), Arrays.asList(InlineFilterStrategy.instance(), FilterRankingStrategy.instance())},
                 {__.V().as("a").dedup().has("name", "marko").or(has("age", 10), has("age", gt(32))).filter(has("name", "bob")).has("lang", "java"),
-                        g_V("name", eq("marko"), "name", eq("bob"), "lang", eq("java")).as("a").or(has("age", 10), has("age", gt(32))).dedup(), TraversalStrategies.GlobalCache.getStrategies(TinkerGraph.class).toList()},
+                        g_V("name", eq("marko"), "age", eq(10).or(gt(32)), "name", eq("bob"), "lang", eq("java")).as("a").dedup(), TraversalStrategies.GlobalCache.getStrategies(TinkerGraph.class).toList()},
                 {__.V().has("name", "marko").or(not(has("age")), has("age", gt(32))).has("name", "bob").has("lang", "java"),
                         g_V("name", eq("marko"), "name", eq("bob"), "lang", eq("java")).or(not(filter(properties("age"))), has("age", gt(32))), TraversalStrategies.GlobalCache.getStrategies(TinkerGraph.class).toList()}
         });