Skip to content

Conversation

@jblomer
Copy link
Contributor

@jblomer jblomer commented Dec 3, 2025

This should be the last piece to fix #20282. In particular, it is necessary to fix [U]Long64_t template arguments where the type is not wrapped in a class. I.e. without this fix

EdmWrapper<std::set<EdmContent<Long64_t>>>; // works
std::set<EdmContent<Long64_t>>; // fails

Another PR with the corresponding test will follow.

@makortel Please let me know if you need this backported to 6.36. This depends on whether your top-level branches are all wrapped in a class or if you have STL collections directly as top-level branches.

@jblomer jblomer self-assigned this Dec 3, 2025
@jblomer jblomer force-pushed the ntuple-propagate-type-alias branch from 3ec064d to 934d6af Compare December 3, 2025 10:41
The variant field, the tuple field, and the pair field all use
approximately the same GetTypeList() private static method. Factor this
out in a single method in an anonymous namespace.
When constructing compound types from item fields, propagate alias types
from the item fields to the parent field. This is already done globally in
RFieldBase::Create() because the entire type name is compared to the
unresolved type name. However, the type alias propagation was missing
when a field is directly constructed with an item field.
@github-actions
Copy link

github-actions bot commented Dec 3, 2025

Test Results

    22 files      22 suites   3d 23h 39m 39s ⏱️
 3 782 tests  3 782 ✅ 0 💤 0 ❌
81 204 runs  81 204 ✅ 0 💤 0 ❌

Results for commit 934d6af.

@makortel
Copy link

makortel commented Dec 4, 2025

Please let me know if you need this backported to 6.36. This depends on whether your top-level branches are all wrapped in a class or if you have STL collections directly as top-level branches.

I'm not fully sure. The framework's EDM wraps everything in edm::Wrapper<T>, so those should be ok. Framework also stores metadata in RNTuple without the edm::Wrapper<T>, but we don't use Long64_t in template arguments there.

I don't know if NanoAOD stores anything in e.g. std::vector<Long64_t>. On the other hand, if this is only about the behavior of streamer fields, NanoAOD shouldn't matter.

We set the rntupleStreamerMode="true" in the selection XML files for the wrapped types, i.e. in your example we would set it for std::set<EdmContent<Long64_t>> and not EdmWrapper<std::set<EdmContent<Long64_t>>> (for an example see
https://github.com/cms-sw/cmssw/blob/61a1113117cf3a7aafee41dee39a175dbda615f4/DataFormats/Histograms/src/classes_def.xml#L37
https://github.com/cms-sw/cmssw/blob/61a1113117cf3a7aafee41dee39a175dbda615f4/DataFormats/Histograms/src/classes_def.xml#L55
https://github.com/cms-sw/cmssw/blob/61a1113117cf3a7aafee41dee39a175dbda615f4/DataFormats/Histograms/src/classes_def.xml#L89
). Does that play any role here?

Copy link
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

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

Thanks.

}

std::string BuildSetTypeName(ROOT::RSetField::ESetType setType, const ROOT::RFieldBase &innerField)
std::string GetTypeList(std::span<std::unique_ptr<ROOT::RFieldBase>> itemFields, bool useTypeAliases)
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.

Consider adding a short summary of the behavior (Intend of the use of the result, reach of the list).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ntuple] use meta normalized name for streamer info records

3 participants