[GitHub] tinkerpop issue #386: TINKERPOP-1274: GraphSON 2.0 [revised]
Github user PommeVerte commented on the issue: https://github.com/apache/tinkerpop/pull/386 Ok cool if we're all ok with the new features then I'm satisfied. Code looked good as well VOTE: +1 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #386: TINKERPOP-1274: GraphSON 2.0 [revised]
Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/386 @newkek I understand. I like the `@value` concept -- its the JSON sub-object for creating the object (defined by `@type`). VOTE +1. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #386: TINKERPOP-1274: GraphSON 2.0 [revised]
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/386 VOTE +1 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #386: TINKERPOP-1274: GraphSON 2.0 [revised]
Github user PommeVerte commented on the issue: https://github.com/apache/tinkerpop/pull/386 I'm waiting to hear back from marko on the points he brought up to cast my vote :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #386: TINKERPOP-1274: GraphSON 2.0 [revised]
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/386 All tests pass with `docker/build.sh -t -n -i` - still going through code though. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #386: TINKERPOP-1274: GraphSON 2.0 [revised]
Github user newkek commented on the issue: https://github.com/apache/tinkerpop/pull/386 I just pushed corrected docs, and renamed everything "domain" to "namespace". Waiting for more consensus to change the default gremlin namespace. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #386: TINKERPOP-1274: GraphSON 2.0 [revised]
Github user PommeVerte commented on the issue: https://github.com/apache/tinkerpop/pull/386 Still reviewing I would like to finish going through the code :) will try and do this later in the day. But looking good so far --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #386: TINKERPOP-1274: GraphSON 2.0 [revised]
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/386 is that an official +1 from you @PommeVerte or are you still reviewing ? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #386: TINKERPOP-1274: GraphSON 2.0 [revised]
Github user PommeVerte commented on the issue: https://github.com/apache/tinkerpop/pull/386 I'm liking how this has turned out. I personally don't have much of an opinion in regards to assuming gremlin is the default namespace and getting rid of `g:` though it does have the merit of being self documenting. I don't know how much of a size difference this would represent. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #386: TINKERPOP-1274: GraphSON 2.0 [revised]
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/386 I think I agree with @newkek to push that off until we can get his massive PR reviewed/merged. Let's see if we can solve that separately. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #386: TINKERPOP-1274: GraphSON 2.0 [revised]
Github user newkek commented on the issue: https://github.com/apache/tinkerpop/pull/386 A remark: @okram requested `TraversalExplanation` to be included in the list of Graph objects. I've noticed that the current `TraversalExplanation` Graphson serializer serializes a `Traversal`. In order to follow the requirement of this GraphSON2.0 type system, we would have to write a deserializer for the `TraversalExplanation` (there isn't currently). However, the current 1.0 serializer serializes the Traversal as the `toString()` of the Traversal's step list, which means that it is not possible to deserialize to a Traversal with only such info. So I believe it would be more appropriate to wait for TP1278 before implementing the `TraversalExplanation` ser/de and leave it out of the scope of this PR. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #386: TINKERPOP-1274: GraphSON 2.0 [revised]
Github user newkek commented on the issue: https://github.com/apache/tinkerpop/pull/386 *NB: the conflicts for merge are caused by the CHANGELOG* --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #386: TINKERPOP-1274: GraphSON 2.0 [revised]
Github user newkek commented on the issue:
https://github.com/apache/tinkerpop/pull/386
Hey @okram ,
> 1. The term is "namespace" not "domain." In case that terminology is used
in the documentation and not just in the PR notes.
Ok, it's some places here and there in the code, I'll correct it.
> 2. Why do we have `@class` and `@type`.
`@class` shouldn't be anywhere near GraphSON 2.0 anymore, everything is
`@type`, if you've seen it somewhere, that should be corrected. While writing
that I realise I might have forgotten to update the "the-graph.asciidoc" doc...
so I'll correct it. But in the actual code it shouldn't be there anymore.
> 3. I read your notes, but its still not clear to me. Why do we have
`@value`? I think we [...]
So, taking a step back and I'm going to use the exampleof ByteCode. As you
can see in the `GraphSONSerializersV2d0`, serializers don't have to care
anymore about types, serializer are put into the context of "You're in a place
where you can write whatever you want, type is handled somewhere else for you".
For the ByteCode serializer it means it would open a Map, put a field
"language" and its value, put a field "argument" and its value, put a field
"value" (or call it "bytecode") and its value, close the map. When
deserializing, read the map, get the fields values, create the ByteCode object.
Let's admit for whatever reason the ByteCode serializer in the future needs
to change and instead of writing a Map, it needs to start by an Array where the
first element of the array is 'something', the rest is the Map described
before, and close the array. Doing this is currently possible with the current
format `{"@type":"typename", "@value":value}`, because the serializer is in a
place where it can write whatever it wants to.
Now let's consider another format: `{"@type":"typename", value}`. There,
the serializer is put in the context "You are currently in a **Map**, whatever
you write, it must be in the form of a key/value pair". The "potential future"
evolution of the ByteCode serializer (writing an array) *wouldn't* be possible.
For the sake of consistency, I think it is important to keep the format as is
> 4. I think we can save some bytes and still be readable if we make our
namespace g
Totally fine with `g`, again for the sake of consistency, I think it'd be
valuable to have a 'name space' all the time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #386: TINKERPOP-1274: GraphSON 2.0 [revised]
Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/386 First off. Thank you for doing this. Here are some notes. 1. The term is "namespace" not "domain." In case that terminology is used in the documentation and not just in the PR notes. 2. Why do we have `@class` and `@type`. Seems we should just pick one. And if the answer is cause `@class` is for Java object mapping infrastructure, that isn't good and we will also use GraphSON to create `Vertex` objects in Python and it might use UUID for its ID as well. Thus, `@class=UUID` is perhaps `@type=UUID`. That is, if an object is "typed" its `@type` and the resolver (for that language) will have to map the correct class. 3. I read your notes, but its still not clear to me. Why do we have `@value`? I think we only need `@type` (or `@class`) and then the rest of the JSON object data is up to the type serializer/deserializer. For instance, in `Bytecode` (TINKEROP-1278) we have `@type=Lambda` and then `value`, `language`, `arguments`. That is, there are 3 "values" we need to make the object and thus `@value` shouldn't be special with a `@`. Plus, we save a char. 4. I think we can save some bytes and still be readable if we make our namespace `g` instead of `gremlin`. Moreover, we should be consistent in our naming convention and either use some standard JSON naming or default to Java naming -- e.g. `g:vertexProperty`. Or better yet, if its NOT namespaced, its assumed to be "Gremlin". That is, TinkerPop owns `vertex`, `vertexProperty`, `edge`. You want to do something else -- its `myapp:vertex`. This also means we own `uuid`, `int64`, etc. etc. Thanks again for the extensive work. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
