Skip to content

[ntuple] automatic evolution of nullable fields #19966

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+137 −26
Open

[ntuple] automatic evolution of nullable fields #19966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6612d99
[ntuple] prepare for automatic evolution T --> unique_ptr/optional<T>
jblomer Sep 24, 2025
edc8d6d
[ntuple] automatic evolution from T --> unique_ptr/optional<T>
jblomer Sep 24, 2025
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
18 changes: 15 additions & 3 deletions tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,9 @@ class RNullableField : public RFieldBase {
ROOT::Internal::RColumnIndex fNWritten{0};

protected:
// For reading, indicates that we read type T as a nullable field of type T, i.e. the value is always present
bool fIsEvolvedFromInnerType = false;

const RFieldBase::RColumnRepresentations &GetColumnRepresentations() const final;
void GenerateColumns() final;
void GenerateColumns(const ROOT::RNTupleDescriptor &) final;
Expand All @@ -205,9 +208,12 @@ protected:

void ReconcileOnDiskField(const RNTupleDescriptor &desc) final;

/// Given the index of the nullable field, returns the corresponding global index of the subfield or,
/// if it is null, returns `kInvalidNTupleIndex`
RNTupleLocalIndex GetItemIndex(ROOT::NTupleSize_t globalIndex);
/// Given the global index of the nullable field, returns the corresponding cluster-local index of the subfield or,
/// if it is null, returns a default constructed RNTupleLocalIndex
RNTupleLocalIndex GetItemIndex(NTupleSize_t globalIndex);
/// Given the cluster-local index of the nullable field, returns the corresponding cluster-local index of
/// the subfield or, if it is null, returns a default constructed RNTupleLocalIndex
RNTupleLocalIndex GetItemIndex(RNTupleLocalIndex localIndex);

RNullableField(std::string_view fieldName, const std::string &typePrefix, std::unique_ptr<RFieldBase> itemField);

Expand Down Expand Up @@ -238,6 +244,7 @@ class ROptionalField : public RNullableField {
const bool *GetEngagementPtr(const void *optionalPtr) const;
bool *GetEngagementPtr(void *optionalPtr) const;
std::size_t GetEngagementPtrOffset() const;
void PrepareRead(void *to, bool hasOnDiskValue);

protected:
std::unique_ptr<RFieldBase> CloneImpl(std::string_view newName) const final;
Expand All @@ -247,6 +254,7 @@ protected:

std::size_t AppendImpl(const void *from) final;
void ReadGlobalImpl(ROOT::NTupleSize_t globalIndex, void *to) final;
void ReadInClusterImpl(ROOT::RNTupleLocalIndex localIndex, void *to) final;

public:
ROptionalField(std::string_view fieldName, std::unique_ptr<RFieldBase> itemField);
Expand Down Expand Up @@ -281,6 +289,9 @@ class RUniquePtrField : public RNullableField {

std::unique_ptr<RDeleter> fItemDeleter;

// Returns the value pointer, i.e. where to read the subfield into
void *PrepareRead(void *to, bool hasOnDiskValue);

protected:
std::unique_ptr<RFieldBase> CloneImpl(std::string_view newName) const final;

Expand All @@ -289,6 +300,7 @@ protected:

std::size_t AppendImpl(const void *from) final;
void ReadGlobalImpl(ROOT::NTupleSize_t globalIndex, void *to) final;
void ReadInClusterImpl(ROOT::RNTupleLocalIndex localIndex, void *to) final;

public:
RUniquePtrField(std::string_view fieldName, std::unique_ptr<RFieldBase> itemField);
Expand Down
109 changes: 86 additions & 23 deletions tree/ntuple/src/RField.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -846,7 +846,7 @@ const ROOT::RFieldBase::RColumnRepresentations &ROOT::RNullableField::GetColumnR
{ENTupleColumnType::kIndex64},
{ENTupleColumnType::kSplitIndex32},
{ENTupleColumnType::kIndex32}},
{});
{{}});
return representations;
}

Expand All @@ -857,7 +857,8 @@ void ROOT::RNullableField::GenerateColumns()

void ROOT::RNullableField::GenerateColumns(const ROOT::RNTupleDescriptor &desc)
{
GenerateColumnsImpl<ROOT::Internal::RColumnIndex>(desc);
if (!fIsEvolvedFromInnerType)
GenerateColumnsImpl<ROOT::Internal::RColumnIndex>(desc);
}

std::size_t ROOT::RNullableField::AppendNull()
Expand All @@ -879,8 +880,13 @@ void ROOT::RNullableField::ReconcileOnDiskField(const RNTupleDescriptor &desc)
static const std::vector<std::string> prefixes = {"std::optional<", "std::unique_ptr<"};

const auto &fieldDesc = desc.GetFieldDescriptor(GetOnDiskId());
EnsureMatchingOnDiskField(fieldDesc, kDiffTypeName);
EnsureMatchingTypePrefix(fieldDesc, prefixes);
try {
EnsureMatchingOnDiskField(fieldDesc, kDiffTypeName);
EnsureMatchingTypePrefix(fieldDesc, prefixes);
} catch (const RException &) {
fSubfields[0]->SetOnDiskId(GetOnDiskId());
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a bit sketchy to me: since we're using RException for everything, how can we know that it's not a fatal exception, possibly coming from a deep call inside the Ensure methods?

Unrelated, for the Ensure methods I would adopt the same approach as e.g. VerifyXxHash3, where the call returns a RResult rather than throwing an exception directly (of course in a separate PR)

Copy link
Member

Choose a reason for hiding this comment

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

Also similar question to #19937 (review): Is unconditionally delegating to the item field maybe confusing for the error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated, for the Ensure methods I would adopt the same approach as e.g. VerifyXxHash3, where the call returns a RResult rather than throwing an exception directly (of course in a separate PR)

Makes sense. See #19971

fIsEvolvedFromInnerType = true;
}
}

ROOT::RNTupleLocalIndex ROOT::RNullableField::GetItemIndex(ROOT::NTupleSize_t globalIndex)
Expand All @@ -891,6 +897,14 @@ ROOT::RNTupleLocalIndex ROOT::RNullableField::GetItemIndex(ROOT::NTupleSize_t gl
return (collectionSize == 0) ? RNTupleLocalIndex() : collectionStart;
}

ROOT::RNTupleLocalIndex ROOT::RNullableField::GetItemIndex(ROOT::RNTupleLocalIndex localIndex)
{
RNTupleLocalIndex collectionStart;
ROOT::NTupleSize_t collectionSize;
fPrincipalColumn->GetCollectionInfo(localIndex, &collectionStart, &collectionSize);
return (collectionSize == 0) ? RNTupleLocalIndex() : collectionStart;
}

void ROOT::RNullableField::AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) const
{
visitor.VisitNullableField(*this);
Expand Down Expand Up @@ -919,33 +933,54 @@ std::size_t ROOT::RUniquePtrField::AppendImpl(const void *from)
}
}

void ROOT::RUniquePtrField::ReadGlobalImpl(ROOT::NTupleSize_t globalIndex, void *to)
void *ROOT::RUniquePtrField::PrepareRead(void *to, bool hasOnDiskValue)
{
auto ptr = static_cast<std::unique_ptr<char> *>(to);
bool isValidValue = static_cast<bool>(*ptr);

auto itemIndex = GetItemIndex(globalIndex);
bool isValidItem = itemIndex.GetIndexInCluster() != ROOT::kInvalidNTupleIndex;

void *valuePtr = nullptr;
if (isValidValue)
valuePtr = ptr->get();

if (isValidValue && !isValidItem) {
if (isValidValue && !hasOnDiskValue) {
ptr->release();
fItemDeleter->operator()(valuePtr, false /* dtorOnly */);
return;
} else if (!isValidValue && hasOnDiskValue) {
valuePtr = CallCreateObjectRawPtrOn(*fSubfields[0]);
ptr->reset(reinterpret_cast<char *>(valuePtr));
}

if (!isValidItem) // On-disk value missing; nothing else to do
return;
return valuePtr;
}

if (!isValidValue) {
valuePtr = CallCreateObjectRawPtrOn(*fSubfields[0]);
ptr->reset(reinterpret_cast<char *>(valuePtr));
void ROOT::RUniquePtrField::ReadGlobalImpl(ROOT::NTupleSize_t globalIndex, void *to)
{
RNTupleLocalIndex itemIndex;
if (!fIsEvolvedFromInnerType)
itemIndex = GetItemIndex(globalIndex);
const bool hasOnDiskValue = fIsEvolvedFromInnerType || itemIndex.GetIndexInCluster() != ROOT::kInvalidNTupleIndex;
auto valuePtr = PrepareRead(to, hasOnDiskValue);
if (hasOnDiskValue) {
if (fIsEvolvedFromInnerType) {
CallReadOn(*fSubfields[0], globalIndex, valuePtr);
} else {
CallReadOn(*fSubfields[0], itemIndex, valuePtr);
}
}
}

CallReadOn(*fSubfields[0], itemIndex, valuePtr);
void ROOT::RUniquePtrField::ReadInClusterImpl(ROOT::RNTupleLocalIndex localIndex, void *to)
{
RNTupleLocalIndex itemIndex;
if (!fIsEvolvedFromInnerType) {
itemIndex = GetItemIndex(localIndex);
} else {
itemIndex = localIndex;
}
const bool hasOnDiskValue = itemIndex.GetIndexInCluster() != ROOT::kInvalidNTupleIndex;
auto valuePtr = PrepareRead(to, hasOnDiskValue);
if (hasOnDiskValue)
CallReadOn(*fSubfields[0], itemIndex, valuePtr);
}

void ROOT::RUniquePtrField::RUniquePtrDeleter::operator()(void *objPtr, bool dtorOnly)
Expand Down Expand Up @@ -1008,20 +1043,48 @@ std::size_t ROOT::ROptionalField::AppendImpl(const void *from)
}
}

void ROOT::ROptionalField::ReadGlobalImpl(ROOT::NTupleSize_t globalIndex, void *to)
void ROOT::ROptionalField::PrepareRead(void *to, bool hasOnDiskValue)
{
auto engagementPtr = GetEngagementPtr(to);
auto itemIndex = GetItemIndex(globalIndex);
if (itemIndex.GetIndexInCluster() == ROOT::kInvalidNTupleIndex) {
if (hasOnDiskValue) {
if (!(*engagementPtr) && !(fSubfields[0]->GetTraits() & kTraitTriviallyConstructible))
CallConstructValueOn(*fSubfields[0], to);
*engagementPtr = true;
} else {
if (*engagementPtr && !(fSubfields[0]->GetTraits() & kTraitTriviallyDestructible))
fItemDeleter->operator()(to, true /* dtorOnly */);
*engagementPtr = false;
}
}

void ROOT::ROptionalField::ReadGlobalImpl(ROOT::NTupleSize_t globalIndex, void *to)
{
RNTupleLocalIndex itemIndex;
if (!fIsEvolvedFromInnerType)
itemIndex = GetItemIndex(globalIndex);
const bool hasOnDiskValue = fIsEvolvedFromInnerType || itemIndex.GetIndexInCluster() != ROOT::kInvalidNTupleIndex;
PrepareRead(to, hasOnDiskValue);
if (hasOnDiskValue) {
if (fIsEvolvedFromInnerType) {
CallReadOn(*fSubfields[0], globalIndex, to);
} else {
CallReadOn(*fSubfields[0], itemIndex, to);
}
}
}

void ROOT::ROptionalField::ReadInClusterImpl(ROOT::RNTupleLocalIndex localIndex, void *to)
{
RNTupleLocalIndex itemIndex;
if (!fIsEvolvedFromInnerType) {
itemIndex = GetItemIndex(localIndex);
} else {
if (!(*engagementPtr) && !(fSubfields[0]->GetTraits() & kTraitTriviallyConstructible))
CallConstructValueOn(*fSubfields[0], to);
CallReadOn(*fSubfields[0], itemIndex, to);
*engagementPtr = true;
itemIndex = localIndex;
}
const bool hasOnDiskValue = itemIndex.GetIndexInCluster() != ROOT::kInvalidNTupleIndex;
PrepareRead(to, hasOnDiskValue);
if (hasOnDiskValue)
CallReadOn(*fSubfields[0], itemIndex, to);
}

void ROOT::ROptionalField::ConstructValue(void *where) const
Expand Down
36 changes: 36 additions & 0 deletions tree/ntuple/test/ntuple_evolution_type.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,3 +251,39 @@ TEST(RNTupleEvolution, ArrayAsRVec)
EXPECT_EQ(1, a(0)[0]);
EXPECT_EQ(2, a(0)[1]);
}

TEST(RNTupleEvolution, CheckNullable)
{
FileRaii fileGuard("test_ntuple_evolution_check_nullable.root");
{
auto model = ROOT::RNTupleModel::Create();
auto o = model->MakeField<std::optional<std::int32_t>>("o");
auto u = model->MakeField<std::unique_ptr<std::int32_t>>("u");
auto i = model->MakeField<std::int32_t>("i");
auto writer = ROOT::RNTupleWriter::Recreate(std::move(model), "ntpl", fileGuard.GetPath());

*o = 7;
*u = std::make_unique<std::int32_t>(11);
*i = 13;
writer->Fill();
}

auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath());

auto v1 = reader->GetView<std::unique_ptr<std::int64_t>>("o");
auto v2 = reader->GetView<std::optional<std::int64_t>>("u");
auto v3 = reader->GetView<std::unique_ptr<std::int64_t>>("i");
auto v4 = reader->GetView<std::optional<std::int64_t>>("i");

try {
reader->GetView<std::optional<std::string>>("i");
FAIL() << "evolution of a nullable field with an invalid inner field should throw";
} catch (const ROOT::RException &err) {
EXPECT_THAT(err.what(), testing::HasSubstr("of type std::string is incompatible with on-disk field"));
}

EXPECT_EQ(7, *v1(0));
EXPECT_EQ(11, *v2(0));
EXPECT_EQ(13, *v3(0));
EXPECT_EQ(13, *v4(0));
}
Loading