Repository navigation
[query] make the locus interface representation-agnostic - #15878
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
|
| Layer / File(s) | Summary |
|---|---|
Locus values and prefix codes hail/hail/src/is/hail/types/physical/PLocus.scala, hail/hail/src/is/hail/types/physical/stypes/interfaces/SLocus.scala, hail/hail/src/is/hail/types/physical/stypes/concrete/SCanonicalLocusPointer.scala, hail/hail/src/is/hail/types/physical/stypes/concrete/SUnreachable.scala, hail/hail/test/src/is/hail/types/physical/stypes/concrete/SCanonicalLocusPointerSuite.scala |
SLocusValue replaces contigLong with contigIdx and adds packed. Canonical locus values memoize the contig index and packed value. prefixCode encodes the packed value. Tests compare the values with reference genome representations and check locus ordering. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Refactor
Suggested reviewers: ehigham
Merge Risk: 
Minimal · previous review 76d36
-
No confirmed issue prevents merging after normal build and test checks.
Security architecture risk: 
Low · up to 76d36
-
The inspected changes preserve copy ownership and include checks for locus ordering. No introduced security issue was identified. Risk remains limited but nonzero because encoded bytes change, and compatibility with external consumers has not been established.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — The demonstrated affected scope is locus encoding within query evaluation, including callers of the existing prefixCode IR function. The available evidence does not establish cross-tenant, credential, infrastructure, or data-store authority changes; deployment exposure and external byte consumers remain unresolved.
Trust Boundaries and Controls
- observed — The encoding path uses the reference genome attached to the locus value. Packing does not add explicit contig-membership or position validation. The former global-position path also relied on reference-genome lookup and did not explicitly validate position; no newly bypassed security control was established.
-
Pre-merge checks: 
4
-
Full details: Description check
-
Explanation
The description explains the interface changes, implementation impact, and security assessment. It omits the explicit Change Description heading and issue reference, but the required information is otherwise present.
-
Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
1c1b7dd to
8df0751
Compare
8df0751 to
49c62ee
Compare
48ce497 to
e1fd6e4
Compare
76e4afd to
280053f
Compare
SLocusValue no longer exposes the contig string's address (contigLong). Instead it provides contigIdx, the contig's index in the reference genome, and packed, the locus as (contig_index << 32) | position. The canonical locus computes both through the reference genome's contig-index lookup. SCanonicalLocusPointer's coercion, the only caller of contigLong, now matches on the canonical value directly. contigType moves from PLocus to PCanonicalLocus, and locus prefix codes encode the packed integer instead of the global position. Prefix codes are unused, so the change is not observable.
280053f to
023711e
Compare
SLocusValue no longer exposes the contig string's address (contigLong). Instead it provides contigIdx, the contig's index in the reference genome, and packed, the locus as (contig_index << 32) | position. The canonical locus computes both through the reference genome's contig-index lookup. SCanonicalLocusPointer's coercion, the only caller of contigLong, now matches on the canonical value directly.
contigType moves from PLocus to PCanonicalLocus, and locus prefix codes encode the packed integer instead of the global position. Prefix codes are unused, so the change is not observable.
Security Assessment
Stack created with GitHub Stacks CLI • Give Feedback 💬