[ntuple] fixes to subfield name handling

tree/ntuple/inc/ROOT/RField.hxx

@@ -69,14 +69,18 @@ class RFieldZero final : public RFieldBase {
    /// This flag is reset on Clone().
    bool fAllowFieldSubstitutions = false;
 
+   std::unordered_set<std::string> fSubFieldNames; ///< Efficient detection of duplicate field names
+
 protected:
    std::unique_ptr<RFieldBase> CloneImpl(std::string_view newName) const final;
    void ConstructValue(void *) const final {}
 
 public:
    RFieldZero() : RFieldBase("", "", ROOT::ENTupleStructure::kRecord, false /* isSimple */) {}
 
-   using RFieldBase::Attach;
+   /// A public version of the Attach method that allows piece-wise construction of the zero field.
+   /// Will throw on duplicate subfield names.
+   void Attach(std::unique_ptr<RFieldBase> child);
    size_t GetValueSize() const final { return 0; }
    size_t GetAlignment() const final { return 0; }
 

tree/ntuple/inc/ROOT/RField/RFieldRecord.hxx

@@ -87,17 +87,17 @@ protected:
    /// that ensure that the resulting memory layout matches std::pair or std::tuple, resp.
    RRecordField(std::string_view fieldName, std::string_view typeName);
 
-   void AttachItemFields(std::vector<std::unique_ptr<RFieldBase>> itemFields);
+   void AttachItemFields(std::vector<std::unique_ptr<RFieldBase>> itemFields, bool useNumberedFields);
 
    template <std::size_t N>
-   void AttachItemFields(std::array<std::unique_ptr<RFieldBase>, N> itemFields)
+   void AttachItemFields(std::array<std::unique_ptr<RFieldBase>, N> itemFields, bool useNumberedFields)
    {
       fTraits |= kTraitTrivialType;
       for (unsigned i = 0; i < N; ++i) {
          fMaxAlignment = std::max(fMaxAlignment, itemFields[i]->GetAlignment());
          fSize += GetItemPadding(fSize, itemFields[i]->GetAlignment()) + itemFields[i]->GetValueSize();
          fTraits &= itemFields[i]->GetTraits();
-         Attach(std::move(itemFields[i]));
+         Attach(std::move(itemFields[i]), useNumberedFields ? ("_" + std::to_string(i)) : "");
       }
       // Trailing padding: although this is implementation-dependent, most add enough padding to comply with the
       // requirements of the type with strictest alignment

tree/ntuple/inc/ROOT/RFieldBase.hxx

@@ -508,8 +508,9 @@ protected:
    // on the data that's written, e.g. for polymorphic types in the streamer field.
    virtual ROOT::RExtraTypeInfoDescriptor GetExtraTypeInfo() const { return ROOT::RExtraTypeInfoDescriptor(); }
 
-   /// Add a new subfield to the list of nested fields
-   void Attach(std::unique_ptr<RFieldBase> child);
+   /// Add a new subfield to the list of nested fields. If childName is non-empty and the passed field has a different
+   /// name, the field will be cloned to enforce the given name.
+   void Attach(std::unique_ptr<RFieldBase> child, std::string_view childName = "");
 
    /// Called by ConnectPageSource() before connecting; derived classes may override this as appropriate, e.g.
    /// for the application of I/O rules. In the process, the field at hand or its subfields may be marked as

tree/ntuple/src/RField.cxx

@@ -38,11 +38,22 @@ void ROOT::Internal::SetAllowFieldSubstitutions(RFieldZero &fieldZero, bool val)
    fieldZero.fAllowFieldSubstitutions = val;
 }
 
+void ROOT::RFieldZero::Attach(std::unique_ptr<RFieldBase> child)
+{
+   const std::string childName = child->GetFieldName();
+   if (fSubFieldNames.count(childName) > 0)
+      throw RException(R__FAIL("duplicate field name: " + childName));
+   RFieldBase::Attach(std::move(child), "");
+   fSubFieldNames.insert(childName);
+}
+
 std::unique_ptr<ROOT::RFieldBase> ROOT::RFieldZero::CloneImpl(std::string_view /*newName*/) const
 {
    auto result = std::make_unique<RFieldZero>();
-   for (auto &f : fSubfields)
+   for (auto &f : fSubfields) {
       result->Attach(f->Clone(f->GetFieldName()));
+      result->fSubFieldNames.insert(f->GetFieldName());
+   }
    return result;
 }
 
@@ -565,14 +576,15 @@ ROOT::RRecordField::RRecordField(std::string_view fieldName, std::string_view ty
 {
 }
 
-void ROOT::RRecordField::AttachItemFields(std::vector<std::unique_ptr<RFieldBase>> itemFields)
+void ROOT::RRecordField::AttachItemFields(std::vector<std::unique_ptr<RFieldBase>> itemFields, bool useNumberedFields)
 {
    fTraits |= kTraitTrivialType;
-   for (auto &item : itemFields) {
-      fMaxAlignment = std::max(fMaxAlignment, item->GetAlignment());
-      fSize += GetItemPadding(fSize, item->GetAlignment()) + item->GetValueSize();
-      fTraits &= item->GetTraits();
-      Attach(std::move(item));
+   const auto N = itemFields.size();
+   for (std::size_t i = 0; i < N; ++i) {
+      fMaxAlignment = std::max(fMaxAlignment, itemFields[i]->GetAlignment());
+      fSize += GetItemPadding(fSize, itemFields[i]->GetAlignment()) + itemFields[i]->GetValueSize();
+      fTraits &= itemFields[i]->GetTraits();
+      Attach(std::move(itemFields[i]), useNumberedFields ? ("_" + std::to_string(i)) : "");
    }
    // Trailing padding: although this is implementation-dependent, most add enough padding to comply with the
    // requirements of the type with strictest alignment
@@ -602,13 +614,19 @@ ROOT::RRecordField::RRecordField(std::string_view fieldName, std::vector<std::un
 {
    fTraits |= kTraitTrivialType;
    fOffsets.reserve(itemFields.size());
+   std::unordered_set<std::string> fieldNames;
    for (auto &item : itemFields) {
+      const auto itemName = item->GetFieldName();
+      if (fieldNames.count(itemName) > 0) {
+         throw RException(R__FAIL("duplicate field name: " + itemName));
+      }
       fSize += GetItemPadding(fSize, item->GetAlignment());
       fOffsets.push_back(fSize);
       fMaxAlignment = std::max(fMaxAlignment, item->GetAlignment());
       fSize += item->GetValueSize();
       fTraits &= item->GetTraits();
       Attach(std::move(item));
+      fieldNames.insert(itemName);
    }
    fTraits |= !emulatedFromType.empty() * kTraitEmulatedField;
    // Trailing padding: although this is implementation-dependent, most add enough padding to comply with the
@@ -842,7 +860,7 @@ ROOT::RNullableField::RNullableField(std::string_view fieldName, const std::stri
    : ROOT::RFieldBase(fieldName, typePrefix + "<" + itemField->GetTypeName() + ">", ROOT::ENTupleStructure::kCollection,
                       false /* isSimple */)
 {
-   Attach(std::move(itemField));
+   Attach(std::move(itemField), "_0");
 }
 
 const ROOT::RFieldBase::RColumnRepresentations &ROOT::RNullableField::GetColumnRepresentations() const
@@ -1183,7 +1201,7 @@ ROOT::RAtomicField::RAtomicField(std::string_view fieldName, std::unique_ptr<RFi
       fTraits |= kTraitTriviallyConstructible;
    if (itemField->GetTraits() & kTraitTriviallyDestructible)
       fTraits |= kTraitTriviallyDestructible;
-   Attach(std::move(itemField));
+   Attach(std::move(itemField), "_0");
 }
 
 std::unique_ptr<ROOT::RFieldBase> ROOT::RAtomicField::CloneImpl(std::string_view newName) const

tree/ntuple/src/RFieldBase.cxx

@@ -648,12 +648,16 @@ std::vector<ROOT::RFieldBase::RValue> ROOT::RFieldBase::SplitValue(const RValue
    return std::vector<RValue>();
 }
 
-void ROOT::RFieldBase::Attach(std::unique_ptr<ROOT::RFieldBase> child)
+void ROOT::RFieldBase::Attach(std::unique_ptr<ROOT::RFieldBase> child, std::string_view childName)
 {
    // Note that during a model update, new fields will be attached to the zero field. The zero field, however,
    // does not change its inital state because only its sub fields get connected by RPageSink::UpdateSchema.
    if (fState != EState::kUnconnected)
       throw RException(R__FAIL("invalid attempt to attach subfield to already connected field"));
+
+   if (!childName.empty() && child->GetFieldName() != childName)
+      child = child->Clone(childName);
+
    child->fParent = this;
    fSubfields.emplace_back(std::move(child));
 }

tree/ntuple/src/RFieldMeta.cxx

@@ -676,15 +676,15 @@ ROOT::RPairField::RPairField(std::string_view fieldName, std::array<std::unique_
                              const std::array<std::size_t, 2> &offsets)
    : ROOT::RRecordField(fieldName, "std::pair<" + GetTypeList(itemFields) + ">")
 {
-   AttachItemFields(std::move(itemFields));
+   AttachItemFields(std::move(itemFields), true /* useNumberedFields */);
    fOffsets.push_back(offsets[0]);
    fOffsets.push_back(offsets[1]);
 }
 
 ROOT::RPairField::RPairField(std::string_view fieldName, std::array<std::unique_ptr<RFieldBase>, 2> itemFields)
    : ROOT::RRecordField(fieldName, "std::pair<" + GetTypeList(itemFields) + ">")
 {
-   AttachItemFields(std::move(itemFields));
+   AttachItemFields(std::move(itemFields), true /* useNumberedFields */);
 
    // ISO C++ does not guarantee any specific layout for `std::pair`; query TClass for the member offsets
    auto *c = TClass::GetClass(GetTypeName().c_str());
@@ -815,11 +815,10 @@ ROOT::RProxiedCollectionField::RProxiedCollectionField(std::string_view fieldNam
 
 std::unique_ptr<ROOT::RFieldBase> ROOT::RProxiedCollectionField::CloneImpl(std::string_view newName) const
 {
-   auto newItemField = fSubfields[0]->Clone(fSubfields[0]->GetFieldName());
    auto clone =
       std::unique_ptr<RProxiedCollectionField>(new RProxiedCollectionField(newName, fProxy->GetCollectionClass()));
    clone->fItemSize = fItemSize;
-   clone->Attach(std::move(newItemField));
+   clone->Attach(fSubfields[0]->Clone(fSubfields[0]->GetFieldName()));
    return clone;
 }
 
@@ -934,7 +933,7 @@ ROOT::RMapField::RMapField(std::string_view fieldName, EMapType mapType, std::un
    auto *itemClass = fProxy->GetValueClass();
    fItemSize = itemClass->GetClassSize();
 
-   Attach(std::move(itemField));
+   Attach(std::move(itemField), "_0");
 }
 
 std::unique_ptr<ROOT::RFieldBase> ROOT::RMapField::CloneImpl(std::string_view newName) const
@@ -964,7 +963,7 @@ ROOT::RSetField::RSetField(std::string_view fieldName, ESetType setType, std::un
      fSetType(setType)
 {
    fItemSize = itemField->GetValueSize();
-   Attach(std::move(itemField));
+   Attach(std::move(itemField), "_0");
 }
 
 std::unique_ptr<ROOT::RFieldBase> ROOT::RSetField::CloneImpl(std::string_view newName) const
@@ -1284,14 +1283,14 @@ ROOT::RTupleField::RTupleField(std::string_view fieldName, std::vector<std::uniq
                                const std::vector<std::size_t> &offsets)
    : ROOT::RRecordField(fieldName, "std::tuple<" + GetTypeList(itemFields) + ">")
 {
-   AttachItemFields(std::move(itemFields));
+   AttachItemFields(std::move(itemFields), true /* useNumberedFields */);
    fOffsets = offsets;
 }
 
 ROOT::RTupleField::RTupleField(std::string_view fieldName, std::vector<std::unique_ptr<RFieldBase>> itemFields)
    : ROOT::RRecordField(fieldName, "std::tuple<" + GetTypeList(itemFields) + ">")
 {
-   AttachItemFields(std::move(itemFields));
+   AttachItemFields(std::move(itemFields), true /* useNumberedFields */);
 
    auto *c = TClass::GetClass(GetTypeName().c_str());
    if (!c)
@@ -1402,7 +1401,7 @@ ROOT::RVariantField::RVariantField(std::string_view fieldName, std::vector<std::
       fMaxItemSize = std::max(fMaxItemSize, itemFields[i]->GetValueSize());
       fMaxAlignment = std::max(fMaxAlignment, itemFields[i]->GetAlignment());
       fTraits &= itemFields[i]->GetTraits();
-      Attach(std::move(itemFields[i]));
+      Attach(std::move(itemFields[i]), "_" + std::to_string(i));
    }
 
    // With certain template parameters, the union of members of an std::variant starts at an offset > 0.

tree/ntuple/src/RFieldSequenceContainer.cxx

@@ -124,7 +124,7 @@ ROOT::RArrayField::RArrayField(std::string_view fieldName, std::unique_ptr<RFiel
      fArrayLength(arrayLength)
 {
    fTraits |= itemField->GetTraits() & ~kTraitMappable;
-   Attach(std::move(itemField));
+   Attach(std::move(itemField), "_0");
 }
 
 std::unique_ptr<ROOT::RFieldBase> ROOT::RArrayField::CloneImpl(std::string_view newName) const
@@ -245,7 +245,7 @@ ROOT::RRVecField::RRVecField(std::string_view fieldName, std::unique_ptr<RFieldB
 {
    if (!(itemField->GetTraits() & kTraitTriviallyDestructible))
       fItemDeleter = GetDeleterOf(*itemField);
-   Attach(std::move(itemField));
+   Attach(std::move(itemField), "_0");
    fValueSize = EvalRVecValueSize(fSubfields[0]->GetAlignment(), fSubfields[0]->GetValueSize(), GetAlignment());
 
    // Determine if we can optimimize bulk reading
@@ -551,7 +551,7 @@ ROOT::RVectorField::RVectorField(std::string_view fieldName, std::unique_ptr<RFi
 
    if (!(itemField->GetTraits() & kTraitTriviallyDestructible))
       fItemDeleter = GetDeleterOf(*itemField);
-   Attach(std::move(itemField));
+   Attach(std::move(itemField), "_0");
 }
 
 ROOT::RVectorField::RVectorField(std::string_view fieldName, std::unique_ptr<RFieldBase> itemField)
@@ -859,7 +859,7 @@ ROOT::RArrayAsRVecField::RArrayAsRVecField(std::string_view fieldName, std::uniq
      fItemSize(itemField->GetValueSize()),
      fArrayLength(arrayLength)
 {
-   Attach(std::move(itemField));
+   Attach(std::move(itemField), "_0");
    fValueSize = EvalRVecValueSize(fSubfields[0]->GetAlignment(), fSubfields[0]->GetValueSize(), GetAlignment());
    if (!(fSubfields[0]->GetTraits() & kTraitTriviallyDestructible))
       fItemDeleter = GetDeleterOf(*fSubfields[0]);
@@ -961,7 +961,7 @@ ROOT::RArrayAsVectorField::RArrayAsVectorField(std::string_view fieldName, std::
      fItemSize(itemField->GetValueSize()),
      fArrayLength(arrayLength)
 {
-   Attach(std::move(itemField));
+   Attach(std::move(itemField), "_0");
    if (!(fSubfields[0]->GetTraits() & kTraitTriviallyDestructible))
       fItemDeleter = GetDeleterOf(*fSubfields[0]);
 }

tree/ntuple/src/RNTupleProcessor.cxx

@@ -401,7 +401,7 @@ class RAuxiliaryProcessorField final : public ROOT::RRecordField {
       for (auto &item : itemFields) {
          fOffsets.push_back(GetItemPadding(fSize, item->GetAlignment()));
       }
-      AttachItemFields(std::move(itemFields));
+      AttachItemFields(std::move(itemFields), false /* useNumberedFields */);
    }
 };
 } // namespace ROOT::Experimental::Internal

tree/ntuple/test/CMakeLists.txt

@@ -43,6 +43,7 @@ if(NOT MSVC)
   ROOT_ADD_GTEST(ntuple_evolution_shape ntuple_evolution_shape.cxx LIBRARIES ROOTNTuple)
   ROOT_ADD_GTEST(ntuple_emulated ntuple_emulated.cxx LIBRARIES ROOTNTuple)
 endif()
+ROOT_ADD_GTEST(ntuple_field_name ntuple_field_name.cxx LIBRARIES ROOTNTuple)
 ROOT_ADD_GTEST(ntuple_join_table ntuple_join_table.cxx LIBRARIES ROOTNTuple)
 ROOT_ADD_GTEST(ntuple_merger ntuple_merger.cxx LIBRARIES ROOTNTuple CustomStruct ZLIB::ZLIB Tree INCLUDE_DIRS ${CMAKE_SOURCE_DIR}/tree/tree/inc)
 ROOT_ADD_GTEST(ntuple_metrics ntuple_metrics.cxx LIBRARIES ROOTNTuple)

tree/ntuple/test/ntuple_field_name.cxx

@@ -0,0 +1,70 @@
+#include "ntuple_test.hxx"
+
+TEST(RNTuple, SubFieldName)
+{
+   EXPECT_EQ("_0", ROOT::RAtomicField("f", std::make_unique<RField<char>>("x")).begin()->GetFieldName());
+   EXPECT_EQ("_0", ROOT::RVectorField("f", std::make_unique<RField<char>>("x")).begin()->GetFieldName());
+   EXPECT_EQ("_0", ROOT::RRVecField("f", std::make_unique<RField<char>>("x")).begin()->GetFieldName());
+   EXPECT_EQ("_0", ROOT::RArrayField("f", std::make_unique<RField<char>>("x"), 2).begin()->GetFieldName());
+   EXPECT_EQ("_0", ROOT::ROptionalField("f", std::make_unique<RField<char>>("x")).begin()->GetFieldName());
+   EXPECT_EQ("_0", ROOT::RAtomicField("f", std::make_unique<RField<char>>("x")).begin()->GetFieldName());
+   EXPECT_EQ("_0", ROOT::RSetField("f", ROOT::RSetField::ESetType::kSet, std::make_unique<RField<char>>("x"))
+                      .begin()
+                      ->GetFieldName());
+
+   {
+      std::unique_ptr<ROOT::RPairField> p{
+         new ROOT::RPairField("x", {std::make_unique<RField<char>>("x"), std::make_unique<RField<char>>("x")})};
+      EXPECT_EQ("_0", ROOT::RMapField("f", ROOT::RMapField::EMapType::kMap, std::move(p)).begin()->GetFieldName());
+   }
+
+   {
+      std::vector<std::unique_ptr<ROOT::RFieldBase>> items;
+      items.emplace_back(std::make_unique<RField<char>>("x"));
+      items.emplace_back(std::make_unique<RField<int>>("x"));
+      ROOT::RVariantField f("f", std::move(items));
+      auto itr = f.begin();
+      EXPECT_EQ("_0", itr->GetFieldName());
+      itr++;
+      EXPECT_EQ("_1", itr->GetFieldName());
+   }
+
+   {
+      std::array<std::unique_ptr<ROOT::RFieldBase>, 2> items;
+      items[0] = std::make_unique<RField<char>>("x");
+      items[1] = std::make_unique<RField<char>>("x");
+      ROOT::RPairField f("f", std::move(items));
+      auto itr = f.begin();
+      EXPECT_EQ("_0", itr->GetFieldName());
+      itr++;
+      EXPECT_EQ("_1", itr->GetFieldName());
+   }
+
+   {
+      std::vector<std::unique_ptr<ROOT::RFieldBase>> items;
+      items.emplace_back(std::make_unique<RField<char>>("x"));
+      items.emplace_back(std::make_unique<RField<char>>("x"));
+      ROOT::RTupleField f("f", std::move(items));
+      auto itr = f.begin();
+      EXPECT_EQ("_0", itr->GetFieldName());
+      itr++;
+      EXPECT_EQ("_1", itr->GetFieldName());
+   }
+}
+
+TEST(RNTuple, FieldNameCollision)
+{
+   ROOT::RFieldZero zero;
+   zero.Attach(std::make_unique<RField<char>>("x"));
+   EXPECT_THROW(zero.Attach(std::make_unique<RField<char>>("x")), ROOT::RException);
+
+   std::vector<std::unique_ptr<ROOT::RFieldBase>> items;
+   items.emplace_back(std::make_unique<RField<char>>("x"));
+   items.emplace_back(std::make_unique<RField<char>>("x"));
+   try {
+      ROOT::RRecordField("f", std::move(items));
+      FAIL() << "duplicate field names should throw";
+   } catch (const ROOT::RException &err) {
+      EXPECT_THAT(err.what(), testing::HasSubstr("duplicate field name"));
+   }
+}

tree/ntuple/test/ntuple_print.cxx

@@ -126,7 +126,7 @@ TEST(RNtuplePrint, ArrayAsRVec)
    testField.AcceptVisitor(visitor);
    std::string expected{std::string("") +
                         "$ Field 1       : arrayasrvecfield (ROOT::VecOps::RVec<float>)                 $\n" +
-                        "$   Field 1.1   : myfloat (float)                                              $\n"};
+                        "$   Field 1.1   : _0 (float)                                                   $\n"};
    EXPECT_EQ(expected, os.str());
 }