Skip to content

[ntuple] fix schema evolution with streamer fields #18451

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
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

[ntuple] fix schema evolution with streamer fields #18451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a76ae80
[ntuple] fix de-s11n of streamer info records
jblomer Apr 21, 2025
14b96e9
[ntuple] add RPageSource::RegisterStreamerInfos()
jblomer Apr 21, 2025
4d4bf55
[ntuple] add RStreamerField::BeforeConnectPageSource()
jblomer Apr 21, 2025
6cf0ca0
[ntuple] test automatic schema evolution for streamer field
jblomer Apr 21, 2025
29ba77a
[ntuple] test I/O rules for streamer fields
jblomer Apr 21, 2025
04b27d7
[ntuple] test failure on streamer field type mismatch
jblomer Apr 21, 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
2 changes: 2 additions & 0 deletions tree/ntuple/inc/ROOT/RField.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,8 @@ protected:
// Returns the list of seen streamer infos
ROOT::RExtraTypeInfoDescriptor GetExtraTypeInfo() const final;

void BeforeConnectPageSource(ROOT::Internal::RPageSource &pageSource) final;

public:
RStreamerField(std::string_view fieldName, std::string_view className, std::string_view typeAlias = "");
RStreamerField(RStreamerField &&other) = default;
Expand Down
5 changes: 5 additions & 0 deletions tree/ntuple/inc/ROOT/RPageStorage.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,6 +612,7 @@ private:
REntryRange fEntryRange; ///< Used by the cluster pool to prevent reading beyond the given range
bool fHasStructure = false; ///< Set to true once `LoadStructure()` is called
bool fIsAttached = false; ///< Set to true once `Attach()` is called
bool fHasStreamerInfosRegistered = false; ///< Set to true when ReisterStreamerInfos() is called.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bool fHasStreamerInfosRegistered = false; ///< Set to true when ReisterStreamerInfos() is called.
bool fHasStreamerInfosRegistered = false; ///< Set to true when RegisterStreamerInfos() is called.


/// Remembers the last cluster id from which a page was requested
ROOT::DescriptorId_t fLastUsedCluster = ROOT::kInvalidDescriptorId;
Expand Down Expand Up @@ -818,6 +819,10 @@ public:
// TODO(gparolini): for symmetry with SealPage(), we should either make this private or SealPage() public.
RResult<ROOT::Internal::RPage>
UnsealPage(const RSealedPage &sealedPage, const ROOT::Internal::RColumnElementBase &element);

// Builds the streamer info records from the descriptor's extra type info section. This is necessary when
// connecting streamer fields so that emulated classes can be read.
Comment on lines +823 to +824
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Builds the streamer info records from the descriptor's extra type info section. This is necessary when
// connecting streamer fields so that emulated classes can be read.
/// Builds the streamer info records from the descriptor's extra type info section. This is necessary when
/// connecting streamer fields so that emulated classes can be read.

void RegisterStreamerInfos();
}; // class RPageSource

} // namespace Internal
Expand Down
5 changes: 5 additions & 0 deletions tree/ntuple/src/RFieldMeta.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,6 +834,11 @@ ROOT::RStreamerField::RStreamerField(std::string_view fieldName, TClass *classp)
fTraits |= kTraitTriviallyDestructible;
}

void ROOT::RStreamerField::BeforeConnectPageSource(ROOT::Internal::RPageSource &pageSource)
{
pageSource.RegisterStreamerInfos();
}

std::unique_ptr<ROOT::RFieldBase> ROOT::RStreamerField::CloneImpl(std::string_view newName) const
{
return std::unique_ptr<RStreamerField>(new RStreamerField(newName, GetTypeName(), GetTypeAlias()));
Expand Down
3 changes: 1 addition & 2 deletions tree/ntuple/src/RNTupleSerialize.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2146,13 +2146,12 @@ ROOT::Internal::RNTupleSerializer::DeserializeStreamerInfos(const std::string &e
TBufferFile buffer(TBuffer::kRead, extraTypeInfoContent.length(), const_cast<char *>(extraTypeInfoContent.data()),
false /* adopt */);
auto infoList = reinterpret_cast<TList *>(buffer.ReadObject(TList::Class()));
infoList->SetOwner(); // delete the TStreamerInfo items of the list

TObjLink *lnk = infoList->FirstLink();
while (lnk) {
auto info = reinterpret_cast<TStreamerInfo *>(lnk->GetObject());
info->BuildCheck();
infoMap[info->GetNumber()] = info->GetClass()->GetStreamerInfo();
infoMap[info->GetNumber()] = info->GetClass()->GetStreamerInfo(info->GetClassVersion());
assert(info->GetNumber() == infoMap[info->GetNumber()]->GetNumber());
lnk = lnk->Next();
}
Expand Down
16 changes: 16 additions & 0 deletions tree/ntuple/src/RPageStorage.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,22 @@ ROOT::RResult<ROOT::Internal::RPage> ROOT::Internal::RPageSource::UnsealPage(con
return page;
}

void ROOT::Internal::RPageSource::RegisterStreamerInfos()
{
if (fHasStreamerInfosRegistered)
return;

for (const auto &extraTypeInfo : fDescriptor.GetExtraTypeInfoIterable()) {
if (extraTypeInfo.GetContentId() != EExtraTypeInfoIds::kStreamerInfo)
continue;
// We don't need the result, it's enough that during deserialization, BuildCheck() is called for every
// streamer info record.
RNTupleSerializer::DeserializeStreamerInfos(extraTypeInfo.GetContent()).Unwrap();
}

fHasStreamerInfosRegistered = true;
}

//------------------------------------------------------------------------------

bool ROOT::Internal::RWritePageMemoryManager::RColumnInfo::operator>(const RColumnInfo &other) const
Expand Down
18 changes: 18 additions & 0 deletions tree/ntuple/test/StreamerField.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,22 @@ struct PolyContainer {
std::unique_ptr<PolyBase> fPoly;
};

template <typename T>
struct OldStreamerName {
T fValue;
};

template <typename T>
struct NewStreamerName {
T fValue;
};

struct TemperatureCelsius {
float fValue;
};

struct TemperatureKelvin {
float fValue;
};

#endif // ROOT_RNTuple_Test_StreamerField
7 changes: 7 additions & 0 deletions tree/ntuple/test/StreamerFieldLinkDef.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,11 @@
#pragma link C++ class PolyB + ;
#pragma link C++ options = rntupleStreamerMode(true) class PolyContainer + ;

#pragma link C++ options = rntupleStreamerMode(true), version(3) class OldStreamerName < int> + ;
#pragma link C++ options = rntupleStreamerMode(true), version(3) class NewStreamerName < int> + ;
#pragma read sourceClass = "OldStreamerName<int>" targetClass = "NewStreamerName<int>" version = "[3]"

#pragma link C++ options = rntupleStreamerMode(true) class TemperatureCelsius + ;
#pragma link C++ options = rntupleStreamerMode(true) class TemperatureKelvin + ;

#endif
58 changes: 58 additions & 0 deletions tree/ntuple/test/ntuple_evolution.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -863,3 +863,61 @@ struct RenamedIntermediateDerived : public RenamedIntermediate2 {
EXPECT_THAT(err.what(), testing::HasSubstr("incompatible type name for field"));
}
}

TEST(RNTupleEvolution, StreamerField)
{
FileRaii fileGuard("test_ntuple_evolution_streamer_field.root");
fileGuard.PreserveFile();
Copy link
Member

Choose a reason for hiding this comment

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

This should not be needed, probably a left-over from debugging


ExecInFork([&] {
// The child process writes the file and exits, but the file must be preserved to be read by the parent.
fileGuard.PreserveFile();

ASSERT_TRUE(gInterpreter->Declare(R"(
struct StreamerField {
int fInt = 1;
int fAnotherInt = 3;

ClassDefNV(StreamerField, 2)
};
)"));

auto model = RNTupleModel::Create();
model->AddField(std::make_unique<ROOT::RStreamerField>("f", "StreamerField"));

auto writer = RNTupleWriter::Recreate(std::move(model), "ntpl", fileGuard.GetPath());
writer->Fill();

void *ptr = writer->GetModel().GetDefaultEntry().GetPtr<void>("f").get();
DeclarePointer("StreamerField", "ptrStreamerField", ptr);
ProcessLine("ptrStreamerField->fInt = 2;");
writer->Fill();

// Reset / close the writer and flush the file.
writer.reset();
});

ASSERT_TRUE(gInterpreter->Declare(R"(
struct StreamerField {
int fInt = 0;
int fAdded = 137;
Comment on lines +902 to +903
Copy link
Member

Choose a reason for hiding this comment

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

To test the indirect collection of StreamerInfo, there should be here a pointer to another class (possibly initialized with an instance of a derived class).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we already test that with

root/tree/ntuple/test/rfield_streamer.cxx

Line 126 in 031f6bd

TEST(RField, StreamerPoly)

Copy link
Member

Choose a reason for hiding this comment

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

Technically the test linked will succeed whether or not the StreamerInfo are stored ... since they are still in memory from the already loaded dictionary.

Copy link
Member

Choose a reason for hiding this comment

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

i.e. like here to test the feature, one need 2 separate processes and 2 distinct schemas/versions.

// removed fAnotherInt

ClassDefNV(StreamerField, 3)
};
)"));

auto reader = RNTupleReader::Open("ntpl", fileGuard.GetPath());
ASSERT_EQ(2, reader->GetNEntries());

void *ptr = reader->GetModel().GetDefaultEntry().GetPtr<void>("f").get();
DeclarePointer("StreamerField", "ptrStreamerField", ptr);

reader->LoadEntry(0);
EXPECT_EVALUATE_EQ("ptrStreamerField->fInt", 1);
EXPECT_EVALUATE_EQ("ptrStreamerField->fAdded", 137);

reader->LoadEntry(1);
EXPECT_EVALUATE_EQ("ptrStreamerField->fInt", 2);
EXPECT_EVALUATE_EQ("ptrStreamerField->fAdded", 137);
}
50 changes: 50 additions & 0 deletions tree/ntuple/test/rfield_streamer.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,3 +305,53 @@ TEST(RField, StreamerMergeIncremental)
EXPECT_TRUE(seenStreamerInfos[2]);
EXPECT_TRUE(seenStreamerInfos[3]);
}

TEST(RField, StreamerSchemaEvolution)
{
FileRaii fileGuard("test_ntuple_rfield_streamer_schema_evolution.root");
{
auto model = RNTupleModel::Create();
model->AddField(RFieldBase::Create("f", "OldStreamerName<int>").Unwrap());
auto writer = RNTupleWriter::Recreate(std::move(model), "ntpl", fileGuard.GetPath());
auto ptrF = writer->GetModel().GetDefaultEntry().GetPtr<OldStreamerName<int>>("f");
ptrF->fValue = 137;
writer->Fill();
}

auto model = RNTupleModel::Create();
model->AddField(RFieldBase::Create("f", "NewStreamerName<int>").Unwrap());
auto reader = RNTupleReader::Open(std::move(model), "ntpl", fileGuard.GetPath());

ASSERT_EQ(1U, reader->GetNEntries());
auto ptrF = reader->GetModel().GetDefaultEntry().GetPtr<NewStreamerName<int>>("f");
reader->LoadEntry(0);
EXPECT_EQ(137, ptrF->fValue);
}

TEST(RField, StreamerClassMismatch)
{
FileRaii fileGuard("test_ntuple_rfield_streamer_class_mismatch.root");
{
auto model = RNTupleModel::Create();
model->AddField(RFieldBase::Create("f", "TemperatureCelsius").Unwrap());
auto writer = RNTupleWriter::Recreate(std::move(model), "ntpl", fileGuard.GetPath());
auto ptrF = writer->GetModel().GetDefaultEntry().GetPtr<TemperatureCelsius>("f");
ptrF->fValue = 100.;
writer->Fill();
}

auto model = RNTupleModel::Create();
model->AddField(RFieldBase::Create("f", "TemperatureKelvin").Unwrap());
auto reader = RNTupleReader::Open(std::move(model), "ntpl", fileGuard.GetPath());

ASSERT_EQ(1U, reader->GetNEntries());
auto ptrF = reader->GetModel().GetDefaultEntry().GetPtr<TemperatureKelvin>("f");

// TODO(jblomer): this should fail with an exception when we connect the page source
ROOT::TestSupport::CheckDiagsRAII diagRAII;
diagRAII.requiredDiag(kError, "TBufferFile::ReadVersion", "Could not find the StreamerInfo with a checksum of",
false /* matchFullMessage */);
diagRAII.requiredDiag(kError, "TBufferFile::CheckByteCount", "object of class TemperatureKelvin read too few bytes",
false /* matchFullMessage */);
Comment on lines +352 to +355
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand those error message. What is the case being tested here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I write a TemperatureCelsius and then try to read it as TemperatureKelvin.

Copy link
Member

Choose a reason for hiding this comment

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

Humm ... why isn't the error message closer to "Can not convert (or read) a TemperatureCelsius (in)to a TemperatureKelvin" ? Without the corresponding checks the failure could be 'serious'. If I understood correctly, the user could have a class A with version 2 which contains a long then request the reading a class B with also version 2 which contains a pointer the the I/O would succeed but not apply any check nor conversion and thus have a random number stored in the pointer. Did I miss something?

Copy link
Member

Choose a reason for hiding this comment

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

I think a proper exception is part of the TODO written above. I don't understand your example, how is this different from the case tested here that even has compatible class layouts?

Copy link
Member

@pcanal pcanal Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I am probably mis-reading the actual test but ...

"TBufferFile::ReadVersion", "Could not find the StreamerInfo with a checksum of",

This message should appear only if there is something wrong with the file and the StreamerInfo record has not been properly stored. Seeing this indicates either a file corruption or a serious deficiency in ROOT (or maybe the user's code)

"TBufferFile::CheckByteCount", "object of class TemperatureKelvin read too few bytes",

This message should only appear if there is an unexpected impedance mismatch between the StreamerInfo and the data onfile. Seeing this indicates either a file corruption or a serious deficiency in ROOT (or maybe the user's code).

As far as my examples is concern, let me try to clarify it. With:

class A {
  int value;
};
class B {
  int value;
};
class C {
  int value;
};
class D {
   long value;
};

i.e. 3 distinct classes with the exact same layout one with a slight difference (And because they have a different name, 4 different CheckSums).
and with a dictionary for all 4 and the following rules:

#pragma read sourceClass="A" targetClass="C";
#pragma read sourceClass="A" targetClass="D" checksum="[correct_value_for_A]" source="int value" \
   target="value" code="{ value = onfile.value*100; } "

you have the following consequences:

Reading From / Onfile Reading Into / In Memory
A A allowed
A B NOT allowed, the user did not say those 2 types were equivalent
A C allowed - need a trivial conversion StreamerInfo
A D allowed (see note (1)) - need a non-trivial conversion StreamerInfo

In the not allowed cases, we should get an error message akin to Can not read a "A" into a "B"

Note (1), since the user specified a checksum, if the file also contains instance of A with a different schema (eg. class A { long value; } from a different release of the software), the conversion is not allowed. In this particular example, the user may or may not have wanted to multiply value when reading those other As.

Copy link
Member

Choose a reason for hiding this comment

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

// TODO(jblomer): this should fail with an exception when we connect the page source

humm ... Maybe the TODO and I agree :) ... Assuming that is correct, I am strongly suggesting that it is actually 'urgent' to implement the TODO as the current situation (if I understand correctly) will be very confusing to users (likely they will think that there is a file corruption (or bug in ROOT) rather than a bug in their code).

reader->LoadEntry(0);
}
Loading