Skip to content

[ntuple] Preserve explicit Double32 representations - #23558

Open
Elvand-Lie wants to merge 1 commit into
root-project:masterfrom
Elvand-Lie:fix/ntuple-double32-explicit-representation
Open

Elvand-Lie wants to merge 1 commit into
root-project:masterfrom
Elvand-Lie:fix/ntuple-double32-explicit-representation

Conversation

@Elvand-Lie

Copy link
Copy Markdown

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.

GTEST_FILTER=RNTupleMerger.Double32UncompressedSource ctest --test-dir /home/elvand/root-canonical-check -R ntuple-merger --output-on-failure

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:

  • tested changes locally (scope and baseline failures described above)
  • updated the docs (if necessary): no public API or file-format change requiring documentation

No existing issue is closed by this PR.

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
@Elvand-Lie
Elvand-Lie marked this pull request as ready for review October 1, 2026 09:32

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants