[ntuple] Renormalize field type names if needed in SetSchemaFromExisting

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)

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;
+}

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<ROOT::Internal::RNTupleClusterBoundaries> 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.

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<RHeaderExtension>(*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<ROOT::DescriptorId_t> 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<ROOT::RFieldDescriptor &>(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);
+}

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<RColumnMergeInfo> &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);
    }
 

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();
 

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);