diff --git a/tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx b/tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx index f2ce05d728909..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; @@ -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 itemField); @@ -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 CloneImpl(std::string_view newName) const final; @@ -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 itemField); @@ -281,6 +289,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 +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 itemField); diff --git a/tree/ntuple/src/RField.cxx b/tree/ntuple/src/RField.cxx index acb4ac0d6c54e..39ae9c3e49ff2 100644 --- a/tree/ntuple/src/RField.cxx +++ b/tree/ntuple/src/RField.cxx @@ -849,18 +849,20 @@ const ROOT::RFieldBase::RColumnRepresentations &ROOT::RNullableField::GetColumnR {ENTupleColumnType::kIndex64}, {ENTupleColumnType::kSplitIndex32}, {ENTupleColumnType::kIndex32}}, - {}); + {{}}); return representations; } void ROOT::RNullableField::GenerateColumns() { + R__ASSERT(!fIsEvolvedFromInnerType); GenerateColumnsImpl(); } void ROOT::RNullableField::GenerateColumns(const ROOT::RNTupleDescriptor &desc) { - GenerateColumnsImpl(desc); + if (!fIsEvolvedFromInnerType) + GenerateColumnsImpl(desc); } std::size_t ROOT::RNullableField::AppendNull() @@ -881,8 +883,16 @@ void ROOT::RNullableField::ReconcileOnDiskField(const RNTupleDescriptor &desc) { static const std::vector prefixes = {"std::optional<", "std::unique_ptr<"}; - EnsureMatchingOnDiskField(desc, kDiffTypeName).ThrowOnError(); - EnsureMatchingTypePrefix(desc, prefixes).ThrowOnError(); + auto success = EnsureMatchingOnDiskField(desc, kDiffTypeName); + if (!success) { + fIsEvolvedFromInnerType = true; + } else { + success = EnsureMatchingTypePrefix(desc, prefixes); + fIsEvolvedFromInnerType = !success; + } + + if (fIsEvolvedFromInnerType) + fSubfields[0]->SetOnDiskId(GetOnDiskId()); } ROOT::RNTupleLocalIndex ROOT::RNullableField::GetItemIndex(ROOT::NTupleSize_t globalIndex) @@ -893,6 +903,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); @@ -921,33 +939,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 *>(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; + } else if (!isValidValue && hasOnDiskValue) { + valuePtr = CallCreateObjectRawPtrOn(*fSubfields[0]); + ptr->reset(reinterpret_cast(valuePtr)); } - if (!isValidItem) // On-disk value missing; nothing else to do - return; + return valuePtr; +} - if (!isValidValue) { - valuePtr = CallCreateObjectRawPtrOn(*fSubfields[0]); - ptr->reset(reinterpret_cast(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) @@ -1010,20 +1049,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 diff --git a/tree/ntuple/test/ntuple_evolution_type.cxx b/tree/ntuple/test/ntuple_evolution_type.cxx index f939370bc7636..e55211a2528b9 100644 --- a/tree/ntuple/test/ntuple_evolution_type.cxx +++ b/tree/ntuple/test/ntuple_evolution_type.cxx @@ -283,6 +283,42 @@ TEST(RNTupleEvolution, ArrayAsVector) EXPECT_TRUE(aAsBool(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)); +} + TEST(RNTupleEvolution, NullableToVector) { FileRaii fileGuard("test_ntuple_evolution_nullable_to_vector.root");