Merge ecc5863 into 353aaf8

README.md

@@ -6,7 +6,7 @@ TP3 Graph Structure Implementation for OrientDB. This started off as just a proo
 
 Warning: While this is (as of now) the only TP3 graph structure implementation for OrientDB, it's not the `official` one - it's not supported by the Orient team. The contributors focused on the functionality needed for their use cases, and it doesn't claim to be complete. 
 
-The main area that need some more work is index lookups - currently it does find the right index for a simple case, e.g. `g.V.hasLabel("myLabel").has("someKey", "someValue")`. However if there are multiple indexes on the same property, or if there the traversal should better use a composite index, that's not handled well yet. If you feel inclined you can add these cases to the OrientGraphIndexTest.java.
+The main area that need some more work is index lookups - currently it does find the right index for a simple case, e.g. `g.V.hasLabel("myLabel").has("someKey", "someValue")`. However if there are multiple indexes on the same property, or if there the traversal should better use a composite index, that's not handled well yet. If you feel inclined you can add these cases to the `OrientGraphIndexTest.java`. The function that looks up indexes is `OrientGraphStep.findIndex`.
 
 ## Tests
 * you can run the standard tinkerpop test suite with `mvn install -P release`

driver/pom.xml

@@ -175,7 +175,7 @@
             <plugin>
                 <groupId>com.marvinformatics.formatter</groupId>
                 <artifactId>formatter-maven-plugin</artifactId>
-                <version>1.6.0.RC4</version>
+                <version>1.8.0</version>
                 <configuration>
                     <sourceDirectory>${project.basedir}</sourceDirectory>
                     <excludes>

driver/src/main/java/org/apache/tinkerpop/gremlin/orientdb/OrientGraph.java

@@ -90,7 +90,7 @@ public static OrientGraph open(final Configuration config) {
         if (config.containsKey(CONFIG_POOL_SIZE))
             if (config.containsKey(CONFIG_MAX_PARTITION_SIZE))
             factory.setupPool(config.getInt(CONFIG_MAX_PARTITION_SIZE), config.getInt(CONFIG_POOL_SIZE));
-        else
+            else
                 factory.setupPool(config.getInt(CONFIG_POOL_SIZE));
 
         return factory.getNoTx();
@@ -262,48 +262,45 @@ protected Object convertValue(final OIndex<?> idx, Object iValue) {
         return iValue;
     }
 
-    public Stream<OrientVertex> getIndexedVertices(OIndex<Object> index, Optional<Object> valueOption) {
-        return getIndexedElements(index, valueOption, OrientVertex::new);
+    public Stream<OrientVertex> getIndexedVertices(OIndex<Object> index, Iterator<Object> valueIter) {
+        return getIndexedElements(index, valueIter, OrientVertex::new);
     }
 
-    public Stream<OrientEdge> getIndexedEdges(OIndex<Object> index, Optional<Object> valueOption) {
-        return getIndexedElements(index, valueOption, OrientEdge::new);
+    public Stream<OrientEdge> getIndexedEdges(OIndex<Object> index, Iterator<Object> valueIter) {
+        return getIndexedElements(index, valueIter, OrientEdge::new);
     }
 
-    private <ElementType extends OrientElement> Stream<ElementType> getIndexedElements(OIndex<Object> index, Optional<Object> valueOption,
+    private <ElementType extends OrientElement> Stream<ElementType> getIndexedElements(
+            OIndex<Object> index,
+            Iterator<Object> valuesIter,
             BiFunction<OrientGraph, OIdentifiable, ElementType> newElement) {
         return executeWithConnectionCheck(() -> {
             makeActive();
 
             if (index == null) {
-                // NO INDEX
                 return Collections.<ElementType> emptyList().stream();
             } else {
-                if (!valueOption.isPresent()) {
+                if (!valuesIter.hasNext()) {
                     return index.cursor().toValues().stream().map(id -> newElement.apply(this, id));
                 } else {
-                    Object value = convertValue(index, valueOption.get());
-                    Object indexValue = index.get(value);
-                    if (indexValue == null) {
-                        return Collections.<ElementType> emptyList().stream();
-                    } else if (!(indexValue instanceof Iterable<?>)) {
-                        indexValue = Collections.singletonList(indexValue);
-                    }
-
-                    // The index value is iterable, but some indices will give ORecordId 
-                    // objects (e.g. OIndexTxAwareOneValue and OIndexTxAwareMultiValue) whilst others will 
-                    // give ORecord objects (e.g. OIndexRemoteMultiValue). 
-                    // Thankfully both ORecordId and ORecord implement OIdentifiable.
-                    @SuppressWarnings("unchecked")
-                    Iterable<OIdentifiable> iterableVals = (Iterable<OIdentifiable>) indexValue;
-                    Stream<OIdentifiable> ids = StreamSupport.stream(iterableVals.spliterator(), false);
-                    Stream<ORecord> records = ids.map(id -> (ORecord) id.getRecord()).filter(r -> r != null);
-                    return records.map(r -> newElement.apply(this, getRawDocument(r)));
+                    Stream<Object> convertedValues = StreamUtils.asStream(valuesIter).map(value -> convertValue(index, value));
+                    Stream<OIdentifiable> ids = convertedValues.flatMap(v -> lookupInIndex(index, v));
+                    Stream<ORecord> records = ids.map(id -> id.getRecord());
+                    Stream<ORecord> recordsWithoutNulls = records.filter(r -> r != null);
+                    return recordsWithoutNulls.map(r -> newElement.apply(this, getRawDocument(r)));
                 }
             }
         });
     }
 
+    private Stream<OIdentifiable> lookupInIndex(OIndex<Object> index, Object value) {
+        Object fromIndex = index.get(value);
+        if (fromIndex instanceof Iterable)
+            return StreamUtils.asStream(((Iterable) fromIndex).iterator());
+        else
+            return Stream.of((OIdentifiable) fromIndex);
+    }
+
     private OIndexManager getIndexManager() {
         return database.getMetadata().getIndexManager();
     }

driver/src/main/java/org/apache/tinkerpop/gremlin/orientdb/OrientIndexQuery.java

@@ -1,18 +1,20 @@
 package org.apache.tinkerpop.gremlin.orientdb;
 
 import com.orientechnologies.orient.core.index.OIndex;
+
+import java.util.Iterator;
 import java.util.Optional;
 
 public class OrientIndexQuery {
-    public final Optional<Object> value;
+    public final Iterator<Object> values;
     public final OIndex index;
 
-    public OrientIndexQuery(OIndex index, Optional<Object> value) {
+    public OrientIndexQuery(OIndex index, Iterator<Object> values) {
         this.index = index;
-        this.value = value;
+        this.values = values;
     }
 
     public String toString() {
-        return "OrientIndexQuery(index=" + index + ", value=" + value + ")";
+        return "OrientIndexQuery(index=" + index + ")";
     }
 }

driver/src/main/java/org/apache/tinkerpop/gremlin/orientdb/OrientVertexProperty.java

@@ -16,7 +16,7 @@
 import com.orientechnologies.orient.core.metadata.schema.OType;
 import com.orientechnologies.orient.core.record.impl.ODocument;
 
-public class OrientVertexProperty<V> extends OrientProperty<V>implements VertexProperty<V> {
+public class OrientVertexProperty<V> extends OrientProperty<V> implements VertexProperty<V> {
 
     public OrientVertexProperty(Property<V> property, OrientVertex vertex) {
         super(property.key(), property.value(), vertex);

driver/src/main/java/org/apache/tinkerpop/gremlin/orientdb/traversal/step/sideEffect/OrientGraphStep.java

@@ -1,23 +1,16 @@
 package org.apache.tinkerpop.gremlin.orientdb.traversal.step.sideEffect;
 
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Optional;
-import java.util.Set;
+import java.util.*;
 import java.util.function.BiFunction;
-import java.util.function.BiPredicate;
 import java.util.function.Function;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
-import org.apache.tinkerpop.gremlin.orientdb.OrientEdge;
+import org.apache.tinkerpop.gremlin.groovy.loaders.ObjectLoader;
 import org.apache.tinkerpop.gremlin.orientdb.OrientGraph;
 import org.apache.tinkerpop.gremlin.orientdb.OrientIndexQuery;
-import org.apache.tinkerpop.gremlin.orientdb.OrientVertex;
 import org.apache.tinkerpop.gremlin.process.traversal.Compare;
+import org.apache.tinkerpop.gremlin.process.traversal.Contains;
 import org.apache.tinkerpop.gremlin.process.traversal.step.HasContainerHolder;
 import org.apache.tinkerpop.gremlin.process.traversal.step.map.GraphStep;
 import org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer;
@@ -27,6 +20,7 @@
 import org.apache.tinkerpop.gremlin.structure.Vertex;
 import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
 import org.apache.tinkerpop.gremlin.util.function.TriFunction;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
 import org.javatuples.Pair;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -35,7 +29,7 @@
 import com.orientechnologies.orient.core.index.OIndexManagerProxy;
 import com.orientechnologies.orient.core.metadata.schema.OImmutableClass;
 
-public class OrientGraphStep<S, E extends Element> extends GraphStep<S, E>implements HasContainerHolder {
+public class OrientGraphStep<S, E extends Element> extends GraphStep<S, E> implements HasContainerHolder {
 
     private final List<HasContainer> hasContainers = new ArrayList<>();
 
@@ -73,8 +67,9 @@ private Iterator<? extends Edge> edges() {
      *            the edges (i.e. full scan)
      * @return An iterator for all the vertices/edges for this step
      */
-    private <ElementType extends Element> Iterator<? extends ElementType> elements(BiFunction<OrientGraph, Object[], Iterator<ElementType>> getElementsByIds,
-            TriFunction<OrientGraph, OIndex<Object>, Optional<Object>, Stream<? extends ElementType>> getElementsByIndex,
+    private <ElementType extends Element> Iterator<? extends ElementType> elements(
+            BiFunction<OrientGraph, Object[], Iterator<ElementType>> getElementsByIds,
+            TriFunction<OrientGraph, OIndex<Object>, Iterator<Object>, Stream<? extends ElementType>> getElementsByIndex,
             Function<OrientGraph, Iterator<ElementType>> getAllElements) {
         final OrientGraph graph = getGraph();
 
@@ -88,7 +83,7 @@ private <ElementType extends Element> Iterator<? extends ElementType> elements(B
                 OrientIndexQuery indexQuery = indexQueryOption.get();
                 OLogManager.instance().debug(this, "using " + indexQuery);
 
-                Stream<? extends ElementType> indexedElements = getElementsByIndex.apply(graph, indexQuery.index, indexQuery.value);
+                Stream<? extends ElementType> indexedElements = getElementsByIndex.apply(graph, indexQuery.index, indexQuery.values);
                 return indexedElements
                         .filter(element -> HasContainer.testAll(element, this.hasContainers))
                         .collect(Collectors.<ElementType> toList())
@@ -111,7 +106,7 @@ private boolean isLabelKey(String key) {
 
     // if one of the HasContainers is a label matching predicate, then return
     // that label
-    private Optional<String> findElementLabelInHasContainers() {
+    private Optional<String> findClassLabelInHasContainers() {
         return this.hasContainers.stream()
                 .filter(hasContainer -> isLabelKey(hasContainer.getKey()))
                 .findFirst()
@@ -124,39 +119,54 @@ private OrientGraph getGraph() {
 
     @VisibleForTesting
     public Optional<OrientIndexQuery> findIndex() {
-        final Optional<String> elementLabel = findElementLabelInHasContainers();
         final OrientGraph graph = getGraph();
         final OIndexManagerProxy indexManager = graph.database().getMetadata().getIndexManager();
 
-        // find indexed keys only for the element subclass (if present)
-        final Set<String> indexedKeys = elementLabel.isPresent() ? graph.getIndexedKeys(this.returnClass, elementLabel.get()) : graph.getIndexedKeys(this.returnClass);
-
-        Optional<Pair<String, Object>> indexedKeyAndValue = this.hasContainers.stream()
-                .filter(c -> indexedKeys.contains(c.getKey()) && c.getPredicate().getBiPredicate() == Compare.eq)
-                .findAny()
-                .map(c -> Optional.of(new Pair<>(c.getKey(), c.getValue())))
-                .orElseGet(Optional::empty);
-
-        if (elementLabel.isPresent() && indexedKeyAndValue.isPresent()) {
-            String key = indexedKeyAndValue.get().getValue0();
-            Object value = indexedKeyAndValue.get().getValue1();
-
-            String className = graph.labelToClassName(elementLabel.get(), isVertexStep() ? OImmutableClass.VERTEX_CLASS_NAME : OImmutableClass.EDGE_CLASS_NAME);
-            Set<OIndex<?>> classIndexes = indexManager.getClassIndexes(className);
-            Iterator<OIndex<?>> keyIndexes = classIndexes.stream().filter(idx -> idx.getDefinition().getFields().contains(key)).iterator();
-
-            if (keyIndexes.hasNext()) {
-                // TODO: implement algorithm to select best index if there are
-                // multiple
-                return Optional.of(new OrientIndexQuery(keyIndexes.next(), Optional.of(value)));
-            } else {
-                OLogManager.instance().warn(this, "no index found for class=[" + className + "] and key=[" + key + "]");
+        // find indexed keys only for the element subclasses (if present)
+        final Optional<String> classLabel = findClassLabelInHasContainers();
+
+        if (classLabel.isPresent()) {
+            final Set<String> indexedKeys = classLabel.isPresent() ? graph.getIndexedKeys(this.returnClass, classLabel.get()) : graph.getIndexedKeys(this.returnClass);
+
+            Optional<Pair<String, Iterator<Object>>> requestedKeyValue = this.hasContainers.stream()
+                    .filter(c -> indexedKeys.contains(c.getKey()) && (c.getPredicate().getBiPredicate() == Compare.eq ||
+                            c.getPredicate().getBiPredicate() == Contains.within))
+                    .findAny()
+                    .map(c -> getValuePair(c))
+                    .orElseGet(Optional::empty);
+
+            if (requestedKeyValue.isPresent()) {
+                String key = requestedKeyValue.get().getValue0();
+                Iterator<Object> values = requestedKeyValue.get().getValue1();
+
+                String className = graph.labelToClassName(classLabel.get(), isVertexStep() ? OImmutableClass.VERTEX_CLASS_NAME : OImmutableClass.EDGE_CLASS_NAME);
+                Set<OIndex<?>> classIndexes = indexManager.getClassIndexes(className);
+                Iterator<OIndex<?>> keyIndexes = classIndexes.stream().filter(idx -> idx.getDefinition().getFields().contains(key)).iterator();
+
+                if (keyIndexes.hasNext()) {
+                    // TODO: select best index if there are multiple options
+                    return Optional.of(new OrientIndexQuery(keyIndexes.next(), values));
+                } else {
+                    OLogManager.instance().warn(this, "no index found for class=[" + className + "] and key=[" + key + "]");
+                }
             }
         }
 
         return Optional.empty();
     }
 
+    /** gets the requested values from the Has step. If it's a single value, wrap it in an array, otherwise return the array
+     *  */
+    private Optional<Pair<String, Iterator<Object>>> getValuePair(HasContainer c) {
+        Iterator<Object> values;
+        if (c.getPredicate().getBiPredicate() == Contains.within)
+            values = ((Iterable<Object>) c.getValue()).iterator();
+        else
+            values = IteratorUtils.of(c.getValue());
+        return Optional.of(new Pair<>(c.getKey(), values));
+
+    }
+
     @Override
     public String toString() {
         if (this.hasContainers.isEmpty())

driver/src/test/java/org/apache/tinkerpop/gremlin/orientdb/OrientGraphIndexTest.java

@@ -105,6 +105,37 @@ public void vertexUniqueIndexLookupWithValue() {
         Assert.assertEquals(v1.id(), result.get(0).id());
     }
 
+    @Test
+    public void vertexUniqueIndexLookupWithMultipleValues() {
+        OrientGraph graph = newGraph();
+        createVertexIndexLabel1(graph);
+        // verify index created
+        Assert.assertEquals(graph.getIndexedKeys(Vertex.class, vertexLabel1), new HashSet<>(Collections.singletonList(key)));
+
+        String value1 = "value1";
+        String value2 = "value2";
+        String value3 = "value3";
+
+        Vertex v1 = graph.addVertex(T.label, vertexLabel1, key, value1);
+        Vertex v2 = graph.addVertex(T.label, vertexLabel1, key, value2);
+        Vertex v3 = graph.addVertex(T.label, vertexLabel1, key, value3);
+
+        // looking deep into the internals here - I can't find a nicer way to
+        // auto verify that an index is actually used
+        // GraphTraversal<Vertex, Vertex> traversal = graph.traversal().V().has(T.label, P.eq(vertexLabel1)).has(key, P.eq(value1));
+        GraphTraversal<Vertex, Vertex> traversal = graph.traversal().V().has(T.label, P.eq(vertexLabel1)).has(key, P.within(value1, value2));
+
+        OIndex index = findUsedIndex(traversal).get().index;
+        Assert.assertEquals(3, index.getSize());
+        Assert.assertEquals(v1.id(), index.get(value1));
+        Assert.assertEquals(v2.id(), index.get(value2));
+
+        List<Vertex> result = traversal.toList();
+        Assert.assertEquals(2, result.size());
+        Assert.assertEquals(v1.id(), result.get(0).id());
+        Assert.assertEquals(v2.id(), result.get(1).id());
+    }
+
     @Test
     public void edgeUniqueIndexLookupWithValue() {
         OrientGraph graph = newGraph();