Skip to content

[ntuple] fixes to subfield name handling #20629

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jblomer wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[ntuple] fixes to subfield name handling #20629

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4a691a6
[ntuple] minor improvement
jblomer Dec 3, 2025
8058253
[ntuple] enforce correct sub field names in compound types
jblomer Dec 3, 2025
e8d96e7
[ntuple] prevent field name collision in model
jblomer Dec 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions tree/ntuple/inc/ROOT/RField/RFieldRecord.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions tree/ntuple/inc/ROOT/RFieldBase.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 9 additions & 8 deletions tree/ntuple/src/RField.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,14 +565,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
Expand Down Expand Up @@ -842,7 +843,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
Expand Down Expand Up @@ -1183,7 +1184,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
Expand Down
6 changes: 5 additions & 1 deletion tree/ntuple/src/RFieldBase.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
14 changes: 7 additions & 7 deletions tree/ntuple/src/RFieldMeta.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -933,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
Expand Down Expand Up @@ -963,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
Expand Down Expand Up @@ -1283,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)
Expand Down Expand Up @@ -1401,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.
Expand Down
10 changes: 5 additions & 5 deletions tree/ntuple/src/RFieldSequenceContainer.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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]);
Expand Down Expand Up @@ -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]);
}
Expand Down
2 changes: 1 addition & 1 deletion tree/ntuple/src/RNTupleProcessor.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions tree/ntuple/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
53 changes: 53 additions & 0 deletions tree/ntuple/test/ntuple_field_name.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
#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());
}
}
2 changes: 1 addition & 1 deletion tree/ntuple/test/ntuple_print.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"};
Copy link
Member

@pcanal pcanal Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a degradation. The user used on line 121 the name myfloat which is now no longer represented in the schema ... is that the intent?

Copy link
Member

@hahnjo hahnjo Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because the binary format specification says that the item field has name _0

Copy link
Member

@pcanal pcanal Dec 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Humm .... when reading back how will the user properly retrieve myfloat vs myotherfloat if they are both turned into _0 and the name are completely forgotten?
i.e. Is the name really gone (likely a blocker of sort) or is it just an artefact of the printing?

EXPECT_EQ(expected, os.str());
}

Expand Down