I actually opened a PR <https://github.com/apache/tinkerpop/pull/993> (makes it easier to look at the changes). Removing transient didn't quite do the job, so I overwrote the (de)serialization methods and only kept the parent element id. The only thing that's broken now is the serialization test suite.
Cheers, Daniel On Mon, Nov 12, 2018 at 4:59 AM Stephen Mallette <[email protected]> wrote: > 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 > > >> > > > >> > > > > > >
