Repository navigation
[ntuple] Reject incompatible late class schemas - #23684
Open
Elvand-Lie wants to merge 1 commit into
Open
Elvand-Lie wants to merge 1 commit into
Elvand-Lie wants to merge 1 commit into
Conversation
Elvand-Lie
marked this pull request as ready for review
October 11, 2026 13:05
1 of 2 tasks
1 of 2 tasks
Elvand-Lie
force-pushed
the
fix/ntuple-union-late-class-schema
branch
from
October 15, 2026 16:09
3be18e6 to
5d174ac
Compare
Late Union fields are built from the loaded dictionary, then their member pages are paired by position. A changed dictionary can therefore silently place source data in the wrong destination members. Preflight every new field against its source schema before extending the model. Route mismatches and ROOT field-creation errors through the existing Abort/Skip policy. Require matching class metadata and ordered member layouts rather than attempting schema conversion. Cover matching, evolved, nested and projected classes, streamer fields, and missing dictionaries, including continuing after a skipped source. Assisted-by: codex:gpt-6.1-sol Assisted-by: claude-code:opus
Elvand-Lie
force-pushed
the
fix/ntuple-union-late-class-schema
branch
from
October 18, 2026 10:37
5d174ac to
15a0390
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This Pull request:
Changes or fixes:
A Union merge can silently copy member pages to the wrong fields when a class first appears in a later source and its loaded dictionary differs from the file. For example, a file written with members
fA,fCand merged with a dictionary containingfA,fB,fCcan putfCdata intofB.Reconstruct and validate all new ordinary and projected fields before extending the destination model. Compare type, structure, versions, checksum and ordered child layout recursively. Report mismatches and ROOT field-creation errors through the existing Abort/Skip policy, so a skipped input leaves no partially added fields.
This deliberately requires the loaded dictionary to match the source schema and class version, including appended members, version-only bumps and streamer fields. It does not add schema conversion or transaction rollback for unrelated failures. Matching scalar, nested and projected late fields retain their values. The existing restriction on adding late streamer fields remains; matching common streamer fields are a readback control.
The schema safety issue predates and is separate from #23646.
This branch is rebased onto master after #23646 and the merger Strategy refactor #23576. It keeps the restoration of truncated and quantized field parameters from #23646, and passes the pre-validated late fields through
RNTupleMergeStrategy::ExtendDestinationModel. It does not include the still-unmerged #23702. When #23702 lands, this change will need a follow-up adjustment to preserve its late-field unknown-type emulation and update the missing-dictionary rejection scenarios.Validation
Validation applies to commit
15a039048d3b8a64b7526a59d38037408be226df, rebased onto masterfbcf83fe9a58e65f36ba55bdc3821a7983f4799a. The RNTuple and RNTupleUtil test targets built successfully with GCC 13.3 at-j 3. The full evolution suite passed all 20 tests, including all 16 late-class scenarios, the full merger executable passed all 340 tests, and the emulated-field suite passed all 12 tests. With only the merger implementation reverted to master,RNTupleEvolution.UnionMergeLateClassSchemafails on the inserted-member scenario.The CTest filter
ntuple|hadd, excludinglargefile|largevector|tutorial, passed all 51 selected tests at-j 2. Runs usedulimit -c 0. Large-file, large-vector and tutorial tests were not run.Changed-line clang-format 18.1.8 and diff whitespace checks are clean. The original diagnostic registrations passed unchanged.
Compact test evidence
The source writer declares
LateMergeClass { int fA = 11; int fC = 33; }. Ordinary cases Union-merge two one-entry files: the first has onlye; the second addsbefore_record=77andrecord={fA=11,fC=33}. A separate merging process uses the dictionary variation below. Abort is the default.fA,fC(0,0)then(11,33)fA,fB,fCfC,fA(11,33)(11,33)in both entries[0,99]unknown type: LateMergeClass; record and before_record absent[0,99]This table shows representative scenarios. The 16 scenarios also cover appended/removed members, a changed member type, a class version-only bump, common matching streamer readback, and an evolved late streamer rejection. The runtime-assertions column lists fatal assertions checked by the test; the test does not print separate value dumps or full error reports for each scenario.
AI assistance: Codex (
gpt-6.1-sol) assisted with implementation, rebase conflict resolution and validation, and Claude Code (opus) independently reviewed the work.Checklist:
This PR fixes #23683