You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2022/01/12 01:33:34 UTC
[GitHub] [tinkerpop] ihoro opened a new pull request #1547: Gremlin javascript GraphBinary serialization support
ihoro opened a new pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547
This is the first tiny step draft, there is a lot of chore work ahead. And this is the reason I would like to request comments in order to make sure I’m going the right direction and to apply comments/suggestions at the early stages.
Shall I continue with such structure?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] ihoro commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
ihoro commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1055620564
Hi, I still have in plans to finish with some types like TraversalStrategy and BulkSet, as I see, according to gremlin-javascript and the tests, it’s important to implement them. Also, I have some technical concerns regarding numeric types (I was planning to discuss it with you).
My apologies, but my plans have been postponed due to my neighbour f(r)iend has been throwing rocks into my garden, breaking my house's windows, and trying to break through my doors.
If you want, please, continue according to your plans, i.e. merge and so on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] ihoro commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
ihoro commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1040698878
I have a question regarding Binding type. I’ve not found Binding mention within current gremlin-javascript implementation, i.e. it looks like it’s not used and can also be kept not implemented for GraphBinary. Or it’s a tricky moment?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1040717100
`Binding` is a way to pass variables in Gremlin bytecode but it doesn't really have good applicability outside of situations where you end up using a lambda (which isn't often). It also has a weird implementation pattern that doesn't always work well in all languages - even in Java it uses awful `ThreadLocal` stuff to work properly. My personal feeling is that you can ignore it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] ihoro commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
ihoro commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1021025058
> It looks like the [GraphBinary documentation](https://github.com/apache/tinkerpop/blob/master/docs/src/dev/io/graphbinary.asciidoc#traverser) is incorrect about traverser, it's a long
Please, consider #1554.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] ihoro commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
ihoro commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1019559674
But I have issue with TraverserSerializer, according to the spec I expect `{bulk}` as Int (32 bits), but gremlin-server responds with Long/Int64. Could you please guide me here?
(this is the same question posted via discord)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] jorgebay commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
jorgebay commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1056598080
@ihoro hope you and your family stay safe
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] ihoro commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
ihoro commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1019559420
FYI: by now I’ve implemented the minimal set of types to get a simple request like `g.V().values()` work with default gremlin-server.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] ihoro commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
ihoro commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1028467119
I’ve found an issue with T type. I’ve implemented its serialization according to the [spec](https://tinkerpop.apache.org/docs/current/dev/io/#_t_3). What means I send it like `{type_code}{type_info}` followed by non-fully-qualified string, i.e. `{length}{text_value}`. For example, T.label is sent as `0x2000 00000005 6c6162656c`. But gremlin-server does not seem to expect it with the following stack trace:
```
[WARN] DefaultChannelPipeline - An exceptionCaught() event was fired, and it reached at the tail of the pipeline. It usually means the last handler in the pipeline did not handle the exception.
io.netty.handler.codec.DecoderException: java.lang.IndexOutOfBoundsException: readerIndex(92) + length(1388) exceeds writerIndex(178): PooledUnsafeDirectByteBuf(ridx: 92, widx: 178, cap: 211)
at io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:98)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
at io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:103)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
at io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:103)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
at io.netty.channel.ChannelInboundHandlerAdapter.channelRead(ChannelInboundHandlerAdapter.java:93)
at io.netty.handler.codec.http.websocketx.Utf8FrameValidator.channelRead(Utf8FrameValidator.java:82)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
at io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:103)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:324)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:296)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
at io.netty.handler.timeout.IdleStateHandler.channelRead(IdleStateHandler.java:286)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919)
at io.netty.channel.epoll.AbstractEpollStreamChannel$EpollStreamUnsafe.epollInReady(AbstractEpollStreamChannel.java:795)
at io.netty.channel.epoll.EpollEventLoop.processReady(EpollEventLoop.java:480)
at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:378)
at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:989)
at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: java.lang.IndexOutOfBoundsException: readerIndex(92) + length(1388) exceeds writerIndex(178): PooledUnsafeDirectByteBuf(ridx: 92, widx: 178, cap: 211)
at io.netty.buffer.AbstractByteBuf.checkReadableBytes0(AbstractByteBuf.java:1442)
at io.netty.buffer.AbstractByteBuf.checkReadableBytes(AbstractByteBuf.java:1428)
at io.netty.buffer.AbstractByteBuf.readBytes(AbstractByteBuf.java:895)
at io.netty.buffer.AbstractByteBuf.readBytes(AbstractByteBuf.java:903)
at org.apache.tinkerpop.gremlin.driver.ser.NettyBuffer.readBytes(NettyBuffer.java:132)
at org.apache.tinkerpop.gremlin.structure.io.binary.types.StringSerializer.readValue(StringSerializer.java:37)
at org.apache.tinkerpop.gremlin.structure.io.binary.types.StringSerializer.readValue(StringSerializer.java:28)
at org.apache.tinkerpop.gremlin.structure.io.binary.types.SimpleTypeSerializer.readValue(SimpleTypeSerializer.java:59)
at org.apache.tinkerpop.gremlin.structure.io.binary.GraphBinaryReader.readValue(GraphBinaryReader.java:77)
at org.apache.tinkerpop.gremlin.structure.io.binary.GraphBinaryReader.read(GraphBinaryReader.java:102)
at org.apache.tinkerpop.gremlin.structure.io.binary.types.EnumSerializer.readValue(EnumSerializer.java:66)
at org.apache.tinkerpop.gremlin.structure.io.binary.types.EnumSerializer.readValue(EnumSerializer.java:44)
at org.apache.tinkerpop.gremlin.structure.io.binary.types.SimpleTypeSerializer.readValue(SimpleTypeSerializer.java:59)
at org.apache.tinkerpop.gremlin.structure.io.binary.types.SimpleTypeSerializer.read(SimpleTypeSerializer.java:47)
at org.apache.tinkerpop.gremlin.structure.io.binary.GraphBinaryReader.read(GraphBinaryReader.java:106)
at org.apache.tinkerpop.gremlin.structure.io.binary.types.ByteCodeSerializer.getInstructionArguments(ByteCodeSerializer.java:56)
at org.apache.tinkerpop.gremlin.structure.io.binary.types.ByteCodeSerializer.readValue(ByteCodeSerializer.java:41)
at org.apache.tinkerpop.gremlin.structure.io.binary.types.ByteCodeSerializer.readValue(ByteCodeSerializer.java:30)
at org.apache.tinkerpop.gremlin.structure.io.binary.types.SimpleTypeSerializer.readValue(SimpleTypeSerializer.java:59)
at org.apache.tinkerpop.gremlin.structure.io.binary.types.SimpleTypeSerializer.read(SimpleTypeSerializer.java:47)
at org.apache.tinkerpop.gremlin.structure.io.binary.GraphBinaryReader.read(GraphBinaryReader.java:106)
at org.apache.tinkerpop.gremlin.structure.io.binary.types.MapSerializer.readValue(MapSerializer.java:42)
at org.apache.tinkerpop.gremlin.structure.io.binary.types.MapSerializer.readValue(MapSerializer.java:31)
at org.apache.tinkerpop.gremlin.structure.io.binary.types.SimpleTypeSerializer.readValue(SimpleTypeSerializer.java:59)
at org.apache.tinkerpop.gremlin.structure.io.binary.GraphBinaryReader.readValue(GraphBinaryReader.java:77)
at org.apache.tinkerpop.gremlin.driver.ser.binary.RequestMessageSerializer.readValue(RequestMessageSerializer.java:55)
at org.apache.tinkerpop.gremlin.driver.ser.GraphBinaryMessageSerializerV1.deserializeRequest(GraphBinaryMessageSerializerV1.java:175)
at org.apache.tinkerpop.gremlin.server.handler.WsGremlinBinaryRequestDecoder.decode(WsGremlinBinaryRequestDecoder.java:77)
at org.apache.tinkerpop.gremlin.server.handler.WsGremlinBinaryRequestDecoder.decode(WsGremlinBinaryRequestDecoder.java:43)
at io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:88)
... 39 more
```
I’ve tried a fully-qualified string instead and gremlin-server is happy with it, i.e. it expects a fully-qualified string.
Q1: Is it a typo within the spec? Usually it explicitly mentions when a fully-qualified form is required, and non-FQ is expected otherwise.
Q2: If A1 is true then, I guess, the same is applied to other Enum based types like Cardinality, Order, and so on, right?
Q3: By the way, when gremlin-server errors in case of a non-fully-qualified string then a client gets no response and waits until its own timeout. Maybe a client should get something like 'wrong request' response instead of keeping silence from the server side? But this is a side question, not related to GraphBinary implementation for gremlin-javascript. Does it need a JIRA ticket creation for future consideration?
Could you please guide me from gremlin-core Java side perspective? I will help with a PR to fix the spec, if it gets confirmed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1055328712
i'm not sure how others feel, but if this PR has reached the same level of compatibility as python (which doesn't cover all the serializers even as of today), I think it should be evaluated for merge for release of 3.5.3.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] ihoro commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
ihoro commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1030700615
FYI (a teaser): one microservice of a project in production has got its integration tests (based on official gremlin-server) passed using current state of GraphBinary support implementation. This is not an evidence, just an interesting, and inspiring for me, fact.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] jorgebay commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
jorgebay commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1034996848
Yeah, it in issue in the docs.
Thanks again!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] ihoro commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
ihoro commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1040721699
Thank you for the confirmation.
And what about Graph type (0x10)? It has two names in the spec: Graph and TinkerGraph. And I also do not see related artifacts within gremlin-javascript. Probably, it’s some higher level concept from Java side what does not require direct (de)serialization in case of a driver like gremlin-javascript?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1011638874
@ihoro I just wanted to say thanks for getting started on this. I didn't get a chance to look at the code itself today, but perhaps we can get you some feedback for tomorrow. As an administrative point, I think it would be great if you could target the `3.5-dev` branch for this rather than `master` as i think it would be excellent to have this feature on the 3.5.x line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] ihoro commented on a change in pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
ihoro commented on a change in pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#discussion_r785316198
##########
File path: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/io/graph-serializer.js
##########
@@ -98,6 +99,47 @@ class GraphSON2Writer {
write(obj) {
return JSON.stringify(this.adaptObject(obj));
}
+
Review comment:
Good idea, please check #1549
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] jorgebay commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
jorgebay commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1028780542
hm... I thought T values, as the rest of enums were encoded as:
`{type_code}{type_info}{value_flag}{value}`
- type_code: `0x20` (1 byte)
- type_info: no data (0 bytes)
- value_flag: `0` (1 byte) - as not null
- value: `0`, `0`, `0`, `0x5` (length), followed by the string `0x6c` `0x61` `0x62` `0x65` `0x6c`
but looking at the java (reference) and C# implementations it looks like it expects a FQ string in the value instead, as you mention. The doc is incorrect.
About the client timing out, it's a client issue, it should error out, we can file a ticket for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] jorgebay commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
jorgebay commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1019856384
Nice progress!!
It looks like the [GraphBinary documentation](https://github.com/apache/tinkerpop/blob/master/docs/src/dev/io/graphbinary.asciidoc#traverser) is incorrect about traverser, it's a long: https://github.com/apache/tinkerpop/blob/master/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Traverser.java#L109
Example serializer: https://github.com/apache/tinkerpop/blob/master/gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphBinary/Types/TraverserSerializer.cs#L52
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] ihoro commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
ihoro commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1029168075
> but looking at the java (reference) and C# implementations it looks like it expects a FQ string in the value instead, as you mention. The doc is incorrect.
okay, thanks for confirmation, I will create respective PR then
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] ihoro commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
ihoro commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1029279338
Please consider #1560 for docs correction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] ihoro commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
ihoro commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1034206555
I’ve been working on PathSerializer and I’ve found out that gremlin-server responds with fully-qualified `{labels}` and `{objects}`, i.e. as `0x0900...`. But the spec says `{labels} is a List` and `{objects} is a List`. You mentioned that Java based gremlin-server is the reference implementation, then, I guess, I should fix the docs respectively to make it explicit that FQ List is expected. Could you please confirm?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1040729118
you can't do the `Graph` type yet so you may skip that. we don't have a construct for that in javascript. hopefully that will be something we look at for 3.7.0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1055696553
@ihoro there are a lot of people that are shocked by your neighbor and are hoping he is made to stop very soon. take care.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] ihoro commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
ihoro commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1021025058
> It looks like the [GraphBinary documentation](https://github.com/apache/tinkerpop/blob/master/docs/src/dev/io/graphbinary.asciidoc#traverser) is incorrect about traverser, it's a long
Please, consider #1554.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] jorgebay commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
jorgebay commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1031211265
> passed using current state of GraphBinary
NICE!!!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] ihoro commented on pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
ihoro commented on pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#issuecomment-1036601435
> issue in the docs.
Thanks for the confirmation. Please, check respective PR #1566.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a change in pull request #1547: Add GraphBinary serialization support to gremlin-javascript
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#discussion_r784819118
##########
File path: gremlin-javascript/src/main/javascript/gremlin-javascript/test/unit/graphbinary-test.js
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.
+ */
+
+/**
+ * @author Igor Ostapenko
+ */
+'use strict';
+
+const assert = require('assert');
Review comment:
I know this PR is work in progress at this stage, so please consider the following comment as proactive caution.
I would suggest adding a "round-tripping" test which serializes the object, deserializes the object and compares with the original version. For completeness of test coverage, we would want to add the following tests:
1. Test for each data type defined at https://tinkerpop.apache.org/docs/3.5.1/dev/io/#_data_type_formats
2. For each data type in point 1 above, test for happy case and edge case e.g. null handling, unicode chars for string, +=inf/NaN/+-0.0 for numbers etc.
3. Negative test cases where we expect serializer to throw an error e.g. if `parent` for vertex property is not `null` as defined in https://tinkerpop.apache.org/docs/3.5.1/dev/io/#_vertexproperty_4 , out of bound value for a number etc.
##########
File path: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/io/graph-serializer.js
##########
@@ -98,6 +99,47 @@ class GraphSON2Writer {
write(obj) {
return JSON.stringify(this.adaptObject(obj));
}
+
Review comment:
I would suggest to create a separate PR for GraphSonV2 changes. It would simplify the GraphBinary PR and also possibly merge the graphsonV2 earlier than this much larger change.
##########
File path: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/connection.js
##########
@@ -40,6 +42,7 @@ const responseStatusCode = {
const defaultMimeType = 'application/vnd.gremlin-v3.0+json';
const graphSON2MimeType = 'application/vnd.gremlin-v2.0+json';
+const graphBinaryMimeType = 'application/vnd.graphbinary-v1.0';
Review comment:
Note that graphBinary has two MimeTypes. `application/vnd.graphbinary-v1.0` and `application/vnd.graphbinary-v1.0-stringd`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org