Repository navigation
[ntuple] Fixes to RNTupleMerger with emulated types - #23702
silverweed wants to merge 3 commits into
Conversation
Test Results 23 files 23 suites 4d 0h 22m 29s ⏱️ For more details on these failures, see this check. Results for commit c8f217b. ♻️ This comment has been updated with latest results. |
1ac51aa to
2d3888e
Compare
| desc = builder.TypeVersion(GetTypeVersion()) | ||
| .TypeChecksum(GetTypeChecksum()) | ||
| .TypeName(GetTypeName()) | ||
| // we need an Id for MoveDescriptor to succeed, but we're not going to use it. |
There was a problem hiding this comment.
Which part makes it optional? (and is it too hard and/or fragile to remove this requirement?
There was a problem hiding this comment.
In this case we're using the descriptor just as a container for the three values we care about (type version/checksum/name), so it's a bit of an improper use.
The alternatives would be to keep passing them as separate args or to come up with a dedicated struct for it
There was a problem hiding this comment.
Since we have the SoA emulation fix waiting for this, I'd suggest to add a TODO so that we can proceed.
2d3888e to
c8f4fb2
Compare
This will collect a bunch of new tests for merging of emulated fields
We need to tell CreateField to emulate late model extended fields.
Also add a test for merging emulated Vector field
c8f4fb2 to
c8f217b
Compare
This branch has not been deployed
3 participants
This Pull request:
starts adding some systematic testing for merging RNTuples containing emulated fields. In particular the end goal is to test all types of emulated fields with and without late model extension.
Adding some basic testing of emulated fields + late model extension (aka Union merging) surfaced some bugs since the code path was never exercised before. This was brought to light by #23684.
This PR starts setting the foundation for this new set of tests along with fixes for the problems found so far. It doesn't address yet the issue brought up by the PR linked above, but the idea is to get to the best solution for it incrementally.
Changes or fixes:
MergeEmulatedtest fromntuple_emulatedto newntuple_merge_emulatedtest fileEmulateUnknownTypesflag toCreateFieldcalls in case of late model extensionChecklist: