[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
jblomer
left a comment
There was a problem hiding this comment.
Nice! Some simplification is possible, I think, by not storing reserved bits altogether.
Test Results 22 files 22 suites 3d 19h 7m 14s ⏱️ For more details on these failures, see this check. Results for commit a78d828. ♻️ This comment has been updated with latest results. |
JasMehta08
force-pushed
the
ntuple-locator-multi-class
branch
from
0d8ca56 to
a78d828
Compare
| EXPECT_EQ(1U, m.GetObjectId()); | ||
| EXPECT_EQ(2U, m.GetOffset()); | ||
|
|
||
| // GetPosition<Object64>() no longer accepts kTypeMulti after the split. |
There was a problem hiding this comment.
| // GetPosition<Object64>() no longer accepts kTypeMulti after the split. |
|
It would be good to also update the PR details. |
| RNTupleSerializer::SerializeUInt64(locator.GetNBytesOnStorage(), buffer); | ||
| } | ||
| RNTupleSerializer::SerializeUInt32(data.GetObjectId(), buffer + sizeofNBytesOnStorage); | ||
| RNTupleSerializer::SerializeUInt32(data.GetOffset(), |
There was a problem hiding this comment.
I would use the pattern found in other serialize functions of:
auto base = reinterpret_cast<unsigned char *>(buffer);
auto pos = base;
void **where = (buffer == nullptr) ? &buffer : reinterpret_cast<void **>(&pos);
pos += RNTupleSerializer::SerializeUInt32(..., *where);
pos += ...instead of manually calculating the write position
| } | ||
| std::uint32_t objectId; | ||
| std::uint32_t offset; | ||
| RNTupleSerializer::DeserializeUInt32(buffer + sizeofNBytesOnStorage, objectId); |
There was a problem hiding this comment.
Like for the serialize: keep track of the buffer position with a variable and increment it with the Deserialize* return values instead of calculating the offset every time
| case 0x03: | ||
| locator.SetType(RNTupleLocator::kTypeMulti); | ||
| DeserializeLocatorPayloadObject64(bytes, payloadSize, locator); | ||
| DeserializeLocatorPayloadMulti(bytes, payloadSize, locator); |
There was a problem hiding this comment.
Both for this and the Object64 case: we need to check the return value and forward RResult in case of failure.
JasMehta08
force-pushed
the
ntuple-locator-multi-class
branch
from
a78d828 to
957239d
Compare
JasMehta08
force-pushed
the
ntuple-locator-multi-class
branch
from
957239d to
8ec164f
Compare
3 participants
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 class stores the object identifier and byte offset as two
uint32_tfields and exposes them viaGetObjectId()/GetOffset(). The 32/32 packing intoRNTupleLocator::fPositionis the only place the in-memory layout shows up, and it lives insideSetPosition(RNTupleLocatorMulti)andRNTupleLocatorHelper<RNTupleLocatorMulti>::Get.RNTupleLocator::SetPositionandGetPosition<>are split accordingly soRNTupleLocatorObject64is now reserved forkTypeObject64only, and the newRNTupleLocatorMultioverload/specialization handlekTypeMulti.Checklist:
This PR is a follow-up #22434