-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ntuple] Ensure type name given by RField<T> is renormalized #17986
base: master
Are you sure you want to change the base?
[ntuple] Ensure type name given by RField<T> is renormalized #17986
Conversation
To avoid inconsistencies between the full type name of a custom class reported by the RNTuple IO and the type name reported by ROOT meta. The commit adds a unittest. Previously, the test would fail with exceptions such as: ``` 321: unknown file: Failure 321: C++ exception with description "type mismatch for field f2: DataVector<std::int32_t,std::vector<CustomStruct>> vs. DataVector<int,vector<CustomStruct> > 321: At: 321: void ROOT::Experimental::REntry::EnsureMatchingType(RFieldToken) const [with T = DataVector<int, std::vector<CustomStruct> >] 321: " thrown in the test body. ``` Co-authored-by: Jonas Hahnfeld <[email protected]>
Test Results 20 files 20 suites 5d 14h 35m 3s ⏱️ Results for commit 950713d. |
@@ -286,7 +287,7 @@ public: | |||
template <typename T, typename = void> | |||
class RField final : public RClassField { | |||
public: | |||
static std::string TypeName() { return ROOT::Internal::GetDemangledTypeName(typeid(T)); } | |||
static std::string TypeName() { return ROOT::Experimental::Internal::GetDemangledRenormalizedTypeName(typeid(T)); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't GetDemangledRenormalizedTypeName
be simply in ROOT::Internal
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RFieldUtils
moves out of experimental with #18023
{ | ||
auto model = RNTupleModel::Create(); | ||
auto f1 = model->MakeField<DataVector<int>>("f1"); | ||
auto f2 = model->MakeField<DataVector<int, std::vector<CustomStruct>>>("f2"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add 'new' dictionaries request to this test in order to fully test the user case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we need the same treatment for RField
inheriting from RProxiedCollectionField
@@ -286,7 +287,7 @@ public: | |||
template <typename T, typename = void> | |||
class RField final : public RClassField { | |||
public: | |||
static std::string TypeName() { return ROOT::Internal::GetDemangledTypeName(typeid(T)); } | |||
static std::string TypeName() { return ROOT::Experimental::Internal::GetDemangledRenormalizedTypeName(typeid(T)); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RFieldUtils
moves out of experimental with #18023
@@ -58,6 +59,8 @@ std::tuple<std::string, std::vector<std::size_t>> ParseArrayType(const std::stri | |||
/// TODO(jblomer): Try to merge with TClassEdit::TSplitType | |||
std::vector<std::string> TokenizeTypeList(std::string_view templateType); | |||
|
|||
std::string GetDemangledRenormalizedTypeName(const std::type_info &ti); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move the declaration below GetRenormalizedTypeName
and add a documentation comment when it should be used?
auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath()); | ||
EXPECT_EQ(0u, reader->GetNEntries()); | ||
|
||
// The test would fail because of an exception thrown in REntry::EnsureMatchingType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is confusing in the future, I would suggest to remove it.
auto f1 = reader->GetModel().GetDefaultEntry().GetPtr<DataVector<int>>("f1"); | ||
auto f2 = reader->GetModel().GetDefaultEntry().GetPtr<DataVector<int, std::vector<CustomStruct>>>("f2"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can go into the existing TClassDefaultTemplateParameter
which already creates the exact fields. It already tests views, which work because we allow casting and only require that the on-disk column types can be connected...
To avoid inconsistencies between the full type name of a custom class reported by the RNTuple IO and the type name reported by ROOT meta. The commit adds a unittest. Previously, the test would fail with exceptions such as: