Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ntuple] add fMaxKeySize field to RNTuple anchor #15504

Closed
wants to merge 1 commit into from
Closed

[ntuple] add fMaxKeySize field to RNTuple anchor #15504

wants to merge 1 commit into from

Conversation

silverweed
Copy link
Contributor

@silverweed silverweed commented May 14, 2024 •
edited

This Pull request:

  • Adds a std::uint64_t fMaxKeySize field to the RNTuple struct
  • Adds the necessary machinery to serialize that field to the StreamerInfo
  • Bumps the RNTuple class version from 4 to 5

TODO

  • Update documentation
  • Actually use the new field to do header chaining when size exceeds maxKeySize
  • Consider adding a fwd compatibility test (read version 5 on disk from version 4 in memory)

Checklist:

  • tested changes locally (one test currently fails due to the tested file missing proper StreamerInfo)
  • updated the docs (if necessary)

@silverweed silverweed self-assigned this May 14, 2024
@silverweed silverweed marked this pull request as draft May 14, 2024 08:41
@github-actions GitHub Actions
Copy link

github-actions bot commented May 14, 2024 •
edited

Test Results

    10 files      10 suites   1d 21h 44m 16s ⏱️
 2 635 tests  2 634 ✅ 0 💤 1 ❌
24 868 runs  24 860 ✅ 0 💤 8 ❌

For more details on these failures, see this check.

Results for commit 3408a44.

♻️ This comment has been updated with latest results.

@silverweed silverweed force-pushed the ntuple_anchor_maxkeysize branch 3 times, most recently from 6409519 to 37b61f5 Compare May 14, 2024 13:53
@silverweed silverweed marked this pull request as ready for review May 14, 2024 13:54
@silverweed
[ntuple] add fMaxKeySize field to RNTuple
3408a44 
Also change RNTuple::Streamer to manually unpack its fields from the
buffer according to its version, rather than relying on
buf.ReadClassBuffer(). This is necessary to handle schema evolution on
the RNTuple anchor itself.
@jblomer jblomer changed the title [ntuple] add maxKeySize field to RNTuple anchor [ntuple] add fMaxKeySize field to RNTuple anchor May 14, 2024
jblomer
jblomer reviewed May 14, 2024
View reviewed changes
tree/ntuple/v7/src/RNTuple.cxx
Comment on lines +55 to +57
if (classVersion > 4)
buf >> fMaxKeySize;
buf >> fChecksum;
Copy link
Contributor

@jblomer jblomer May 14, 2024 •
edited

Choose a reason for hiding this comment

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

That's much better than it was before but still not forward compatible. After reading the last known member, we should skip all but the last 8 bytes, which we stream into the checksum member. I think we should also first make a check on the buffer size to make sure we can actually stream the number of bytes we are going to stream.

Could you put up this patch to reading the RNTuple anchor as a separate PR? I think this may be something that we even want to backport to the 6.32 branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#15516

@silverweed silverweed marked this pull request as draft May 15, 2024 15:56
@silverweed
Copy link
Contributor Author

Will open a new PR after #15553 is merged

@silverweed silverweed closed this May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jblomer jblomer jblomer left review comments

@pcanal pcanal Awaiting requested review from pcanal

Assignees

@silverweed silverweed

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@silverweed @jblomer