[ntuple] Fix handling of I/O rules' inputs

[jblomer]

Fix execution of I/O customization rules that have on-disk data member sources (inputs). Previously, TVirtualObject::fObject pointed to the target object, which is plain wrong (but happens to work in some cases). Now, rule inputs are examined, fields are created according to the provided input's type, and the data is read in a staging area that follows the memory layout of the on-disk class given by TClass. This staging area is now what TVirtualObject::fObject points to.

Note that the target still has to be a transient member. This restriction is going to be lifted in a second PR.

[pcanal]

Suggested change

	auto referencesNonTransientMembers = [classp = fClass](const ROOT::TSchemaRule *rule) {
	auto targetsNonTransientMembers = [classp = fClass](const ROOT::TSchemaRule *rule) {

'references' could also means 'listed as one of the sources'

[hahnjo]

(side question: can transient members ever be a source? They are not onfile after all...)

[jblomer]

As far as I understand, transient member must not be a source; @pcanal ?

[github-actions]

Test Results

    18 files      18 suites   4d 1h 16m 8s ⏱️
 2 688 tests  2 687 ✅ 0 💤 1 ❌
46 688 runs  46 686 ✅ 0 💤 2 ❌

For more details on these failures, see this check.

Results for commit aac01e8.

♻️ This comment has been updated with latest results.

[pcanal]

Finding the @@ class relies on the related StreamerInfo to have been build, is that a guarantee here?

[jblomer]

I thought that the line before, calling TClass::GetClass(<class name>)->GetStreamerInfo(<version used in @@>) is enough. Is another call necessary?

[pcanal]

Technically this is not the right test. There can be staging even for the current version.

Inside TStreamerInfo the equivalent test are either (during building) whether there is a rule that applies for the pair (current version and onfile version) or whether first element of the TStreamerInfo being used start with an @@alloc element (i.e. an element with type 'TStreamerInfo::kCacheNew') [The element also give direct access to the staging/alloc TClass).

With the file sameversion.h, we get:

root [0] .L sameversion.h+
root [1] auto cl = TClass::GetClass("SameClassRule");
root [2] cl->GetStreamerInfo()->ls()

StreamerInfo for class: SameClassRule, version=3, checksum=0x1bf8119e
  SameClassRule@@3 @@alloc         offset=  0 type=1001                     
  int            fValue          offset=  8 type= 3  (cached,repeat)                     
  int            fValue          offset=  0 type= 3                     
  void           SameClassRule_rule0 offset=  0 type=1000  (wholeObject)                     
  SameClassRule@@3 @@dealloc       offset=  0 type=1002                     
   i= 0, @@alloc         type=1001, offset=  0, len=1, method=0
   i= 1, fValue          type=  3, offset=  8, len=1, method=0 [cached,repeat]
   i= 2, fValue          type=  3, offset=  0, len=1, method=0
   i= 3, SameClassRule_rule0 type=1000, offset=  0, len=1, method=0 [wholeObject]
   i= 4, @@dealloc       type=1002, offset=  0, len=1, method=0

// File sameversion.h
#pragma once

struct SameClassRule
{
   int fValue;
};


#ifdef __ROOTCLING__
#pragma link C++ options = version(3) class SameClassRule+;
#pragma read sourceClass="SameClassRule" targetClass="SameClassRule" versions="[1-]" \
   source="int fValue" target="" code="{ printf(\"inside rule\n\"); }"
#endif

[hahnjo]

As far as I understand the code, the staging area is created regardless of this check, which is only there to determine which clName to ask from TClass::GetClass.

[pcanal]

The stagingAreaSize is already calculated and we should have at the end stagingAreaSize == cl->Size();

[jblomer]

We may skip rules, so we'll check for stagingAreaSize <= fStagingClass->Size()

[hahnjo]

(note for future reviewers: the in-memory class version is asserted in the constructor)

[hahnjo]

(side question: can transient members ever be a source? They are not onfile after all...)

[hahnjo]

Not sure if this is important, but before the refactoring GetOnDiskId() == kInvalidDescriptorId would early-exit while now we land here. I think there's no observable difference because SetArtificial() applies recursively to all sub-fields, so they should have already been marked by the parent...

[jblomer]

Isn't the point that if GetOnDiskId() == kInvalidDescriptorId then knownSubFields is empty, so the loop does nothing?

[hahnjo]

Yes, knownSubFields is empty so knownSubFields.count(field->GetFieldName()) will always be 0, marking all sub-fields as artificial. As I said, this may be fine...

[hahnjo]

As far as I understand the code, the staging area is created regardless of this check, which is only there to determine which clName to ask from TClass::GetClass.