fixed a JavaTranslator bug where Bytecode instructions were being mutated 
during translation. This is because I was accessing an Object[] and not cloning 
it first. Dar. Added BytecodeTest which tests 
hashCode/equality/bindings/clone/etc. Also, for @spmallette, I added a bytecode 
cache benchmark to TinkerGraphPlayTest.testPlay8(). CTR.


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

Branch: refs/heads/TINKERPOP-1280
Commit: 13782aad50b3a41c252cab0f7d45ce76b359eb59
Parents: d659e15
Author: Marko A. Rodriguez <okramma...@gmail.com>
Authored: Tue Sep 13 13:04:44 2016 -0600
Committer: Marko A. Rodriguez <okramma...@gmail.com>
Committed: Tue Sep 13 13:04:44 2016 -0600

----------------------------------------------------------------------
 CHANGELOG.asciidoc                              |  1 +
 .../gremlin/jsr223/JavaTranslator.java          | 37 ++++----
 .../gremlin/process/traversal/BytecodeTest.java | 97 ++++++++++++++++++++
 .../structure/TinkerGraphPlayTest.java          | 59 +++++++-----
 4 files changed, 155 insertions(+), 39 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/13782aad/CHANGELOG.asciidoc
----------------------------------------------------------------------
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index 9998a24..24ed601 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 `JavaTranslator` bug where `Bytecode` instructions were being 
mutated during translation.
 * Added `Path` to Gremlin-Python with respective GraphSON 2.0 deserializer.
 * Added missing `InetAddress` to GraphSON extension module.
 

http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/13782aad/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/JavaTranslator.java
----------------------------------------------------------------------
diff --git 
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/JavaTranslator.java
 
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/JavaTranslator.java
index c39896c..dac6583 100644
--- 
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/JavaTranslator.java
+++ 
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/JavaTranslator.java
@@ -116,43 +116,46 @@ public final class JavaTranslator<S extends 
TraversalSource, T extends Traversal
                 }
             }
         }
-        ///
+        // create a copy of the argument array so as not to mutate the 
original bytecode
+        final Object[] argumentsCopy = new Object[arguments.length];
         for (int i = 0; i < arguments.length; i++) {
             if (arguments[i] instanceof Bytecode.Binding)
-                arguments[i] = ((Bytecode.Binding) arguments[i]).value();
+                argumentsCopy[i] = ((Bytecode.Binding) arguments[i]).value();
             else if (arguments[i] instanceof Bytecode)
-                arguments[i] = translateFromAnonymous((Bytecode) arguments[i]);
+                argumentsCopy[i] = translateFromAnonymous((Bytecode) 
arguments[i]);
+            else
+                argumentsCopy[i] = arguments[i];
         }
         try {
             for (final Method method : methodCache.get(methodName)) {
                 if (returnType.isAssignableFrom(method.getReturnType())) {
-                    if (method.getParameterCount() == arguments.length || 
(method.getParameterCount() > 0 && 
method.getParameters()[method.getParameters().length - 1].isVarArgs())) {
+                    if (method.getParameterCount() == argumentsCopy.length || 
(method.getParameterCount() > 0 && 
method.getParameters()[method.getParameters().length - 1].isVarArgs())) {
                         final Parameter[] parameters = method.getParameters();
                         final Object[] newArguments = new 
Object[parameters.length];
                         boolean found = true;
                         for (int i = 0; i < parameters.length; i++) {
                             if (parameters[i].isVarArgs()) {
                                 final Class<?> parameterClass = 
parameters[i].getType().getComponentType();
-                                if (arguments.length > i && 
!parameterClass.isAssignableFrom(arguments[i].getClass())) {
+                                if (argumentsCopy.length > i && 
!parameterClass.isAssignableFrom(argumentsCopy[i].getClass())) {
                                     found = false;
                                     break;
                                 }
-                                Object[] varArgs = (Object[]) 
Array.newInstance(parameterClass, arguments.length - i);
+                                Object[] varArgs = (Object[]) 
Array.newInstance(parameterClass, argumentsCopy.length - i);
                                 int counter = 0;
-                                for (int j = i; j < arguments.length; j++) {
-                                    varArgs[counter++] = arguments[j];
+                                for (int j = i; j < argumentsCopy.length; j++) 
{
+                                    varArgs[counter++] = argumentsCopy[j];
                                 }
                                 newArguments[i] = varArgs;
                                 break;
                             } else {
-                                if (i < arguments.length &&
-                                        
(parameters[i].getType().isAssignableFrom(arguments[i].getClass()) ||
+                                if (i < argumentsCopy.length &&
+                                        
(parameters[i].getType().isAssignableFrom(argumentsCopy[i].getClass()) ||
                                                 
(parameters[i].getType().isPrimitive() &&
-                                                        
(Number.class.isAssignableFrom(arguments[i].getClass()) ||
-                                                                
arguments[i].getClass().equals(Boolean.class) ||
-                                                                
arguments[i].getClass().equals(Byte.class) ||
-                                                                
arguments[i].getClass().equals(Character.class))))) {
-                                    newArguments[i] = arguments[i];
+                                                        
(Number.class.isAssignableFrom(argumentsCopy[i].getClass()) ||
+                                                                
argumentsCopy[i].getClass().equals(Boolean.class) ||
+                                                                
argumentsCopy[i].getClass().equals(Byte.class) ||
+                                                                
argumentsCopy[i].getClass().equals(Character.class))))) {
+                                    newArguments[i] = argumentsCopy[i];
                                 } else {
                                     found = false;
                                     break;
@@ -166,8 +169,8 @@ public final class JavaTranslator<S extends 
TraversalSource, T extends Traversal
                 }
             }
         } catch (final Throwable e) {
-            throw new IllegalStateException(e.getMessage() + ":" + methodName 
+ "(" + Arrays.toString(arguments) + ")", e);
+            throw new IllegalStateException(e.getMessage() + ":" + methodName 
+ "(" + Arrays.toString(argumentsCopy) + ")", e);
         }
-        throw new IllegalStateException("Could not locate method: " + 
delegate.getClass().getSimpleName() + "." + methodName + "(" + 
Arrays.toString(arguments) + ")");
+        throw new IllegalStateException("Could not locate method: " + 
delegate.getClass().getSimpleName() + "." + methodName + "(" + 
Arrays.toString(argumentsCopy) + ")");
     }
 }

http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/13782aad/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/BytecodeTest.java
----------------------------------------------------------------------
diff --git 
a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/BytecodeTest.java
 
b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/BytecodeTest.java
new file mode 100644
index 0000000..0b9c65a
--- /dev/null
+++ 
b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/BytecodeTest.java
@@ -0,0 +1,97 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ */
+
+package org.apache.tinkerpop.gremlin.process.traversal;
+
+import 
org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__;
+import org.apache.tinkerpop.gremlin.structure.Column;
+import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyGraph;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+
+/**
+ * @author Marko A. Rodriguez (http://markorodriguez.com)
+ */
+public class BytecodeTest {
+
+    @Test
+    public void shouldHaveProperHashAndEquality() {
+        final GraphTraversalSource g = EmptyGraph.instance().traversal();
+        final Traversal.Admin traversal1 = 
g.V().out().repeat(__.out().in()).times(2).groupCount().by(__.outE().count()).select(Column.keys).order().by(Order.decr).asAdmin();
+        final Traversal.Admin traversal2 = 
g.V().out().repeat(__.out().in()).times(2).groupCount().by(__.outE().count()).select(Column.keys).order().by(Order.decr).asAdmin();
+        final Traversal.Admin traversal3 = 
g.V().out().repeat(__.out().in()).times(2).groupCount().by(__.outE().count()).select(Column.values).order().by(Order.decr).asAdmin();
+
+        assertEquals(traversal1, traversal2);
+        assertNotEquals(traversal1, traversal3);
+        assertNotEquals(traversal2, traversal3);
+        //
+        assertEquals(traversal1.hashCode(), traversal2.hashCode());
+        assertNotEquals(traversal1.hashCode(), traversal3.hashCode());
+        assertNotEquals(traversal2.hashCode(), traversal3.hashCode());
+        //
+        assertEquals(traversal1.getBytecode(), traversal2.getBytecode());
+        assertNotEquals(traversal1.getBytecode(), traversal3.getBytecode());
+        assertNotEquals(traversal2.getBytecode(), traversal3.getBytecode());
+        //
+        assertEquals(traversal1.getBytecode().hashCode(), 
traversal2.getBytecode().hashCode());
+        assertNotEquals(traversal1.getBytecode().hashCode(), 
traversal3.getBytecode().hashCode());
+        assertNotEquals(traversal2.getBytecode().hashCode(), 
traversal3.getBytecode().hashCode());
+
+    }
+
+    @Test
+    public void shouldCloneCorrectly() {
+        final GraphTraversalSource g = EmptyGraph.instance().traversal();
+        final Bytecode bytecode = g.V().out().asAdmin().getBytecode();
+        final Bytecode bytecodeClone = bytecode.clone();
+        assertEquals(bytecode, bytecodeClone);
+        assertEquals(bytecode.hashCode(), bytecodeClone.hashCode());
+        bytecodeClone.addStep("in", "created"); // mutate the clone and the 
original should stay the same
+        assertNotEquals(bytecode, bytecodeClone);
+        assertNotEquals(bytecode.hashCode(), bytecodeClone.hashCode());
+        assertEquals(2, IteratorUtils.count(bytecode.getInstructions()));
+        assertEquals(3, IteratorUtils.count(bytecodeClone.getInstructions()));
+    }
+
+    @Test
+    public void shouldIncludeBindingsInEquality() {
+        final Bindings b = new Bindings();
+        final GraphTraversalSource g = 
EmptyGraph.instance().traversal().withBindings(b);
+
+        final Bytecode bytecode1 = g.V().out(b.of("a", 
"created")).asAdmin().getBytecode();
+        final Bytecode bytecode2 = g.V().out(b.of("a", 
"knows")).asAdmin().getBytecode();
+        final Bytecode bytecode3 = g.V().out(b.of("b", 
"knows")).asAdmin().getBytecode();
+        final Bytecode bytecode4 = g.V().out(b.of("b", 
"knows")).asAdmin().getBytecode();
+
+        assertNotEquals(bytecode1, bytecode2);
+        assertNotEquals(bytecode1, bytecode3);
+        assertNotEquals(bytecode2, bytecode3);
+        assertNotEquals(bytecode2, bytecode4);
+        assertNotEquals(bytecode1, bytecode4);
+        assertEquals(bytecode3, bytecode4);
+
+        assertEquals(1, bytecode1.getBindings().size());
+        assertEquals("created", bytecode1.getBindings().get("a"));
+
+    }
+}

http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/13782aad/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphPlayTest.java
----------------------------------------------------------------------
diff --git 
a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphPlayTest.java
 
b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphPlayTest.java
index 9f2b12a..94fd250 100644
--- 
a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphPlayTest.java
+++ 
b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphPlayTest.java
@@ -22,29 +22,30 @@ import org.apache.tinkerpop.gremlin.jsr223.JavaTranslator;
 import org.apache.tinkerpop.gremlin.process.computer.Computer;
 import org.apache.tinkerpop.gremlin.process.traversal.Bytecode;
 import org.apache.tinkerpop.gremlin.process.traversal.Operator;
-import org.apache.tinkerpop.gremlin.process.traversal.P;
+import org.apache.tinkerpop.gremlin.process.traversal.Order;
 import org.apache.tinkerpop.gremlin.process.traversal.Scope;
 import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
 import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal;
 import 
org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
 import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__;
 import 
org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.PathRetractionStrategy;
+import org.apache.tinkerpop.gremlin.structure.Column;
 import org.apache.tinkerpop.gremlin.structure.Graph;
 import org.apache.tinkerpop.gremlin.structure.T;
 import org.apache.tinkerpop.gremlin.structure.Vertex;
 import org.apache.tinkerpop.gremlin.structure.io.graphml.GraphMLIo;
-import org.apache.tinkerpop.gremlin.structure.io.graphson.GraphSONReader;
-import org.apache.tinkerpop.gremlin.structure.io.graphson.GraphSONWriter;
 import org.apache.tinkerpop.gremlin.util.TimeUtil;
 import org.junit.Ignore;
 import org.junit.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import java.io.ByteArrayInputStream;
-import java.io.ByteArrayOutputStream;
 import java.util.Arrays;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.function.Supplier;
 
 import static org.apache.tinkerpop.gremlin.process.traversal.P.lt;
@@ -61,7 +62,6 @@ import static 
org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.outE;
 import static 
org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.select;
 import static 
org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.union;
 import static 
org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.valueMap;
-import static org.apache.tinkerpop.gremlin.util.function.Lambda.function;
 
 /**
  * @author Stephen Mallette (http://stephen.genoprime.com)
@@ -73,24 +73,39 @@ public class TinkerGraphPlayTest {
     @Ignore
     public void testPlay8() throws Exception {
         Graph graph = TinkerFactory.createModern();
-        GraphTraversalSource g = graph.traversal().withComputer();
+        GraphTraversalSource g = graph.traversal();
 
-        Traversal<?, ?> traversal1 = g.V().has("age", 
P.gt(10).and(P.lt(30))).out("knows", 
"created").repeat(__.as("a").out().as("b").hasLabel("software")).times(1).select("b").by(T.label).groupCount().map(function("a.get()"));
-        Bytecode bytecode1 = traversal1.asAdmin().getBytecode();
-        System.out.println("BYTECODE 1: \n  " + bytecode1 + "\n");
-        final ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
-        GraphSONWriter.build().create().writeObject(outputStream, bytecode1);
-        //
-        System.out.println("GRAPHSON BYTECODE: \n  " + new 
String(outputStream.toByteArray()) + "\n");
-        //
-        Traversal<?, ?> traversal2 = 
JavaTranslator.of(graph.traversal()).translate(GraphSONReader.build().create().readObject(new
 ByteArrayInputStream(outputStream.toByteArray()), Bytecode.class));
-        Bytecode bytecode2 = traversal2.asAdmin().getBytecode();
-        System.out.println("BYTECODE 2: \n  " + bytecode2 + "\n");
-        assert traversal1.equals(traversal2);
-        assert bytecode1.equals(bytecode2);
-        System.out.println("RESULT: \n  " + traversal2.toList());
-    }
+        final Traversal<?, ?> traversal = 
g.V().repeat(out()).times(2).groupCount().by("name").select(Column.keys).order().by(Order.decr);
+        final Bytecode bytecode = traversal.asAdmin().getBytecode();
+
+        final Map<Bytecode, Traversal.Admin<?, ?>> cache = new HashMap<>();
+        cache.put(bytecode, traversal.asAdmin());
+        final HashSet<?> result = new LinkedHashSet<>(Arrays.asList("ripple", 
"lop"));
 
+        System.out.println("BYTECODE: " + bytecode + "\n");
+        System.out.println("Bytecode->Traversal.clone() cache: " + 
TimeUtil.clock(1000, () -> {
+            final Traversal.Admin<?, ?> t = cache.get(bytecode).clone();
+            //assertEquals(result, t.next());
+        }));
+
+        System.out.println("Bytecode->JavaTranslator call    : " + 
TimeUtil.clock(1000, () -> {
+            final Traversal t = JavaTranslator.of(g).translate(bytecode);
+            //assertEquals(result, t.next());
+        }));
+
+        System.out.println("\n==Second test with reversed execution==\n");
+
+        System.out.println("BYTECODE: " + bytecode + "\n");
+        System.out.println("Bytecode->JavaTranslator call    : " + 
TimeUtil.clock(1000, () -> {
+            final Traversal t = JavaTranslator.of(g).translate(bytecode);
+            //assertEquals(result, t.next());
+        }));
+
+        System.out.println("Bytecode->Traversal.clone() cache: " + 
TimeUtil.clock(1000, () -> {
+            final Traversal.Admin<?, ?> t = cache.get(bytecode).clone();
+            //assertEquals(result, t.next());
+        }));
+    }
 
     @Test
     @Ignore

Reply via email to