From 6612d99a65da64fc571115c5f0b38c8bd930773f Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Wed, 24 Sep 2025 03:20:36 +0200 Subject: [PATCH 1/2] [ntuple] prepare for automatic evolution T --> unique_ptr/optional --- tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx | 15 +++- tree/ntuple/src/RField.cxx | 75 +++++++++++++------ 2 files changed, 66 insertions(+), 24 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx b/tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx index f2ce05d728909..c38b91b0f67f6 100644 --- a/tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx +++ b/tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx @@ -205,9 +205,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 itemField); @@ -238,6 +241,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 CloneImpl(std::string_view newName) const final; @@ -247,6 +251,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 itemField); @@ -281,6 +286,9 @@ class RUniquePtrField : public RNullableField { std::unique_ptr fItemDeleter; + // Returns the value pointer, i.e. where to read the subfield into + void *PrepareRead(void *to, bool hasOnDiskValue); + protected: std::unique_ptr CloneImpl(std::string_view newName) const final; @@ -289,6 +297,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 itemField); diff --git a/tree/ntuple/src/RField.cxx b/tree/ntuple/src/RField.cxx index e7e32ca2856bb..3a34b08fba355 100644 --- a/tree/ntuple/src/RField.cxx +++ b/tree/ntuple/src/RField.cxx @@ -891,6 +891,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); @@ -919,33 +927,42 @@ 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 *>(to); bool isValidValue = static_cast(*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; - } - - if (!isValidItem) // On-disk value missing; nothing else to do - return; - - if (!isValidValue) { + } else if (!isValidValue && hasOnDiskValue) { valuePtr = CallCreateObjectRawPtrOn(*fSubfields[0]); ptr->reset(reinterpret_cast(valuePtr)); } - CallReadOn(*fSubfields[0], itemIndex, valuePtr); + return valuePtr; +} + +void ROOT::RUniquePtrField::ReadGlobalImpl(ROOT::NTupleSize_t globalIndex, void *to) +{ + auto itemIndex = GetItemIndex(globalIndex); + const bool hasOnDiskValue = itemIndex.GetIndexInCluster() != ROOT::kInvalidNTupleIndex; + auto valuePtr = PrepareRead(to, hasOnDiskValue); + if (hasOnDiskValue) + CallReadOn(*fSubfields[0], itemIndex, valuePtr); +} + +void ROOT::RUniquePtrField::ReadInClusterImpl(ROOT::RNTupleLocalIndex localIndex, void *to) +{ + auto itemIndex = GetItemIndex(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) @@ -1008,22 +1025,38 @@ 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 (*engagementPtr && !(fSubfields[0]->GetTraits() & kTraitTriviallyDestructible)) - fItemDeleter->operator()(to, true /* dtorOnly */); - *engagementPtr = false; - } else { + if (hasOnDiskValue) { if (!(*engagementPtr) && !(fSubfields[0]->GetTraits() & kTraitTriviallyConstructible)) CallConstructValueOn(*fSubfields[0], to); - CallReadOn(*fSubfields[0], itemIndex, 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) +{ + auto itemIndex = GetItemIndex(globalIndex); + const bool hasOnDiskValue = itemIndex.GetIndexInCluster() != ROOT::kInvalidNTupleIndex; + PrepareRead(to, hasOnDiskValue); + if (hasOnDiskValue) + CallReadOn(*fSubfields[0], itemIndex, to); +} + +void ROOT::ROptionalField::ReadInClusterImpl(ROOT::RNTupleLocalIndex localIndex, void *to) +{ + auto itemIndex = GetItemIndex(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 { *GetEngagementPtr(where) = false; From edc8d6d4cfd4ed89eff3987aa975229023b089e8 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Wed, 24 Sep 2025 03:53:42 +0200 Subject: [PATCH 2/2] [ntuple] automatic evolution from T --> unique_ptr/optional --- tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx | 3 + tree/ntuple/src/RField.cxx | 58 ++++++++++++++----- tree/ntuple/test/ntuple_evolution_type.cxx | 36 ++++++++++++ 3 files changed, 83 insertions(+), 14 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx b/tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx index c38b91b0f67f6..55af24daabaf4 100644 --- a/tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx +++ b/tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx @@ -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; diff --git a/tree/ntuple/src/RField.cxx b/tree/ntuple/src/RField.cxx index 3a34b08fba355..788e63bb3161a 100644 --- a/tree/ntuple/src/RField.cxx +++ b/tree/ntuple/src/RField.cxx @@ -846,7 +846,7 @@ const ROOT::RFieldBase::RColumnRepresentations &ROOT::RNullableField::GetColumnR {ENTupleColumnType::kIndex64}, {ENTupleColumnType::kSplitIndex32}, {ENTupleColumnType::kIndex32}}, - {}); + {{}}); return representations; } @@ -857,7 +857,8 @@ void ROOT::RNullableField::GenerateColumns() void ROOT::RNullableField::GenerateColumns(const ROOT::RNTupleDescriptor &desc) { - GenerateColumnsImpl(desc); + if (!fIsEvolvedFromInnerType) + GenerateColumnsImpl(desc); } std::size_t ROOT::RNullableField::AppendNull() @@ -879,8 +880,13 @@ void ROOT::RNullableField::ReconcileOnDiskField(const RNTupleDescriptor &desc) static const std::vector 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()); + fIsEvolvedFromInnerType = true; + } } ROOT::RNTupleLocalIndex ROOT::RNullableField::GetItemIndex(ROOT::NTupleSize_t globalIndex) @@ -949,16 +955,28 @@ void *ROOT::RUniquePtrField::PrepareRead(void *to, bool hasOnDiskValue) void ROOT::RUniquePtrField::ReadGlobalImpl(ROOT::NTupleSize_t globalIndex, void *to) { - auto itemIndex = GetItemIndex(globalIndex); - const bool hasOnDiskValue = itemIndex.GetIndexInCluster() != ROOT::kInvalidNTupleIndex; + RNTupleLocalIndex itemIndex; + if (!fIsEvolvedFromInnerType) + itemIndex = GetItemIndex(globalIndex); + const bool hasOnDiskValue = fIsEvolvedFromInnerType || itemIndex.GetIndexInCluster() != ROOT::kInvalidNTupleIndex; auto valuePtr = PrepareRead(to, hasOnDiskValue); - if (hasOnDiskValue) - CallReadOn(*fSubfields[0], itemIndex, valuePtr); + if (hasOnDiskValue) { + if (fIsEvolvedFromInnerType) { + CallReadOn(*fSubfields[0], globalIndex, valuePtr); + } else { + CallReadOn(*fSubfields[0], itemIndex, valuePtr); + } + } } void ROOT::RUniquePtrField::ReadInClusterImpl(ROOT::RNTupleLocalIndex localIndex, void *to) { - auto itemIndex = GetItemIndex(localIndex); + RNTupleLocalIndex itemIndex; + if (!fIsEvolvedFromInnerType) { + itemIndex = GetItemIndex(localIndex); + } else { + itemIndex = localIndex; + } const bool hasOnDiskValue = itemIndex.GetIndexInCluster() != ROOT::kInvalidNTupleIndex; auto valuePtr = PrepareRead(to, hasOnDiskValue); if (hasOnDiskValue) @@ -1041,16 +1059,28 @@ void ROOT::ROptionalField::PrepareRead(void *to, bool hasOnDiskValue) void ROOT::ROptionalField::ReadGlobalImpl(ROOT::NTupleSize_t globalIndex, void *to) { - auto itemIndex = GetItemIndex(globalIndex); - const bool hasOnDiskValue = itemIndex.GetIndexInCluster() != ROOT::kInvalidNTupleIndex; + RNTupleLocalIndex itemIndex; + if (!fIsEvolvedFromInnerType) + itemIndex = GetItemIndex(globalIndex); + const bool hasOnDiskValue = fIsEvolvedFromInnerType || itemIndex.GetIndexInCluster() != ROOT::kInvalidNTupleIndex; PrepareRead(to, hasOnDiskValue); - if (hasOnDiskValue) - CallReadOn(*fSubfields[0], itemIndex, to); + if (hasOnDiskValue) { + if (fIsEvolvedFromInnerType) { + CallReadOn(*fSubfields[0], globalIndex, to); + } else { + CallReadOn(*fSubfields[0], itemIndex, to); + } + } } void ROOT::ROptionalField::ReadInClusterImpl(ROOT::RNTupleLocalIndex localIndex, void *to) { - auto itemIndex = GetItemIndex(localIndex); + RNTupleLocalIndex itemIndex; + if (!fIsEvolvedFromInnerType) { + itemIndex = GetItemIndex(localIndex); + } else { + itemIndex = localIndex; + } const bool hasOnDiskValue = itemIndex.GetIndexInCluster() != ROOT::kInvalidNTupleIndex; PrepareRead(to, hasOnDiskValue); if (hasOnDiskValue) diff --git a/tree/ntuple/test/ntuple_evolution_type.cxx b/tree/ntuple/test/ntuple_evolution_type.cxx index c70bb30dfda4d..1477b204af54f 100644 --- a/tree/ntuple/test/ntuple_evolution_type.cxx +++ b/tree/ntuple/test/ntuple_evolution_type.cxx @@ -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>("o"); + auto u = model->MakeField>("u"); + auto i = model->MakeField("i"); + auto writer = ROOT::RNTupleWriter::Recreate(std::move(model), "ntpl", fileGuard.GetPath()); + + *o = 7; + *u = std::make_unique(11); + *i = 13; + writer->Fill(); + } + + auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath()); + + auto v1 = reader->GetView>("o"); + auto v2 = reader->GetView>("u"); + auto v3 = reader->GetView>("i"); + auto v4 = reader->GetView>("i"); + + try { + reader->GetView>("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)); +}