[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 19 files 19 suites 3d 2h 44m 25s ⏱️ For more details on these failures, see this check. Results for commit 0d8ca56. |
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. |
|
|
||
| /// RNTupleLocator payload for the kTypeMulti locator (type 0x03). Used by storage | ||
| /// backends that pack multiple pages into shared objects (e.g., S3 Mode A). The two | ||
| /// 32-bit fields are written to disk as separate integers, so the wire format is |
There was a problem hiding this comment.
I meant the on disk format I will update the comment so its more clear
|
|
||
| /// Note that for GetPosition() / SetPosition(), the locator type must correspond (kTypeFile, kTypeObject64). | ||
| /// Note that for GetPosition() / SetPosition(), the locator type must correspond | ||
| /// (kTypeFile, kTypeObject64, kTypeMulti). |
There was a problem hiding this comment.
For convenience we probably want to phrase it like
| /// (kTypeFile, kTypeObject64, kTypeMulti). | |
| /// (kTypeFile, kTypeObject64, ...). |
and not update it with every locator type
There was a problem hiding this comment.
I will update this comment as well
| 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.
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