diff --git a/tree/ntuple/inc/ROOT/RField.hxx b/tree/ntuple/inc/ROOT/RField.hxx index b8783121174f6..20a7c6b94a7c9 100644 --- a/tree/ntuple/inc/ROOT/RField.hxx +++ b/tree/ntuple/inc/ROOT/RField.hxx @@ -69,6 +69,8 @@ class RFieldZero final : public RFieldBase { /// This flag is reset on Clone(). bool fAllowFieldSubstitutions = false; + std::unordered_set fSubFieldNames; ///< Efficient detection of duplicate field names + protected: std::unique_ptr CloneImpl(std::string_view newName) const final; void ConstructValue(void *) const final {} @@ -76,7 +78,9 @@ protected: 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 child); size_t GetValueSize() const final { return 0; } size_t GetAlignment() const final { return 0; } diff --git a/tree/ntuple/inc/ROOT/RField/RFieldRecord.hxx b/tree/ntuple/inc/ROOT/RField/RFieldRecord.hxx index 921d724c2cfc9..7b1b21f798742 100644 --- a/tree/ntuple/inc/ROOT/RField/RFieldRecord.hxx +++ b/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> itemFields); + void AttachItemFields(std::vector> itemFields, bool useNumberedFields); template - void AttachItemFields(std::array, N> itemFields) + void AttachItemFields(std::array, 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 diff --git a/tree/ntuple/inc/ROOT/RFieldBase.hxx b/tree/ntuple/inc/ROOT/RFieldBase.hxx index cf9c5f1a09332..aec54f18532ab 100644 --- a/tree/ntuple/inc/ROOT/RFieldBase.hxx +++ b/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 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 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 diff --git a/tree/ntuple/src/RField.cxx b/tree/ntuple/src/RField.cxx index c565c9c8778c5..e05e91e4e2377 100644 --- a/tree/ntuple/src/RField.cxx +++ b/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 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::RFieldZero::CloneImpl(std::string_view /*newName*/) const { auto result = std::make_unique(); - 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> itemFields) +void ROOT::RRecordField::AttachItemFields(std::vector> 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 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_ptrGetTraits() & kTraitTriviallyDestructible) fTraits |= kTraitTriviallyDestructible; - Attach(std::move(itemField)); + Attach(std::move(itemField), "_0"); } std::unique_ptr ROOT::RAtomicField::CloneImpl(std::string_view newName) const diff --git a/tree/ntuple/src/RFieldBase.cxx b/tree/ntuple/src/RFieldBase.cxx index 1cc69fa63ba4c..e78d83cef3233 100644 --- a/tree/ntuple/src/RFieldBase.cxx +++ b/tree/ntuple/src/RFieldBase.cxx @@ -648,12 +648,16 @@ std::vector ROOT::RFieldBase::SplitValue(const RValue return std::vector(); } -void ROOT::RFieldBase::Attach(std::unique_ptr child) +void ROOT::RFieldBase::Attach(std::unique_ptr 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)); } diff --git a/tree/ntuple/src/RFieldMeta.cxx b/tree/ntuple/src/RFieldMeta.cxx index 5fdfd99aad56a..a26b1a6900701 100644 --- a/tree/ntuple/src/RFieldMeta.cxx +++ b/tree/ntuple/src/RFieldMeta.cxx @@ -676,7 +676,7 @@ ROOT::RPairField::RPairField(std::string_view fieldName, std::array &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]); } @@ -684,7 +684,7 @@ ROOT::RPairField::RPairField(std::string_view fieldName, std::array, 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::RProxiedCollectionField::CloneImpl(std::string_view newName) const { - auto newItemField = fSubfields[0]->Clone(fSubfields[0]->GetFieldName()); auto clone = std::unique_ptr(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::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::RSetField::CloneImpl(std::string_view newName) const @@ -1284,14 +1283,14 @@ ROOT::RTupleField::RTupleField(std::string_view fieldName, std::vector &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> 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::vectorGetValueSize()); 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. diff --git a/tree/ntuple/src/RFieldSequenceContainer.cxx b/tree/ntuple/src/RFieldSequenceContainer.cxx index d780ec31ade08..612e616a301e1 100644 --- a/tree/ntuple/src/RFieldSequenceContainer.cxx +++ b/tree/ntuple/src/RFieldSequenceContainer.cxx @@ -124,7 +124,7 @@ ROOT::RArrayField::RArrayField(std::string_view fieldName, std::unique_ptrGetTraits() & ~kTraitMappable; - Attach(std::move(itemField)); + Attach(std::move(itemField), "_0"); } std::unique_ptr ROOT::RArrayField::CloneImpl(std::string_view newName) const @@ -245,7 +245,7 @@ ROOT::RRVecField::RRVecField(std::string_view fieldName, std::unique_ptrGetTraits() & 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_ptrGetTraits() & kTraitTriviallyDestructible)) fItemDeleter = GetDeleterOf(*itemField); - Attach(std::move(itemField)); + Attach(std::move(itemField), "_0"); } ROOT::RVectorField::RVectorField(std::string_view fieldName, std::unique_ptr 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]); } diff --git a/tree/ntuple/src/RNTupleProcessor.cxx b/tree/ntuple/src/RNTupleProcessor.cxx index 5ab94a2465db0..7db30a98da256 100644 --- a/tree/ntuple/src/RNTupleProcessor.cxx +++ b/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 diff --git a/tree/ntuple/test/CMakeLists.txt b/tree/ntuple/test/CMakeLists.txt index 1df3d41dc0ef5..8aa0c4192a053 100644 --- a/tree/ntuple/test/CMakeLists.txt +++ b/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) diff --git a/tree/ntuple/test/ntuple_field_name.cxx b/tree/ntuple/test/ntuple_field_name.cxx new file mode 100644 index 0000000000000..2ab618d478bfa --- /dev/null +++ b/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>("x")).begin()->GetFieldName()); + EXPECT_EQ("_0", ROOT::RVectorField("f", std::make_unique>("x")).begin()->GetFieldName()); + EXPECT_EQ("_0", ROOT::RRVecField("f", std::make_unique>("x")).begin()->GetFieldName()); + EXPECT_EQ("_0", ROOT::RArrayField("f", std::make_unique>("x"), 2).begin()->GetFieldName()); + EXPECT_EQ("_0", ROOT::ROptionalField("f", std::make_unique>("x")).begin()->GetFieldName()); + EXPECT_EQ("_0", ROOT::RAtomicField("f", std::make_unique>("x")).begin()->GetFieldName()); + EXPECT_EQ("_0", ROOT::RSetField("f", ROOT::RSetField::ESetType::kSet, std::make_unique>("x")) + .begin() + ->GetFieldName()); + + { + std::unique_ptr p{ + new ROOT::RPairField("x", {std::make_unique>("x"), std::make_unique>("x")})}; + EXPECT_EQ("_0", ROOT::RMapField("f", ROOT::RMapField::EMapType::kMap, std::move(p)).begin()->GetFieldName()); + } + + { + std::vector> items; + items.emplace_back(std::make_unique>("x")); + items.emplace_back(std::make_unique>("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, 2> items; + items[0] = std::make_unique>("x"); + items[1] = std::make_unique>("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> items; + items.emplace_back(std::make_unique>("x")); + items.emplace_back(std::make_unique>("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>("x")); + EXPECT_THROW(zero.Attach(std::make_unique>("x")), ROOT::RException); + + std::vector> items; + items.emplace_back(std::make_unique>("x")); + items.emplace_back(std::make_unique>("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")); + } +} diff --git a/tree/ntuple/test/ntuple_print.cxx b/tree/ntuple/test/ntuple_print.cxx index 1ac73392ff0d5..71f0bbaec187b 100644 --- a/tree/ntuple/test/ntuple_print.cxx +++ b/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) $\n" + - "$ Field 1.1 : myfloat (float) $\n"}; + "$ Field 1.1 : _0 (float) $\n"}; EXPECT_EQ(expected, os.str()); }