[DF] Fix snapshot of array branch with out-of-order size branch #10934
Conversation
|
Starting build on |
|
Starting build on |
Sometimes, when users specify an explicit list of columns to be written out by a Snapshot, they might forget to list the names of branches the store the sizes of array branches (e.g. `sz` for a branch `x` with leaflist `x[sz]/F`). We now explicitly check for this case and provide a (hopefully) helpful error message.
Even when users do not specify them explicitly, we now add size branches required by array branches to the list of branches that will be written out by a Snapshot.
eguiraud
force-pushed
the
fix-snapshot-size-columns
branch
from
ee61235
to
1b9e9ca
Compare
|
Starting build on |
|
@vepadulano @Axel-Naumann with the last commit the situation is now the following: explicit column list passed, a necessary size branch was omitted:
explicit column list passed, a necessary size branch is listed after the branch that needs it:
no explicit column list passed:
|
|
Build failed on ROOT-ubuntu18.04/nortcxxmod. Failing tests:
And 11 more |
|
Build failed on ROOT-ubuntu2004/python3. Failing tests:
And 13 more |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Failing tests:
And 49 more |
|
Build failed on ROOT-debian10-i386/soversion. Failing tests: |
|
Build failed on mac11/cxx14. Failing tests: |
|
Build failed on windows10/cxx14. Failing tests: |
|
Build failed on mac1015/cxx17. Failing tests: |
| const auto counterStr = | ||
| leaf->GetLeafCount() ? std::string(leaf->GetLeafCount()->GetName()) : std::to_string(leaf->GetLenStatic()); | ||
| auto *sizeLeaf = leaf->GetLeafCount(); | ||
| const auto sizeLeafName = sizeLeaf ? std::string(sizeLeaf->GetName()) : std::to_string(leaf->GetLenStatic()); |
There was a problem hiding this comment.
FYI, for a leaf [4][4], leaf->GetLenStatic() returns 16. I guess that's good?
There was a problem hiding this comment.
I...don't know, we don't have test coverage for 2-dimensional arrays in RDF
| continue; // not a variable-sized array or the size branch is already there, nothing to do | ||
|
|
||
| // otherwise we must insert the size in colsWithoutAliases _and_ colsWithAliases | ||
| colsWithoutAliases.insert(colsWithoutAliases.begin() + i, countLeaf->GetName()); |
There was a problem hiding this comment.
This means you insert the size column for branch X right before branch X, and then you check that branch X again in the next for loop iteration. Is that intentional?
There was a problem hiding this comment.
ah thank you , I guess we can also increment the counter to avoid the redundant work, will do
|
|
||
| // make input tree | ||
| { | ||
| TFile f(inFile, "recreate"); |
There was a problem hiding this comment.
Would it make sense to use in-memory trees or TMemFile for these kind of tests?
There was a problem hiding this comment.
yes in principle it makes sense, in practice I don't know if it works e.g. with TTreeProcessorMT (which has to re-open the file in each thread). I noted it down to try and convert all our RDF tests to use TMemFiles whenever possible, but I'd address this separately.
Co-authored-by: Axel Naumann <Axel.Naumann@cern.ch>
|
Starting build on |
Co-authored-by: Axel Naumann <Axel.Naumann@cern.ch>
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Failing tests: |
|
Build failed on ROOT-ubuntu2004/python3. Failing tests: |
This PR fixes #10920 and #6932 (see those for more information).