[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/922 PR for this is now on #952 ---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/922 @mattallenuk I figured out what was going on. The `EnumValue` was being interpreted as a `Map` when serializing to GraphSON. The change is pretty trivial, but I have a bunch of other little changes here/there. I'm going to merge this PR to a TINKERPOP-1959 branch for review (hope that's ok with you) and hopefully we can get this merged up today. ---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/922 "gremlin-groovy" is fine. The server should default to that if the argument isn't present. ---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user mattallenuk commented on the issue: https://github.com/apache/tinkerpop/pull/922 @spmallette I can confirm that this test works for bytecode submission so possibly an issue in the way the bindings are being submitted? Should the language parameter be gremlin-groovy or something else? ---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user mattallenuk commented on the issue: https://github.com/apache/tinkerpop/pull/922 @spmallette Ok, have pushed the changes along with the documentation changes we'd discussed. Hopefully it's an easy fix. ---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/922
It works with Java and GraphSON 2.0:
```text
gremlin> cluster = Cluster.build().serializer("GRAPHSON_V2D0").create()
==>localhost/127.0.0.1:8182
gremlin> client = cluster.connect()
==>org.apache.tinkerpop.gremlin.driver.Client$ClusteredClient@24a298a6
gremlin> client.submit("g.addV().property(c,k,v)",
[c:set,k:'name',v:123]).all().get()
==>result{object=v[4]
class=org.apache.tinkerpop.gremlin.structure.util.detached.DetachedVertex}
gremlin> client.submit("g.V().has('name',123).count()").all().get()
==>result{object=1 class=java.lang.Long}
```
So, i'm not sure what's wrong. If you want to commit/push what you have i
can try to test on my own to see what I (or maybe someone else) can figure out.
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user mattallenuk commented on the issue:
https://github.com/apache/tinkerpop/pull/922
@spmallette Ok so your above code runs fine and returns a value. It appears
that my call to g.V(1).property() is not liked.
My request looks like this now:
`[RequestMessage{, requestId=7baf2b77-ca79-4679-9812-f3e0d8d3dbb5,
op='eval', processor='', args={gremlin=g.V(1).property(card, nm, val),
aliases={g=gmodern}, bindings={card={typeName=Cardinality, elementName=set},
nm=test, val=12}, language=gremlin-groovy,
accept=application/vnd.gremlin-v2.0+json}}]`
The full error I get is as follows:
`java.lang.IllegalArgumentException: The provided key/value array length
must be a multiple of two
at
org.apache.tinkerpop.gremlin.structure.Element$Exceptions.providedKeyValuesMustBeAMultipleOfTwo
(Element.java:125)
at
org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters.set
(Parameters.java:175)
at
org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.AddPropertyStep.addPropertyMutations
(AddPropertyStep.java:80)
at
org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal.property
(GraphTraversal.java:2066)
at
org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal.property
(GraphTraversal.java:2094)
at sun.reflect.NativeMethodAccessorImpl.invoke0 (Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke
(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke
(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke (Method.java:498)
at
org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite$PojoCachedMethodSite.invoke
(PojoMetaMethodSite.java:192)
at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite.call
(PojoMetaMethodSite.java:56)
at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall
(CallSiteArray.java:47)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call
(AbstractCallSite.java:116)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call
(AbstractCallSite.java:144)
at Script7.run (Script7.groovy:1)
at
org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinGroovyScriptEngine.eval
(GremlinGroovyScriptEngine.java:856)
at
org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinGroovyScriptEngine.eval
(GremlinGroovyScriptEngine.java:561)
at javax.script.AbstractScriptEngine.eval
(AbstractScriptEngine.java:233)
at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.eval
(ScriptEngines.java:135)
at
org.apache.tinkerpop.gremlin.groovy.engine.GremlinExecutor.lambda$eval$0
(GremlinExecutor.java:291)
at java.util.concurrent.FutureTask.run (FutureTask.java:266)
at java.util.concurrent.Executors$RunnableAdapter.call
(Executors.java:511)
at java.util.concurrent.FutureTask.run (FutureTask.java:266)
at java.util.concurrent.ThreadPoolExecutor.runWorker
(ThreadPoolExecutor.java:1142)
at java.util.concurrent.ThreadPoolExecutor$Worker.run
(ThreadPoolExecutor.java:617)
at java.lang.Thread.run (Thread.java:748)`
What is the best way forward, your test proves that non native JS types are
being sent correctly, but my test should also work...
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/922
`addProperty()` looks right. On `tp32` I guess the default is GraphSON 2.0
which has this form for `Cardinality` values:
http://tinkerpop.apache.org/docs/3.2.9/dev/io/#_cardinality
I see that the `accept` is set to "application/json". On `tp32` I think
that defaults to GraphSON 1.0 which wouldn't be able to deal with complex
objects. You can only pass primitives that are compatible with JSON data types.
Does it work if you don't use `Cardinality`? maybe, just do something more
simple like:
js
connection.submit('x+y', { x: 1, y: 1 } );
```
If that works then change to GraphSON 2.0 -
"application/vnd.gremlin-v2.0+json" - and see what happens for complex stuff
like:
js
connection.submit(card.toString()', { card: t.cardinality.set } );
```
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user mattallenuk commented on the issue:
https://github.com/apache/tinkerpop/pull/922
I've just tried it not parsing the bindings to strings and get the same
issue. So I'm wondering now if I do need to parse the parameters or if I'm just
calling the addProperty function wrong?
`[RequestMessage{, requestId=27ad0188-8b5a-40e0-bf52-b158a1e92507,
op='eval', processor='', args={gremlin=g.V(1).property(card, nm, val),
aliases={g=gmodern}, bindings={card={typeName=Cardinality, elementName=set},
nm=test, val=12}, language=gremlin-groovy, accept=application/json}}].`
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user mattallenuk commented on the issue: https://github.com/apache/tinkerpop/pull/922 @spmallette Doing better now thank you! ---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user mattallenuk commented on the issue:
https://github.com/apache/tinkerpop/pull/922
@jorgebay @spmallette So I've written the following test, however I'm
getting a Provided key/value array length must be a multiple of two when
submitting. I am guessing this is due to submitting the cardinality as a string
value. How should non-native JS types be submitted via script?
`connection.submit('g.V(vert).property(card, nm, val)', { vert: 1, card:
t.cardinality.set, nm: 'test', val: 12 } )`
this produces:
[RequestMessage{, requestId='reqid', op='eval', processor='',
args={gremlin=g.V(1).property(card, nm, val), aliases={g=gmodern},
bindings={vert=1, card=set, nm=test, val=12}, language=gremlin-groovy,
accept=application/json}}]
---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/922 sorry to hear about the sickness. i hope your doing well now. you're right about the documentation. i think we wanted to focus folks on bytecode based traversals. we do have a section for gremlin-python for submitting scripts, but i think i discouraged the one for .NET. I'd forgotten that. I'd had the idea that i was going to massively rework the documentation on 3.4.0 to better organize all this GLV stuff and that hasn't fully happened yet. ok, so, i guess you just need the test/serialization fixups then, a changelog entry and a note in upgrade docs to say that this is now there...perhaps just add some example usage there. thanks ---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user mattallenuk commented on the issue: https://github.com/apache/tinkerpop/pull/922 @spmallette, my apologies I've been unwell the last week. I have done the test but it showed that I need to parse the bindings so that the standard Op processor can read them, it doesn't appear to like bytecode bindings. I'm hoping to have that done and tested today. On the documentation side I noticed that there doesn't appear to be any documentation in the other glv sections about submitting scripts, am I okay to add this documentation to the javascript glv section? ---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/922 hi @mattallenuk - Judging from the comments it seems like the only thing missing here code-wise is an added test. Administratively, there's some documentation (reference docs, upgrade docs, changelog entry) to add for this feature as well. I don't mean to pressure or rush you but do you happen to have any idea when you might come back to this to finish up the final bits? I mainly ask because we are in the midst of planning the release of 3.2.10 and 3.3.4 and we'd like to see this work of yours included. If your schedule doesn't allow for you to come back to this right now, perhaps I could assist in polishing up the final odds/ends for you? ---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user jorgebay commented on the issue: https://github.com/apache/tinkerpop/pull/922 > I haven't considered the impact of this upon Bytecode so this may need to be changed or addressed on the Bytecode side, or Bytecode may already handle parameter maps? The Bytecode generation already has a way to denote parameters embedded in the query, so it's ok to implement this independently. ---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user mattallenuk commented on the issue: https://github.com/apache/tinkerpop/pull/922 Hiya guys, I have moved the code out of the existing classes and have created a Client class that can handle script sending and a Translator class that can convert bytecode into a string. The Translator class accepts bindings in the form of a javascript object passed into the step function and converts it into Gremlin Groovy Tuple in the translated script. I haven't considered the impact of this upon Bytecode so this may need to be changed or addressed on the Bytecode side, or Bytecode may already handle parameter maps? In terms of consumption is there a way we could make the Translator more transparent to the user, I did have an eval function added to Traversal which did the conversion and sending, I removed this but would that be an acceptable solution? It's not inline with other GLV's unfortunately. ---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user jorgebay commented on the issue: https://github.com/apache/tinkerpop/pull/922 Glad to see you working on this @mattallenuk ! In line with what @spmallette was mentioning, I think we need expose a similar API as the rest of GLVs. Python GLV is a good example (because its a scripting language as JavaScript) but if you have some understanding of C#, you can check out gremlin-dotnet as well. Besides the `RemoteConnection` the other GLVs expose a `Client` (one abstraction above), that is able to submit traversals in bytecode or in script to evaluate, you can reading here: - https://github.com/apache/tinkerpop/blob/tp32/gremlin-python/src/main/jython/gremlin_python/driver/client.py - https://github.com/apache/tinkerpop/blob/tp32/gremlin-python/src/main/jython/tests/driver/test_client.py#L39-L48 ---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user mattallenuk commented on the issue: https://github.com/apache/tinkerpop/pull/922 @spmallette thanks for the feedback. I'll wait on further input from @jorgebay and implement it to be more closely aligned with the other GLVs based on this and further feedback. ---
[GitHub] tinkerpop issue #922: TINKERPOP-1959: Gremlin Javascript ability to send a s...
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/922
Excellent - thanks for doing this. @jorgebay is the better person to handle
this review in full, but I'd like to offer a few points:
> Is it safe to assume that a returned value not containing a bulk property
was executed by an eval op?
I'm not sure I would call it "safe", but that is typically what happens. I
would say that it is not completely "safe" to do so because an `eval()` could
return anything and thus return a `BulkSet` and the user would expect that and
I guess it would unroll to individual items when you iterated which the user
wouldn't expect.
> Should every parameter be bound when building a dynamic script?
We refer to what you've done there as a `Translator` (i.e. something that
converts bytecode to the string representation of the Gremlin) and that's neat
that you have that. We don't implement that as a `toString()` on `Bytecode` but
perhaps that's ok though. FYI, here's what a `Translator` looks like in Java:
```groovy
String gremlin = GroovyTranslator.of("g").translate(bytecode)
```
I don't believe that our translators convert arguments to bindings. It
assumes them to be literals. We only construct bindings if the user constructs
them as first order `Bindings` in the Gremlin itself, but I don't know if we
have that concept here in javascript. For consistency sake, we might want to.
You can see the approach taken by Python to handle that as an example:
http://tinkerpop.apache.org/docs/current/reference/#_bindings
> Does the unit test need to be more thorough and check more varied
instructions and more complex glv queries/instructions?
On the JVM side we test the translators through the GLV suite (essentially
we use the JVM process test suite which is basically the same thing). Maybe
the same should be done here.
Separate from your questions, I'd offer that `g.eval()` isn't really in
line with how other GLV drivers submit scripts. While I'd agree it to be
convenient to have it work that way, I think it would be better to see that
working in a way more consistent with other GLVs.
---
