Treating null parents as a wildcard makes the majority of tests just pass, but Spark integration tests fail. Spark is all about detached elements (or reference elements) and I guess there's no way to make those tests pass without making detached properties remember their parent elements (or at least just their parent element ids).
Cheers, Daniel On Fri, Nov 9, 2018 at 4:53 AM Stephen Mallette <[email protected]> wrote: > 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 <[email protected]> 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 > > >
