fix: invalid chromosome query pollutes fRefVec (issue #23)#36
Closed
ZaGrayWolf wants to merge 12 commits intocompiler-research:developfrom
Closed
fix: invalid chromosome query pollutes fRefVec (issue #23)#36ZaGrayWolf wants to merge 12 commits intocompiler-research:developfrom
ZaGrayWolf wants to merge 12 commits intocompiler-research:developfrom
Conversation
Collaborator
|
@ZaGrayWolf please fix the conflicts |
…ch#23) - Add FindRefId() to RAMNTupleRefs: lookup-only, returns -1 for unknown chromosomes without inserting them into fRefVec - Use FindRefId() in ramntupleview() instead of GetRefId() to prevent silent corruption of the reference name table during queries - Add early exit in ramntupleview() with clear error message for unknown chromosomes - Add test InvalidChromosomeDoesNotPolluteFRefVec to verify the fix - Fix ROOT::Experimental::RNTupleParallelWriter namespace for ROOT 6.38 - Add PRIVATE keyword to benchmark target_link_libraries (CMake best practice) Fixes compiler-research#23
ZaGrayWolf
force-pushed
the
fix/build-compatibility-root638
branch
from
099a89e to
b83eef3
Compare
Author
|
Conflicts resolved |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
vgvassilev
requested changes
Mar 21, 2026
vgvassilev
left a comment
vgvassilev
left a comment
There was a problem hiding this comment.
Please make sure your pull request is minimal and touches only the parts that it needs to.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
ZaGrayWolf
commented
Mar 25, 2026
Author
ZaGrayWolf
left a comment
ZaGrayWolf
left a comment
There was a problem hiding this comment.
Fixed CI issues raised by clang-tidy:
- Corrected include path for RAMNTupleView.h
- Added missing include for std::stoi
- Cleaned up unused headers as suggested
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Author
|
Superseding with #53, that PR accumulated merge commits and unrelated clang-tidy changes over time. The new one is a clean single commit touching only the 4 files needed for this fix. Closing this one. |
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
3 participants
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.
Problem
ramntupleview()calledGetRefId()during region queries. SinceGetRefId()always inserts unknown names intofRefVec, querying anon-existent chromosome silently corrupted the reference name table.
Fix
Added
FindRefId()toRAMNTupleRefs— a const lookup-only methodthat returns -1 for unknown chromosomes without inserting them.
ramntupleview()now callsFindRefId()and exits early with a clearerror message for unknown chromosomes.
Test
Added
InvalidChromosomeDoesNotPolluteFRefVectoramcoretests.cxx.All 3 tests pass:
Additional fixes
ROOT::Experimental::RNTupleParallelWriterwithROOT::RNTupleParallelWriterfor ROOT 6.38 compatibilityPRIVATEkeyword tobenchmark::benchmarklink librariesFixes #23