Repository navigation
[ntuple] Preserve trunc/quant params of late-merged fields - #23646
Merged
silverweed merged 1 commit intoOct 13, 2026
Merged
silverweed merged 1 commit into
silverweed merged 1 commit into
Conversation
Elvand-Lie
force-pushed
the
fix/ntuple-merger-late-trunc-params
branch
from
October 7, 2026 13:44
4b8cc68 to
6567a9d
Compare
silverweed
reviewed
Oct 7, 2026
silverweed
left a comment
silverweed
left a comment
Contributor
There was a problem hiding this comment.
Thanks. A few minor comments
Elvand-Lie
force-pushed
the
fix/ntuple-merger-late-trunc-params
branch
from
October 7, 2026 14:59
6567a9d to
830706e
Compare
Union merging a late low-precision field recreates it from the descriptor and pins only the column type. Truncation parameters live in the in-memory field, so the destination column got default (and for kReal32Trunc out-of-range) values, failing the merge or corrupting quantized data. Restore them from the source descriptor, recursing into subfields by on-disk ID. Add a merger regression checking descriptor parameters, zero-filled earlier entries and read-back values. Assisted-by: Muse Spark (Meta) via OpenCode
Elvand-Lie
force-pushed
the
fix/ntuple-merger-late-trunc-params
branch
from
October 7, 2026 16:02
830706e to
4b32efb
Compare
This was referenced Oct 11, 2026
Contributor
Author
|
Hi @silverweed, ready for another look. I've picked up your error-handling suggestions and run the tests locally. I kept the invalid-ID check since that ID can be invalid with an evolved class dictionary. Asserting there would crash a merge. While checking this, I found a separate schema-mismatch bug and opened #23684 to fix it. |
Test Results 24 files 24 suites 4d 4h 53m 17s ⏱️ For more details on these failures, see this check. Results for commit 4b32efb. |
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:
Late model extension in RNTupleMerger recreates a missing field from its descriptor but pins only the column type, losing the truncation/quantization bit width and value range. This can produce an invalid truncation width or decode copied quantized pages with the wrong range. Restore those parameters before extending the destination schema, including nested subfields, using the existing field setters. Restoration failures propagate through RResult; setter exceptions are converted to failures. Reconstructed class members absent from the source descriptor are skipped because they have no source column parameters to restore.
The regression RNTupleMerger.MergeLateExtensionReal32TruncQuant checks float and double fields, a nested vector item, stored widths and ranges, earlier-entry defaults, and read-back values.
Validation:
Tested published head 4b32efb on Ubuntu WSL with GCC 13.3.0, RelWithDebInfo, testing enabled, and C++ modules disabled:
Upstream-disabled tests retained their defaults. Native Windows, C++ modules enabled, tutorials, the remaining ROOT test suite, and CI were not run.
Checklist:
No existing issue tracks this bug. Before opening the PR, open and closed issues, open PRs, and recent merger commits were searched for truncation, quantization, and ExtendDestinationModel; no competing fix was found.
AI disclosure: implementation assistance from Muse Spark (Meta) via OpenCode, code review by Opus 5.5, and build recovery, test validation, and review-follow-up verification by Codex:gpt-6.1-sol. Final review and decisions remain with the author.