Just a quick follow-up, I'm planning to make a small change that I thought you all might want to be aware of but it's probably small enough to not need its own thread. In GraphSON, edge had a "inVLabel", "outVLabel", "inV", and "outV" to refer to the vertices in both directions. However, this isn't the same as the format for a vertex which is an object that contains an "id" and "label". I think it makes more sense to modify "inV" and "outV" to be reference vertices that contain a "id" and a "label" without "properties". I've made a PR for that at https://github.com/apache/tinkerpop/pull/2810
On Thu, Sep 12, 2024 at 1:42 PM Yang Xia <xia...@apache.org> wrote: > Thanks Ken! Given consensus on this discussion, I've began implementing > some of the changes in Java and Python to align the GraphBinary serializers > with the spec, starting with removing old serializers and updating > DateTime: https://github.com/apache/tinkerpop/pull/2776. Reviews and > comments are welcome. > > Cheers, > > Yang > > On 2024/08/28 04:01:51 Ken Hu wrote: > > Here are some other updates that I think should be made for > GraphBinaryV4. > > > > There are steps that can return a distinct ordering in Maps like > > order(local). However, these aren't necessarily deserialized in GLVs as > > Map-types that preserve this ordering. I think that we should add a > > value_flag to the Map type in GraphBinary that signals that the > > deserialized version of this Map should maintain the ordering. Similar to > > the BulkSet change, we can use a value_flag with the value 0x02 to denote > > ordering. The value itself would remain unchanged, but the order of the > > items in that list should be the insertion/iteration order of the > > deserialized Map. > > > > All values that the Date type can represent are covered by OffsetDateTime > > so I think it makes sense to remove even the Date type. Date is also > > outdated in Java and its use isn't recommended. > > > > The current GraphBinary standard states that properties for elements like > > Vertex, Edge and VertexProperty should be set to null. The reasoning for > > this was that properties were never returned in remote traversals. > However, > > this changed starting in 3.7, and properties can now be returned. > > Properties should instead be a List of Properties and if there are no > > properties then an empty list should be returned. > > > > Labels for elements are currently serialized as String for both > GraphBinary > > and GraphSON. However, we may want to support multi-labels or no-label > > scenarios in the future. To prevent needing a breaking change if this > were > > to happen, I propose that "label" become a List of String. > > > > In the new HTTP API, a ReponseMessage is returned per HTTP chunk and the > > HTTP response could end with a trailer. The GLVs may need some way to > > distinguish between chunks and trailers which could be accomplished > using a > > one byte buffer in between the last ResponseMessage and the trailing > > header. The GLVs may need information to recreate a chunk as some HTTP > > clients transparently dechunk the body. A 4-byte header that indicates > the > > size of ResponseMessage could be added to help GLVs recognize chunk > > boundaries which simplifies deserialization. > > > > On Thu, Aug 22, 2024 at 2:34 PM Ken Hu <kenhu...@gmail.com> wrote: > > > > > Hi All, > > > > > > I've noticed that there are some serializers/deserializers that exist > for > > > GraphBinary and GraphSON that may not serve a real purpose and I'm > > > proposing that we remove them in V4. In general, I think we should move > > > away from providing serializers for all types in the Java Standard > Library, > > > as there are many types that other GLVs simply won't support. The > types in > > > GraphSONv3/GraphBinaryV3 that I would remove include: > > > > > > Barrier, Cardinality, Column, DT, Merge, Operator, Order, Pick, Pop, > > > Scope, T > > > > > > Because the GLVs will now use GremlinLang (instead of Bytecode), these > > > values will be translated to Strings so they don't need their own > specific > > > serializers. T and Direction are the two exceptions because they are > > > returned in the valueMap() and elementMap() steps, although it's likely > > > they will be replaced in the future as well. For similar reasons, P and > > > TextP should also be removed since they will also be Strings in > GremlinLang. > > > > > > Period, Timestamp, Instant, ZonedDateTime, OffsetTime, LocalDateTime, > > > LocalDate, LocalTime, MonthDay, YearMonth, Year, ZoneOffset > > > > > > These are from the java.time package but are all variants of > > > OffsetDateTime and Date. I don't think it makes sense to provide > > > serializers for these Java-specific types that are mostly unsupported > by > > > the other GLVs and can be represented by either an OffsetDateTime > and/or > > > Date, so I think we should only have OffsetDateTime and Date. > > > > > > BulkSet and Traverser are two optimizations that exist to increase > > > performance but that the user shouldn't be able to interact with. I'm > > > suggesting that we remove these serializers, however, to keep the > > > performance optimizations, I would suggest that we add a flag to the > List > > > type so that it can achieve the same sort of bulking that those types > > > provided. GraphSON would simply serialize these as arrays without a > bulk so > > > it would be the expanded BulkSet. GraphBinary would have an additional > > > value_flag for List with the value 0x02 to denote bulking. This would > mean > > > the item_i is a fully qualified item followed by a Long value that is > the > > > bulk. > > > > > > Class and InetAddress are highly Java-specific and can't be used by the > > > other GLVs. Binding and Bytecode have both been removed so it doesn't > make > > > sense to keep their serializers around. Metrics, TraversalMetrics can > be > > > represented as a list of Strings so they shouldn't require their own > > > serializers. > > > > > > The last type that I think should be removed is Custom. The most common > > > complaint that I've seen from end-users regarding custom types is that > they > > > lack serializers in their GLV of choice. Custom serializers require > extra > > > effort from providers as it is they that need to maintain and publish > them. > > > Users often don't realize that they can't just use the GLV without > adding > > > the custom serializer from their provider which can lead to a > frustrating > > > initial experience. So, instead, to provide a more seamless user > experience > > > I'm proposing we replace Custom with a Provider Defined Type (PDT). > > > Actually, two PDTs, one for primitive types and one for composite > types. > > > These PDTs will also serve as a mechanism to allow reading of values > whose > > > serializer has been removed in V4. This would allow a provider that > > > supports one of those Java-specific types to still return the value as > a > > > PDT. > > > > > > The PrimitivePdt will contain two fields: a String for the type name > and a > > > String for the actual value. The CompositePdt will contain two fields: > a > > > String for the type name and a Map that contains the fields of the > > > composite type (most likely a class). PDTs will come with default > > > serializers and deserializers. On the user-side, the values will be > > > deserialized into either PrimitivePdt or CompositePdt. In order to > write a > > > PDT to the server, the user would have to look up the required fields > for > > > that specific type name. This information would likely need to exist > in the > > > provider's documentation. On the server, there will be an API (probably > > > attached to the Graph interface) that will allow the provider to add > types > > > that should be able to be serialized/deserialized from PDT. In order to > > > achieve this, there will need to be a TinkerPopCustomType interface > that > > > has two methods: serialize() and deserialize(). Then, any type that > > > implements TinkerPopCustomType could automatically be > > > serialized/deserialized with the provided PDT serializers for both > > > GraphSONv4 and GraphBinaryV4. > > > > > > Does anyone have any comments or concerns about these proposed changes? > > > > > > Thanks, > > > Ken > > > > > >
