AIDASoft / podio

PODIO
GNU General Public License v3.0
24 stars 59 forks source link

Fix OneToManyRelations and VectorMembers in cloned objects #583

Closed jmcarcell closed 4 months ago

jmcarcell commented 5 months ago

The context is the following. Let's say we have a collection and each member has a OneToManyRelation to another type, for this example something that looks like a string. Let's say the first element in the collection has a relation to 'a', then the second to 'b' and so on. Each 'a', 'b', 'c', ... are in a different vector at this point. When saving the collection, all of 'a', 'b', 'c', ... go into the same vector, and there are some indexes to know which part of the vector corresponds to each object in the collection (for example for the first element it would be start=0 and end=1. When reading, each object still points to this big vector ['a', 'b', 'c', ...] so if the object is cloned the clone will also point to this big vector. If I clone the first object and then do a addToRelation('z'), the big vector will have one more element ['a', 'b', 'c', ..., 'z'] and the indexes will be updated so that now the relations of the first element will be 'a' and 'b' (start=0 and end=2), when the expected result is 'a' and 'z'.

BEGINRELEASENOTES

ENDRELEASENOTES

Initially I implemented it (check if this is the case and if it is then make a new vector) for the clone() method but in this commit I have done it for the addX methods, since it's closer to on demand and won't make all the clone() a bit slower.

jmcarcell commented 5 months ago

I changed the fix to be in clone() and moved the tests in a unittest (maybe I should add an rntuple one...)

I suppose it also works for things that have just been created in memory.

For memory only everything works fine because there is never this conversion to a single vector, that only happens when writing

tmadlener commented 4 months ago

Can you rebase this to pick the changes from master?

jmcarcell commented 4 months ago

After #588 and #589 I can easily test this and now the implementation doesn't have any leaks and does the minimum possible work; only copying the elements that are being pointed to. I was going to exclude the RNTuple test from the tests with sanitizers since it fails for me, but there are others that do so they will have to be changed anyway, I think the same will be seen when ROOT 6.30.06 is available.

hegner commented 4 months ago

Thanks! Quite important to fix this! If you fix the format check issues, I could merge it today

veprbl commented 2 months ago

This caused a regression for us https://github.com/AIDASoft/podio/issues/631