Skip to content

[ntuple] Add RNTupleLocatorMulti class for kTypeMulti locator - #22577

Merged
jblomer merged 1 commit into
root-project:masterfrom
JasMehta08:ntuple-locator-multi-class
Jun 23, 2026
Merged

[ntuple] Add RNTupleLocatorMulti class for kTypeMulti locator#22577
jblomer merged 1 commit into
root-project:masterfrom
JasMehta08:ntuple-locator-multi-class

Conversation

@JasMehta08

@JasMehta08 JasMehta08 commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

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 RNTupleLocatorMulti class for the kTypeMulti locator, replacing the previous reuse of RNTupleLocatorObject64.

The class stores the object identifier and byte offset as two uint32_t fields and exposes them via GetObjectId() / GetOffset(). The 32/32 packing into RNTupleLocator::fPosition is the only place the in-memory layout shows up, and it lives inside SetPosition(RNTupleLocatorMulti) and RNTupleLocatorHelper<RNTupleLocatorMulti>::Get.

RNTupleLocator::SetPosition and GetPosition<> are split accordingly so RNTupleLocatorObject64 is now reserved for kTypeObject64 only, and the new RNTupleLocatorMulti overload/specialization handle kTypeMulti.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR is a follow-up #22434

@JasMehta08
JasMehta08 requested a review from jblomer as a code owner June 11, 2026 09:18
@jblomer jblomer self-assigned this Jun 11, 2026
@jblomer jblomer assigned JasMehta08 and unassigned jblomer Jun 11, 2026

@jblomer jblomer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Some simplification is possible, I think, by not storing reserved bits altogether.

Comment thread tree/ntuple/inc/ROOT/RNTupleTypes.hxx Outdated
Comment thread tree/ntuple/src/RNTupleSerialize.cxx Outdated
@github-actions

github-actions Bot commented Jun 16, 2026

Copy link
Copy Markdown

Test Results

    21 files      21 suites   3d 9h 15m 15s ⏱️
 3 870 tests  3 870 ✅ 0 💤 0 ❌
72 930 runs  72 930 ✅ 0 💤 0 ❌

Results for commit 8ec164f.

♻️ This comment has been updated with latest results.

@JasMehta08
JasMehta08 force-pushed the ntuple-locator-multi-class branch from 0d8ca56 to a78d828 Compare June 18, 2026 09:17
@JasMehta08
JasMehta08 requested a review from silverweed as a code owner June 18, 2026 09:17

@jblomer jblomer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Comment thread tree/ntuple/test/ntuple_serialize.cxx Outdated
@jblomer

jblomer commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

It would be good to also update the PR details.

Comment thread tree/ntuple/inc/ROOT/RNTupleTypes.hxx Outdated
Comment thread tree/ntuple/inc/ROOT/RNTupleTypes.hxx Outdated
Comment thread tree/ntuple/src/RNTupleSerialize.cxx Outdated
Comment thread tree/ntuple/src/RNTupleSerialize.cxx Outdated
Comment thread tree/ntuple/src/RNTupleSerialize.cxx Outdated
@JasMehta08
JasMehta08 force-pushed the ntuple-locator-multi-class branch from a78d828 to 957239d Compare June 21, 2026 12:11
@JasMehta08
JasMehta08 force-pushed the ntuple-locator-multi-class branch from 957239d to 8ec164f Compare June 21, 2026 12:15
@JasMehta08

Copy link
Copy Markdown
Contributor Author

@jblomer and @silverweed

I have made the changes according to the comments please let me know if there is anything that I should change further

@silverweed silverweed left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JasMehta08 thanks! When the CI is green it can be merged for me

@jblomer
jblomer merged commit caf49e8 into root-project:master Jun 23, 2026
32 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants