[ntuple] Add support for std::map fields
#13904
Conversation
|
Starting build on |
bef293b to
d017349
Compare
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
d017349 to
972a45e
Compare
|
Starting build on |
1 similar comment
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
|
Build failed on windows10/default. |
There was a problem hiding this comment.
Very nice!
I think we should make the following adjustments:
- Let's change the on-disk representation to a collection of pairs. This would correspond to the collection proxy representation. And it would naturally let
SplitValuereturnstd::pairvalues, which I'd find more natural than alternating key and value. - I'm not sure we should make the associative collection proxy field inherit from the simple one. It looks to me cleaner to add the additional functionality directly to the
RProxiedCollectionFieldclass.
9dd8ba6 to
a6236d4
Compare
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Warnings:
Failing tests: |
|
Build failed on ROOT-performance-centos8-multicore/soversion. Warnings:
|
|
Build failed on ROOT-ubuntu2004/python3. Warnings:
|
a6236d4 to
f37d1b3
Compare
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
|
@phsft-bot build just on ROOT-ubuntu2204/nortcxxmod |
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
| void ReadGlobalImpl(NTupleSize_t globalIndex, void *to) final; | ||
|
|
||
| public: | ||
| RMapField(std::string_view fieldName, std::string_view typeName, std::unique_ptr<Detail::RFieldBase> itemField); |
There was a problem hiding this comment.
Can we use a PairField?
| RMapField(std::string_view fieldName, std::string_view typeName, std::unique_ptr<Detail::RFieldBase> itemField); | |
| RMapField(std::string_view fieldName, std::string_view typeName, std::unique_ptr<RPairField> itemField); |
There was a problem hiding this comment.
Hmm, this causes trouble with CloneImpl because when we call Clone on the subfield, a RFieldBase is returned (unless there's a way to cast it to an RPairField that I'm unaware of?)
There was a problem hiding this comment.
Right, I think we can do a dynamic_cast to a RPairField. We know it must be a RPairField.
f37d1b3 to
58795eb
Compare
|
Starting build on |
|
Build failed on mac12arm/cxx20. Failing tests: |
58795eb to
55c9ca4
Compare
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
d9f7457 to
3f73dea
Compare
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
| @@ -66,6 +66,13 @@ | |||
| #pragma link C++ class std::map<char, std::map<int, CustomStruct>> +; | |||
| #pragma link C++ class std::map<float, std::map<char, std::int32_t>> +; | |||
|
|
|||
| #pragma link C++ class std::pair <char, long> +; | |||
There was a problem hiding this comment.
The dictionary requests for std::pair do not hurt but they also should not be needed. What lead you to add them?
There was a problem hiding this comment.
If I don't include them, the std::map tests will fail on builds with runtime modules disabled (see all the failing tests on ROOT-ubuntu2204/nortcxxmod in this thread). I'm not really knowledgeable on these modules, so I don't know if this just hides some other underlying issue or not..
There was a problem hiding this comment.
What was the error? Note: it is a goal/requirement that the support for std::map does not require the user to request the dictionary for std::pair
There was a problem hiding this comment.
You can actually see it on the latest Jenkins build failure: https://lcgapp-services.cern.ch/root-jenkins/job/root-pullrequests-build/189474/testReport/projectroot.tree.ntuple.v7/test/gtest_tree_ntuple_v7_test_ntuple_types/, because apparently now it also fails for a std::set test that contains a std::pair without a dictionary. Which is also weird, because these tests used to pass just fine on the same platform before (it does not seem something changed in the Jenkins/build config since).
There was a problem hiding this comment.
You will need to reproduce the problem locally and/or instrument:
ROOT::Experimental::RPairField::RPairField(std::string_view fieldName,
std::array<std::unique_ptr<Detail::RFieldBase>, 2> &itemFields)
: ROOT::Experimental::RRecordField(fieldName, std::move(itemFields), {},
"std::pair<" + GetTypeList(itemFields) + ">")
{
// ISO C++ does not guarantee any specific layout for `std::pair`; query TClass for the member offsets
fClass = TClass::GetClass(GetType().c_str());
if (!fClass)
throw RException(R__FAIL("cannot get type information for " + GetType()));
fSize = fClass->Size();
if (fClass->GetDataMember("first") == nullptr || fClass->GetDataMember("second") == nullptr) {
std::cerr << "The TClass for " << GetType() << " is in state: " << fClass->GetState << " and has: \n";
fClass->GetListOfDataMembers()->ls();
fClass->GetStreamerInfo()->ls();
throw RException(R__FAIL("Insufficient information for " + GetType()));
}
fOffsets[0] = fClass->GetDataMember("first")->GetOffset();
fOffsets[1] = fClass->GetDataMember("second")->GetOffset();
}
There was a problem hiding this comment.
Thanks, I observe the same thing locally. For a std::map<char,std::int64_t> field (and thus with a child field of type std::pair<char,std::int64_t>) as defined in the test here:
root/tree/ntuple/v7/test/ntuple_types.cxx
Line 377 in b5e9092
The TClass for std::pair<char,std::int64_t> is in state: 2 and has:
OBJ: TListOfDataMembers TListOfDataMembers List of TDataMembers for a class : 0
StreamerInfo for class: pair<char,Long64_t>, checksum=0xb5fb752
char first offset= 0 type= 1 Emulation
Long64_t second offset= 8 type=16 Emulation
i= 0, first type= 1, offset= 0, len=1, method=0
i= 1, second type= 16, offset= 8, len=1, method=0
unknown file: Failure
C++ exception with description "Insufficient information for std::pair<char,std::int64_t>
When I add the dictionary entry for this std::pair type the test passes without issue.
There was a problem hiding this comment.
Let's discuss this in a follow-up issue / PR.
There was a problem hiding this comment.
C++ exception with description "Insufficient information for std::pair<char,std::int64_t>
Where does the error comes from (stack trace)?
There was a problem hiding this comment.
I've opened a issue to further discuss this: #14084
b5e9092 to
0d52cbb
Compare
|
Starting build on |
1 similar comment
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
This macro is conditionally supported and with the addition of `std::map` fields, compilation warnings will be thrown for nested maps. To prevent these warnings, the offsets are determined in a similar way to `std::tuple` fields.
Necessary for builds without runtime modules enabled.
Inner fields *must* be of type `RPairField`. Ideally this should be done at compile time through the constructor argument type, but this is still causing issues and needs further investigation. Thus, this runtime check is added as a temporary solution.
25f5a2b to
573abaa
Compare
|
Starting build on |
This PR adds support for
std::mapfields through collection proxies (similar tostd::set).In theory, other (custom) associative collections (provided they implement a collection proxy) should now also be able to be added as fields. Because the use case for this is not yet obvious, this is still disabled.