-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[ntuple] various interface improvements #14454
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
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
98b5f5e
[ntuple] return reference from RNTupleModel::GetDefaultEntry()
jblomer 53673b7
[ntuple] remove RNTupleModel::Get() shortcut
jblomer 06cd9d0
[ntuple] return const ref from RNTupleReader::GetDescriptor
jblomer a3a737c
[ntuple] add RNTupleModel::GenerateBulk()
jblomer 56cbd3a
[ntuple] return const ref from RNTupleReader::GetModel()
jblomer 776a8cc
[ntuple] remove REntry::fValuePtrs
jblomer a35fa03
[ntuple] pass field ref to REntry::AddValue
jblomer 6a4186c
[ntuple] remove raw pointer version of RNTupleModel::AddField
jblomer 4eb071d
[ntuple] add doc and error handling to RNTupleModel::GenerateBulk
jblomer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -157,18 +157,20 @@ public: | |
| /// Upon completion, `BeginUpdate()` can be called again to begin a new set of changes. | ||
| void CommitUpdate(); | ||
|
|
||
| void AddField(std::unique_ptr<Detail::RFieldBase> field); | ||
| template <typename T> | ||
| void AddField(const NameWithDescription_t &fieldNameDesc, T *fromWhere) | ||
| template <typename T, typename... ArgsT> | ||
| std::shared_ptr<T> MakeField(const NameWithDescription_t &fieldNameDesc, ArgsT &&...args) | ||
| { | ||
| fOpenChangeset.fModel.AddField<T>(fieldNameDesc, fromWhere); | ||
| auto objPtr = fOpenChangeset.fModel.MakeField<T>(fieldNameDesc, std::forward<ArgsT>(args)...); | ||
| auto fieldZero = fOpenChangeset.fModel.fFieldZero.get(); | ||
| auto it = std::find_if(fieldZero->begin(), fieldZero->end(), | ||
| [&](const auto &f) { return f.GetName() == fieldNameDesc.fName; }); | ||
| R__ASSERT(it != fieldZero->end()); | ||
| fOpenChangeset.fAddedFields.emplace_back(&(*it)); | ||
| return objPtr; | ||
| } | ||
|
|
||
| void AddField(std::unique_ptr<Detail::RFieldBase> field); | ||
|
|
||
| RResult<void> AddProjectedField(std::unique_ptr<Detail::RFieldBase> field, | ||
| std::function<std::string(const std::string &)> mapping); | ||
| }; | ||
|
|
@@ -260,16 +262,15 @@ public: | |
| /// }); | ||
| /// ~~~ | ||
| template <typename T, typename... ArgsT> | ||
| std::shared_ptr<T> MakeField(const NameWithDescription_t &fieldNameDesc, | ||
| ArgsT&&... args) | ||
| std::shared_ptr<T> MakeField(const NameWithDescription_t &fieldNameDesc, ArgsT &&...args) | ||
| { | ||
| EnsureNotFrozen(); | ||
| EnsureValidFieldName(fieldNameDesc.fName); | ||
| auto field = std::make_unique<RField<T>>(fieldNameDesc.fName); | ||
| field->SetDescription(fieldNameDesc.fDescription); | ||
| std::shared_ptr<T> ptr; | ||
| if (fDefaultEntry) | ||
| ptr = fDefaultEntry->AddValue<T>(field.get(), std::forward<ArgsT>(args)...); | ||
| ptr = fDefaultEntry->AddValue<T>(*field, std::forward<ArgsT>(args)...); | ||
| fFieldZero->Attach(std::move(field)); | ||
| return ptr; | ||
| } | ||
|
|
@@ -279,34 +280,11 @@ public: | |
| /// Throws an exception if the field is null. | ||
| void AddField(std::unique_ptr<Detail::RFieldBase> field); | ||
|
|
||
| /// Throws an exception if fromWhere is null. | ||
| template <typename T> | ||
| void AddField(const NameWithDescription_t &fieldNameDesc, T* fromWhere) { | ||
| EnsureNotFrozen(); | ||
| EnsureNotBare(); | ||
| if (!fromWhere) | ||
| throw RException(R__FAIL("null field fromWhere")); | ||
| EnsureValidFieldName(fieldNameDesc.fName); | ||
|
|
||
| auto field = std::make_unique<RField<T>>(fieldNameDesc.fName); | ||
| field->SetDescription(fieldNameDesc.fDescription); | ||
| fDefaultEntry->AddValue(field->BindValue(std::shared_ptr<void>(fromWhere, [](void *) {}))); | ||
| fFieldZero->Attach(std::move(field)); | ||
| } | ||
|
|
||
| /// Adds a top-level field based on existing fields. The mapping function is called with the qualified field names | ||
| /// of the provided field and the subfields. It should return the qualified field names used as a mapping source. | ||
| /// Projected fields can only be used for models used to write data. | ||
| RResult<void> AddProjectedField(std::unique_ptr<Detail::RFieldBase> field, | ||
| std::function<std::string(const std::string &)> mapping); | ||
|
|
||
| template <typename T> | ||
| T *Get(std::string_view fieldName) const | ||
| { | ||
| EnsureNotBare(); | ||
| return fDefaultEntry->GetPtr<T>(fieldName).get(); | ||
| } | ||
|
|
||
| const RProjectedFields &GetProjectedFields() const { return *fProjectedFields; } | ||
|
|
||
| void Freeze(); | ||
|
|
@@ -325,7 +303,11 @@ public: | |
| /// In a bare entry, all values point to nullptr. The resulting entry shall use BindValue() in order | ||
| /// set memory addresses to be serialized / deserialized | ||
| std::unique_ptr<REntry> CreateBareEntry() const; | ||
| REntry *GetDefaultEntry() const; | ||
| /// Calls the given field's GenerateBulk() method. Throws an exception if no field with the given name exists. | ||
| Detail::RFieldBase::RBulk GenerateBulk(std::string_view fieldName) const; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps add some documentation |
||
|
|
||
| REntry &GetDefaultEntry(); | ||
| const REntry &GetDefaultEntry() const; | ||
|
|
||
| /// Non-const access to the root field is used to commit clusters during writing | ||
| /// and to set the on-disk field IDs when connecting a model to a page source or sink. | ||
|
|
||
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Would it be an idea to have a
Get<T>(perhaps named differently) that returns a shared pointer to the object? The use case would be for type-erased fields. Right now the (only) way to do it is throughntuple->GetModel().GetDefaultEntry()->GetPtr<T>("fieldname"), which feels a bit unwieldy.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.
That's a possibility, a getter acting as a shortcut for
GetModel().GetDefaultEntry()->GetPtr<T>. On the other hand, callers can could also first get the entry and then call (multiple times)entry.GetPtr<T>(). How about we see if users stumble across it and defer the addition of the method until then?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.
Yes fair enough, sounds good!