-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[ntuple] add streaming vector tutorial #17139
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] add streaming vector tutorial #17139
Conversation
77f0ca8
to
26e598e
Compare
Make RNTupleGlobalRange and RNTupleClusterRange copyable and default-constructible.
Workaround. The underlying problem, moving the RField, is still broken. To be addressed at a later point.
26e598e
to
570e576
Compare
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 is a very nice addition thanks! A few considerations from my side
|
||
public: | ||
iterator(RNTupleClusterRange::RIterator index, RNTupleView<T> &view) : fIndex(index), fView(view) {} | ||
~iterator() = default; |
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.
If we have to define this destructor, we should follow rule of five
RStreamingVector(RNTupleCollectionView &&vectorView) | ||
: fVectorView(std::move(vectorView)), fItemView(fVectorView.GetView<T>("_0")) | ||
{ | ||
} | ||
~RStreamingVector() = default; |
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.
Similar comment about rule of five
/// \file | ||
/// \ingroup tutorial_ntuple | ||
/// | ||
/// Example of a streaming vector: a special purpose container that reads large vectors piece-wise. |
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 feel like the tutorial is lacking some explanation of the interplay between the various components. Especially in places like RStreamingVector::iterator constructor and operator*
. We should probably point the user to also check the functioning of RNTupleCollectionView
to understand better the rest of the tutorial
Test Results 18 files 18 suites 3d 23h 25m 59s ⏱️ For more details on these failures, see this check. Results for commit 570e576. |
public: | ||
class iterator { | ||
RNTupleClusterRange::RIterator fIndex; | ||
RNTupleView<T> &fView; |
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.
should store a pointer to make the type copyable
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.
Should this be copyable though? To me the semantics would be a bit fuzzy (would the copy automatically get the same window of entries loaded in? Also, when you copy a std::vector you do a deep copy, but in this case to my understanding it wouldn'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.
Doesn't that hint that the class is more of a RStreamingVectorView
and/or RVectorStreamingView
?
bool operator!=(const iterator &rh) const { return fIndex != rh.fIndex; } | ||
}; | ||
|
||
RStreamingVector(RNTupleCollectionView &&vectorView) |
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.
should take the view by value (cf previous discussion on taking move-only types by value vs by xvalue reference)
|
||
void ReadRNTupleStreamingVector() | ||
{ | ||
auto reader = RNTupleReader::Open(kNTupleName, kFileName); |
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.
should open the reader with disabled cluster cache, otherwise all pages will still send up in memory I think
Replaced by #19748 |
Demonstrates how to use the RNTuple API to build a special purpose container that reads large vectors piece-wise.
Original idea & code from @gemmeren