If we can, I think that we should just keep derser semantics as they are. We've gone this long with them having null parent element ids and there haven't been any raised complains that I'm aware of.
> I thought about skipping the parent id comparison in case one of the elements parents is *null* I suppose the question we need to answer stated directly is: What do we want equals() to mean for "detached" properties? Maybe it's not wrong to consider "detached" properties without a parent to have a form of wildcard status. If you were to "attach" such a property then its id space would be restricted to the element you're adding it to, so it seems safe in that respect. In the "tree test" that's failing that you referenced, the deserialized tree is a wildcard match for the original tree...the idea sorta works in that capacity, but perhaps there are other scenarios where that's a really bad thing? Does a "null" parent element mean "detached"? I guess the answer is "sometimes" because in some cases "detached" keeps the parent element. Are there other scenarios though where the parent element can be null? I can't really think of any right now. On Thu, Nov 8, 2018 at 12:48 PM Daniel Kuppitz <m...@gremlin.guru> wrote: > Working on TINKERPOP-2051 > <https://issues.apache.org/jira/browse/TINKERPOP-2051>, I thought I found > a > pretty easy solution: In order to allow locally unique property ids, we > only need to compare the ids of two properties as well as the ids of their > parent elements. That was basically a one-line change and had the expected > effect: > > > gremlin> g = TinkerGraph.open().traversal() > ==>graphtraversalsource[tinkergraph[vertices:0 edges:0], standard] > gremlin> g.addV("person").property("name", "alice", id, "name").as("a"). > ......1> addV("person").property("name", "bob", id, "name"). > ......2> addE("knows").from("a") > ==>e[2][0-knows->1] > gremlin> > gremlin> g.V().properties("name") > ==>vp[name->alice] > ==>vp[name->bob] > gremlin> g.V().properties("name").dedup() > ==>vp[name->alice] > ==>vp[name->bob] > > > Without the change in id comparison, the last statement would have only > returned 1 of the properties. Looked easy enough, but - of course - it > broke the test suite. This change had an impact on 2 types of tests: > > - those, that deal with serialization and deserialization > - those, that make use of bytecode > > Perhaps the latter can be seen as a more specific case of the former. The > problem is that serialized/deserialized/detached properties no longer have > a reference to their former parent element. I thought about skipping the > parent id comparison in case one of the elements parents is *null*, but at > some point that just felt wrong and made me think of all kind of horror > scenarios this could/would lead to. > > So my question is, what would be the smartest way to handle that? Adjust > all test cases, so they no longer rely on full equality? Fix > (de)serialization to include parent element ids? Something else? > > *Example of a failing test:* > > [ERROR] SerializationTest$GryoV1d0Test.shouldSerializeTree:254 The > objects differ > expected: > > org.apache.tinkerpop.gremlin.process.traversal.step.util.Tree<{v[1]={v[2]={vp[name->vadas]={}}, > v[3]={vp[name->lop]={}}, v[4]={vp[name->josh]={}}}}> > but was: > > org.apache.tinkerpop.gremlin.process.traversal.step.util.Tree<{v[1]={v[2]={vp[name->vadas]={}}, > v[3]={vp[name->lop]={}}, v[4]={vp[name->josh]={}}}}> > > > Crazy thought: We could just change the tests to compare the toString() > versions of results.... > > Cheers, > Daniel >
