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.

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

[ntuple] set NEntries in RPagePersistentSink::InitFromDescriptor #17306

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

silverweed
Copy link
Contributor

Also add a bunch of reserve() where appropriate

Checklist:

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

Copy link
Contributor

@jblomer jblomer left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

Test Results

    18 files      18 suites   4d 10h 15m 38s ⏱️
 2 682 tests  2 680 ✅ 0 💤 2 ❌
46 556 runs  46 553 ✅ 0 💤 3 ❌

For more details on these failures, see this check.

Results for commit 1925f52.

Copy link
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

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

(also for the record, I'm getting less sure about all those reserves sprinkled in; at least for me, they really decrease code readability for oftentimes questionable benefit...)

Comment on lines +951 to +953
// Even though the number of entries are set when we call AddClusterGroup, we want them to be correct already
// at the end of this function.
fDescriptorBuilder.AddNEntries(nEntries);
Copy link
Member

Choose a reason for hiding this comment

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

Can you comment on why we want this? This now makes InitFromDescriptor different from normal descriptor building where NEntries is only updated when adding a cluster group. This is also documented in the comment of RNTupleDescriptor::fNEntries, which would need at least some updating with this change. More generally, for knowing the number of "entries in flight", we can just look at fPrevClusterNEntries, no?

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