diff --git a/roottest/root/io/hadd/CMakeLists.txt b/roottest/root/io/hadd/CMakeLists.txt index dc25e3d81b16b..ebbde6dd99ab5 100644 --- a/roottest/root/io/hadd/CMakeLists.txt +++ b/roottest/root/io/hadd/CMakeLists.txt @@ -270,3 +270,14 @@ ROOTTEST_ADD_TEST(test_hadd_regr_20872_2 PASSREGEX "root://eospublic.cern.ch//eos/root-eos/h1/dstarmb.root cannot be both the target and an input!" ) endif() + +# Verify that the hadd (or rather, the RNTupleMerger) is able to cope with RNTuples written before schema version +# 1.0.0.1 (meaning they have the wrong type name normalization) +configure_file(test_hadd_merge_rntuple_634_1.root . COPYONLY) +configure_file(test_hadd_merge_rntuple_634_2.root . COPYONLY) +configure_file(check_merge_rntuple_634.C . COPYONLY) +ROOTTEST_ADD_TEST(test_hadd_merge_rntuple_634, + PRECMD ${CMAKE_COMMAND} -E rm -f test_hadd_merge_rntuple_634_merged.root + COMMAND ${ROOT_hadd_CMD} -f test_hadd_merge_rntuple_634_merged.root test_hadd_merge_rntuple_634_1.root test_hadd_merge_rntuple_634_2.root + POSTCMD ${ROOT_root_CMD} -q check_merge_rntuple_634.C + PASSRC 0) diff --git a/roottest/root/io/hadd/check_merge_rntuple_634.C b/roottest/root/io/hadd/check_merge_rntuple_634.C new file mode 100644 index 0000000000000..81a089c0d7ee4 --- /dev/null +++ b/roottest/root/io/hadd/check_merge_rntuple_634.C @@ -0,0 +1,16 @@ +int check_merge_rntuple_634() +{ + // Verify that the given RNTuple is readable + auto reader = ROOT::RNTupleReader::Open("Events", "test_hadd_merge_rntuple_634_merged.root"); + const auto &model = reader->GetModel(); + const auto &desc = reader->GetDescriptor(); + for (const auto &fdesc : desc.GetFieldIterable(desc.GetFieldZeroId())) { + if (fdesc.GetTypeName() != ROOT::Internal::GetRenormalizedTypeName(fdesc.GetTypeName())) { + std::cerr << "Type name is not renormalized! " << fdesc.GetTypeName() << " vs " << + ROOT::Internal::GetRenormalizedTypeName(fdesc.GetTypeName()) << "\n"; + return 1; + } + } + + return 0; +} diff --git a/roottest/root/io/hadd/test_hadd_merge_rntuple_634_1.root b/roottest/root/io/hadd/test_hadd_merge_rntuple_634_1.root new file mode 100644 index 0000000000000..fa6afc42f5b3a Binary files /dev/null and b/roottest/root/io/hadd/test_hadd_merge_rntuple_634_1.root differ diff --git a/roottest/root/io/hadd/test_hadd_merge_rntuple_634_2.root b/roottest/root/io/hadd/test_hadd_merge_rntuple_634_2.root new file mode 100644 index 0000000000000..a97d966d70440 Binary files /dev/null and b/roottest/root/io/hadd/test_hadd_merge_rntuple_634_2.root differ diff --git a/tree/ntuple/inc/ROOT/RNTupleDescriptor.hxx b/tree/ntuple/inc/ROOT/RNTupleDescriptor.hxx index dcd5e0432a7b1..43421780512e1 100644 --- a/tree/ntuple/inc/ROOT/RNTupleDescriptor.hxx +++ b/tree/ntuple/inc/ROOT/RNTupleDescriptor.hxx @@ -49,6 +49,7 @@ class RColumnElementBase; } class RNTupleDescriptor; +class RFieldDescriptor; namespace Internal { class RColumnDescriptorBuilder; @@ -65,6 +66,8 @@ struct RNTupleClusterBoundaries { }; std::vector GetClusterBoundaries(const RNTupleDescriptor &desc); + +void FixupFieldTypeName(ROOT::RFieldDescriptor &fieldDesc); } // namespace Internal namespace Experimental { @@ -121,6 +124,7 @@ class RNTupleAttrSetDescriptorIterable; class RFieldDescriptor final { friend class Internal::RNTupleDescriptorBuilder; friend class Internal::RFieldDescriptorBuilder; + friend void Internal::FixupFieldTypeName(ROOT::RFieldDescriptor &fieldDesc); private: ROOT::DescriptorId_t fFieldId = ROOT::kInvalidDescriptorId; @@ -768,6 +772,12 @@ private: /// when merging two RNTuples. RNTupleDescriptor CloneSchema() const; + /// ROOT v6.34, with spec versions before 1.0.0.1, did not properly renormalize the type name. + /// This function returns true if this descriptor has a version prior to 1.0.0.1 and may therefore contain such + /// fields. This is only valid to call after SetVersion() or SetVersionForWriting() has been called on this + /// descriptor. + bool FieldTypeNamesMayNeedFixup() const; + public: /// All known feature flags. /// Note that the flag values represent the bit _index_, not the already-bitshifted integer. diff --git a/tree/ntuple/src/RNTupleDescriptor.cxx b/tree/ntuple/src/RNTupleDescriptor.cxx index 95f104e01c038..632ef55f9c3d0 100644 --- a/tree/ntuple/src/RNTupleDescriptor.cxx +++ b/tree/ntuple/src/RNTupleDescriptor.cxx @@ -420,13 +420,17 @@ std::string ROOT::RNTupleDescriptor::GetQualifiedFieldName(ROOT::DescriptorId_t return prefix + "." + fieldDescriptor.GetFieldName(); } +bool ROOT::RNTupleDescriptor::FieldTypeNamesMayNeedFixup() const +{ + R__ASSERT(fVersionEpoch == 1); + return fVersionMajor == 0 && fVersionMinor == 0 && fVersionPatch < 1; +} + std::string ROOT::RNTupleDescriptor::GetTypeNameForComparison(const RFieldDescriptor &fieldDesc) const { std::string typeName = fieldDesc.GetTypeName(); - // ROOT v6.34, with spec versions before 1.0.0.1, did not properly renormalize the type name. - R__ASSERT(fVersionEpoch == 1); - if (fVersionMajor == 0 && fVersionMinor == 0 && fVersionPatch < 1) { + if (FieldTypeNamesMayNeedFixup()) { typeName = ROOT::Internal::GetRenormalizedTypeName(typeName); } @@ -790,6 +794,21 @@ ROOT::RNTupleDescriptor ROOT::RNTupleDescriptor::CloneSchema() const if (fHeaderExtension) clone.fHeaderExtension = std::make_unique(*fHeaderExtension); + // In case we are copying the schema from a pre-1.0.0.1 RNTuple we need to patch all field type names + // to use the proper normalization. + if (FieldTypeNamesMayNeedFixup()) { + std::vector toVisit; + toVisit.push_back(GetFieldZeroId()); + while (!toVisit.empty()) { + auto fieldId = toVisit.back(); + toVisit.pop_back(); + for (auto &field : clone.GetFieldIterable(fieldId)) { + Internal::FixupFieldTypeName(const_cast(field)); + toVisit.push_back(field.GetId()); + } + } + } + return clone; } @@ -1568,3 +1587,8 @@ bool ROOT::Internal::IsStdAtomicFieldDesc(const RFieldDescriptor &fieldDesc) return false; return (fieldDesc.GetTypeName().rfind("std::atomic<", 0) == 0); } + +void ROOT::Internal::FixupFieldTypeName(ROOT::RFieldDescriptor &fieldDesc) +{ + fieldDesc.fTypeName = ROOT::Internal::GetRenormalizedTypeName(fieldDesc.fTypeName); +} diff --git a/tree/ntuple/src/RNTupleMerger.cxx b/tree/ntuple/src/RNTupleMerger.cxx index 22eda7acfd09d..1d7b57d2a29b6 100644 --- a/tree/ntuple/src/RNTupleMerger.cxx +++ b/tree/ntuple/src/RNTupleMerger.cxx @@ -518,7 +518,8 @@ CompareDescriptorStructure(const ROOT::RNTupleDescriptor &dst, const ROOT::RNTup // Require that fields types match // TODO(gparolini): allow non-identical but compatible types - const auto &srcTyName = field.fSrc->GetTypeName(); + const auto &srcTyName = ROOT::Internal::GetRenormalizedTypeName(field.fSrc->GetTypeName()); + // This is already renormalized by construction (see RNTupleDescriptorBuilder::SetSchemaFromExisting) const auto &dstTyName = field.fDst->GetTypeName(); if (srcTyName != dstTyName) { std::stringstream ss; @@ -1135,7 +1136,7 @@ static void AddColumnsFromField(std::vector &columns, const RO } // Since we disallow merging fields of different types, src and dstFieldDesc must have the same type name. - assert(srcFieldDesc.GetTypeName() == dstFieldDesc.GetTypeName()); + assert(srcDesc.GetTypeNameForComparison(srcFieldDesc) == dstFieldDesc.GetTypeName()); info.fInMemoryType = ColumnInMemoryType(srcFieldDesc.GetTypeName(), info.fColumnType); columns.emplace_back(info); } diff --git a/tree/ntuple/src/RPageStorage.cxx b/tree/ntuple/src/RPageStorage.cxx index f5be9ec70af18..29c168a3538e3 100644 --- a/tree/ntuple/src/RPageStorage.cxx +++ b/tree/ntuple/src/RPageStorage.cxx @@ -982,6 +982,8 @@ ROOT::Internal::RPagePersistentSink::InitFromDescriptor(const ROOT::RNTupleDescr { // Create new descriptor fDescriptorBuilder.SetSchemaFromExisting(srcDescriptor); + // This is needed to be able to use GetTypeNameForComparison() + fDescriptorBuilder.SetVersionForWriting(); const auto &descriptor = fDescriptorBuilder.GetDescriptor(); // Create column/page ranges diff --git a/tree/ntuple/test/ntuple_serialize.cxx b/tree/ntuple/test/ntuple_serialize.cxx index 4867a7353b9b8..903524680b715 100644 --- a/tree/ntuple/test/ntuple_serialize.cxx +++ b/tree/ntuple/test/ntuple_serialize.cxx @@ -1650,6 +1650,10 @@ TEST(RNTuple, DeserializeDescriptorModes) { // Deserialize page list in various modes RNTupleDescriptorBuilder builder; + // Normally SetVersion() would be called in LoadStructure(). Since it's required for cloning the descriptor + // but we don't really care otherwise, we just set it as the current version (which is what the writer's + // descriptor builder does in this test). + builder.SetVersionForWriting(); RNTupleSerializer::DeserializeHeader(bufHeader.get(), sizeHeader, builder); RNTupleSerializer::DeserializeFooter(bufFooter.get(), sizeFooter, builder);