-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add GraphSON 2 support for JanusGraph predicates #1060 #1061
Add GraphSON 2 support for JanusGraph predicates #1060 #1061
Conversation
I looked a bit more into the issue with GraphSON 3 and confirmed that it has nothing to do with predicates. JanusGraph just can't deserialize GraphSON 3 right now. The tests I added even failed for a traversal like
I created #1075 for GraphSON 3 support. So this PR is now ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a couple small fixes to do, otherwise nice work on this @FlorianHockmann!
I verified this manually with Gremlin Server configured with GraphSONMessageSerializerV2d0
as the only serializer then connected from Gremlin Console using the same serializer in the remote-objects.yaml
.
@@ -0,0 +1,91 @@ | |||
package org.janusgraph.graphdb.tinkerpop.io.graphson; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to include the JanusGraph copyright header
|
||
for (GraphTraversal traversal : traversals) { | ||
Bytecode expectedBytecode = traversal.asAdmin().getBytecode(); | ||
RequestMessage requestMessage = RequestMessage.build(Tokens.OPS_BYTECODE).processor("traversal") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace "traversal"
with a constant GraphSONTokens.TRAVERSAL
Signed-off-by: Florian Hockmann <fh@florian-hockmann.de>
1481501
to
f3fb87d
Compare
I just amended my commit to include the two changes you requested, @pluradj. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @FlorianHockmann
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, thanks @FlorianHockmann
This adds GraphSON support for JanusGraph predicates (Text and Geo) which is necessary to use them in GLVs.
I only implemented this for GraphSON 2 as it looks like GraphSON 3 currently doesn't even work with the default TinkerPop predicates (
P.within()
,P.gt()
, ...). At least the test I added for that (testTinkerPopPredicatesAsGraphSON
) fails with GraphSON 3 independent of my code changes.Would be good to hear from someone with more knowledge of JanusGraph I/O if I'm doing something wrong here. When there are more problems with GraphSON 3 then a separate PR for that might be a good idea but I marked this PR as WIP until that has been clarified.
Issue: #1060
Thank you for contributing to JanusGraph!
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
master
)?For code changes:
For documentation related changes:
[skip ci]
tag to the first line of your commit message to avoid spending CPU cycles in
Travis CI when no code, tests, or build configuration are modified?
Note:
Please ensure that once the PR is submitted, you check Travis CI for build issues and submit an update to your PR as soon as possible.