I don't think we can remove the transient keyword. Doesn't that balloon the amount of data shuffled around the network? I think that was the reason we made that transient in the first place.
What parts of the test suite are failing because of this? Could you post links to a couple of examples of failing assertions? On Sat, Nov 10, 2018 at 2:01 PM Daniel Kuppitz <[email protected]> wrote: > Removing the transient attribute from the DetachedVertexProperty's vertex > field [1] makes all Spark tests pass. Easy peasy, but holy cow, that little > change probably has a huge impact on things I can't even think of right > now. > > [1] > > https://github.com/apache/tinkerpop/blob/master/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/detached/DetachedVertexProperty.java#L40 > > Cheers, > Daniel > > > On Sat, Nov 10, 2018 at 11:49 AM Daniel Kuppitz <[email protected]> wrote: > > > 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 > >> > > >> > > >
