Skip to content

[ntuple] Add some Internal helpers needed for Attributes #19060

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[ntuple] Add some Internal helpers needed for Attributes #19060

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions tree/ntuple/inc/ROOT/RMiniFile.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ class TVirtualStreamerInfo;
namespace ROOT {

namespace Internal {
class RNTupleFileWriter;
class RRawFile;

TDirectory *GetUnderlyingDirectory(ROOT::Internal::RNTupleFileWriter &writer);
}

class RNTupleWriteOptions;
Expand Down Expand Up @@ -104,6 +107,8 @@ A stand-alone version of RNTuple can remove the TFile based writer.
*/
// clang-format on
class RNTupleFileWriter {
friend TDirectory *GetUnderlyingDirectory(ROOT::Internal::RNTupleFileWriter &writer);

public:
/// The key length of a blob. It is always a big key (version > 1000) with class name RBlob.
static constexpr std::size_t kBlobKeyLen = 42;
Expand Down
2 changes: 2 additions & 0 deletions tree/ntuple/inc/ROOT/RNTupleModel.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ class RProjectedFields;

ROOT::RFieldZero &GetFieldZeroOfModel(RNTupleModel &model);
RProjectedFields &GetProjectedFieldsOfModel(RNTupleModel &model);
const RProjectedFields &GetProjectedFieldsOfModel(const RNTupleModel &model);

// clang-format off
/**
Expand Down Expand Up @@ -139,6 +140,7 @@ that were used for writing and are no longer connected to a page sink.
class RNTupleModel {
friend ROOT::RFieldZero &Internal::GetFieldZeroOfModel(RNTupleModel &);
friend Internal::RProjectedFields &Internal::GetProjectedFieldsOfModel(RNTupleModel &);
friend const Internal::RProjectedFields &Internal::GetProjectedFieldsOfModel(const RNTupleModel &);

public:
/// User-provided function that describes the mapping of existing source fields to projected fields in terms
Expand Down
2 changes: 2 additions & 0 deletions tree/ntuple/inc/ROOT/RPageStorage.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,8 @@ public:

virtual ROOT::NTupleSize_t GetNEntries() const = 0;

virtual TDirectory *GetUnderlyingDirectory() const { return nullptr; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the abstraction of RPageSink quite heavily, since not all sinks are a file and even of those not all have a TDirectory. I'm not sure this makes sense to ask a TDirectory from all page sinks...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the wanted functionality is to open a new page sink that ends up in the same storage? Maybe we can have an interface virtual std::unique_ptr<RPageSink> CreateAtSameLocation(std::string_view ntupleName) that throws if not implemented by a sink, or if the sink is unable to provide that functionality (simple RMiniFile, for the moment)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but for the prototype Attributes only work with TFile-based RNTuples and breaking the abstraction makes it much easier to deal with...at this point maybe the prototype should not be merged in master yet, until there is a more finalized design?
Either that or we merge it and later change it (making sure everything is in Internal/Experimental so we can do that easily)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the prototype itself, I think we cannot merge until we are sure the on-disk format is right because it requires bumping (at least) the minor version. In the past, we did merge interface extensions to make work-in-progress prototypes easier (my work on distributed parallel writing, the RNTupleProcessor while it was / is still evolving), but I think we should still strive to get the interfaces right (my 2 cents at least).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point on the binary format...we definitely cannot merge that until we're sure.


/// Physically creates the storage container to hold the ntuple (e.g., a keys a TFile or an S3 bucket)
/// Init() associates column handles to the columns referenced by the model
void Init(RNTupleModel &model)
Expand Down
2 changes: 2 additions & 0 deletions tree/ntuple/inc/ROOT/RPageStorageFile.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ public:
RPageSinkFile(RPageSinkFile &&) = default;
RPageSinkFile &operator=(RPageSinkFile &&) = default;
~RPageSinkFile() override;

TDirectory *GetUnderlyingDirectory() const final { return ROOT::Internal::GetUnderlyingDirectory(*fWriter); }
}; // class RPageSinkFile

// clang-format off
Expand Down
8 changes: 8 additions & 0 deletions tree/ntuple/src/RMiniFile.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1548,3 +1548,11 @@ void ROOT::Internal::RNTupleFileWriter::WriteTFileSkeleton(int defaultCompressio
fileSimple.Write(&padding, sizeof(padding));
fileSimple.fKeyOffset = fileSimple.fFilePos;
}

TDirectory *ROOT::Internal::GetUnderlyingDirectory(ROOT::Internal::RNTupleFileWriter &writer)
{
if (auto *proper = std::get_if<ROOT::Internal::RNTupleFileWriter::RFileProper>(&writer.fFile)) {
return proper->fDirectory;
}
return nullptr;
}
6 changes: 6 additions & 0 deletions tree/ntuple/src/RNTupleModel.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ ROOT::RFieldZero &ROOT::Internal::GetFieldZeroOfModel(ROOT::RNTupleModel &model)
}

ROOT::Internal::RProjectedFields &ROOT::Internal::GetProjectedFieldsOfModel(ROOT::RNTupleModel &model)
{
return const_cast<ROOT::Internal::RProjectedFields &>(
GetProjectedFieldsOfModel(const_cast<const ROOT::RNTupleModel &>(model)));
}

const ROOT::Internal::RProjectedFields &ROOT::Internal::GetProjectedFieldsOfModel(const ROOT::RNTupleModel &model)
{
if (model.IsExpired()) {
throw RException(R__FAIL("invalid use of expired model"));
Expand Down
Loading