Skip to content

[ntuple] Renormalize field type names if needed in SetSchemaFromExisting#22816

Open
silverweed wants to merge 2 commits into
root-project:masterfrom
silverweed:ntuple_merge_pre636
Open

[ntuple] Renormalize field type names if needed in SetSchemaFromExisting#22816
silverweed wants to merge 2 commits into
root-project:masterfrom
silverweed:ntuple_merge_pre636

Conversation

@silverweed

Copy link
Copy Markdown
Contributor

Fixes #22130.

I have a local integration test for this change but I'm waiting on a user's permission to use their files.
Otherwise we can craft our own files to be merged instead.

@silverweed
silverweed requested review from enirolf and hahnjo July 15, 2026 07:48
@silverweed silverweed self-assigned this Jul 15, 2026
@silverweed
silverweed force-pushed the ntuple_merge_pre636 branch 3 times, most recently from 1750f9b to e4b5e2e Compare July 15, 2026 07:54
@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown

Test Results

    23 files      23 suites   3d 15h 23m 5s ⏱️
 3 874 tests  3 874 ✅ 0 💤 0 ❌
78 808 runs  78 808 ✅ 0 💤 0 ❌

Results for commit 6e804fa.

♻️ This comment has been updated with latest results.

@silverweed
silverweed marked this pull request as ready for review July 16, 2026 11:44
@silverweed

Copy link
Copy Markdown
Contributor Author

Update: the user has kindly granted us permission to use his shared files, so I uploaded the integration test.

@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.

How about this approach, which would avoid another friend declaration:

We add an RFieldDescriptorBuilder::FromFieldDescriptor() method, and then we do the fixup in RNTupleDescriptor::Clone(): we replace RFieldDescriptor::Clone() with creating field descriptor builders first, and fixup the type name in that descriptor (using GetTypeNameForComparison()) before moving it into a destination field descriptor.

@silverweed

silverweed commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

How about this approach, which would avoid another friend declaration:

Btw I just realized the friend declaration is an unnecessary remnant from a previous experiment, I removed it. Do you think I should still try your proposal?

@silverweed
silverweed force-pushed the ntuple_merge_pre636 branch 2 times, most recently from b311627 to 3c62a30 Compare July 24, 2026 12:56
@silverweed
silverweed force-pushed the ntuple_merge_pre636 branch from 3c62a30 to 2224cb8 Compare July 24, 2026 13:58
@silverweed
silverweed force-pushed the ntuple_merge_pre636 branch from 2224cb8 to f5d3b77 Compare July 24, 2026 14:05
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.

[ntuple] merging RNTuples produced with ROOT <= 6.34 fails

2 participants