[ntuple] Add RNTupleLocatorMulti class for kTypeMulti locator#22577
[ntuple] Add RNTupleLocatorMulti class for kTypeMulti locator#22577JasMehta08 wants to merge 1 commit into
Conversation
jblomer
left a comment
There was a problem hiding this comment.
Nice! Some simplification is possible, I think, by not storing reserved bits altogether.
|
|
||
| std::uint32_t fObjectId = 0; | ||
| std::uint32_t fOffset = 0; | ||
| std::uint8_t fReserved = 0; |
There was a problem hiding this comment.
Thinking about it, we probably don't need to store the reserved bits because we anyway don't know what to do with them. That should also avoid the need for masks.
| } else { | ||
| RNTupleSerializer::SerializeUInt64(locator.GetNBytesOnStorage(), buffer); | ||
| } | ||
| RNTupleSerializer::SerializeUInt64(data.GetLocation(), buffer + sizeofNBytesOnStorage); |
There was a problem hiding this comment.
Here, I think we should write the two integers (object id, offset) as 32bit integers to disk. This avoids any potential issues of platform-independence when we combine the two integers in a single 64 bit int.
Test Results 19 files 19 suites 3d 2h 44m 25s ⏱️ For more details on these failures, see this check. Results for commit 0d8ca56. |
This Pull request:
(Is a part of the GSoC 2026 project
S3 Backend for RNTuple.)Changes or fixes:
Follow-up to #22434. Introduces a dedicated
RNTupleLocatorMulticlass for thekTypeMultilocator, replacing the previous reuse ofRNTupleLocatorObject64.The new class stores the object identifier and byte offset as two explicit 32-bit fields plus a separate 4-bit reserved field.
GetLocation()returns the canonical packed 64-bit value (parallel toRNTupleLocatorObject64::GetLocation()), with the layout:The
(uint32_t, uint32_t)constructor andSetReserved()validate their inputs against the 30-bit and 4-bit ranges respectively. The0x03payload remains a 4/8-byte size followed by an 8-byte packed value, so the wire format stays identical.RNTupleLocator::SetPositionandGetPosition<>are split accordingly soRNTupleLocatorObject64is now reserved forkTypeObject64only, and the newRNTupleLocatorMultioverload/specialization handlekTypeMulti.Checklist:
This PR is a follow-up #22434