[ntuple] Preserve explicit Double32 representations - #23558
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
Union merging a late Double32_t field pins the source column encoding before connecting the field to the destination sink. Recompression copies encoded element bytes, but the automatic Double32_t selection used to replace that pin with the destination default and silently corrupt values. Honor explicit representatives and sample the default state before any adjustment changes it. Add a merger regression checking the descriptor encoding, zero-filled earlier entries and values read from copied pages. Assisted-by: reasonix:omniroute/cl/cline-free/deepseek-v4.1-flash Assisted-by: codex:gpt-6.1-sol
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:
Preserve explicitly selected Double32_t column representations when connecting a field to a page sink. A union merge can otherwise succeed while silently returning incorrect values.
A minimal trigger is two uncompressed RNTuples: the first contains an integer field, and the second adds a Double32_t field with values 4.5, 5.5, 6.5 and 7.5. Union-merging them into a compressed destination copies the second source's Real32 pages. ExtendDestinationModel pins that source encoding, but AutoAdjustColumnTypes previously replaced it with SplitReal32 according to destination compression. Recompression preserves the encoded element bytes, so the resulting metadata makes the reader decode those bytes incorrectly.
Only apply the Double32_t default selection when the field originally had a default representation. Capture that state before the uncompressed adjustment calls SetColumnRepresentatives, so default uncompressed Double32_t writes still select Real32. Explicit representatives continue through the existing setter validation.
The new RNTupleMerger.Double32UncompressedSource regression checks merge success, the Real32 representative, eight entries, four zero-filled earlier entries and the four source values through RNTupleReader. Restoring the exact old production behavior fails this regression: the representative becomes SplitReal32 and the four values read as 0, 0, approximately -5.1669e29 and 3.0039. Restoring the fix passes again.
Validation used a separate canonical ROOT CMake build with testing=ON, testsupport=ON, runtime_cxxmodules=ON, IMT and Clad off, GCC 13.3, C++17 and RelWithDebInfo. It reused the existing ROOT LLVM/Clang exports and built the ntuple_merger target with -j2. The existing minimal build was preserved.
The exact new regression passes through ROOT's own CTest registration. The canonical merger binary passes 337 tests when excluding MergeLateModelExtension; the unfiltered suite has a SIGFPE in that existing test with both pristine and patched production code. The descriptor, multi-column, model-extension and extended neighboring suites pass. The types suite aborts in StdUnorderedMap with both versions; its three Double32 writer tests pass when selected individually. These baseline failures remain unresolved and no repository test was removed or weakened.
The validation source base was 4f7625b. The patch applies to current master ff6db60, where both changed files have identical bases. Windows/MSVC, parallel IMT, ASan and full ROOT CI were not tested. An additional harness using the real ROOT test sources, dictionary, TestSupport and production library passed the complete merger suite and four neighboring suites, with the new regression as the only pristine-versus-fixed failure delta.
Scope is preserving explicit representatives. It does not repair previously malformed output or address the separate case where newly added record descendants never receive an explicit source representative.
AI assistance: reasonix:omniroute/cl/cline-free/deepseek-v4.1-flash implemented and validated the change; codex:gpt-6.1-sol investigated and reviewed it; grok:grok-4.7 performed an independent adversarial review. This PR is a draft pending the contributor's full diff review, as required by AGENTS.md.
Checklist:
No existing issue is closed by this PR.