[ntuple] Schema evolution fixes - #23196
Conversation
fe4b41b to
a879404
Compare
Test Results 22 files 22 suites 3d 15h 41m 56s ⏱️ For more details on these failures, see this check. Results for commit c0423bd. ♻️ This comment has been updated with latest results. |
35b0064 to
1942bf1
Compare
hahnjo
left a comment
hahnjo
left a comment
There was a problem hiding this comment.
Some considerations for simplification
| if (fieldDesc.GetTypeVersion() != GetTypeVersion() || fieldDesc.GetTypeName() != GetTypeName()) { | ||
| // We need the on-disk streamer info for the conversion streamer info | ||
| pageSource.LoadStreamerInfo(); | ||
| } |
There was a problem hiding this comment.
Is this an optimization to only load the streamer infos when needed? How much would it cost to always load them when opening the file?
There was a problem hiding this comment.
I agree, the benefits of loading the streamer info on demand are not clear enough.
I would like to implement this as a separate PR (#23252) though to keep this one lean and to ease backporting.
1942bf1 to
4c9c7d4
Compare
|
I can confirm that this fixes #23146 (at least in our workflow where I first noticed this). I'd appreciate a backport to 6.40.x if possible once merged. |
4c9c7d4 to
c0423bd
Compare
If during schema evolution a conversion streamer info is required, we have to load the schema info records from the ROOT file. If the RNTuple is loaded through the RMiniFile, this requires an extra call.
3 participants
Fixes #23146
Fixes #17713