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 9434960df6a19..03fcdf54acbeb 100644 --- a/tree/ntuple/inc/ROOT/RNTupleDescriptor.hxx +++ b/tree/ntuple/inc/ROOT/RNTupleDescriptor.hxx @@ -48,6 +48,7 @@ class RColumnElementBase; } class RNTupleDescriptor; +class RFieldDescriptor; namespace Internal { class RColumnDescriptorBuilder; @@ -64,6 +65,8 @@ struct RNTupleClusterBoundaries { }; std::vector GetClusterBoundaries(const RNTupleDescriptor &desc); + +void FixupFieldTypeName(ROOT::RFieldDescriptor &fieldDesc); } // namespace Internal namespace Experimental { @@ -120,6 +123,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; @@ -764,6 +768,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 4cee2de687124..8dbe4f0b085cc 100644 --- a/tree/ntuple/src/RNTupleDescriptor.cxx +++ b/tree/ntuple/src/RNTupleDescriptor.cxx @@ -388,13 +388,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); } @@ -758,6 +762,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; } @@ -1536,3 +1555,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 3b54445c39287..15876da705e28 100644 --- a/tree/ntuple/src/RNTupleMerger.cxx +++ b/tree/ntuple/src/RNTupleMerger.cxx @@ -604,7 +604,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; @@ -1250,8 +1251,8 @@ 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()); - info.fInMemoryType = ColumnInMemoryType(srcFieldDesc.GetTypeName(), columnType); + assert(srcDesc.GetTypeNameForComparison(srcFieldDesc) == dstFieldDesc.GetTypeName()); + info.fInMemoryType = ColumnInMemoryType(dstFieldDesc.GetTypeName(), columnType); columns.emplace_back(info); } diff --git a/tree/ntuple/src/RPageStorage.cxx b/tree/ntuple/src/RPageStorage.cxx index 0d7be23860605..aba06aec7ac61 100644 --- a/tree/ntuple/src/RPageStorage.cxx +++ b/tree/ntuple/src/RPageStorage.cxx @@ -1110,6 +1110,7 @@ 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(); diff --git a/tree/ntuple/test/ntuple_serialize.cxx b/tree/ntuple/test/ntuple_serialize.cxx index 71181c5a007fb..bf3e211f29a4b 100644 --- a/tree/ntuple/test/ntuple_serialize.cxx +++ b/tree/ntuple/test/ntuple_serialize.cxx @@ -1797,6 +1797,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);